feat(render): compose independent picture audio and caption timelines - #170
Open
DonIsmaelito wants to merge 48 commits into
Open
DonIsmaelito wants to merge 48 commits into
DonIsmaelito wants to merge 48 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
DonIsmaelito
marked this pull request as ready for review
September 17, 2026 22:20
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
13 issues found across 58 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="tests/test_render_treatment.py">
<violation number="1" location="tests/test_render_treatment.py:155">
P2: The integration test does not prove that treatment graphics or captions reach the encoded video. Assert representative output pixels or inspect a decoded frame/filter invocation so generated-but-unapplied overlays cannot pass.</violation>
</file>
<file name="helpers/map_transcript.py">
<violation number="1" location="helpers/map_transcript.py:131">
P2: When `--out` points at an existing review artifact, this command overwrites it. Reject existing output paths, including symlinks, before calling `save_json`.</violation>
</file>
<file name="references/overlays.md">
<violation number="1" location="references/overlays.md:35">
P2: The composition rule contradicts the documented `picture_in_picture` exception. Scope this restriction to fixed-layout compositions or explicitly exempt `picture_in_picture`, otherwise authors may reject a supported custom-rect manifest.</violation>
<violation number="2" location="references/overlays.md:57">
P2: For a custom `captions.safe_region` that is not full-width, `render.py` rejects a `full` layout rather than shortening it. Qualify this sentence to describe the full-width-only behavior, or change the renderer if all safe regions should be shortened.</violation>
</file>
<file name="SKILL.md">
<violation number="1" location="SKILL.md:200">
P2: When an EDL customizes `captions.safe_region`, the documented `captions.py` command ignores it and emits captions in the default bottom 16% rail. Match the CLI's bottom-rail setting to the EDL or document that only the default bottom rail is supported.</violation>
</file>
<file name="helpers/caption_raster.py">
<violation number="1" location="helpers/caption_raster.py:73">
P2: When an SRT contains one malformed timestamp block, `parse_srt` raises instead of skipping that block, so a single bad cue aborts caption-track generation. Catch `ValueError` while parsing each cue and continue to the next block.</violation>
</file>
<file name="helpers/edl.py">
<violation number="1" location="helpers/edl.py:426">
P2: When a grade has leading or trailing whitespace, validation accepts it but rendering receives the original value and fails as an invalid filter. Reject surrounding whitespace or normalize the grade before rendering.</violation>
<violation number="2" location="helpers/edl.py:453">
P2: When an integration imports `helpers.edl` from the project root, version-3 validation raises `ModuleNotFoundError` before checking the manifest. Make the helper imports package-safe, or consistently establish the helpers module path for library callers.</violation>
</file>
<file name="tests/test_visuals.py">
<violation number="1" location="tests/test_visuals.py:254">
P2: This test invokes the ffmpeg binary unconditionally, so on a machine or CI runner without ffmpeg it fails the whole test file with FileNotFoundError instead of skipping the one ffmpeg-dependent case. Every other ffmpeg-dependent test in this suite (test_audio_tracks.py:15, test_composition.py:35, test_render_treatment.py:54-56, test_sources.py:29, test_mix_audio.py:19, test_cards.py:111) guards with `shutil.which(...)` and `pytest.skip`/`skipif`. Add the same guard here.</violation>
</file>
<file name="helpers/mix_audio.py">
<violation number="1" location="helpers/mix_audio.py:95">
P2: When a CLI manifest contains a malformed `gain_points` entry, this code indexes `p[1]` before checking its shape. `build()` then exits with `IndexError`/`TypeError` instead of a clear validation error; validate each point as a two-item numeric pair first.</violation>
</file>
<file name="helpers/captions.py">
<violation number="1" location="helpers/captions.py:125">
P2: When callers pass `break_on_punctuation`, `max_characters` is ignored because this delegation forwards only `max_words` and punctuation. Enforce the character limit in the compatibility path or reject the incompatible option before returning overlong cues.</violation>
</file>
<file name="helpers/source_scan.py">
<violation number="1" location="helpers/source_scan.py:142">
P2: When `--out` is a hard link to the source, this pathname check passes and `save_json()` truncates the source media. Compare existing output and source with `Path.samefile()` as well, in both source-writing CLIs.</violation>
</file>
<file name="helpers/effects.py">
<violation number="1" location="helpers/effects.py:340">
P2: A layer with `fill` but no `fill_opacity` passes validation and renders fully opaque: `validate_layers` checks the fallback 0 while `LayerCompositor.frame` blends with fallback 1.0 (`Image.blend(im, filled, ...)`), so the fill completely covers the layer image. Make the defaults agree (validation should also default to 1, or the blend should default to 0) and add a test for fill without fill_opacity.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
|
|
||
| # generated canvas filters execute successfully with actual ffmpeg input | ||
| @pytest.mark.parametrize("fit", ["contain", "cover", "blur"]) | ||
| def test_canvas_filters_encode(fit, tmp_path): |
There was a problem hiding this comment.
P2: This test invokes the ffmpeg binary unconditionally, so on a machine or CI runner without ffmpeg it fails the whole test file with FileNotFoundError instead of skipping the one ffmpeg-dependent case. Every other ffmpeg-dependent test in this suite (test_audio_tracks.py:15, test_composition.py:35, test_render_treatment.py:54-56, test_sources.py:29, test_mix_audio.py:19, test_cards.py:111) guards with shutil.which(...) and pytest.skip/skipif. Add the same guard here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_visuals.py, line 254:
<comment>This test invokes the ffmpeg binary unconditionally, so on a machine or CI runner without ffmpeg it fails the whole test file with FileNotFoundError instead of skipping the one ffmpeg-dependent case. Every other ffmpeg-dependent test in this suite (test_audio_tracks.py:15, test_composition.py:35, test_render_treatment.py:54-56, test_sources.py:29, test_mix_audio.py:19, test_cards.py:111) guards with `shutil.which(...)` and `pytest.skip`/`skipif`. Add the same guard here.</comment>
<file context>
@@ -0,0 +1,284 @@
+
+# generated canvas filters execute successfully with actual ffmpeg input
+@pytest.mark.parametrize("fit", ["contain", "cover", "blur"])
+def test_canvas_filters_encode(fit, tmp_path):
+ import subprocess
+
</file context>
Comment on lines
+95
to
+98
| if not math.isfinite(float(base_db)) or any( | ||
| not math.isfinite(float(p[1])) for p in points | ||
| ): | ||
| raise ValueError("gain must be finite") |
There was a problem hiding this comment.
P2: When a CLI manifest contains a malformed gain_points entry, this code indexes p[1] before checking its shape. build() then exits with IndexError/TypeError instead of a clear validation error; validate each point as a two-item numeric pair first.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/mix_audio.py, line 95:
<comment>When a CLI manifest contains a malformed `gain_points` entry, this code indexes `p[1]` before checking its shape. `build()` then exits with `IndexError`/`TypeError` instead of a clear validation error; validate each point as a two-item numeric pair first.</comment>
<file context>
@@ -0,0 +1,303 @@
+def gain_envelope(count, points, base_db=0):
+ if type(count) is not int or count <= 0:
+ raise ValueError("gain envelope needs a positive integer sample count")
+ if not math.isfinite(float(base_db)) or any(
+ not math.isfinite(float(p[1])) for p in points
+ ):
</file context>
Suggested change
| if not math.isfinite(float(base_db)) or any( | |
| not math.isfinite(float(p[1])) for p in points | |
| ): | |
| raise ValueError("gain must be finite") | |
| try: | |
| base_finite = math.isfinite(float(base_db)) | |
| malformed = not isinstance(points, (list, tuple)) or any( | |
| not isinstance(p, (list, tuple)) | |
| or len(p) != 2 | |
| or type(p[0]) is not int | |
| or not math.isfinite(float(p[1])) | |
| for p in points | |
| ) | |
| except (TypeError, ValueError, IndexError): | |
| base_finite = False | |
| malformed = True | |
| if not base_finite or malformed: | |
| raise ValueError("gain must be finite and points must be two-item numeric pairs") |
Author
|
@cubic-dev-ai please review the latest commits again. Correct phrase caption timing and default reframe filters, preflight gain envelopes and word fields, and generate composition review sheets. Includes updated source, audio, caption, effects and legacy delivery prerequisites. Validation: 250 branch tests passed, with one optional RAQM test skipped. 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 33 new issues found across 60 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/prepare_source.py">
<violation number="1" location="helpers/prepare_source.py:19">
P2: When the source has no video stream, `prepare` raises an unhelpful `StopIteration` traceback. Handle the missing stream explicitly and raise an actionable error stating that the source needs a video stream.</violation>
</file>
<file name="helpers/edit_io.py">
<violation number="1" location="helpers/edit_io.py:56">
P2: When a composition manifest contains non-ASCII text on a non-UTF-8 locale, `load_json` can raise `UnicodeDecodeError` before rendering starts. Read JSON files with `encoding="utf-8"`.</violation>
</file>
<file name="helpers/find_shot.py">
<violation number="1" location="helpers/find_shot.py:53">
P3: When library callers pass `every=True`, `search` accepts it as a one-second interval instead of rejecting an invalid sampling value. Reject booleans consistently with the other clock validators.</violation>
<violation number="2" location="helpers/find_shot.py:78">
P3: When `limit` is negative, Python slicing returns most candidates instead of enforcing the requested bound. Clamp negative limits to zero or reject them before slicing.</violation>
</file>
<file name="helpers/cards.py">
<violation number="1" location="helpers/cards.py:518">
P2: When a concurrent writer creates the still output after the preflight check, `image.save` overwrites that file. Create the still with exclusive file creation (and handle the sidecar atomically) so the CLI preserves concurrent or pre-existing outputs.</violation>
</file>
<file name="helpers/source_scan.py">
<violation number="1" location="helpers/source_scan.py:118">
P2: When FFmpeg fails after emitting the requested frames, `selected_frames` still returns them because it ignores `process.wait()`'s exit status and discards stderr. Capture the diagnostic and reject nonzero decoder exits before treating the selection as valid.</violation>
</file>
<file name="helpers/project_state.py">
<violation number="1" location="helpers/project_state.py:37">
P2: Malformed `depends_on` values crash validation or are interpreted as character lists instead of being rejected as invalid context metadata. Require a list before traversing dependencies so `show` and `record` fail with an actionable validation error.</violation>
<violation number="2" location="helpers/project_state.py:60">
P2: Absolute artifact paths are accepted and persisted even though the project-state contract requires paths relative to the context file. Reject absolute paths before resolving them so context files remain portable and cannot silently reference another file after a move.</violation>
</file>
<file name="helpers/track_mask.py">
<violation number="1" location="helpers/track_mask.py:24">
P2: A non-adjacent edge that touches or overlaps another edge is accepted as a valid polygon because this test handles only strict crossings. Check collinear/on-segment intersections as well, otherwise malformed seed polygons and tracked mattes reach the renderer without `needs_review`.</violation>
<violation number="2" location="helpers/track_mask.py:70">
P2: Calling `track` permanently changes OpenCV's process-wide thread count, slowing subsequent image operations in a long-lived caller. Remove this global mutation or save and restore the previous thread count around tracking.</violation>
</file>
<file name="helpers/edl.py">
<violation number="1" location="helpers/edl.py:104">
P2: When a version 2 EDL contains an empty `subtitles` value and no `captions` block, this early return skips subtitle validation and silently treats the malformed declaration as no subtitles. Return early only when both fields are absent so present empty paths reach the non-empty path check.</violation>
</file>
<file name="helpers/captions.py">
<violation number="1" location="helpers/captions.py:57">
P2: When a generic word has `end < start`, `_as_word` rewrites it to `start + 0.08`, producing a caption at the wrong time. Reject reversed intervals and reserve the fallback duration for zero-length timestamps.</violation>
</file>
<file name="helpers/verify_edit.py">
<violation number="1" location="helpers/verify_edit.py:132">
P2: When the standalone verifier receives a manifest with a non-30 fps declaration, it measures against 30 fps and can report a false pass. Reject manifests whose `fps` is not 30 before calculating duration and sample targets.</violation>
<violation number="2" location="helpers/verify_edit.py:240">
P2: When audio exists only outside these three probe windows, all references are skipped and `technical_pass` fails. Select alignment windows from non-silent master samples before requiring a successful correlation.</violation>
</file>
<file name="helpers/caption_raster.py">
<violation number="1" location="helpers/caption_raster.py:162">
P2: When a selected range lacks its transcript, `build_master_srt` silently omits that segment's captions and still produces the subtitle file. Fail preflight instead of rendering an incomplete caption track.</violation>
</file>
<file name="helpers/_composition.py">
<violation number="1" location="helpers/_composition.py:163">
P2: When a referenced caption font is missing, this line only records the path, so `CardRenderer` fails after expensive media rendering. Validate referenced font files before creating the build directory and staging media.</violation>
<violation number="2" location="helpers/_composition.py:318">
P2: When an audio source has no track, an insufficient window, or only silence, the build performs the full picture render before failing in `mix_audio`. Preflight the audio mix before staging video so invalid composition deliveries fail before expensive work.</violation>
</file>
<file name="helpers/mix_audio.py">
<violation number="1" location="helpers/mix_audio.py:252">
P2: When the standalone mixer receives an invalid gain envelope, `build` decodes the source before `apply_clip` rejects it and leaves the output directory created. Validate `filter_chain` and `gain_envelope` in the initial clip-validation loop before `dest.mkdir` and media decoding.</violation>
</file>
<file name="pyproject.toml">
<violation number="1" location="pyproject.toml:17">
P3: The committed `uv.lock` was not regenerated after this change added the `test` extra: pytest is absent from the lock even though the lock does include scipy and opencv-python-headless from the other new entries. A developer following the README workflow (`uv sync --extra test` or `uv run pytest`) will hit a lockfile mismatch and must relock, or pytest will be missing from the environment entirely. Run `uv lock` (or `uv sync --extra test`) and commit the updated lock so the committed lock and `[project.optional-dependencies] test` stay in sync.</violation>
</file>
<file name="tests/test_source_review.py">
<violation number="1" location="tests/test_source_review.py:37">
P3: When cv2 (the optional `editing` extra) is not installed, this `importorskip` skips the entire parametrized test, including the `source_scan.py` case. `source_scan.py` never imports cv2 (only numpy/PIL), so its hardlink-protection regression is silently dropped in environments that have the required deps but not the extra. Move the skip inside so it applies only to the `find_shot.py` case, and add a reason string to match the repo convention used elsewhere.</violation>
</file>
<file name="tests/test_effects.py">
<violation number="1" location="tests/test_effects.py:107">
P3: When ffmpeg or ffprobe is missing, this test fails the whole file with FileNotFoundError instead of skipping, unlike the ffmpeg guard used in tests/test_composition.py. Guard the encoded frames test with `if not shutil.which("ffmpeg"): pytest.skip(...)` (and ffprobe) before the subprocess calls.</violation>
</file>
<file name="helpers/render.py">
<violation number="1" location="helpers/render.py:1045">
P2: When a protected region contains `NaN` or infinity, this validation accepts it and skips the collision check. Require finite start and end values before testing the region bounds.</violation>
<violation number="2" location="helpers/render.py:1401">
P2: When `--preflight-overlays` targets an existing PNG or symlink, `sheet.save(output)` overwrites it. Reject existing outputs before saving the review sheet.</violation>
<violation number="3" location="helpers/render.py:1656">
P2: When `--deliverable` is combined with `--preflight-overlays`, the sheet ignores that deliverable's reframe and dimensions. Reframe the selected deliverable before preflight, or reject deliverable selectors for this mode.</violation>
</file>
<file name="tests/test_composition_contract.py">
<violation number="1" location="tests/test_composition_contract.py:69">
P3: The subprocess calls use the relative path `helpers/" + script`, so the test only works when pytest runs with the repository root as the working directory. If the suite is invoked from any other directory, the subprocess raises FileNotFoundError (or a Python "can't open file" message) and the test errors instead of verifying the output guard. Anchor the script path to this test file with `__file__` so the test is directory-independent.</violation>
<violation number="2" location="tests/test_composition_contract.py:87">
P3: When a contributor installs only the declared core + `test` extras, this test errors with `ModuleNotFoundError: cv2` before it ever checks the intended guard. `review_sheets` (helpers/verify_edit.py) starts with `import cv2`, but pyproject.toml declares opencv only under the optional `editing` extra (`opencv-python-headless>=4.10,<5`) while `test = ["pytest>=7"]`. Add opencv to the test extras (or make the cv2 import lazy) so the composition review-sheet tests run in a plain test environment.</violation>
</file>
<file name="helpers/map_transcript.py">
<violation number="1" location="helpers/map_transcript.py:22">
P2: When a transcript word has `"type": null`, this helper silently drops it even though the repository accepts null as a word type. Treat `None` like a missing type in both word filters so valid words are not omitted from captions or comparison reports.</violation>
</file>
<file name="helpers/edit_clock.py">
<violation number="1" location="helpers/edit_clock.py:61">
P2: `ass_stamp` is dead, so the ASS interface continues using its separate float-based timestamp conversion. Wire this helper into the ASS writer or remove it and keep one timestamp implementation.</violation>
<violation number="2" location="helpers/edit_clock.py:90">
P2: `check_partition` accepts non-integer totals that compare equal to frame counts, allowing malformed timeline metadata through this shared validator. Require a nonnegative integer `total` before checking the partition.</violation>
</file>
<file name="helpers/cut_list.py">
<violation number="1" location="helpers/cut_list.py:133">
P2: When a time-mapped shot explicitly declares the default `speed: 1`, validation succeeds but `stage_mapped` rejects it before encoding. Reject the presence of `speed` here, matching the renderer's absolute-source-clock contract.</violation>
<violation number="2" location="helpers/cut_list.py:164">
P2: Invalid fade lengths pass composition validation and fail only after picture staging and audio decoding. Validate both explicit fades and the default voice fade before starting the render.</violation>
<violation number="3" location="helpers/cut_list.py:166">
P2: A malformed word can pass `validate` without captions, and a referenced malformed word crashes caption validation instead of producing an actionable manifest error. Require nonempty string word text in this preflight loop.</violation>
<violation number="4" location="helpers/cut_list.py:260">
P2: Malformed delivery targets pass composition validation and fail later during mixing or verification, potentially after partial audio artifacts are written. Validate these numeric loudness fields and their ranges in `cut_list.validate`.</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
| 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.
P2: When the source has no video stream, prepare raises an unhelpful StopIteration traceback. Handle the missing stream explicitly and raise an actionable error stating that the source needs a video stream.
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, `prepare` raises an unhelpful `StopIteration` traceback. Handle the missing stream explicitly and raise an actionable error stating that the source needs a video stream.</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}") |
| # read a project or evidence document from disk | ||
| def load_json(path): | ||
| """Read a project or evidence document from disk.""" | ||
| return json.loads(Path(path).read_text()) |
There was a problem hiding this comment.
P2: When a composition manifest contains non-ASCII text on a non-UTF-8 locale, load_json can raise UnicodeDecodeError before rendering starts. Read JSON files with encoding="utf-8".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/edit_io.py, line 56:
<comment>When a composition manifest contains non-ASCII text on a non-UTF-8 locale, `load_json` can raise `UnicodeDecodeError` before rendering starts. Read JSON files with `encoding="utf-8"`.</comment>
<file context>
@@ -0,0 +1,111 @@
+# read a project or evidence document from disk
+def load_json(path):
+ """Read a project or evidence document from disk."""
+ return json.loads(Path(path).read_text())
+
+
</file context>
Suggested change
| return json.loads(Path(path).read_text()) | |
| return json.loads(Path(path).read_text(encoding="utf-8")) |
| output.parent.mkdir(parents=True, exist_ok=True) | ||
| if a.frame is not None: | ||
| image, boxes = renderer.frame(a.frame, True) | ||
| image.save(a.out) |
There was a problem hiding this comment.
P2: When a concurrent writer creates the still output after the preflight check, image.save overwrites that file. Create the still with exclusive file creation (and handle the sidecar atomically) so the CLI preserves concurrent or pre-existing outputs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/cards.py, line 518:
<comment>When a concurrent writer creates the still output after the preflight check, `image.save` overwrites that file. Create the still with exclusive file creation (and handle the sidecar atomically) so the CLI preserves concurrent or pre-existing outputs.</comment>
<file context>
@@ -0,0 +1,525 @@
+ output.parent.mkdir(parents=True, exist_ok=True)
+ if a.frame is not None:
+ image, boxes = renderer.frame(a.frame, True)
+ image.save(a.out)
+ save_json(str(a.out) + ".json", boxes)
+ else:
</file context>
Suggested change
| image.save(a.out) | |
| with output.open("xb") as stream: | |
| image.save(stream, format=output.suffix.lstrip(".").upper()) |
| 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 still returns them because it ignores process.wait()'s exit status and discards stderr. Capture the diagnostic and reject nonzero decoder exits before treating the selection as valid.
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` still returns them because it ignores `process.wait()`'s exit status and discards stderr. Capture the diagnostic and reject nonzero decoder exits before treating the selection as valid.</comment>
<file context>
@@ -0,0 +1,163 @@
+ process.stdout.close()
+ if process.poll() is None:
+ process.terminate()
+ process.wait()
+
+
</file context>
| if ident not in rows: | ||
| raise ValueError(f"unknown context dependency {ident}") | ||
| visiting.add(ident) | ||
| for dep in rows[ident].get("depends_on", []): |
There was a problem hiding this comment.
P2: Malformed depends_on values crash validation or are interpreted as character lists instead of being rejected as invalid context metadata. Require a list before traversing dependencies so show and record fail with an actionable validation error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/project_state.py, line 37:
<comment>Malformed `depends_on` values crash validation or are interpreted as character lists instead of being rejected as invalid context metadata. Require a list before traversing dependencies so `show` and `record` fail with an actionable validation error.</comment>
<file context>
@@ -0,0 +1,133 @@
+ if ident not in rows:
+ raise ValueError(f"unknown context dependency {ident}")
+ visiting.add(ident)
+ for dep in rows[ident].get("depends_on", []):
+ visit(dep)
+ visiting.remove(ident)
</file context>
Suggested change
| for dep in rows[ident].get("depends_on", []): | |
| dependencies = rows[ident].get("depends_on", []) | |
| if not isinstance(dependencies, list): | |
| raise ValueError("context dependencies must be a list") | |
| for dep in dependencies: |
| ] | ||
|
|
||
| [project.optional-dependencies] | ||
| test = ["pytest>=7"] |
There was a problem hiding this comment.
P3: The committed uv.lock was not regenerated after this change added the test extra: pytest is absent from the lock even though the lock does include scipy and opencv-python-headless from the other new entries. A developer following the README workflow (uv sync --extra test or uv run pytest) will hit a lockfile mismatch and must relock, or pytest will be missing from the environment entirely. Run uv lock (or uv sync --extra test) and commit the updated lock so the committed lock and [project.optional-dependencies] test stay in sync.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pyproject.toml, line 17:
<comment>The committed `uv.lock` was not regenerated after this change added the `test` extra: pytest is absent from the lock even though the lock does include scipy and opencv-python-headless from the other new entries. A developer following the README workflow (`uv sync --extra test` or `uv run pytest`) will hit a lockfile mismatch and must relock, or pytest will be missing from the environment entirely. Run `uv lock` (or `uv sync --extra test`) and commit the updated lock so the committed lock and `[project.optional-dependencies] test` stay in sync.</comment>
<file context>
@@ -10,14 +10,21 @@ dependencies = [
]
[project.optional-dependencies]
+test = ["pytest>=7"]
animations = ["manim"]
+editing = ["opencv-python-headless>=4.10,<5"]
</file context>
| # existing aliases are rejected before either source command reads media | ||
| @pytest.mark.parametrize('helper', ['source_scan.py', 'find_shot.py']) | ||
| def test_hardlink_report_preserves_input(tmp_path, helper): | ||
| pytest.importorskip('cv2') |
There was a problem hiding this comment.
P3: When cv2 (the optional editing extra) is not installed, this importorskip skips the entire parametrized test, including the source_scan.py case. source_scan.py never imports cv2 (only numpy/PIL), so its hardlink-protection regression is silently dropped in environments that have the required deps but not the extra. Move the skip inside so it applies only to the find_shot.py case, and add a reason string to match the repo convention used elsewhere.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_source_review.py, line 37:
<comment>When cv2 (the optional `editing` extra) is not installed, this `importorskip` skips the entire parametrized test, including the `source_scan.py` case. `source_scan.py` never imports cv2 (only numpy/PIL), so its hardlink-protection regression is silently dropped in environments that have the required deps but not the extra. Move the skip inside so it applies only to the `find_shot.py` case, and add a reason string to match the repo convention used elsewhere.</comment>
<file context>
@@ -0,0 +1,77 @@
+# existing aliases are rejected before either source command reads media
+@pytest.mark.parametrize('helper', ['source_scan.py', 'find_shot.py'])
+def test_hardlink_report_preserves_input(tmp_path, helper):
+ pytest.importorskip('cv2')
+ source = tmp_path / 'source'; source.write_bytes(b'original source')
+ out = tmp_path / 'report'; os.link(source, out)
</file context>
|
|
||
|
|
||
| # real encoded retiming selects requested source frames and honors output rate | ||
| def test_retime_and_composite_encoded_frames(tmp_path): |
There was a problem hiding this comment.
P3: When ffmpeg or ffprobe is missing, this test fails the whole file with FileNotFoundError instead of skipping, unlike the ffmpeg guard used in tests/test_composition.py. Guard the encoded frames test with if not shutil.which("ffmpeg"): pytest.skip(...) (and ffprobe) before the subprocess calls.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_effects.py, line 107:
<comment>When ffmpeg or ffprobe is missing, this test fails the whole file with FileNotFoundError instead of skipping, unlike the ffmpeg guard used in tests/test_composition.py. Guard the encoded frames test with `if not shutil.which("ffmpeg"): pytest.skip(...)` (and ffprobe) before the subprocess calls.</comment>
<file context>
@@ -0,0 +1,199 @@
+
+
+# real encoded retiming selects requested source frames and honors output rate
+def test_retime_and_composite_encoded_frames(tmp_path):
+ source = tmp_path / "source.mkv"
+ frames = np.stack(
</file context>
| result = subprocess.run( | ||
| [ | ||
| sys.executable, | ||
| "helpers/" + script, |
There was a problem hiding this comment.
P3: The subprocess calls use the relative path helpers/" + script, so the test only works when pytest runs with the repository root as the working directory. If the suite is invoked from any other directory, the subprocess raises FileNotFoundError (or a Python "can't open file" message) and the test errors instead of verifying the output guard. Anchor the script path to this test file with __file__ so the test is directory-independent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_composition_contract.py, line 69:
<comment>The subprocess calls use the relative path `helpers/" + script`, so the test only works when pytest runs with the repository root as the working directory. If the suite is invoked from any other directory, the subprocess raises FileNotFoundError (or a Python "can't open file" message) and the test errors instead of verifying the output guard. Anchor the script path to this test file with `__file__` so the test is directory-independent.</comment>
<file context>
@@ -0,0 +1,114 @@
+ result = subprocess.run(
+ [
+ sys.executable,
+ "helpers/" + script,
+ str(path),
+ *arguments,
</file context>
| image = tmp_path / "review_001.jpg" | ||
| image.write_bytes(b"previous review") | ||
| with pytest.raises(FileExistsError): | ||
| review_sheets({}, "missing.mp4", tmp_path) |
There was a problem hiding this comment.
P3: When a contributor installs only the declared core + test extras, this test errors with ModuleNotFoundError: cv2 before it ever checks the intended guard. review_sheets (helpers/verify_edit.py) starts with import cv2, but pyproject.toml declares opencv only under the optional editing extra (opencv-python-headless>=4.10,<5) while test = ["pytest>=7"]. Add opencv to the test extras (or make the cv2 import lazy) so the composition review-sheet tests run in a plain test environment.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_composition_contract.py, line 87:
<comment>When a contributor installs only the declared core + `test` extras, this test errors with `ModuleNotFoundError: cv2` before it ever checks the intended guard. `review_sheets` (helpers/verify_edit.py) starts with `import cv2`, but pyproject.toml declares opencv only under the optional `editing` extra (`opencv-python-headless>=4.10,<5`) while `test = ["pytest>=7"]`. Add opencv to the test extras (or make the cv2 import lazy) so the composition review-sheet tests run in a plain test environment.</comment>
<file context>
@@ -0,0 +1,114 @@
+ image = tmp_path / "review_001.jpg"
+ image.write_bytes(b"previous review")
+ with pytest.raises(FileExistsError):
+ review_sheets({}, "missing.mp4", tmp_path)
+ assert image.read_bytes() == b"previous review"
+
</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
Picture cuts should not force music, dialogue and captions to restart together. This connects the editing helpers so one declared timeline can render and check the finished video.
Builds on #147, #167 and #169.
Changes
Add an explicit composition timeline with independently placed picture, audio, captions and moving layers.
Connect it to the render command while retaining the existing EDL v1/v2 path and ASS caption interface.
Check encoded frames, timing, audio alignment and loudness, and generate review sheets. Protect existing outputs and reports.
Feature commits and review fixes cover validation, rendering and documentation. All 250 branch tests pass; one RAQM-dependent test is skipped. Encoded review frames were inspected.
Review follow-up: Correct phrase caption timing and default reframe filters, preflight gain envelopes and word fields, and generate composition review sheets. Includes updated source, audio, caption, effects and legacy delivery prerequisites.
Limits
The new composition path requires 30 fps and declared non-silent audio, delivered at 48 kHz stereo. It needs FFmpeg and the existing editing dependencies. Other frame rates and silent-only composition exports are not supported yet.
Technical checks do not replace visual and listening review or verify transcription and asset ownership. Caption timing checks use the manifest rather than OCR. Listening review remains pending.
Existing renderer proposals overlap this integration area. This does not resolve legacy caption drift in #161, AAC joins in #163 or probe caching in #150. The separate montage command and specialized workflow skills remain outside this PR. No new assets or fonts are bundled.