feat(effects): add canvas treatments layered transforms and mask tracking - #169
Open
DonIsmaelito wants to merge 10 commits into
Open
DonIsmaelito wants to merge 10 commits into
DonIsmaelito wants to merge 10 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.
34 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/effects.py">
<violation number="1" location="helpers/effects.py:412">
P2: When `shots` does not cover the full picture clock, this loop advances `index` past the end of the list and the next `manifest["shots"][index]` access raises IndexError. A manifest without `shots` (or with a trailing gap after the last shot's `end_frame`) fails mid-encode with a raw traceback. Validate before encoding that shots exist and that the final shot's `end_frame` reaches `manifest["total_frames"]`.</violation>
<violation number="2" location="helpers/effects.py:450">
P2: When a shot declares `source_start`, `source_frame`, or `speed`, `stage_mapped` accepts those controls but always reads from native frame zero and uses only `time_map`. Apply the source origin and speed when deriving `targets`, or reject these fields here so manifests cannot silently produce the wrong retiming.</violation>
<violation number="3" location="helpers/effects.py:499">
P2: When `source_crop` extends past the source frame, `Image.crop` pads the missing region with black instead of rejecting it, so retimed output silently contains black borders. Check `x + w` and `y + h` against `image.size` before cropping.</violation>
</file>
<file name="helpers/prepare_source.py">
<violation number="1" location="helpers/prepare_source.py:25">
P2: When callers pass an explicitly empty crop sequence, `if crop:` skips validation and emits an uncropped derivative instead of rejecting the invalid rectangle. Check `crop is not None` so only the default `None` disables cropping.</violation>
</file>
<file name="helpers/visuals.py">
<violation number="1" location="helpers/visuals.py:104">
P3: Negative pixel offsets such as `x: -100` are inside `_scaled_pixel_value`'s `0.0 <= numeric <= 1.0` check, so they are scaled and then clamped: `max(1.000001, -66.7)` returns `1.000001`, turning a requested negative offset into pixel 1 in previews. The clamp is meant to protect small positive strokes; it should not rewrite negative coordinates. Only clamp non-negative scaled values.</violation>
<violation number="2" location="helpers/visuals.py:213">
P2: When a reframe is used without a canvas, the filter can shrink the actual output by two pixels per axis while the API reports the fallback dimensions. Preserve the source dimensions exactly or return the dimensions produced by the reframe filter.</violation>
<violation number="3" 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="4" location="helpers/visuals.py:403">
P2: When `initial - minimum` is odd, this loop never tests the declared minimum size, so text that fits only at that size raises `ValueError`. Iterate through the minimum size inclusively.</violation>
<violation number="5" 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>
<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>
<violation number="2" location="helpers/project_state.py:62">
P2: When a recorded entry names an unknown dependency, `record()` raises an unhandled `KeyError` before dependency validation runs. Validate each dependency before indexing it so the CLI reports a clear project-state error.</violation>
<violation number="3" location="helpers/project_state.py:98">
P2: When an entry JSON contains an `id` field, `view()` emits that value instead of the recorded artifact identifier, so consumers can associate evidence with the wrong artifact. Spread `row` before assigning `id: ident`, or reject `id` as a reserved field.</violation>
</file>
<file name="helpers/edit_clock.py">
<violation number="1" location="helpers/edit_clock.py:25">
P2: When a non-positive frame or sample rate reaches these helpers, they return invalid boundaries or fail with an internal `ZeroDivisionError`. Validate every `fps` and `rate` as a finite positive value before converting clocks.</violation>
<violation number="2" location="helpers/edit_clock.py:57">
P2: When `total` is a float such as `10.0`, `allocate_frames` crashes with a slice `TypeError` instead of reporting an invalid frame budget. Validate `total` as a non-boolean integer before the allocation arithmetic.</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="helpers/cards.py">
<violation number="1" location="helpers/cards.py:144">
P3: An 8-digit RGBA hex (for example "#FFFFFF80") makes ImageColor.getrgb return a 4-tuple, so `(*color, 255)` builds a 5-tuple and Image.new raises TypeError instead of honoring the alpha. The gradient path hits the same wall: an RGBA "to" color produces a 4-channel array that Image.fromarray(..., "RGB") rejects. Since the shared reference doc states colors accept RGBA hex values, either support alpha explicitly or reject it with a clear message; the current failure is an opaque crash.</violation>
<violation number="2" location="helpers/cards.py:298">
P2: A negative or fractional `entry_frames` is accepted as a duration, so malformed card metadata silently runs the entry animation for the wrong number of frames. Validate it as a nonnegative integer before computing `progress`.</violation>
<violation number="3" location="helpers/cards.py:420">
P3: write_movie passes `-y` (overwrite output) to ffmpeg right after the FileExistsError pre-checks. The overwrite guarantee is then only as strong as the check-to-spawn race: if the path appears after the pre-check, ffmpeg silently clobbers it, contradicting the documented "must not replace existing output" contract that the tests (test_movie_preserves_existing_files) assert. Drop the flag so ffmpeg itself errors instead of overwriting.</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>
<violation number="2" location="helpers/edit_io.py:95">
P2: When command output contains nested JSON, `last_json` returns the innermost child rather than the complete measurement object. Track the decodable candidate with the furthest closing position and prefer its earliest opening brace on ties.</violation>
</file>
<file name="tests/test_effects.py">
<violation number="1" location="tests/test_effects.py:7">
P2: cv2 is part of the optional editing extra (per the PR description), but this module imports it at the top unconditionally, so running the suite on the documented base deps (numpy/pillow/etc., no opencv) fails at collection with ModuleNotFoundError instead of skipping. Sibling tests guard this: tests/test_sources.py uses `cv2 = pytest.importorskip("cv2", reason="install the editing extra")`, and tests/test_sources.py, tests/test_caption_raster.py, tests/test_cards.py skip ffmpeg-dependent bodies with `shutil.which` checks. Apply the same guards here (the `test_retime_and_composite_encoded_frames` body calls ffmpeg/ffprobe directly via subprocess).</violation>
</file>
<file name="helpers/source_scan.py">
<violation number="1" location="helpers/source_scan.py:79">
P1: When a source contains multiple video streams, `selected_frames()` can decode a different stream than `catalog()` indexed because the FFmpeg command relies on automatic stream selection. Add `-map 0:v:0` so selected pixels and catalog PTS always refer to the same video stream.</violation>
<violation number="2" location="helpers/source_scan.py:142">
P1: When `--out` is a hard-link alias of the source, this path check passes and `save_json()` truncates the source through the shared inode. Reject existing aliases with `Path.samefile()` before writing the catalog.</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/find_shot.py">
<violation number="1" location="helpers/find_shot.py:50">
P2: When `--every nan` is supplied, the positive-interval check passes and `search` silently samples only the first frame. Reject non-finite sampling intervals before iterating.</violation>
<violation number="2" location="helpers/find_shot.py:74">
P2: When an exact match has `reprojection_px == 0.0`, the sort treats it as missing and ranks it last among equal-inlier candidates. Test explicitly for `None` instead of truthiness.</violation>
<violation number="3" location="helpers/find_shot.py:89">
P1: When `--out` is an existing hard link to the query or source, this guard misses the alias and `save_json` destroys the input. Check existing outputs with `Path.samefile` as well as resolved-path equality.</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:308">
P2: When `stroke_width` is nonzero, `_fit_text` measures only fill glyphs, so captions can exceed the configured `max_width` after drawing. Compute stroke width before fitting and include it in wrapping and width checks.</violation>
<violation number="3" 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/track_mask.py">
<violation number="1" location="helpers/track_mask.py:89">
P2: Collinear or duplicate vertices pass this validation and produce a zero-area seed instead of being rejected as an invalid polygon. Reject zero shoelace area before optical flow.</violation>
<violation number="2" location="helpers/track_mask.py:103">
P2: When a caller passes a NumPy polygon to `track()` and writes its result with `save_json`, the seed row raises `TypeError`; store the normalized Python list instead.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| "-threads", | ||
| "2", | ||
| "-i", | ||
| str(path), |
There was a problem hiding this comment.
P1: When a source contains multiple video streams, selected_frames() can decode a different stream than catalog() indexed because the FFmpeg command relies on automatic stream selection. Add -map 0:v:0 so selected pixels and catalog PTS always refer to the same video stream.
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 79:
<comment>When a source contains multiple video streams, `selected_frames()` can decode a different stream than `catalog()` indexed because the FFmpeg command relies on automatic stream selection. Add `-map 0:v:0` so selected pixels and catalog PTS always refer to the same video stream.</comment>
<file context>
@@ -0,0 +1,151 @@
+ "-threads",
+ "2",
+ "-i",
+ str(path),
+ "-an",
+ "-sn",
</file context>
Suggested change
| str(path), | |
| str(path), | |
| "-map", | |
| "0:v:0", |
| parser.add_argument("--out", required=True) | ||
| parser.add_argument("--scenes", action="store_true") | ||
| args = parser.parse_args() | ||
| if Path(args.source).resolve() == Path(args.out).resolve(): |
There was a problem hiding this comment.
P1: When --out is a hard-link alias of the source, this path check passes and save_json() truncates the source through the shared inode. Reject existing aliases with Path.samefile() before writing the catalog.
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 142:
<comment>When `--out` is a hard-link alias of the source, this path check passes and `save_json()` truncates the source through the shared inode. Reject existing aliases with `Path.samefile()` before writing the catalog.</comment>
<file context>
@@ -0,0 +1,151 @@
+ parser.add_argument("--out", required=True)
+ parser.add_argument("--scenes", action="store_true")
+ args = parser.parse_args()
+ if Path(args.source).resolve() == Path(args.out).resolve():
+ parser.error("output cannot replace source")
+ data = catalog(args.source)
</file context>
Suggested change
| if Path(args.source).resolve() == Path(args.out).resolve(): | |
| if Path(args.source).resolve() == Path(args.out).resolve() or ( | |
| Path(args.out).exists() and Path(args.source).samefile(args.out) | |
| ): |
| p.add_argument("--every", type=float, default=1) | ||
| p.add_argument("--out", required=True) | ||
| a = p.parse_args() | ||
| if Path(a.out).resolve() in (Path(a.query).resolve(), Path(a.source).resolve()): |
There was a problem hiding this comment.
P1: When --out is an existing hard link to the query or source, this guard misses the alias and save_json destroys the input. Check existing outputs with Path.samefile as well as resolved-path equality.
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 89:
<comment>When `--out` is an existing hard link to the query or source, this guard misses the alias and `save_json` destroys the input. Check existing outputs with `Path.samefile` as well as resolved-path equality.</comment>
<file context>
@@ -0,0 +1,95 @@
+ p.add_argument("--every", type=float, default=1)
+ p.add_argument("--out", required=True)
+ a = p.parse_args()
+ if Path(a.out).resolve() in (Path(a.query).resolve(), Path(a.source).resolve()):
+ p.error("output would overwrite input")
+ save_json(a.out, search(a.query, a.source, a.every))
</file context>
Suggested change
| if Path(a.out).resolve() in (Path(a.query).resolve(), Path(a.source).resolve()): | |
| output = Path(a.out).resolve() | |
| inputs = (Path(a.query).resolve(), Path(a.source).resolve()) | |
| if output in inputs or any( | |
| output.exists() and input_path.exists() and output.samefile(input_path) | |
| for input_path in inputs | |
| ): |
| image = Image.fromarray(cv2.cvtColor(bgr, cv2.COLOR_BGR2RGB)) | ||
| cropped = image | ||
| if shot.get("source_crop"): | ||
| x, y, w, h = shot["source_crop"] |
There was a problem hiding this comment.
P2: When source_crop extends past the source frame, Image.crop pads the missing region with black instead of rejecting it, so retimed output silently contains black borders. Check x + w and y + h against image.size before cropping.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/effects.py, line 499:
<comment>When `source_crop` extends past the source frame, `Image.crop` pads the missing region with black instead of rejecting it, so retimed output silently contains black borders. Check `x + w` and `y + h` against `image.size` before cropping.</comment>
<file context>
@@ -0,0 +1,517 @@
+ image = Image.fromarray(cv2.cvtColor(bgr, cv2.COLOR_BGR2RGB))
+ cropped = image
+ if shot.get("source_crop"):
+ x, y, w, h = shot["source_crop"]
+ cropped = image.crop((x, y, x + w, y + h))
+ writer.stdin.write(
</file context>
| np.interp(np.arange(count), points[:, 0], points[:, 1]) + 0.5 | ||
| ).astype(int) | ||
| reader = cv2.VideoCapture(str(source_path(manifest, root, shot["source"]))) | ||
| index = -1 |
There was a problem hiding this comment.
P2: When a shot declares source_start, source_frame, or speed, stage_mapped accepts those controls but always reads from native frame zero and uses only time_map. Apply the source origin and speed when deriving targets, or reject these fields here so manifests cannot silently produce the wrong retiming.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/effects.py, line 450:
<comment>When a shot declares `source_start`, `source_frame`, or `speed`, `stage_mapped` accepts those controls but always reads from native frame zero and uses only `time_map`. Apply the source origin and speed when deriving `targets`, or reject these fields here so manifests cannot silently produce the wrong retiming.</comment>
<file context>
@@ -0,0 +1,517 @@
+ np.interp(np.arange(count), points[:, 0], points[:, 1]) + 0.5
+ ).astype(int)
+ reader = cv2.VideoCapture(str(source_path(manifest, root, shot["source"])))
+ index = -1
+ image = None
+ with path.with_suffix(".log").open("wb") as log:
</file context>
| "ffmpeg", | ||
| "-v", | ||
| "error", | ||
| "-y", |
There was a problem hiding this comment.
P3: write_movie passes -y (overwrite output) to ffmpeg right after the FileExistsError pre-checks. The overwrite guarantee is then only as strong as the check-to-spawn race: if the path appears after the pre-check, ffmpeg silently clobbers it, contradicting the documented "must not replace existing output" contract that the tests (test_movie_preserves_existing_files) assert. Drop the flag so ffmpeg itself errors instead of overwriting.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/cards.py, line 420:
<comment>write_movie passes `-y` (overwrite output) to ffmpeg right after the FileExistsError pre-checks. The overwrite guarantee is then only as strong as the check-to-spawn race: if the path appears after the pre-check, ffmpeg silently clobbers it, contradicting the documented "must not replace existing output" contract that the tests (test_movie_preserves_existing_files) assert. Drop the flag so ffmpeg itself errors instead of overwriting.</comment>
<file context>
@@ -0,0 +1,482 @@
+ "ffmpeg",
+ "-v",
+ "error",
+ "-y",
+ "-threads",
+ "2",
</file context>
| raise ValueError("invalid caption dimensions") | ||
| mask = mask.resize((width, height), Image.Resampling.LANCZOS) | ||
| color = ImageColor.getrgb(line.get("color", "#ffffff")) | ||
| layer = Image.new("RGBA", mask.size, (*color, 255)) |
There was a problem hiding this comment.
P3: An 8-digit RGBA hex (for example "#FFFFFF80") makes ImageColor.getrgb return a 4-tuple, so (*color, 255) builds a 5-tuple and Image.new raises TypeError instead of honoring the alpha. The gradient path hits the same wall: an RGBA "to" color produces a 4-channel array that Image.fromarray(..., "RGB") rejects. Since the shared reference doc states colors accept RGBA hex values, either support alpha explicitly or reject it with a clear message; the current failure is an opaque crash.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/cards.py, line 144:
<comment>An 8-digit RGBA hex (for example "#FFFFFF80") makes ImageColor.getrgb return a 4-tuple, so `(*color, 255)` builds a 5-tuple and Image.new raises TypeError instead of honoring the alpha. The gradient path hits the same wall: an RGBA "to" color produces a 4-channel array that Image.fromarray(..., "RGB") rejects. Since the shared reference doc states colors accept RGBA hex values, either support alpha explicitly or reject it with a clear message; the current failure is an opaque crash.</comment>
<file context>
@@ -0,0 +1,482 @@
+ raise ValueError("invalid caption dimensions")
+ mask = mask.resize((width, height), Image.Resampling.LANCZOS)
+ color = ImageColor.getrgb(line.get("color", "#ffffff"))
+ layer = Image.new("RGBA", mask.size, (*color, 255))
+ if line.get("gradient"):
+ end = np.array(ImageColor.getrgb(line["gradient"]["to"]), float)
</file context>
Suggested change
| layer = Image.new("RGBA", mask.size, (*color, 255)) | |
| color = ImageColor.getrgb(line.get("color", "#ffffff"))[:3] |
| 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) |
| if 0.0 <= numeric <= 1.0: | ||
| return value | ||
| scaled = numeric * scale | ||
| return max(1.000001, scaled) |
There was a problem hiding this comment.
P3: Negative pixel offsets such as x: -100 are inside _scaled_pixel_value's 0.0 <= numeric <= 1.0 check, so they are scaled and then clamped: max(1.000001, -66.7) returns 1.000001, turning a requested negative offset into pixel 1 in previews. The clamp is meant to protect small positive strokes; it should not rewrite negative coordinates. Only clamp non-negative scaled values.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/visuals.py, line 104:
<comment>Negative pixel offsets such as `x: -100` are inside `_scaled_pixel_value`'s `0.0 <= numeric <= 1.0` check, so they are scaled and then clamped: `max(1.000001, -66.7)` returns `1.000001`, turning a requested negative offset into pixel 1 in previews. The clamp is meant to protect small positive strokes; it should not rewrite negative coordinates. Only clamp non-negative scaled values.</comment>
<file context>
@@ -0,0 +1,617 @@
+ if 0.0 <= numeric <= 1.0:
+ return value
+ scaled = numeric * scale
+ return max(1.000001, scaled)
+
+
</file context>
Suggested change
| return max(1.000001, scaled) | |
| return max(1.000001, scaled) if scaled > 0 else scaled |
| 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") |
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
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.