From 32be357078563f4d0ad445920c4077896783617f Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Thu, 22 Oct 2026 04:48:30 -0700 Subject: [PATCH] fix(inspect): size video probe bounds for JSON escaping, exact-case envelopes ffprobe JSON-escapes tag text, so bounds measured on its stdout could mark a video workflow limited, or fail a role record, that the identical PNG reads within its decoded budget. The first probe now allows twice the role and workflow budgets plus 64 KiB (12 MiB + 64 KiB) and the role-only re-probe twice the role budget plus 64 KiB, leaving the decoded per-record budgets as the only gate; a reader-level test reads near-budget quote-dense records as PNG and as video and asserts identical metadata and formats. Envelope keys (comment, description) are matched exactly as the legacy readers did; only the four per-key carriers fold Matroska case. Fixtures can write the legacy ilst comment atom (metadata_tags=False), and the docs now say the admin scan reads the carrier while the live monitor still processes images only, with a follow-up recorded in TESTING.md. --- TESTING.md | 46 ++++++++++------- docs/prompt-inspector.md | 5 +- tests/test_api_admin.py | 3 +- tests/test_api_prompt_inspector.py | 27 +++++----- tests/test_inspection_response.py | 82 ++++++++++++++++++++++++++++++ tests/test_utils_video_metadata.py | 52 +++++++++++++++---- utils/prompt_inspector.py | 16 ++++-- utils/video_metadata.py | 16 +++--- 8 files changed, 195 insertions(+), 52 deletions(-) diff --git a/TESTING.md b/TESTING.md index d2b031dd..936139cf 100644 --- a/TESTING.md +++ b/TESTING.md @@ -332,23 +332,29 @@ becomes record format `limited` (no raw text, graph `limited`, roles and facts unaffected); other text chunks are skipped unread; byte-identical duplicates are accepted within a bounded read budget, while differing duplicates (and any duplicate role record over that budget) still fail. Other read bounds are unchanged: 8 MiB -non-PNG header, 10,000 PNG chunks and a three-second ffprobe. Video containers are -read through one shared carrier selector (`utils/video_metadata.select_video_carrier`, -used by the inspector and by scans): carriers are, in order, the per-key tags that -current savers write (`prompt`, `workflow`, `parameters`, `comfyassets_prompt_pairs`, -each `original-text`; Matroska upper-cased names fold on our side), then the old -VideoHelperSuite `comment` JSON envelope, then `description` (object values are -`reserialized-json`). The first carrier holding a role key wins, otherwise the first -holding a workflow, and carriers never merge. Two tags folding to one key are a -duplicate record: byte-identical ones are one record, differing ones fail the read. -ffprobe is asked for exactly those six tags and its output is bounded at the role -and workflow budgets plus 64 KiB of envelope (6 MiB + 64 KiB); past that the -inspector probes once more without the `workflow` tag within the role budget and -the per-key workflow becomes `limited`, while an envelope over budget still fails -the read because it cannot be split. Scans and the live monitor read the same -carrier with the default resolver profile, so a per-key video links like the same -graph in a PNG (`tests/test_utils_video_metadata.py`, `tests/test_api_admin.py` -`TestScanReadsVideos`, `tests/test_api_prompt_inspector.py` `test_video_*`; the +non-PNG header, 10,000 PNG chunks and a three-second ffprobe (per probe, at most two +probes). Video containers are read through one shared carrier selector +(`utils/video_metadata.select_video_carrier`, used by the inspector and by the admin +scan): carriers are, in order, the per-key tags that current savers write (`prompt`, +`workflow`, `parameters`, `comfyassets_prompt_pairs`, each `original-text`; Matroska +upper-cased names fold on our side), then the old VideoHelperSuite `comment` JSON +envelope, then `description` (object values are `reserialized-json`; envelope keys are +matched exactly, as the legacy readers did, never folded). The first carrier holding a +role key wins, otherwise the first holding a workflow, and carriers never merge. Two +per-key tags folding to one key are a duplicate record: byte-identical ones are one +record, differing ones fail the read. ffprobe is asked for exactly those six tags. +Its JSON output escapes tag text (at most doubling it), so the stdout bounds are sized +so that only the decoded per-record budgets above can fail or limit a record, and a +video reads exactly as the identical PNG: the first probe allows twice the role and +workflow budgets plus 64 KiB of envelope (12 MiB + 64 KiB); past that the inspector +probes once more without the `workflow` tag within twice the role budget plus 64 KiB +(4 MiB + 64 KiB) and the per-key workflow becomes `limited`, while an envelope over +that bound still fails the read because it cannot be split. The admin scan reads the +same carrier with the default resolver profile, so a per-key video links like the same +graph in a PNG; the live monitor processes images only (unchanged, see the follow-up +below) (`tests/test_utils_video_metadata.py`, `tests/test_api_admin.py` +`TestScanReadsVideos`, `tests/test_api_prompt_inspector.py` `test_video_*`, +`tests/test_inspection_response.py` `TestVideoProbeBoundsMatchPngBudgets`; the real-container round trips skip without ffmpeg). Resolution under `INSPECTION_PROFILE` bounds work, not graph size: 2,000 visits per candidate chain, 20,000 in total, 64 link/JSON-container levels, 100,000 resolved characters, and a @@ -1673,6 +1679,12 @@ responsive production Viewer keeps its toolbar in the media panel and prompt det mobile toolbar collision. These are explicit remaining visual differences, not a whole-application or full-gallery/Viewer pixel-parity claim. +Follow-up: monitor video support. The live image monitor (`utils/image_monitor.py`, +`GalleryConfig.SUPPORTED_EXTENSIONS`) processes images only; videos written by +ComfyUI savers reach the Library through the admin scan, which reads the shared video +carrier. Accepting `VIDEO_EXTENSIONS` in the monitor and routing them through +`read_video_metadata` is a separate, issue-linked change. + Preserve raw logs, failed traces, paired PNGs, geometry, runtime versions and source hashes before later managed commands clear their output directories. Frontend baseline updates are additive through `coverage:update` only after diff --git a/docs/prompt-inspector.md b/docs/prompt-inspector.md index 6b55d1f9..81366b7a 100644 --- a/docs/prompt-inspector.md +++ b/docs/prompt-inspector.md @@ -33,7 +33,8 @@ unresolved rather than becoming sendable placeholder text. Videos are read from one container carrier: the per-key tags written by core SaveVideo/SaveWEBM and current VideoHelperSuite (`prompt`, `workflow`, `parameters`, `comfyassets_prompt_pairs`), else the old VideoHelperSuite `comment` envelope, -else `description`. The scan reads the same carrier, so Images and ingestion -agree on a video's prompts (see TESTING.md for the order and budgets). +else `description`. The admin scan reads the same carrier, so Images and +ingestion agree on a video's prompts (see TESTING.md for the order and budgets; +the live monitor processes images only). Python route changes require restarting ComfyUI. Browser assets require a refresh. diff --git a/tests/test_api_admin.py b/tests/test_api_admin.py index e07e0c57..bddcc39a 100644 --- a/tests/test_api_admin.py +++ b/tests/test_api_admin.py @@ -2476,7 +2476,8 @@ def _write_mp4(path, comment): class TestScanReadsVideos(AdminAPITestCase): - """Videos saved by ComfyUI carry the graph in their comment tag.""" + """Videos carry the graph in per-key container tags (current savers) or in + the old VideoHelperSuite comment envelope; the scan reads one carrier.""" def setUp(self): super().setUp() diff --git a/tests/test_api_prompt_inspector.py b/tests/test_api_prompt_inspector.py index 3a84f42e..fcd92613 100644 --- a/tests/test_api_prompt_inspector.py +++ b/tests/test_api_prompt_inspector.py @@ -35,11 +35,13 @@ def graph(positive="portrait", negative="blurry"): } -def write_video(path, **tags): +def write_video(path, metadata_tags=True, **tags): """Tiny black clip carrying one container tag per keyword (needs ffmpeg). - MP4 keeps arbitrary keys only with ``use_metadata_tags``, the movflag the - core SaveVideo node sets; Matroska and WebM upper-case the key names. + MP4 keeps arbitrary keys only with ``use_metadata_tags`` (mdta atoms), the + movflag the core SaveVideo node sets; without it ffmpeg writes the legacy + ilst atoms that old VideoHelperSuite files carry. Matroska and WebM + upper-case the key names. """ command = [ "ffmpeg", @@ -50,7 +52,7 @@ def write_video(path, **tags): "-i", "color=c=black:s=16x16:d=0.2", ] - if path.suffix.lower() in (".mp4", ".mov", ".m4v"): + if metadata_tags and path.suffix.lower() in (".mp4", ".mov", ".m4v"): command += ["-movflags", "use_metadata_tags"] for key, value in tags.items(): command += ["-metadata", f"{key}={value}"] @@ -365,7 +367,7 @@ class TestPromptInspector(ImageAPITestCase): async def test_corrupt_truncated_duplicate_and_expanding_metadata_fail_closed(self): import struct import zlib - from utils.prompt_inspector import MAX_METADATA_BYTES + from utils.prompt_inspector import VIDEO_REPROBE_BYTES path = self.output_dir / "image.png" base = png(path, prompt=graph()).read_bytes() @@ -378,8 +380,8 @@ class TestPromptInspector(ImageAPITestCase): cases.append(bytes(broken)) for kind, payload in ( (b"tEXt", b"prompt\0{}"), - (b"zTXt", b"prompt\0\0" + zlib.compress(b"x" * (MAX_METADATA_BYTES + 1))), - (b"tEXt", b"prompt\0" + b"x" * (MAX_METADATA_BYTES + 1)), + (b"zTXt", b"prompt\0\0" + zlib.compress(b"x" * (VIDEO_REPROBE_BYTES + 1))), + (b"tEXt", b"prompt\0" + b"x" * (VIDEO_REPROBE_BYTES + 1)), (b"tEXt", b"bad-key-without-separator"), (b"zTXt", b"prompt\0\1bad-compression"), (b"iTXt", b"prompt\0\2\0\0\0text"), @@ -438,7 +440,7 @@ class TestPromptInspector(ImageAPITestCase): self.assertEqual(result["prompts"]["positive"]["status"], "absent") async def test_video_inspection_uses_bounded_container_evidence(self): - from utils.prompt_inspector import MAX_METADATA_BYTES, VIDEO_PROBE_BYTES + from utils.prompt_inspector import VIDEO_PROBE_BYTES, VIDEO_REPROBE_BYTES (self.output_dir / "clip.mp4").write_bytes(b"video fixture") metadata = {"prompt": graph("Video portrait", "")} @@ -467,7 +469,7 @@ class TestPromptInspector(ImageAPITestCase): # re-probe overflows too and the read fails instead of guessing. processes = [ probe_process(b"x" * (VIDEO_PROBE_BYTES + 1)), - probe_process(b"x" * (MAX_METADATA_BYTES + 1)), + probe_process(b"x" * (VIDEO_REPROBE_BYTES + 1)), ] with ( patch("utils.prompt_inspector.ffprobe_path", return_value="/tool/ffprobe"), @@ -1049,9 +1051,10 @@ class TestPromptInspector(ImageAPITestCase): {"prompt": "original-text", "workflow": "original-text"}, name, ) - # Old VideoHelperSuite envelope, unchanged. + # Old VideoHelperSuite envelope in the legacy ilst comment atom, unchanged. write_video( self.output_dir / "vhs.mp4", + metadata_tags=False, comment=json.dumps({"prompt": json.loads(source), "workflow": {"v": 1}}), ) _, result = await self.inspect("vhs.mp4") @@ -1063,7 +1066,7 @@ class TestPromptInspector(ImageAPITestCase): self.assertEqual(result["metadata"]["workflow_graph"]["value"], {"v": 1}) async def test_video_oversize_workflow_reprobes_role_only_and_marks_limited(self): - from utils.prompt_inspector import MAX_METADATA_BYTES, VIDEO_PROBE_BYTES + from utils.prompt_inspector import VIDEO_PROBE_BYTES, VIDEO_REPROBE_BYTES path = self.output_dir / "clip.mp4" path.write_bytes(b"fixture") @@ -1093,7 +1096,7 @@ class TestPromptInspector(ImageAPITestCase): "comment,description", ) self.assertEqual(processes[0].stdout.requested_size, VIDEO_PROBE_BYTES + 1) - self.assertEqual(processes[1].stdout.requested_size, MAX_METADATA_BYTES + 1) + self.assertEqual(processes[1].stdout.requested_size, VIDEO_REPROBE_BYTES + 1) for process in processes: self.assertTrue(process.stdout.closed) process.wait.assert_called() diff --git a/tests/test_inspection_response.py b/tests/test_inspection_response.py index 37de619e..936caa68 100644 --- a/tests/test_inspection_response.py +++ b/tests/test_inspection_response.py @@ -823,6 +823,88 @@ def _png_with_chunks(path, chunks): return path +class TestVideoProbeBoundsMatchPngBudgets(unittest.TestCase): + """ffprobe escapes tag text; the stdout bounds must never gate a record + that the identical PNG reads within its decoded budget (I3).""" + + @staticmethod + def _dense(size): + # Quote-dense JSON text: every other byte escapes, so ffprobe's JSON + # output is close to twice the decoded size. + unit = '{"k":"v",' + "".join('"' for _ in range(6)) + "}" + return (unit * (size // len(unit) + 1))[:size] + + def _video_read(self, path, tags, bound): + payload = json.dumps({"format": {"tags": tags}}).encode() + self.assertGreater(len(payload), bound) + process = MagicMock() + process.stdout = io.BytesIO(payload) + process.poll.return_value = 0 + process.returncode = 0 + with ( + patch("utils.prompt_inspector.ffprobe_path", return_value="/tool/ffprobe"), + patch( + "utils.prompt_inspector.subprocess.Popen", return_value=process + ) as spawn, + ): + read = read_inspection_file(path) + self.assertEqual(spawn.call_count, 1) + self.assertTrue(process.stdout.closed) + return read + + def test_near_budget_records_read_the_same_as_png(self): + from utils.prompt_inspector import ( + ROLE_METADATA_BYTES, + VIDEO_PROBE_BYTES, + VIDEO_REPROBE_BYTES, + WORKFLOW_METADATA_BYTES, + ) + + prompt = self._dense(ROLE_METADATA_BYTES - 1024) + workflow = self._dense(WORKFLOW_METADATA_BYTES - 1024) + with tempfile.TemporaryDirectory() as directory: + png = _png_with_chunks( + Path(directory) / "same.png", + [ + _text_chunk(b"tEXt", "prompt", prompt.encode("latin-1")), + _text_chunk(b"tEXt", "workflow", workflow.encode("latin-1")), + ], + ) + expected = read_inspection_file(png) + self.assertEqual( + expected.record_formats, + {"prompt": "original-text", "workflow": "original-text"}, + ) + video = Path(directory) / "same.mp4" + video.write_bytes(b"fixture") + # Escaped output is over the old 6 MiB + 64 KiB stdout bound. + read = self._video_read( + video, + {"prompt": prompt, "workflow": workflow}, + ROLE_METADATA_BYTES + WORKFLOW_METADATA_BYTES + 64 * 1024, + ) + self.assertEqual(read.metadata, expected.metadata) + self.assertEqual(read.record_formats, expected.record_formats) + self.assertEqual(read.limited_records, frozenset()) + self.assertGreaterEqual( + VIDEO_PROBE_BYTES, + 2 * (ROLE_METADATA_BYTES + WORKFLOW_METADATA_BYTES) + 64 * 1024, + ) + self.assertGreaterEqual( + VIDEO_REPROBE_BYTES, 2 * ROLE_METADATA_BYTES + 64 * 1024 + ) + # A role record near its budget alone escapes past 2 MiB and must + # still read as the PNG does. + png = _png_with_chunks( + Path(directory) / "role.png", + [_text_chunk(b"tEXt", "prompt", prompt.encode("latin-1"))], + ) + expected = read_inspection_file(png) + read = self._video_read(video, {"prompt": prompt}, ROLE_METADATA_BYTES) + self.assertEqual(read.metadata, expected.metadata) + self.assertEqual(read.record_formats, {"prompt": "original-text"}) + + class TestPerRecordBudgets(unittest.TestCase): """Role records keep the 2 MiB budget; a workflow has its own 4 MiB budget.""" diff --git a/tests/test_utils_video_metadata.py b/tests/test_utils_video_metadata.py index 5d04a860..1cacefa8 100644 --- a/tests/test_utils_video_metadata.py +++ b/tests/test_utils_video_metadata.py @@ -33,11 +33,13 @@ HAVE_FFMPEG = bool(shutil.which("ffmpeg") and shutil.which("ffprobe")) GRAPH = json.dumps({"1": {"class_type": "CLIPTextEncode", "inputs": {"text": "a cat"}}}) -def write_video(path, **tags): +def write_video(path, metadata_tags=True, **tags): """Write a tiny black video carrying one container tag per keyword. - MP4 keeps arbitrary keys only with ``use_metadata_tags``, the movflag the - core SaveVideo node sets; Matroska and WebM upper-case the key names. + MP4 keeps arbitrary keys only with ``use_metadata_tags`` (mdta atoms), the + movflag the core SaveVideo node sets; without it ffmpeg writes the legacy + ilst atoms (``comment`` becomes ``©cmt``) that old VideoHelperSuite files + carry. Matroska and WebM upper-case the key names. """ command = [ "ffmpeg", @@ -48,7 +50,7 @@ def write_video(path, **tags): "-i", "color=c=black:s=16x16:d=0.2", ] - if str(path).lower().endswith((".mp4", ".mov", ".m4v")): + if metadata_tags and str(path).lower().endswith((".mp4", ".mov", ".m4v")): command += ["-movflags", "use_metadata_tags"] for key, value in tags.items(): command += ["-metadata", f"{key}={value}"] @@ -56,8 +58,8 @@ def write_video(path, **tags): def write_mp4(path, comment): - """Write a tiny black video carrying ``comment`` in its container tags.""" - write_video(path, comment=comment) + """Write a tiny black MP4 with ``comment`` in its legacy ilst ``©cmt`` atom.""" + write_video(path, metadata_tags=False, comment=comment) class TestMetadataFromTags(unittest.TestCase): @@ -106,20 +108,29 @@ class TestSelectVideoCarrier(unittest.TestCase): formats, {"prompt": "original-text", "workflow": "original-text"} ) self.assertIsNone(carrier) - folded = {"COMMENT": json.dumps({"prompt": GRAPH})} - self.assertEqual(select_video_carrier(folded)[0], {"prompt": GRAPH}) + # Envelope keys are read exactly as legacy did: no fold, so a Matroska + # upper-cased COMMENT is not an envelope. + self.assertIsNone( + select_video_carrier({"COMMENT": json.dumps({"prompt": GRAPH})}) + ) def test_folded_duplicate_keys_fail(self): for tags in ( {"PROMPT": GRAPH, "prompt": json.dumps({"9": {}})}, {"Workflow": self.WORKFLOW, "WORKFLOW": "{}", "prompt": GRAPH}, - {"comment": json.dumps({"prompt": GRAPH}), "COMMENT": "{}"}, ): with self.assertRaises(DuplicateVideoTagError): select_video_carrier(tags) # Byte-identical duplicates are one record, as for PNG text chunks. metadata, _, _ = select_video_carrier({"PROMPT": GRAPH, "prompt": GRAPH}) self.assertEqual(metadata, {"prompt": GRAPH}) + # Envelope keys never fold, so a foreign COMMENT note is not a duplicate. + envelope = json.dumps({"prompt": GRAPH}) + metadata, _, carrier = select_video_carrier( + {"comment": envelope, "COMMENT": "a free-text note"} + ) + self.assertEqual(metadata, {"prompt": GRAPH}) + self.assertEqual(carrier, envelope) def test_comment_envelope_still_read(self): envelope = json.dumps({"prompt": GRAPH, "workflow": {"nodes": []}}) @@ -350,6 +361,29 @@ class TestReadVideoMetadataWithRealFfprobe(unittest.TestCase): name, ) + def test_legacy_ilst_comment_atom_is_read(self): + path = os.path.join(self.tmpdir, "vhs.mp4") + write_mp4(path, json.dumps({"prompt": GRAPH})) + probe = subprocess.run( + [ + shutil.which("ffprobe"), + "-v", + "quiet", + "-show_entries", + "format_tags", + "-of", + "json", + path, + ], + capture_output=True, + text=True, + check=True, + timeout=60, + ) + tags = json.loads(probe.stdout)["format"]["tags"] + self.assertIn("comment", tags) + self.assertEqual(read_video_metadata(path), {"prompt": GRAPH}) + def test_video_without_a_comment_is_empty(self): path = os.path.join(self.tmpdir, "plain.mp4") write_mp4(path, "") diff --git a/utils/prompt_inspector.py b/utils/prompt_inspector.py index d544751c..9c1bb08a 100644 --- a/utils/prompt_inspector.py +++ b/utils/prompt_inspector.py @@ -44,12 +44,16 @@ WORKFLOW_METADATA_BYTES = 4 * 1024 * 1024 # repeated chunks cost bounded I/O instead of file size. DUPLICATE_WORKFLOW_BYTES = 2 * WORKFLOW_METADATA_BYTES DUPLICATE_ROLE_BYTES = 2 * ROLE_METADATA_BYTES -# ffprobe stdout bounds: every carrier tag fits the role and workflow budgets -# plus 64 KiB of JSON envelope; past that only the per-key ``workflow`` tag can -# be dropped, so the re-probe asks for role and envelope tags within the role +# ffprobe stdout bounds. ffprobe JSON-escapes tag text (at most doubling it), +# and the decoded per-record budgets below are the only gate that may make a +# record fail or ``limited``: a video must read exactly as the identical PNG. +# So the first probe allows twice the role and workflow budgets plus 64 KiB of +# JSON envelope; past that only the per-key ``workflow`` tag can be dropped, +# and the re-probe asks for role and envelope tags within twice the role # budget (an envelope cannot be split, so an over-budget one fails the read). MAX_METADATA_BYTES = ROLE_METADATA_BYTES -VIDEO_PROBE_BYTES = ROLE_METADATA_BYTES + WORKFLOW_METADATA_BYTES + 64 * 1024 +VIDEO_PROBE_BYTES = 2 * (ROLE_METADATA_BYTES + WORKFLOW_METADATA_BYTES) + 64 * 1024 +VIDEO_REPROBE_BYTES = 2 * ROLE_METADATA_BYTES + 64 * 1024 VIDEO_PROBE_SECONDS = 3 _VIDEO_ROLE_PROBE_KEYS = ROLE_TAG_KEYS + ENVELOPE_TAG_KEYS MAX_KEYWORD_BYTES = 80 # a 1-79 byte PNG keyword plus its NUL separator @@ -344,7 +348,9 @@ def _video_metadata(path: Path) -> _VideoInspection: limited = frozenset() raw = _probe_video_tags(probe, path, VIDEO_TAG_KEYS, VIDEO_PROBE_BYTES) if raw is None: - raw = _probe_video_tags(probe, path, _VIDEO_ROLE_PROBE_KEYS, MAX_METADATA_BYTES) + raw = _probe_video_tags( + probe, path, _VIDEO_ROLE_PROBE_KEYS, VIDEO_REPROBE_BYTES + ) if raw is None: raise MetadataReadError("Video metadata exceeds inspection limits") # Only the per-key workflow tag was left out, so it is the record over diff --git a/utils/video_metadata.py b/utils/video_metadata.py index f3fe6d5b..382560de 100644 --- a/utils/video_metadata.py +++ b/utils/video_metadata.py @@ -60,19 +60,23 @@ class DuplicateVideoTagError(ValueError): def _folded_tags(tags: dict) -> dict: - """Known tags keyed by ASCII-lowercased name. + """Known tags keyed by their carrier name. - Matroska muxers upper-case tag names and ffprobe reports them as written, - so the fold happens here. Two tags folding to one key are a duplicate - record: byte-identical duplicates are one record (as for PNG text chunks), - differing ones fail the read. + Per-key prompt carriers fold ASCII case: Matroska muxers upper-case tag + names and ffprobe reports them as written. Two tags folding to one key are + a duplicate record: byte-identical duplicates are one record (as for PNG + text chunks), differing ones fail the read. Envelope keys (``comment``, + ``description``) are read exactly as the legacy readers did, never folded. """ folded = {} for key, value in tags.items(): if not isinstance(key, str): continue + if key in ENVELOPE_TAG_KEYS: + folded[key] = value + continue name = key.lower() - if name not in VIDEO_TAG_KEYS: + if name not in METADATA_TAG_KEYS: continue if name in folded and folded[name] != value: raise DuplicateVideoTagError(f"Duplicate video metadata tag: {name}")