feat(render): reuse unchanged parts of an edit - #201
Open
DonIsmaelito wants to merge 49 commits into
Open
DonIsmaelito wants to merge 49 commits into
DonIsmaelito wants to merge 49 commits into
Conversation
…ames in the skill contract
…nd guard hard rule thirteen
…audio events in captions and tidy docs
# Conflicts: # SKILL.md
# Conflicts: # pyproject.toml
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
60 issues found across 65 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/visuals.py">
<violation number="1" location="helpers/visuals.py:573">
P1: This guard makes graphics unusable for a second output: after the first render writes `graphic_000.png`, rerendering with a new delivery path and every `--all-deliverables` run fails with `FileExistsError`. Allocate a fresh generated-overlay directory per output (or safely replace only the helper's own generated artifacts).</violation>
</file>
<file name="helpers/render_cache.py">
<violation number="1" location="helpers/render_cache.py:16">
P3: `file_digest` is a byte-for-byte copy of `edit_io.sha256` (same 1 MB bounded-memory chunked hashing, same helpers directory). Reuse the existing helper instead of keeping two implementations that can drift.</violation>
<violation number="2" location="helpers/render_cache.py:92">
P3: Cache-hit copies are published with mode 0600 because `tempfile.mkstemp` creates the temp file owner-only, `copyfile` keeps its mode, and `os.replace` preserves it. Freshly rendered clips (written by ffmpeg / extract_segment) get the normal umask-derived mode, so reused clips become unreadable to other users or tools in a shared edit directory. Restore the source's permission bits after copying.</violation>
<violation number="3" location="helpers/render_cache.py:143">
P2: `restore` replaces `destination` before this source-state check, so a source mutation makes the call fail while leaving stale cached bytes at the build path. Copy to a temporary destination and commit only after validation.</violation>
</file>
<file name="helpers/effects.py">
<violation number="1" location="helpers/effects.py:220">
P3: This branch is unreachable: when `layer["source"]` is missing from `manifest["sources"]`, the earlier `source = manifest["sources"].get(...)` returns `None` and already raises "layer needs an independently sourced render source with provenance". Delete these two lines (and consider `manifest.get("sources", {})` in the earlier lookup so a manifest without a `sources` key raises a clear ValueError instead of KeyError).</violation>
<violation number="2" location="helpers/effects.py:223">
P2: `ImageColor.getrgb` accepts RGBA color strings, but validation does not reject the four-channel result; this line then appends another alpha and Pillow raises on a five-tuple. Reject alpha-bearing fills during validation or handle RGBA explicitly.</violation>
<violation number="3" location="helpers/effects.py:448">
P2: `cut_list.validate` allows a `time_map` shot to spell out `speed: 1`, but this check rejects any explicit `speed` key, so a valid manifest fails during staging. Reject only non-default speed values.</violation>
</file>
<file name="helpers/cut_list.py">
<violation number="1" location="helpers/cut_list.py:66">
P3: `validate()` dereferences required manifest keys (`total_frames`, `shots`, `canvas`, `picture`, `sources`) with `[]` and calls `check_partition` before checking shot fields, so malformed input (missing key, or shots left in uncompiled `weight`/`frames` form) dies with a bare `KeyError` rather than the function's own actionable `ValueError` convention. Validate presence/types up front and run `check_partition` after shot-field checks.</violation>
<violation number="2" location="helpers/cut_list.py:133">
P2: An explicit `speed: 1` passes this condition, but `stage_mapped()` rejects any `speed` key with a `time_map`. Reject the key by presence, or normalize it away before staging.</violation>
<violation number="3" location="helpers/cut_list.py:159">
P2: Check that `clip["source"]` exists in `manifest["sources"]` independently of file probing; `check_files=False` should skip filesystem access, not permit dangling audio references.</violation>
<violation number="4" location="helpers/cut_list.py:257">
P2: Validate `minimum_rms_dbfs` as a finite numeric threshold here; otherwise malformed music-presence settings survive composition validation and crash final verification.</violation>
<violation number="5" location="helpers/cut_list.py:263">
P2: This whitelist does not validate delivery values as finite numbers, so malformed targets pass `validate()` and trigger a late `TypeError` in normalization or verification. Validate the numeric ranges before staging.</violation>
</file>
<file name="helpers/render.py">
<violation number="1" location="helpers/render.py:772">
P3: In `_load_track_keyframes`, a `subject_box`/`box` object missing `x`, `y`, `width`, or `height` raises a raw `KeyError` because the surrounding `except` clause only catches `TypeError` and `ValueError`. The intent (per the catch block) is that malformed keyframes fail with the actionable `ValueError` about numeric time/center_x/center_y. Add `KeyError` to the except clause.</violation>
<violation number="2" location="helpers/render.py:777">
P2: The timestamp check accepts `inf`, causing `_piecewise_track_expression` to emit an invalid `inf` FFmpeg expression and fail late during rendering. Require `math.isfinite(timestamp)` in this guard.</violation>
<violation number="3" location="helpers/render.py:1121">
P2: This branch skips protected-region checks for legacy overlays, even though `rect is None` means full-frame behavior and only cutaways are exempt. Treat a legacy overlay as `{x: 0, y: 0, width: 1, height: 1}` before testing the intersection.</violation>
<violation number="4" location="helpers/render.py:1598">
P2: This output-preservation guard runs after segment extraction, concatenation, and subtitle preparation. Check the final output path before `extract_all_segments` so reruns fail immediately instead of spending expensive media work first.</violation>
<violation number="5" location="helpers/render.py:1683">
P2: `--no-subtitles` no longer skips invalid or missing subtitle declarations for version-2 EDLs. Validate a copy with `subtitles` and `captions` removed when `args.no_subtitles` is set, then continue validating the original render inputs.</violation>
<violation number="6" location="helpers/render.py:1698">
P2: An existing declared deliverable blocks an explicit `-o` override because normalization checks the declared path before `main` selects the override. Skip the declared-path collision check when an explicit output is supplied, or pass the selected output into normalization.</violation>
</file>
<file name="helpers/source_scan.py">
<violation number="1" location="helpers/source_scan.py:63">
P2: `selected_frames` computes its output aspect ratio from coded dimensions even though this FFmpeg command auto-rotates display-matrix inputs before `scale`. Portrait phone footage encoded as landscape is therefore stretched into landscape thumbnails, which can corrupt scene-difference and screenshot-matching results; derive dimensions after rotation or disable autorotation explicitly.</violation>
<violation number="2" location="helpers/source_scan.py:106">
P3: `selected_frames` discards ffmpeg's stderr, so a truncated or corrupt source surfaces only as "source ended before native frame N" with no underlying ffmpeg diagnostic. Pipe stderr and include its start in the raised ValueError so decode failures are diagnosable.</violation>
</file>
<file name="helpers/verify_edit.py">
<violation number="1" location="helpers/verify_edit.py:180">
P3: `verify()` loads the full decoded PCM of the output (and the master/music stems) into memory and decodes the whole output up to four times (`probe -count_frames`, per-frame ffprobe, `decode_audio`, `-f null`). For hour-long deliverables this is multiple GB of RAM and redundant decode work that the windowed alignment and tail checks don't need. Consider decoding only the samples near the verification windows (e.g. via ffmpeg `-t`/`-ss` ranges or true streaming) and deduplicating the frame-count/timestamp probes.</violation>
<violation number="2" location="helpers/verify_edit.py:204">
P2: The verifier ignores the declared `delivery.true_peak` unless the undocumented `max_final_true_peak` is also set. Use `true_peak` as the fallback, and enforce the stricter limit when both fields are present.</violation>
<violation number="3" location="helpers/verify_edit.py:225">
P2: Do not downmix stereo by averaging before correlation: valid opposite-polarity channels can cancel completely, making the verifier reject an aligned master. Compare channels independently or select a non-silent channel/window for alignment.</violation>
</file>
<file name="helpers/edl.py">
<violation number="1" location="helpers/edl.py:186">
P3: Oversized JSON dimensions can raise an uncaught `OverflowError` during the integer check. Catch the conversion failure and report an `EDLValidationError` so hostile or malformed EDL input cannot crash validation with a traceback.</violation>
<violation number="2" location="helpers/edl.py:354">
P3: `sample_rate = int(audio.get("sample_rate_hz", 48_000))` silently truncates fractional rates: `int(48000.5)` becomes 48000, so a request for 48000.5 Hz passes the `sample_rate != 48_000` check and is silently downgraded to 48000. That contradicts the module's own no-silent-truncation rule (the `_dimensions` check rejects fractional values, and `tests/test_delivery_review.py::test_fractional_dimensions` guards exactly this). Verify the value is whole before converting, or accept only non-fractional numeric input.</violation>
<violation number="3" location="helpers/edl.py:461">
P2: Version-3 validation fails for normal package imports because this top-level import depends on callers mutating `sys.path`. Use an import path that works for both `helpers.edl` and the renderer’s script-style imports.</violation>
<violation number="4" location="helpers/edl.py:564">
P2: Tracked reframe validation only checks that the keyframe list is non-empty. Validate each keyframe’s numeric time, increasing order, and normalized coordinates here, otherwise malformed tracking spends the full extraction cost before failing in the renderer.</violation>
</file>
<file name="helpers/caption_raster.py">
<violation number="1" location="helpers/caption_raster.py:73">
P2: A malformed timestamp aborts the entire `parse_srt` call instead of skipping that malformed block, so one bad cue prevents all captions from rendering. Catch the timestamp parse error around both parses and continue to the next block.</violation>
<violation number="2" location="helpers/caption_raster.py:93">
P2: `_words_in_range` drops word records without `type`, although the EDL and shared word loader treat missing type as a normal word. Building subtitles from such a validated transcript silently produces missing captions; accept an omitted type here too.</violation>
<violation number="3" location="helpers/caption_raster.py:207">
P2: This raise breaks the render pipeline's re-render flow: `helpers/render.py` calls `build_master_srt(edl, edit_dir, subs_path)` with the fixed path `edit_dir / "master.srt"` on every `--build-subtitles` render and never removes the file first (grep of `render.py` shows no unlink for master.srt). The pr-base implementation overwrote the existing file, so a second render on the same edit dir now aborts — exactly the repeated-render scenario this PR targets. Similarly `build_caption_track` rejects a non-empty `output_dir`, while `render.py` line 1205 passes the same `edit_dir / "overlays/captions"` directory each time, failing the second PIL-caption render. Clear the previous artifacts at the call site, or make these guards opt-out for the renderer's internal reuse.</violation>
<violation number="4" location="helpers/caption_raster.py:423">
P2: A failed cue render leaves `output_dir` non-empty, so the next build fails the empty-directory guard before it can recover. Render into a temporary directory and publish it only after the complete FFconcat track succeeds, or clean up the partial directory on failure.</violation>
</file>
<file name="helpers/cards.py">
<violation number="1" location="helpers/cards.py:70">
P2: For `kind="circle"`, `radius = width / angle` grows without bound as `angle_degrees` approaches 0, yet the validation only requires `0 < angle_degrees < 180`. A small legal angle (e.g. 0.5°) makes `Image.new("L", ...)` allocate height ≈ 2*radius ≈ 230× the text width — a crafted or accidental tight-arc value can exhaust memory instead of failing with a clear layout error. Bound the arc angle (e.g. require `1 <= angle_degrees <= 179`) or cap the computed radius/output size.</violation>
<violation number="2" location="helpers/cards.py:176">
P2: Polygon masks are reparsed and linearly searched for every rendered frame, making longer tracked-caption renders increasingly slow. Cache parsed polygon rows by path (and reuse static mask images) before looking up the requested frame.</violation>
</file>
<file name="helpers/map_transcript.py">
<violation number="1" location="helpers/map_transcript.py:21">
P3: `map_words` and `compare_words` dereference `transcript["words"]` / `observed["words"]` directly. A JSON document that parses but has no `words` key (an empty `{}` is produced by real transcription runs for silent takes) crashes with a bare `KeyError` traceback instead of an actionable message. Validate the loaded documents and raise a `ValueError` naming the missing key and file.</violation>
<violation number="2" location="helpers/map_transcript.py:132">
P2: The compare command silently turns a manifest with no voice clips into an all-deletions report. Reject a missing voice interval before filtering the ASR, otherwise invalid or muted projects are presented as transcript disagreement.</violation>
</file>
<file name="helpers/captions.py">
<violation number="1" location="helpers/captions.py:57">
P2: `_as_word` turns reversed timestamps into valid-looking captions. Reject `end < start` before applying the short-duration fallback for zero-length words, otherwise corrupted alignment data produces misleading caption timing.</violation>
<violation number="2" location="helpers/captions.py:132">
P2: `chunk_words` silently changes contract based on one optional flag: with `break_on_punctuation` supplied it delegates to `caption_raster.chunk_words` (returns `list[list[dict]]`, default `max_words=2`, and `max_characters` is dropped); without it, it returns `(start, end, text)` tuples with a default of 6 words. A caller toggling the flag gets different types and different grouping rules, and `max_characters` is ignored on the delegation path. Return one shape from both paths and honor `max_characters` everywhere, or expose the two behaviors as separate functions.</violation>
<violation number="3" location="helpers/captions.py:249">
P2: The alignment CLI uses the platform default encoding, so non-ASCII speech can fail before ASS generation on non-UTF-8 locales. Read the JSON with `encoding="utf-8"` to match the UTF-8 output and the other caption APIs.</violation>
</file>
<file name="helpers/track_mask.py">
<violation number="1" location="helpers/track_mask.py:24">
P2: `self_intersects` only detects proper crossings; non-adjacent edges that touch or overlap return false because the orientation products become zero. `track` can therefore accept a non-simple polygon and emit an ambiguous matte; reject inclusive segment intersections too.</violation>
<violation number="2" location="helpers/track_mask.py:185">
P3: A missing or unreadable input is reported as a frame-rate violation: `cv2.VideoCapture` on a bad file yields FPS 0, so this check raises "prepared picture must use the composition clock of 30 fps", obscuring that the file does not exist. Check `cap.isOpened()` first and report the missing/undecodable input, then perform the strict 30 fps check.</violation>
</file>
<file name="tests/test_visuals.py">
<violation number="1" location="tests/test_visuals.py:254">
P2: `test_canvas_filters_encode` calls `subprocess.run(["ffmpeg", ...], check=True)` without a skip guard, so on any machine or CI agent without ffmpeg the whole test module errors (FileNotFoundError) instead of skipping. Every other ffmpeg-dependent test in this repo guards first — e.g. `tests/test_render_treatment.py:55` uses a module-level `pytest.mark.skipif(not shutil.which("ffmpeg")...)`, and `tests/test_render_reuse_media.py:23` uses `if not shutil.which("ffmpeg"): pytest.skip(...)`. Add the same guard, and pass `timeout=` to the subprocess call so a hung ffmpeg doesn't stall the suite.</violation>
</file>
<file name="tests/test_sources.py">
<violation number="1" location="tests/test_sources.py:185">
P2: `test_state_rejects_missing_or_cyclic_dependencies` passes for the wrong reason: `validate` checks `row["sha256"]` and raises "context artifact needs a sha256 fingerprint" (helpers/project_state.py) before it ever inspects `depends_on`. The `[</violation>
</file>
<file name="helpers/mix_audio.py">
<violation number="1" location="helpers/mix_audio.py:260">
P2: The standalone mixer validates clip filters, gains, and fades only inside the processing loop, after creating the output directory and starting FFmpeg work. Prevalidate these per-clip settings in the first validation loop so malformed manifests fail before expensive decoding and output staging.</violation>
</file>
<file name="helpers/edit_clock.py">
<violation number="1" location="helpers/edit_clock.py:103">
P2: `check_partition` accepts non-integer frame totals such as `10.0` because numeric equality succeeds, then direct rendering fails later at `range(manifest["total_frames"])` with a `TypeError`. Reject non-integer totals before comparing the partition length.</violation>
</file>
<file name="tests/test_skill_contract.py">
<violation number="1" location="tests/test_skill_contract.py:41">
P2: `hard_rules_section` only matches when the Hard Rules section is followed by another `## ` heading. If an agent editing SKILL.md moves Hard Rules to the end of the file (the very change this guard exists to police), the regex fails to match and every test in the module errors with the misleading message "SKILL.md must contain a '## Hard Rules' section" even though the section exists. Allow the section to terminate at EOF.</violation>
</file>
<file name="tests/test_captions.py">
<violation number="1" location="tests/test_captions.py:21">
P3: This file duplicates the first ~165 lines of tests/test_caption_raster.py, which is added by the same PR: the nine test bodies are identical apart from the import target (`helpers.captions` facade vs `helpers.caption_raster`). Since captions.py just re-exports the raster functions and thin-wraps `chunk_words`/`choose_caption_renderer`, every assertion here runs the same behavior already covered by test_caption_raster.py. Keep one canonical test file (the raster one, which also holds the FFmpeg and error-case coverage) and drop the duplicate, or restrict this file to facade-specific cases such as `chunk_words` with `break_on_punctuation=None` that the raster module does not exercise.</violation>
</file>
<file name="helpers/prepare_source.py">
<violation number="1" location="helpers/prepare_source.py:84">
P2: If ffmpeg fails after creating the output file (disk full, killed process, filter error), the partial `.mkv` stays on disk while the command log is written, and every later `prepare` call raises FileExistsError until the file is deleted by hand. Clean up the partial output in the failure path before re-raising, keeping the log for diagnosis.</violation>
</file>
<file name="pyproject.toml">
<violation number="1" location="pyproject.toml:17">
P3: The new `test` extra is missing from the committed uv.lock: the lock's `video-use` entry declares `provides-extras = ["animations", "editing"]` and no pytest package appears anywhere in uv.lock, even though `uv sync` on the default install never pulls it. `uv sync --extra test --locked` therefore fails with a lockfile-out-of-date error, and the committed lock claims extras that do not match pyproject.toml. Regenerate and commit the lock with the test extra included (e.g. `uv lock --extra test`).</violation>
<violation number="2" location="pyproject.toml:17">
P3: The `test` extra installs only pytest, yet the OpenCV-backed tests (test_effects.py, test_track_mask.py, plus parts of test_composition.py, test_sources.py, test_source_review.py) all reach `pytest.importorskip("cv2")`. A plain `pip install -e '.[test]'` installs no OpenCV and silently skips the whole editing-backed suite, so the declared test extra cannot exercise it. Either add the editing dependency to the test extra, or document that the test suite requires `--extra editing`.</violation>
</file>
<file name="references/render-reuse.md">
<violation number="1" location="references/render-reuse.md:25">
P3: `subtle` is a built-in grade (helpers/grade.py PRESETS) but is missing from the cacheable whitelist in helpers/render.py, so a `subtle`-graded EDL never reuses clips. The doc claims "Built-in grades and auto-grade are supported" for reuse; either add `subtle` to the whitelist or qualify the claim.</violation>
</file>
<file name="helpers/edit_io.py">
<violation number="1" location="helpers/edit_io.py:12">
P2: `run()` blocks on media commands with no timeout, so a hung ffmpeg/ffprobe stalls the whole render pipeline with no way to fail. Give `run()` a `timeout` parameter (e.g. default None, or a sane media-command default), pass it to `subprocess.run`, and convert `TimeoutExpired` into a `RuntimeError` so callers can report it.</violation>
<violation number="2" location="helpers/edit_io.py:65">
P3: `save_json` truncates and rewrites the target in place, so a crash, kill, or full disk mid-write leaves `context.json` truncated/unreadable and `project_state.load_json`/`view` then fail on every subsequent run. Write the payload to a sibling temp file and `os.replace()` it into place (for `exclusive`, use `os.link(tmp, path)` + unlink so `FileExistsError` semantics are preserved atomically). Also move the `mkdir` after the exclusive-open check so a failed create leaves no side effects.</violation>
</file>
<file name="install.md">
<violation number="1" location="install.md:54">
P3: This sentence overstates the libass requirement. For SRT captions (including the default `--build-subtitles` flow, which writes `master.srt`), `choose_caption_renderer` returns `"pil"` when the `subtitles` filter is absent, and `render_one_output` then overlays the rasterized track with plain `ffmpeg` — no libass needed. Only a `.ass` subtitles file hard-requires libass (`render.py` forces `libass` for `.ass` and `ffmpeg_with_subtitles()` raises if unavailable). So on macOS this pushes every user through a large `brew install ffmpeg-full` for the SRT default that would already render, and Linux/Arch users are told captions "require" libass while neither section provides an install path. Rephrase to "ASS subtitles require libass; SRT captions fall back to PIL rendering when it's missing."</violation>
<violation number="2" location="install.md:54">
P3: `render.py` finds the `ffmpeg-full` keg only for `.ass` caption files. For `.srt`, the renderer is chosen via `ffmpeg_has_subtitle_filter()`, which probes only the PATH `ffmpeg` binary; with the keg-only install documented here, that check fails and captions silently fall back to PIL, so the automatic fallback never happens. Qualify the claim (e.g., "for ASS captions") or make the SRT detection consult the keg paths too.</violation>
</file>
<file name="tests/test_render_treatment.py">
<violation number="1" location="tests/test_render_treatment.py:54">
P3: `pytestmark` skips these tests only when the ffmpeg/ffprobe binaries are absent, but `test_synthetic_render_applies_treatment_graphics_and_pil_captions` and `test_preview_render_scales_full_vertical_treatment_to_720p` also require the libx264 and aac encoders (the tests generate the source with `-c:v libx264 -c:a aac`, and the render pipeline re-encodes with libx264). On an ffmpeg build compiled without those encoders the tests fail with a subprocess error instead of being skipped, and the skip reason text misstates the actual requirement. Check encoder availability in the guard, e.g. run `ffmpeg -hide_banner -encoders` and skip when `libx264` or `aac` is missing, and mention the codec requirement in the reason.</violation>
</file>
<file name="SKILL.md">
<violation number="1" location="SKILL.md:34">
P3: This new rule brings the Hard Rules section to 13 items, but `README.md` (line 110, design principle 5) still says "12 hard rules". Update README so the documented rule count matches the contract.</violation>
</file>
<file name="references/sources.md">
<violation number="1" location="references/sources.md:12">
P3: OpenCV is not needed only by `find_shot.py`: `helpers/effects.py`, `helpers/track_mask.py`, and `helpers/verify_edit.py` also import cv2, so the screenshot-matching extra covers more than this sentence implies. Qualify the claim to the helpers documented here, or list the other cv2 users.</violation>
</file>
<file name="references/deliverables.md">
<violation number="1" location="references/deliverables.md:64">
P3: This sentence states interpolation is always linear, but `helpers/edl.py` (`_reframe_for`) accepts `interpolation: linear | smooth | hold` (default linear) and `helpers/render.py` `_load_track_keyframes` implements smoothstep and hold behavior. Document the `interpolation` field so EDL authors know the non-linear options exist; right now the doc contradicts EDLs that set `"interpolation": "smooth"`.</violation>
<violation number="2" location="references/deliverables.md:71">
P3: The rendering commands imply deliverables can be re-rendered, but `normalize_deliverables` and `render_one_output` in helpers/render.py reject any output path that already exists ("output already exists choose a new path"), and `--output-dir` writes `<id>.mp4` regardless of the declared `file`. Re-running `--all-deliverables` with the same output dir therefore fails. Note this in the Rendering section so users expect a one-shot-per-path workflow.</violation>
</file>
<file name="tests/test_track_mask.py">
<violation number="1" location="tests/test_track_mask.py:74">
P3: `test_cli_preserves_seed` invokes the script as the bare relative path `"helpers/track_mask.py"`, which only works when pytest is launched from the repository root (subprocess inherits the test runner's cwd). The rest of this suite resolves the repo root explicitly, e.g. `str(Path(__file__).resolve().parents[1] / "helpers/...")` in test_source_review.py and test_sources.py, so the test fails or silently tests nothing when invoked from another working directory. Resolve the script path from `__file__` so the subprocess does not depend on the invocation directory.</violation>
</file>
Heads up: you’ve reached your flex budget. Increase your flex budget or wait for usage to reset.
Re-trigger cubic
| base_dir: Path, | ||
| ) -> list[dict[str, Any]]: | ||
| """Render EDL graphic specs to full-canvas PNG overlay entries.""" | ||
| if output_dir.is_symlink() or (output_dir.exists() and any(output_dir.iterdir())): |
There was a problem hiding this comment.
P1: This guard makes graphics unusable for a second output: after the first render writes graphic_000.png, rerendering with a new delivery path and every --all-deliverables run fails with FileExistsError. Allocate a fresh generated-overlay directory per output (or safely replace only the helper's own generated artifacts).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/visuals.py, line 573:
<comment>This guard makes graphics unusable for a second output: after the first render writes `graphic_000.png`, rerendering with a new delivery path and every `--all-deliverables` run fails with `FileExistsError`. Allocate a fresh generated-overlay directory per output (or safely replace only the helper's own generated artifacts).</comment>
<file context>
@@ -0,0 +1,617 @@
+ base_dir: Path,
+) -> list[dict[str, Any]]:
+ """Render EDL graphic specs to full-canvas PNG overlay entries."""
+ if output_dir.is_symlink() or (output_dir.exists() and any(output_dir.iterdir())):
+ raise FileExistsError("graphic output directory must be new or empty")
+ output_dir.mkdir(parents=True, exist_ok=True)
</file context>
| payload = {"version": 1, "runtime": self.runtime, "settings": settings, | ||
| "inputs": [{"path": row["path"], "sha256": row["sha256"]} for row in before]} | ||
| key = hashlib.sha256(json.dumps(payload, sort_keys=True, allow_nan=False).encode()).hexdigest() | ||
| if self.restore(key, destination): |
There was a problem hiding this comment.
P2: restore replaces destination before this source-state check, so a source mutation makes the call fail while leaving stale cached bytes at the build path. Copy to a temporary destination and commit only after validation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/render_cache.py, line 143:
<comment>`restore` replaces `destination` before this source-state check, so a source mutation makes the call fail while leaving stale cached bytes at the build path. Copy to a temporary destination and commit only after validation.</comment>
<file context>
@@ -0,0 +1,174 @@
+ payload = {"version": 1, "runtime": self.runtime, "settings": settings,
+ "inputs": [{"path": row["path"], "sha256": row["sha256"]} for row in before]}
+ key = hashlib.sha256(json.dumps(payload, sort_keys=True, allow_nan=False).encode()).hexdigest()
+ if self.restore(key, destination):
+ if any(file_state(row["path"]) != row["state"] for row in before):
+ raise RuntimeError("Source changed while restoring a cached clip")
</file context>
| width, height = manifest["picture"][2:] | ||
| validate_time_map(shot["time_map"], count) | ||
| validate_source_window(shot) | ||
| if any(key in shot for key in ("source_start", "source_frame", "speed")): |
There was a problem hiding this comment.
P2: cut_list.validate allows a time_map shot to spell out speed: 1, but this check rejects any explicit speed key, so a valid manifest fails during staging. Reject only non-default speed values.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/effects.py, line 448:
<comment>`cut_list.validate` allows a `time_map` shot to spell out `speed: 1`, but this check rejects any explicit `speed` key, so a valid manifest fails during staging. Reject only non-default speed values.</comment>
<file context>
@@ -0,0 +1,524 @@
+ width, height = manifest["picture"][2:]
+ validate_time_map(shot["time_map"], count)
+ validate_source_window(shot)
+ if any(key in shot for key in ("source_start", "source_frame", "speed")):
+ raise ValueError("time_map uses absolute native frames and cannot combine with source origin or speed")
+ points = np.asarray(shot["time_map"])
</file context>
Suggested change
| if any(key in shot for key in ("source_start", "source_frame", "speed")): | |
| if any(key in shot for key in ("source_start", "source_frame")) or shot.get("speed", 1) != 1: |
| if ( | ||
| shot.get("grade") | ||
| or shot.get("isolation_mask") | ||
| or shot.get("speed", 1) != 1 |
There was a problem hiding this comment.
P2: An explicit speed: 1 passes this condition, but stage_mapped() rejects any speed key with a time_map. Reject the key by presence, or normalize it away before staging.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/cut_list.py, line 133:
<comment>An explicit `speed: 1` passes this condition, but `stage_mapped()` rejects any `speed` key with a `time_map`. Reject the key by presence, or normalize it away before staging.</comment>
<file context>
@@ -0,0 +1,313 @@
+ if (
+ shot.get("grade")
+ or shot.get("isolation_mask")
+ or shot.get("speed", 1) != 1
+ ):
+ raise ValueError(
</file context>
Suggested change
| or shot.get("speed", 1) != 1 | |
| "speed" in shot |
| for overlay, rect, start, end in resolved: | ||
| if overlay.get("composition") == "cutaway": | ||
| continue | ||
| if rect is None or not _time_ranges_intersect(start, end, region_start, region_end): |
There was a problem hiding this comment.
P2: This branch skips protected-region checks for legacy overlays, even though rect is None means full-frame behavior and only cutaways are exempt. Treat a legacy overlay as {x: 0, y: 0, width: 1, height: 1} before testing the intersection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/render.py, line 1121:
<comment>This branch skips protected-region checks for legacy overlays, even though `rect is None` means full-frame behavior and only cutaways are exempt. Treat a legacy overlay as `{x: 0, y: 0, width: 1, height: 1}` before testing the intersection.</comment>
<file context>
@@ -627,92 +716,916 @@ def apply_loudnorm_two_pass(
+ for overlay, rect, start, end in resolved:
+ if overlay.get("composition") == "cutaway":
+ continue
+ if rect is None or not _time_ranges_intersect(start, end, region_start, region_end):
+ continue
+ if _rects_intersect(rect, region_rect):
</file context>
Suggested change
| if rect is None or not _time_ranges_intersect(start, end, region_start, region_end): | |
| if rect is None: | |
| rect = {"x": 0.0, "y": 0.0, "width": 1.0, "height": 1.0} | |
| if not _time_ranges_intersect(start, end, region_start, region_end): |
| raise FileExistsError("tracking output must be a new file") | ||
| seed = load_json(a.seed_json) | ||
| cap = cv2.VideoCapture(a.prepared_picture) | ||
| if not np.isclose(cap.get(cv2.CAP_PROP_FPS), 30, rtol=0, atol=1e-6): |
There was a problem hiding this comment.
P3: A missing or unreadable input is reported as a frame-rate violation: cv2.VideoCapture on a bad file yields FPS 0, so this check raises "prepared picture must use the composition clock of 30 fps", obscuring that the file does not exist. Check cap.isOpened() first and report the missing/undecodable input, then perform the strict 30 fps check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/track_mask.py, line 185:
<comment>A missing or unreadable input is reported as a frame-rate violation: `cv2.VideoCapture` on a bad file yields FPS 0, so this check raises "prepared picture must use the composition clock of 30 fps", obscuring that the file does not exist. Check `cap.isOpened()` first and report the missing/undecodable input, then perform the strict 30 fps check.</comment>
<file context>
@@ -0,0 +1,206 @@
+ raise FileExistsError("tracking output must be a new file")
+ seed = load_json(a.seed_json)
+ cap = cv2.VideoCapture(a.prepared_picture)
+ if not np.isclose(cap.get(cv2.CAP_PROP_FPS), 30, rtol=0, atol=1e-6):
+ cap.release()
+ raise ValueError("prepared picture must use the composition clock of 30 fps")
</file context>
|
|
||
| FFmpeg and ffprobe must be on PATH. Install the optional screenshot-matching | ||
| backend with `uv sync --extra editing` or `pip install -e '.[editing]'`. | ||
| OpenCV is needed only by `find_shot.py`; the other helpers use the base Python |
There was a problem hiding this comment.
P3: OpenCV is not needed only by find_shot.py: helpers/effects.py, helpers/track_mask.py, and helpers/verify_edit.py also import cv2, so the screenshot-matching extra covers more than this sentence implies. Qualify the claim to the helpers documented here, or list the other cv2 users.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At references/sources.md, line 12:
<comment>OpenCV is not needed only by `find_shot.py`: `helpers/effects.py`, `helpers/track_mask.py`, and `helpers/verify_edit.py` also import cv2, so the screenshot-matching extra covers more than this sentence implies. Qualify the claim to the helpers documented here, or list the other cv2 users.</comment>
<file context>
@@ -0,0 +1,96 @@
+
+FFmpeg and ffprobe must be on PATH. Install the optional screenshot-matching
+backend with `uv sync --extra editing` or `pip install -e '.[editing]'`.
+OpenCV is needed only by `find_shot.py`; the other helpers use the base Python
+dependencies. Tests additionally require pytest.
+
</file context>
Suggested change
| OpenCV is needed only by `find_shot.py`; the other helpers use the base Python | |
| OpenCV is used by `find_shot.py` (and by `effects.py`, `track_mask.py`, and `verify_edit.py`); among the source-inspection helpers in this guide, only `find_shot.py` needs it. The others here use the base Python dependencies. |
| ## Rendering | ||
|
|
||
| ```bash | ||
| python helpers/render.py edit/edl.json --all-deliverables |
There was a problem hiding this comment.
P3: The rendering commands imply deliverables can be re-rendered, but normalize_deliverables and render_one_output in helpers/render.py reject any output path that already exists ("output already exists choose a new path"), and --output-dir writes <id>.mp4 regardless of the declared file. Re-running --all-deliverables with the same output dir therefore fails. Note this in the Rendering section so users expect a one-shot-per-path workflow.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At references/deliverables.md, line 71:
<comment>The rendering commands imply deliverables can be re-rendered, but `normalize_deliverables` and `render_one_output` in helpers/render.py reject any output path that already exists ("output already exists choose a new path"), and `--output-dir` writes `<id>.mp4` regardless of the declared `file`. Re-running `--all-deliverables` with the same output dir therefore fails. Note this in the Rendering section so users expect a one-shot-per-path workflow.</comment>
<file context>
@@ -0,0 +1,78 @@
+## Rendering
+
+```bash
+python helpers/render.py edit/edl.json --all-deliverables
+python helpers/render.py edit/edl.json --deliverable social_9x16
+python helpers/render.py edit/edl.json --all-deliverables --output-dir <dir>
</file context>
| } | ||
| ``` | ||
|
|
||
| Keyframes are interpolated linearly between times. Each deliverable names its |
There was a problem hiding this comment.
P3: This sentence states interpolation is always linear, but helpers/edl.py (_reframe_for) accepts interpolation: linear | smooth | hold (default linear) and helpers/render.py _load_track_keyframes implements smoothstep and hold behavior. Document the interpolation field so EDL authors know the non-linear options exist; right now the doc contradicts EDLs that set "interpolation": "smooth".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At references/deliverables.md, line 64:
<comment>This sentence states interpolation is always linear, but `helpers/edl.py` (`_reframe_for`) accepts `interpolation: linear | smooth | hold` (default linear) and `helpers/render.py` `_load_track_keyframes` implements smoothstep and hold behavior. Document the `interpolation` field so EDL authors know the non-linear options exist; right now the doc contradicts EDLs that set `"interpolation": "smooth"`.</comment>
<file context>
@@ -0,0 +1,78 @@
+}
+```
+
+Keyframes are interpolated linearly between times. Each deliverable names its
+track with `reframe_track`; the root `reframe` object supplies the shared mode
+and track file.
</file context>
| result = subprocess.run( | ||
| [ | ||
| sys.executable, | ||
| "helpers/track_mask.py", |
There was a problem hiding this comment.
P3: test_cli_preserves_seed invokes the script as the bare relative path "helpers/track_mask.py", which only works when pytest is launched from the repository root (subprocess inherits the test runner's cwd). The rest of this suite resolves the repo root explicitly, e.g. str(Path(__file__).resolve().parents[1] / "helpers/...") in test_source_review.py and test_sources.py, so the test fails or silently tests nothing when invoked from another working directory. Resolve the script path from __file__ so the subprocess does not depend on the invocation directory.
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 74:
<comment>`test_cli_preserves_seed` invokes the script as the bare relative path `"helpers/track_mask.py"`, which only works when pytest is launched from the repository root (subprocess inherits the test runner's cwd). The rest of this suite resolves the repo root explicitly, e.g. `str(Path(__file__).resolve().parents[1] / "helpers/...")` in test_source_review.py and test_sources.py, so the test fails or silently tests nothing when invoked from another working directory. Resolve the script path from `__file__` so the subprocess does not depend on the invocation directory.</comment>
<file context>
@@ -0,0 +1,96 @@
+ result = subprocess.run(
+ [
+ sys.executable,
+ "helpers/track_mask.py",
+ "missing.mp4",
+ str(seed),
</file context>
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
Repeated edits currently rebuild picture clips even when their sources and settings have not changed. Add an optional
--reuseflag so caption, audio and single-shot revisions can reuse completed clips.Changes
helpers/render_cache.pyto compare source contents, cut and picture settings, helper code, media tools and relevant library versions before reusing a clip.references/render-reuse.md.Validation: the full local suite passed with 277 passed, 1 skipped and 18 subtests passed using FFmpeg with libass. The 15 new cache tests cover source/settings/tool changes, damaged entries, failed attempts and source protection. Real media tests show identical decoded frames for uncached, first cached and repeated renders; a repeated render reuses both shots, changing one cut rebuilds one shot, and changing a mask invalidates its shot. The generated contact sheet was inspected. These are technical checks, not an audio aesthetic review.
Limits
main. Review only this addition at submit/rendering...submit/render-reuse.Summary by cubic
Adds an optional
--reuseflag so caption, audio, and single-shot revisions reuse picture clips whose sources and settings haven't changed.Depends on unmerged commits from #170 against
main; review only this addition at the listed compare URL.Written for commit 6a23c2e. Summary will update on new commits.