feat(effects): add canvas treatments layered transforms and mask tracking - #169
Open
DonIsmaelito wants to merge 15 commits into
Open
DonIsmaelito wants to merge 15 commits into
DonIsmaelito wants to merge 15 commits into
Conversation
DonIsmaelito
marked this pull request as ready for review
September 17, 2026 22:03
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
10 issues found across 25 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="helpers/project_state.py">
<violation number="1" location="helpers/project_state.py:57">
P2: `record()` accepts absolute and escaping artifact paths even though project artifacts are documented as context-relative. Reject absolute paths and paths outside `context.parent` before hashing, and enforce the same constraint when viewing stored contexts.</violation>
</file>
<file name="tests/test_caption_raster.py">
<violation number="1" location="tests/test_caption_raster.py:199">
P3: `test_ffmpeg_preserves_short_cue_boundaries` runs two external processes (ffmpeg encode and ffprobe probe) with no `timeout`, so a wedged or slow FFmpeg build hangs the whole test suite with no recovery. Add `timeout=60` (or similar) to both `subprocess.run` calls, matching `helpers/caption_raster.py:461` which already uses `timeout=15` for its ffmpeg probe.</violation>
</file>
<file name="references/effects.md">
<violation number="1" location="references/effects.md:9">
P3: This says the PR adds no assets or fonts, but the repository ships Alfa Slab One and this API uses it as a fallback. Document the bundled font instead so setup, packaging, and licensing guidance is not contradictory.</violation>
</file>
<file name="tests/test_sources.py">
<violation number="1" location="tests/test_sources.py:262">
P3: The subprocess exit-code assertion here can pass for the wrong reason. Any unrelated CLI failure — a missing import, an exception inside `catalog`, or an argparse error — also yields a non-zero return code, so `test_catalog_cli_cannot_overwrite_source` would stay green even if the `source == out` guard were removed (only `sha256(source) == digest` would catch that regression). Pin the exit code to the actual rejection: `parser.error` exits with status 2, so assert `== 2`, or assert the stderr message.</violation>
</file>
<file name="helpers/edit_io.py">
<violation number="1" location="helpers/edit_io.py:10">
P3: When ffprobe/ffmpeg stalls on a corrupt or otherwise unresponsive source, `run()` never returns because `subprocess.run` has no `timeout`. The slowest path is `probe(path, frames=True)` (used on the full FFV1 copy in prepare_source.py), which decodes every frame via `-count_frames`, so a hang blocks the whole agent session. Add an optional `timeout` parameter (default `None` to preserve current callers) and pass it through to `subprocess.run`; bind it for probe/decode calls.</violation>
</file>
<file name="helpers/caption_raster.py">
<violation number="1" location="helpers/caption_raster.py:285">
P2: When `font_size` and `min_font_size` have opposite parity, the `-2` loop skips the declared minimum and can reject captions that fit within the configured limits. Include `minimum_size` explicitly in the candidate sizes.</violation>
<violation number="2" location="helpers/caption_raster.py:386">
P2: When the caption output path contains backslashes, `_quote_concat_path` leaves FFconcat escape characters unescaped and the consumer cannot open the generated images. Escape backslashes before applying the apostrophe quoting.</violation>
</file>
<file name="tests/test_visuals.py">
<violation number="1" location="tests/test_visuals.py:254">
P2: test_canvas_filters_encode invokes ffmpeg unconditionally, so on machines where ffmpeg is absent the suite fails with FileNotFoundError instead of skipping. Every other ffmpeg-dependent test in this suite (test_sources.py, test_cards.py, test_caption_raster.py) guards with `shutil.which` + `pytest.skip`. Add the same guard before building the filter graph.</violation>
</file>
<file name="helpers/visuals.py">
<violation number="1" location="helpers/visuals.py:222">
P3: An 8-digit `#RRGGBBAA` canvas color is silently truncated to `#RRGGBB`, dropping the alpha it the caller provided, instead of being rejected as the function's message states ("canvas colors must use #RRGGBB"). Reject 8-digit values so spec errors surface at build time rather than producing an unintended opaque background.</violation>
<violation number="2" location="helpers/visuals.py:549">
P3: `Image.thumbnail` never enlarges an image, so an image graphic declared wider/taller than its source (including fraction sizes such as `"width": 0.5`) renders at the source's native size instead of the requested size, silently. Use `source.resize(...)` for exact requested dimensions, or clamp with explicit `source.width`/`source.height` checks if upscaling is intentionally disallowed.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| target_height = _pixel(spec.get("height"), height, source.height) | ||
| if target_width <= 0 or target_height <= 0: | ||
| raise ValueError("image graphic dimensions must be positive") | ||
| source.thumbnail((target_width, target_height), Image.Resampling.LANCZOS) |
There was a problem hiding this comment.
P3: Image.thumbnail never enlarges an image, so an image graphic declared wider/taller than its source (including fraction sizes such as "width": 0.5) renders at the source's native size instead of the requested size, silently. Use source.resize(...) for exact requested dimensions, or clamp with explicit source.width/source.height checks if upscaling is intentionally disallowed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/visuals.py, line 549:
<comment>`Image.thumbnail` never enlarges an image, so an image graphic declared wider/taller than its source (including fraction sizes such as `"width": 0.5`) renders at the source's native size instead of the requested size, silently. Use `source.resize(...)` for exact requested dimensions, or clamp with explicit `source.width`/`source.height` checks if upscaling is intentionally disallowed.</comment>
<file context>
@@ -0,0 +1,617 @@
+ target_height = _pixel(spec.get("height"), height, source.height)
+ if target_width <= 0 or target_height <= 0:
+ raise ValueError("image graphic dimensions must be positive")
+ source.thumbnail((target_width, target_height), Image.Resampling.LANCZOS)
+ x = _pixel(spec.get("x"), width, 0.5) - source.width // 2
+ y = _pixel(spec.get("y"), height, 0.5) - source.height // 2
</file context>
Suggested change
| source.thumbnail((target_width, target_height), Image.Resampling.LANCZOS) | |
| if source.size != (target_width, target_height): | |
| source = source.resize((target_width, target_height), Image.Resampling.LANCZOS) |
| def _ffmpeg_color(value: Any, default: str = "000000") -> str: | ||
| text = str(value or default).strip().lstrip("#") | ||
| if len(text) == 8: | ||
| text = text[:6] |
There was a problem hiding this comment.
P3: An 8-digit #RRGGBBAA canvas color is silently truncated to #RRGGBB, dropping the alpha it the caller provided, instead of being rejected as the function's message states ("canvas colors must use #RRGGBB"). Reject 8-digit values so spec errors surface at build time rather than producing an unintended opaque background.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/visuals.py, line 222:
<comment>An 8-digit `#RRGGBBAA` canvas color is silently truncated to `#RRGGBB`, dropping the alpha it the caller provided, instead of being rejected as the function's message states ("canvas colors must use #RRGGBB"). Reject 8-digit values so spec errors surface at build time rather than producing an unintended opaque background.</comment>
<file context>
@@ -0,0 +1,617 @@
+def _ffmpeg_color(value: Any, default: str = "000000") -> str:
+ text = str(value or default).strip().lstrip("#")
+ if len(text) == 8:
+ text = text[:6]
+ if len(text) != 6 or not re.fullmatch(r"[0-9a-fA-F]{6}", text):
+ raise ValueError("canvas colors must use #RRGGBB")
</file context>
Suggested change
| text = text[:6] | |
| if len(text) == 8: | |
| raise ValueError("canvas colors must use #RRGGBB") |
Author
|
@cubic-dev-ai please review the latest commits again. Correct negative preview positions and zoom dimensions, validate retiming and shot coverage, and reject degenerate polygons and incompatible matte clocks. Inherits #168. Validation: 129 branch tests passed. The combined core preview passed 765 tests with two optional skips. Existing threads remain open for rechecking; this follow-up does not claim every previous finding is resolved. |
@DonIsmaelito I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 existing issues remain and 28 new issues found across 26 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="helpers/find_shot.py">
<violation number="1" location="helpers/find_shot.py:61">
P2: On variable-frame-rate sources, this re-anchors the sampling grid from each selected frame's PTS, so lateness accumulates and later frames can be skipped. Advance from a fixed initial-PTS grid so `--every` remains a wall-clock sampling interval.</violation>
</file>
<file name="helpers/edit_clock.py">
<violation number="1" location="helpers/edit_clock.py:103">
P2: When `total_frames` is `3.0`, `check_partition` accepts the shot, then `render_layers` reaches `range(3.0)` and fails after creating output files. Reject non-integer, nonnegative totals before comparing coverage.</violation>
</file>
<file name="helpers/source_scan.py">
<violation number="1" location="helpers/source_scan.py:97">
P3: `-fps_mode` only exists in FFmpeg 6.0+; on distro FFmpeg 5.x (Debian 12, Ubuntu 22.04 LTS — both `apt-get install ffmpeg` targets documented in install.md) every invocation of `selected_frames`, `catalog`-adjacent encoding, and `prepare()` fails with "Unrecognized option 'fps_mode'". The project already depends on zscale/tonemap, so document a minimum FFmpeg version in install.md and either require it or use the long-standing `-vsync passthrough` spelling, which all supported builds accept.</violation>
<violation number="2" location="helpers/source_scan.py:106">
P3: FFmpeg's stderr is discarded, so when a read ends short the caller only sees the generic "source ended before native frame N" and cannot tell whether the source was truncated, corrupt, or the filter expression rejected the stream. Capture stderr (or print it) before terminating, and include it in the `ValueError` so the agent can diagnose rejected sources.</violation>
<violation number="3" location="helpers/source_scan.py:118">
P2: When FFmpeg fails after emitting the requested frames, `selected_frames` accepts the images because the `finally` block discards the decoder exit status. Check the return code after normal completion, while preserving the current cleanup behavior for intentionally truncated generator consumption.</violation>
</file>
<file name="helpers/caption_raster.py">
<violation number="1" location="helpers/caption_raster.py:73">
P2: When any SRT block has a malformed timestamp, `parse_srt` raises and aborts the entire caption track instead of skipping that block. Catch the timestamp parsing error and continue with the remaining cues.</violation>
</file>
<file name="helpers/cards.py">
<violation number="1" location="helpers/cards.py:185">
P2: A polygon matte with fewer than three points or zero area is accepted and rasterized, so card occlusion can silently disappear or cover the wrong pixels. Validate each polygon’s finite coordinates and nonzero area before drawing it.</violation>
<violation number="2" location="helpers/cards.py:207">
P2: Non-pattern mattes accept clock offsets that the renderer ignores, allowing a declared matte clock to disagree with the rows actually used. Reject `first_index` for files and polygons, and reject `start_frame` for static files.</violation>
</file>
<file name="helpers/prepare_source.py">
<violation number="1" location="helpers/prepare_source.py:19">
P3: When the source has no video stream, this bare `next()` raises `StopIteration` instead of a readable error, leaving a traceback that mentions neither the file nor the reason. `source_scan.catalog()` handles the same case with a `ValueError("no video stream in ...")`. Guard with the default-arg pattern and raise a clear `ValueError` the CLI/agent can act on.</violation>
<violation number="2" location="helpers/prepare_source.py:32">
P2: When the source has a 90°/270° display-matrix rotation, these checks use coded dimensions although FFmpeg applies `crop` after autorotation. A valid display-space crop is rejected, while a coded-dimension crop can pass validation and then fail in FFmpeg; validate against the post-rotation dimensions or explicitly control autorotation.</violation>
<violation number="3" location="helpers/prepare_source.py:66">
P2: For sources with a nonzero start PTS, `-fps_mode passthrough` preserves only the rebased timestamps, so prepared-media times no longer match the cataloged source times and time-based shot ranges can select the wrong frames. Preserve the input timestamps with `-copyts` or explicitly record and apply the normalization when deriving shot ranges.</violation>
</file>
<file name="helpers/visuals.py">
<violation number="1" location="helpers/visuals.py:136">
P2: When a treatment omits `canvas`, a downscaled preview keeps the original output dimensions while scaling its graphics and captions. Materialize a scaled canvas from the fallback dimensions so preview overlays remain aligned and the preview is actually bounded.</violation>
</file>
<file name="helpers/project_state.py">
<violation number="1" location="helpers/project_state.py:37">
P2: When a context entry has a non-list `depends_on`, `validate()` crashes or builds the wrong graph instead of rejecting the context cleanly. Validate that dependencies are a list of artifact IDs before traversing them.</violation>
</file>
<file name="helpers/track_mask.py">
<violation number="1" location="helpers/track_mask.py:71">
P2: When callers pass a stacked NumPy frame array, `if not frames` raises before tracking starts. Check `len(frames) == 0` instead.</violation>
<violation number="2" location="helpers/track_mask.py:92">
P2: `track()` accepts polygons with overlapping or touching nonadjacent edges because `self_intersects()` ignores collinear contacts. Reject non-simple polygons, including collinear overlaps and nonadjacent vertex touches, before tracking.</violation>
<violation number="3" location="helpers/track_mask.py:148">
P2: When optical flow collapses vertices without failing its point checks, the tracker emits a degenerate matte as approved. Apply the nonzero-area check to every propagated polygon before writing `needs_review`.</violation>
<violation number="4" location="helpers/track_mask.py:164">
P2: The tracking document does not carry its 30-fps clock, so downstream matte consumers cannot reject a composition with a different fps. Include the source clock in the document and validate it before frame-based matte lookup.</violation>
<violation number="5" location="helpers/track_mask.py:191">
P2: A premature decode failure is accepted as normal EOF, allowing truncated shots to produce apparently complete tracking output. Validate the expected frame count or decoder state and fail when the shot is not fully covered.</violation>
</file>
<file name="pyproject.toml">
<violation number="1" location="pyproject.toml:16">
P2: The new `test` extra is not reflected in the committed `uv.lock` (provides-extras lists only ["animations", "editing"] and no pytest package is locked), so the lockfile is out of date with this pyproject change. A later `uv sync --locked` or `uv run --locked --extra test pytest` fails with a lock-out-of-date error, and plain `uv sync`/`uv run` silently rewrites the committed lock. Regenerate the lock after adding the extra.</violation>
</file>
<file name="tests/test_visuals.py">
<violation number="1" location="tests/test_visuals.py:91">
P3: test_reframe_filter_encodes_zoom_and_focus never asserts the crop part computed after zoom (crop=trunc(iw/1.080000/2)*2:trunc(ih/1.080000/2)*2), which is the even-dimension piece of the zoom fix. Add assertions on that substring so a regression in zoom crop dimensions is caught.</violation>
</file>
<file name="helpers/effects.py">
<violation number="1" location="helpers/effects.py:175">
P2: A foreground layer cannot declare the supported explicit `fit: "contain"` setting because `validate_layers` omits `fit` from its allowlist. Add `fit` to the allowed layer fields so layer source-window validation matches the schema.</violation>
<violation number="2" location="helpers/effects.py:224">
P2: The default `fill_opacity` disagrees between validation and rendering: `validate_layers` validates the absent value as 0, but `LayerCompositor.frame()` renders it as 1 (`curve(layer.get("fill_opacity", 1), n)`). A layer with `"fill"` and no `fill_opacity` is therefore drawn fully filled, while validation implies the default is no fill. Make both defaults agree so validation describes what will render.</violation>
<violation number="3" location="helpers/effects.py:337">
P2: When a layer fill uses an RGBA color such as `#ff000080`, validation accepts it but `LayerCompositor.frame` appends a second alpha and raises while constructing the filled image. Normalize the parsed color to RGB before adding the compositor alpha.</violation>
<violation number="4" location="helpers/effects.py:418">
P2: Shot-level `effects` are applied at encode time without ever being validated. `validate_effects` is only called for layers (`validate_layers`), never for `manifest["shots"][...].effects` before `render_layers` starts encoding, so an invalid shot effect (NaN scale, out-of-range samples, malformed curve) surfaces as a mid-encode failure that leaves a partial output file, or silently produces wrong frames. Validate every shot's effects before opening the encoder.</violation>
<violation number="5" location="helpers/effects.py:444">
P2: When a shot has a non-integer or out-of-bounds frame interval, `stage_mapped` still computes a count and can write a retimed output. Validate `start_frame` and `end_frame` against the manifest clock before deriving `count`.</violation>
<violation number="6" location="helpers/effects.py:484">
P3: `stage_mapped` encodes libx264 with `yuv420p`, which fails when either picture dimension is odd, while the rest of the pipeline (rawvideo input resolution and the `render_layers` ffv1/bgr0 output) tolerates odd dimensions. A manifest with an odd picture width or height will therefore fail only at encode time, leaving a partial retimed file. Pad to even dimensions or validate picture evenness before opening the retime encoder.</violation>
</file>
<file name="tests/test_track_mask.py">
<violation number="1" location="tests/test_track_mask.py:74">
P3: The subprocess invocation resolves `helpers/track_mask.py` (and `missing.mp4`) against the current working directory, so the test passes only when pytest is launched from the repo root. Since `Path` is already imported but unused, use it to anchor the paths to `__file__` so the test is cwd-independent.</violation>
<violation number="2" location="tests/test_track_mask.py:75">
P3: The matte-clock rejection (exact-30-fps check in main()) has no coverage: the only CLI test short-circuits at the output-exclusivity check before decoding. The review follow-up explicitly asks to validate this path, and the degenerate-polygon cases are covered while this one is not. Add a test that feeds a non-30-fps source and asserts the "30 fps" ValueError, matching the pattern of test_cli_preserves_seed.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| for row in index["frames"]: | ||
| if row["pts"] >= next_time: | ||
| selected.append(row["frame"]) | ||
| next_time = row["pts"] + every |
There was a problem hiding this comment.
P2: On variable-frame-rate sources, this re-anchors the sampling grid from each selected frame's PTS, so lateness accumulates and later frames can be skipped. Advance from a fixed initial-PTS grid so --every remains a wall-clock sampling interval.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/find_shot.py, line 61:
<comment>On variable-frame-rate sources, this re-anchors the sampling grid from each selected frame's PTS, so lateness accumulates and later frames can be skipped. Advance from a fixed initial-PTS grid so `--every` remains a wall-clock sampling interval.</comment>
<file context>
@@ -0,0 +1,100 @@
+ for row in index["frames"]:
+ if row["pts"] >= next_time:
+ selected.append(row["frame"])
+ next_time = row["pts"] + every
+ with Image.open(query) as im:
+ reference = im.convert("RGB")
</file context>
Suggested change
| next_time = row["pts"] + every | |
| next_time = index["frames"][0]["pts"] + ( | |
| math.floor( | |
| (row["pts"] - index["frames"][0]["pts"]) / every | |
| ) | |
| + 1 | |
| ) * every |
| f'non-contiguous or invalid shot: {shot.get("id", "unknown")}' | ||
| ) | ||
| cursor = end | ||
| if cursor != total: |
There was a problem hiding this comment.
P2: When total_frames is 3.0, check_partition accepts the shot, then render_layers reaches range(3.0) and fails after creating output files. Reject non-integer, nonnegative totals before comparing coverage.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/edit_clock.py, line 103:
<comment>When `total_frames` is `3.0`, `check_partition` accepts the shot, then `render_layers` reaches `range(3.0)` and fails after creating output files. Reject non-integer, nonnegative totals before comparing coverage.</comment>
<file context>
@@ -0,0 +1,104 @@
+ f'non-contiguous or invalid shot: {shot.get("id", "unknown")}'
+ )
+ cursor = end
+ if cursor != total:
+ raise ValueError(f"shots cover {cursor} frames, expected {total}")
</file context>
Suggested change
| if cursor != total: | |
| if type(total) is not int or total < 0: | |
| raise ValueError("total frame count must be a nonnegative integer") | |
| if cursor != total: |
| process.stdout.close() | ||
| if process.poll() is None: | ||
| process.terminate() | ||
| process.wait() |
There was a problem hiding this comment.
P2: When FFmpeg fails after emitting the requested frames, selected_frames accepts the images because the finally block discards the decoder exit status. Check the return code after normal completion, while preserving the current cleanup behavior for intentionally truncated generator consumption.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/source_scan.py, line 118:
<comment>When FFmpeg fails after emitting the requested frames, `selected_frames` accepts the images because the `finally` block discards the decoder exit status. Check the return code after normal completion, while preserving the current cleanup behavior for intentionally truncated generator consumption.</comment>
<file context>
@@ -0,0 +1,163 @@
+ process.stdout.close()
+ if process.poll() is None:
+ process.terminate()
+ process.wait()
+
+
</file context>
Comment on lines
+73
to
+74
| start = parse_srt_timestamp(start_text) | ||
| end = parse_srt_timestamp(end_text) |
There was a problem hiding this comment.
P2: When any SRT block has a malformed timestamp, parse_srt raises and aborts the entire caption track instead of skipping that block. Catch the timestamp parsing error and continue with the remaining cues.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/caption_raster.py, line 73:
<comment>When any SRT block has a malformed timestamp, `parse_srt` raises and aborts the entire caption track instead of skipping that block. Catch the timestamp parsing error and continue with the remaining cues.</comment>
<file context>
@@ -0,0 +1,514 @@
+ if len(lines) < 3 or " --> " not in lines[1]:
+ continue
+ start_text, end_text = lines[1].split(" --> ", 1)
+ start = parse_srt_timestamp(start_text)
+ end = parse_srt_timestamp(end_text)
+ text = " ".join(lines[2:]).strip()
</file context>
Suggested change
| start = parse_srt_timestamp(start_text) | |
| end = parse_srt_timestamp(end_text) | |
| try: | |
| start = parse_srt_timestamp(start_text) | |
| end = parse_srt_timestamp(end_text) | |
| except ValueError: | |
| continue |
| "feather", | ||
| }: | ||
| raise ValueError("unsupported matte fields") | ||
| kinds = set(spec) & {"file", "pattern", "polygons"} |
There was a problem hiding this comment.
P2: Non-pattern mattes accept clock offsets that the renderer ignores, allowing a declared matte clock to disagree with the rows actually used. Reject first_index for files and polygons, and reject start_frame for static files.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/cards.py, line 207:
<comment>Non-pattern mattes accept clock offsets that the renderer ignores, allowing a declared matte clock to disagree with the rows actually used. Reject `first_index` for files and polygons, and reject `start_frame` for static files.</comment>
<file context>
@@ -0,0 +1,525 @@
+ "feather",
+ }:
+ raise ValueError("unsupported matte fields")
+ kinds = set(spec) & {"file", "pattern", "polygons"}
+ if len(kinds) != 1:
+ raise ValueError("matte needs exactly one file pattern or polygon sequence")
</file context>
Suggested change
| kinds = set(spec) & {"file", "pattern", "polygons"} | |
| kinds = set(spec) & {"file", "pattern", "polygons"} | |
| if "first_index" in spec and "pattern" not in kinds: | |
| raise ValueError("first_index is only valid for matte patterns") | |
| if "start_frame" in spec and "file" in kinds: | |
| raise ValueError("start_frame is only valid for sequenced mattes") |
| [ | ||
| sys.executable, | ||
| "helpers/track_mask.py", | ||
| "missing.mp4", |
There was a problem hiding this comment.
P3: The matte-clock rejection (exact-30-fps check in main()) has no coverage: the only CLI test short-circuits at the output-exclusivity check before decoding. The review follow-up explicitly asks to validate this path, and the degenerate-polygon cases are covered while this one is not. Add a test that feeds a non-30-fps source and asserts the "30 fps" ValueError, matching the pattern of test_cli_preserves_seed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_track_mask.py, line 75:
<comment>The matte-clock rejection (exact-30-fps check in main()) has no coverage: the only CLI test short-circuits at the output-exclusivity check before decoding. The review follow-up explicitly asks to validate this path, and the degenerate-polygon cases are covered while this one is not. Add a test that feeds a non-30-fps source and asserts the "30 fps" ValueError, matching the pattern of test_cli_preserves_seed.</comment>
<file context>
@@ -0,0 +1,96 @@
+ [
+ sys.executable,
+ "helpers/track_mask.py",
+ "missing.mp4",
+ str(seed),
+ "--out",
</file context>
| if out.suffix.lower() != ".mkv": | ||
| raise ValueError("lossless prepared sources use a Matroska mkv file") | ||
| before = probe(source) | ||
| video = next(s for s in before["streams"] if s["codec_type"] == "video") |
There was a problem hiding this comment.
P3: When the source has no video stream, this bare next() raises StopIteration instead of a readable error, leaving a traceback that mentions neither the file nor the reason. source_scan.catalog() handles the same case with a ValueError("no video stream in ..."). Guard with the default-arg pattern and raise a clear ValueError the CLI/agent can act on.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/prepare_source.py, line 19:
<comment>When the source has no video stream, this bare `next()` raises `StopIteration` instead of a readable error, leaving a traceback that mentions neither the file nor the reason. `source_scan.catalog()` handles the same case with a `ValueError("no video stream in ...")`. Guard with the default-arg pattern and raise a clear `ValueError` the CLI/agent can act on.</comment>
<file context>
@@ -0,0 +1,112 @@
+ if out.suffix.lower() != ".mkv":
+ raise ValueError("lossless prepared sources use a Matroska mkv file")
+ before = probe(source)
+ video = next(s for s in before["streams"] if s["codec_type"] == "video")
+ hdr = video.get("color_transfer") in ("smpte2084", "arib-std-b67")
+ if hdr and not tonemap:
</file context>
Suggested change
| video = next(s for s in before["streams"] if s["codec_type"] == "video") | |
| video = next( | |
| (s for s in before["streams"] if s["codec_type"] == "video"), None | |
| ) | |
| if video is None: | |
| raise ValueError(f"no video stream in {source}") |
| "-threads", | ||
| "2", | ||
| "-pix_fmt", | ||
| "yuv420p", |
There was a problem hiding this comment.
P3: stage_mapped encodes libx264 with yuv420p, which fails when either picture dimension is odd, while the rest of the pipeline (rawvideo input resolution and the render_layers ffv1/bgr0 output) tolerates odd dimensions. A manifest with an odd picture width or height will therefore fail only at encode time, leaving a partial retimed file. Pad to even dimensions or validate picture evenness before opening the retime encoder.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/effects.py, line 484:
<comment>`stage_mapped` encodes libx264 with `yuv420p`, which fails when either picture dimension is odd, while the rest of the pipeline (rawvideo input resolution and the `render_layers` ffv1/bgr0 output) tolerates odd dimensions. A manifest with an odd picture width or height will therefore fail only at encode time, leaving a partial retimed file. Pad to even dimensions or validate picture evenness before opening the retime encoder.</comment>
<file context>
@@ -0,0 +1,524 @@
+ "-threads",
+ "2",
+ "-pix_fmt",
+ "yuv420p",
+ "-video_track_timescale",
+ str(fps.numerator * 512),
</file context>
| "pipe:1", | ||
| ], | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.DEVNULL, |
There was a problem hiding this comment.
P3: FFmpeg's stderr is discarded, so when a read ends short the caller only sees the generic "source ended before native frame N" and cannot tell whether the source was truncated, corrupt, or the filter expression rejected the stream. Capture stderr (or print it) before terminating, and include it in the ValueError so the agent can diagnose rejected sources.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/source_scan.py, line 106:
<comment>FFmpeg's stderr is discarded, so when a read ends short the caller only sees the generic "source ended before native frame N" and cannot tell whether the source was truncated, corrupt, or the filter expression rejected the stream. Capture stderr (or print it) before terminating, and include it in the `ValueError` so the agent can diagnose rejected sources.</comment>
<file context>
@@ -0,0 +1,163 @@
+ "pipe:1",
+ ],
+ stdout=subprocess.PIPE,
+ stderr=subprocess.DEVNULL,
+ )
+ try:
</file context>
Comment on lines
+97
to
+98
| "-fps_mode", | ||
| "passthrough", |
There was a problem hiding this comment.
P3: -fps_mode only exists in FFmpeg 6.0+; on distro FFmpeg 5.x (Debian 12, Ubuntu 22.04 LTS — both apt-get install ffmpeg targets documented in install.md) every invocation of selected_frames, catalog-adjacent encoding, and prepare() fails with "Unrecognized option 'fps_mode'". The project already depends on zscale/tonemap, so document a minimum FFmpeg version in install.md and either require it or use the long-standing -vsync passthrough spelling, which all supported builds accept.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/source_scan.py, line 97:
<comment>`-fps_mode` only exists in FFmpeg 6.0+; on distro FFmpeg 5.x (Debian 12, Ubuntu 22.04 LTS — both `apt-get install ffmpeg` targets documented in install.md) every invocation of `selected_frames`, `catalog`-adjacent encoding, and `prepare()` fails with "Unrecognized option 'fps_mode'". The project already depends on zscale/tonemap, so document a minimum FFmpeg version in install.md and either require it or use the long-standing `-vsync passthrough` spelling, which all supported builds accept.</comment>
<file context>
@@ -0,0 +1,163 @@
+ "1",
+ "-vf",
+ f"select='{expression}',scale={width}:{height}",
+ "-fps_mode",
+ "passthrough",
+ "-f",
</file context>
Suggested change
| "-fps_mode", | |
| "passthrough", | |
| "-vsync", | |
| "passthrough", |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Edits need control over framing, graphic placement and moving foregrounds. These helpers let the agent fit footage to a canvas, animate layers and follow a manually selected subject while flagging frames that need correction.
Builds on #168 for shared mask utilities and its #164 prerequisite.
Changes
Add canvas treatments and transparent text, shape and image layers without deleting existing outputs.
Add keyframed transforms, masked layers and explicit source-frame retiming. Intermediate videos honor the declared frame rate.
Track manually seeded polygons in both directions and flag uncertain spans for review.
Organize the change into focused feature and review commits with usage documentation. All 129 branch tests pass, including 39 Effects cases with real encoded output and measured tracking. No new media assets or fonts are bundled.
Review follow-up: Correct negative preview positions and zoom dimensions, validate retiming and shot coverage, and reject degenerate polygons and incompatible matte clocks. Inherits feat(captions): add styled images and measured word cards #168.
Limits
These are standalone helpers; main-renderer integration follows in the Rendering PR. OpenCV uses the optional editing extra, and encoding requires FFmpeg. Retiming samples existing frames without optical-flow interpolation or audio retiming. Moving layers must be staged and read sequentially.
Tracking holds grayscale frames in memory and is not automatic subject segmentation. Inspect and correct masks before use. Failed runs can leave partial output files; retry in a new location.
Canvas and reframing capabilities overlap #153, #157, #158, #104, #43 and #63. This PR does not change their legacy renderer paths or adopt their EDL fields.