feat(captions): add styled images and measured word cards - #168
Open
DonIsmaelito wants to merge 6 commits into
Open
DonIsmaelito wants to merge 6 commits into
DonIsmaelito wants to merge 6 commits into
Conversation
DonIsmaelito
marked this pull request as ready for review
September 17, 2026 21:40
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
27 issues found across 18 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/edit_clock.py">
<violation number="1" location="helpers/edit_clock.py:25">
P2: When a non-positive frame or sample rate is supplied, these helpers silently generate invalid timestamps instead of rejecting the clock. Validate `fps` and `rate` as positive before performing the conversions.</violation>
<violation number="2" location="helpers/edit_clock.py:38">
P2: When callers provide a decimal sample rate, `frame_to_sample` raises before converting it. Normalize `frame` and `rate` with `fraction()` before forming the sample-clock numerator.</violation>
</file>
<file name="helpers/project_state.py">
<violation number="1" location="helpers/project_state.py:63">
P2: When `record()` receives a new entry with a missing or self dependency, this comprehension raises `KeyError` before `validate()` can reject the graph with its intended validation error. Validate the candidate graph before dereferencing dependency hashes, or explicitly preserve missing hashes until `validate()` runs.</violation>
<violation number="2" location="helpers/project_state.py:98">
P2: When an entry JSON contains an `id` field, `view()` reports that value instead of the artifact key supplied to `record()`, so `show` can identify the wrong evidence row. Put `id` after `**row` so the context key remains authoritative.</violation>
</file>
<file name="helpers/edit_io.py">
<violation number="1" location="helpers/edit_io.py:93">
P3: `last_json` is dead code in this revision: no caller uses it, while `measure_loudness` retains a separate extractor. Remove it or route the existing measurement path through this shared helper so the code is exercised and maintained.</violation>
<violation number="2" location="helpers/edit_io.py:95">
P2: When command output contains a nested JSON object, `last_json` returns the innermost object instead of the final complete measurement. Select the complete candidate with the latest closing offset and cover nested objects with a test.</violation>
</file>
<file name="helpers/prepare_source.py">
<violation number="1" location="helpers/prepare_source.py:13">
P2: An existing `<out>.log` or `<out>.json` is not checked, so this run overwrites prior logs or provenance records. `Path.exists()` also misses dangling output symlinks; reject the media path, log, and JSON record with `exists()`/`is_symlink()` before rendering.</violation>
</file>
<file name="references/sources.md">
<violation number="1" location="references/sources.md:25">
P3: The catalog silently fails on sources whose best-effort PTS are not strictly increasing: `catalog()` in helpers/source_scan.py raises `ValueError('source needs nonempty strictly increasing presentation timestamps')`. This sentence tells the reader PTS may vary, but not that equal or reordered timestamps make the command fail. Add the strictly-increasing requirement so an agent inspecting a VFR source learns the failure mode before running it.</violation>
<violation number="2" location="references/sources.md:40">
P3: `--tonemap` is rejected on sources that are not tagged PQ/HLG: prepare_source.py raises `ValueError('tonemap requires tagged PQ or HLG input')`. The doc only states the forward requirement (HDR needs `--tonemap`) and never warns that passing the flag on ordinary SDR footage is an error. A user following the advice to 'always pass --tonemap to be safe' would hit this. State that the flag is only valid on tagged HDR input.</violation>
</file>
<file name="helpers/source_scan.py">
<violation number="1" location="helpers/source_scan.py:16">
P3: `catalog()` raises a bare `StopIteration` when the input file has no video stream (e.g., an audio-only file), because `next(...)` has no default. The same pattern is duplicated in `selected_frames()`. Raise a descriptive ValueError naming the input instead.</violation>
<violation number="2" location="helpers/source_scan.py:33">
P3: `catalog()` raises a bare `KeyError` for any frame that ffprobe does not emit `best_effort_timestamp_time` for (frames without a decodable best-effort PTS, common with some containers/raw captures where ffprobe omits the key). Guard with `f.get(...)` and raise a ValueError naming the offending frame so the failure is attributable.</violation>
<violation number="3" location="helpers/source_scan.py:79">
P1: When the source contains multiple video streams, FFmpeg can decode a different stream than the `v:0` stream cataloged above, causing scene candidates and screenshot matches to report incorrect frames and PTS. Explicitly map `0:v:0` in this decode command.</violation>
<violation number="4" location="helpers/source_scan.py:96">
P3: The decoding subprocess discards ffmpeg's stderr, so any real ffmpeg failure (invalid filter expression, undecodable stream, codec error) surfaces only as the generic "source ended before native frame N" message, with no diagnostics. Capture stderr and include its tail when the read-size check fails; this matters for an agent-facing helper where the failure cause is otherwise invisible.</violation>
</file>
<file name="helpers/find_shot.py">
<violation number="1" location="helpers/find_shot.py:50">
P2: When `--every inf` is supplied, the positive-only check passes and `search` samples just the first frame. Reject non-finite intervals before advancing `next_time`.</violation>
<violation number="2" location="helpers/find_shot.py:74">
P3: When an exact match has `reprojection_px == 0.0`, the `or` expression replaces it with `1e9`, so it can rank below a worse candidate with the same inlier count. Check explicitly for `None`.</violation>
<violation number="3" location="helpers/find_shot.py:89">
P2: When `--out` names an existing report, the input-alias check passes and `save_json` truncates it. Reject existing output paths before running the search.</violation>
</file>
<file name="helpers/caption_raster.py">
<violation number="1" location="helpers/caption_raster.py:46">
P3: SRT stamps are the millisecond clock this helper guarantees, but `round()` rounds halves to even, so 1.0025 s formats as `,002` instead of `,003` — the exact banker's rounding the `edit_clock.nearest()` added in this same PR deliberately avoids. Use half-up rounding: `int(max(0.0, seconds) * 1000 + 0.5)`.</violation>
<violation number="2" location="helpers/caption_raster.py:72">
P2: A malformed timestamp in any SRT block aborts parsing and prevents all later valid cues from rendering. Catch the timestamp parsing error and continue to the next block.</violation>
<violation number="3" location="helpers/caption_raster.py:285">
P2: When the initial and minimum font sizes have different parity, the shrink loop skips the declared minimum size and can falsely reject captions that fit there. Iterate through the minimum size, or otherwise ensure it is tested.</violation>
<violation number="4" location="helpers/caption_raster.py:386">
P2: The returned FFconcat manifest fails with a normal `ffmpeg -f concat -i` invocation because its absolute file entries require `-safe 0`. Emit paths relative to the manifest, or make the required unsafe-mode invocation part of this helper's documented contract.</violation>
</file>
<file name="tests/test_caption_raster.py">
<violation number="1" location="tests/test_caption_raster.py:216">
P3: The ffmpeg/ffprobe calls in `test_ffmpeg_preserves_short_cue_boundaries` run with `check=True` but no `timeout`, so a stalled media process blocks the entire branch test suite with no fail-fast path. Add a `timeout` (e.g., 60) to both `subprocess.run` calls.</violation>
</file>
<file name="helpers/cards.py">
<violation number="1" location="helpers/cards.py:108">
P3: When a curved line is empty or all whitespace, the glyph loop skips every character, `out.getbbox()` returns None, and `crop(None)` returns the entire zero-filled canvas instead of raising. The result is a silent invisible card (full-size layer with alpha 0) rather than the "caption has no visible glyphs" error that `text_mask` raises for the straight-line path. Raise when no ink was placed so a bad manifest fails loudly and consistently.</violation>
<violation number="2" location="helpers/cards.py:228">
P2: The new `validate_mask` path is never used, so full-interval mask validation is dead and malformed masks can fail only during rendering. Call it for each card interval during renderer setup, or remove the unused validation path.</violation>
<violation number="3" location="helpers/cards.py:298">
P2: A negative `entry_frames` is accepted and produces the wrong animation timing. Require a nonnegative integer before computing `progress`.</violation>
<violation number="4" location="helpers/cards.py:314">
P2: NaN or Infinity animation values bypass these guards, then crash later with a cryptic error. `json.loads` (via `load_json`) accepts `NaN`/`Infinity` tokens, so `power`, `scale_from`, `blur_from`, and `blur_to` can be non-finite: NaN fails both `power <= 0` and `scale <= 0 or blur < 0` (NaN comparisons are False), and `Inf` is not `<= 0`. The NaN/Inf then reaches `round(original.width * scale)` (ValueError "cannot convert float NaN to integer") or `GaussianBlur`, after ffmpeg has already opened the output and a frame loop has started. Validate finiteness in the same guards that reject negative values, e.g. `if not math.isfinite(power) or power <= 0` and `if not math.isfinite(scale) or not math.isfinite(blur) or scale <= 0 or blur < 0`.</violation>
<violation number="5" location="helpers/cards.py:411">
P2: When `path` ends in `.log`, the movie output and encoder log share one file, corrupting or failing the export. Reject this path collision before opening the log.</violation>
<violation number="6" location="helpers/cards.py:450">
P3: When movie encoding fails (a caption that clips, or an ffmpeg error), ffmpeg has already created `path` at startup and this exception path kills it without removing the partial file. The next run of the same command then gets `FileExistsError("choose a new caption movie path")` for a path that only contains a broken partial file, forcing manual cleanup or an unrelated name. Delete the partial output (and log, unless it is intentionally retained) after the failed run so a retry stays on the same path.</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 the source contains multiple video streams, FFmpeg can decode a different stream than the v:0 stream cataloged above, causing scene candidates and screenshot matches to report incorrect frames and PTS. Explicitly map 0:v:0 in this decode command.
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 the source contains multiple video streams, FFmpeg can decode a different stream than the `v:0` stream cataloged above, causing scene candidates and screenshot matches to report incorrect frames and PTS. Explicitly map `0:v:0` in this decode command.</comment>
<file context>
@@ -0,0 +1,151 @@
+ "-threads",
+ "2",
+ "-i",
+ str(path),
+ "-an",
+ "-sn",
</file context>
| # convert seconds to a frame boundary using the requested rounding policy | ||
| def seconds_to_frame(seconds, fps=30, mode="nearest"): | ||
| """Convert seconds to a frame boundary using the requested rounding policy.""" | ||
| value = fraction(seconds) * fraction(fps) |
There was a problem hiding this comment.
P2: When a non-positive frame or sample rate is supplied, these helpers silently generate invalid timestamps instead of rejecting the clock. Validate fps and rate as positive before performing the conversions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/edit_clock.py, line 25:
<comment>When a non-positive frame or sample rate is supplied, these helpers silently generate invalid timestamps instead of rejecting the clock. Validate `fps` and `rate` as positive before performing the conversions.</comment>
<file context>
@@ -0,0 +1,87 @@
+# convert seconds to a frame boundary using the requested rounding policy
+def seconds_to_frame(seconds, fps=30, mode="nearest"):
+ """Convert seconds to a frame boundary using the requested rounding policy."""
+ value = fraction(seconds) * fraction(fps)
+ if mode == "nearest":
+ return nearest(value)
</file context>
| # map a frame boundary onto the audio sample clock | ||
| def frame_to_sample(frame, fps=30, rate=SAMPLE_RATE): | ||
| """Map a frame boundary onto the audio sample clock.""" | ||
| return nearest(Fraction(frame * rate, 1) / fraction(fps)) |
There was a problem hiding this comment.
P2: When callers provide a decimal sample rate, frame_to_sample raises before converting it. Normalize frame and rate with fraction() before forming the sample-clock numerator.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/edit_clock.py, line 38:
<comment>When callers provide a decimal sample rate, `frame_to_sample` raises before converting it. Normalize `frame` and `rate` with `fraction()` before forming the sample-clock numerator.</comment>
<file context>
@@ -0,0 +1,87 @@
+# map a frame boundary onto the audio sample clock
+def frame_to_sample(frame, fps=30, rate=SAMPLE_RATE):
+ """Map a frame boundary onto the audio sample clock."""
+ return nearest(Fraction(frame * rate, 1) / fraction(fps))
+
+
</file context>
Comment on lines
+63
to
+65
| dep: data["artifacts"][dep]["sha256"] for dep in entry.get("depends_on", []) | ||
| } | ||
| data["artifacts"][ident] = entry |
There was a problem hiding this comment.
P2: When record() receives a new entry with a missing or self dependency, this comprehension raises KeyError before validate() can reject the graph with its intended validation error. Validate the candidate graph before dereferencing dependency hashes, or explicitly preserve missing hashes until validate() runs.
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 63:
<comment>When `record()` receives a new entry with a missing or self dependency, this comprehension raises `KeyError` before `validate()` can reject the graph with its intended validation error. Validate the candidate graph before dereferencing dependency hashes, or explicitly preserve missing hashes until `validate()` runs.</comment>
<file context>
@@ -0,0 +1,126 @@
+ entry = dict(entry)
+ entry["sha256"] = sha256(path)
+ entry["dependency_hashes"] = {
+ dep: data["artifacts"][dep]["sha256"] for dep in entry.get("depends_on", [])
+ }
+ data["artifacts"][ident] = entry
</file context>
Suggested change
| dep: data["artifacts"][dep]["sha256"] for dep in entry.get("depends_on", []) | |
| } | |
| data["artifacts"][ident] = entry | |
| entry["dependency_hashes"] = { | |
| dep: data["artifacts"].get(dep, {}).get("sha256") | |
| for dep in entry.get("depends_on", []) | |
| } |
|
|
||
| return { | ||
| "artifacts": [ | ||
| {"id": ident, **row, "stale": stale(ident)} |
There was a problem hiding this comment.
P2: When an entry JSON contains an id field, view() reports that value instead of the artifact key supplied to record(), so show can identify the wrong evidence row. Put id after **row so the context key remains authoritative.
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 98:
<comment>When an entry JSON contains an `id` field, `view()` reports that value instead of the artifact key supplied to `record()`, so `show` can identify the wrong evidence row. Put `id` after `**row` so the context key remains authoritative.</comment>
<file context>
@@ -0,0 +1,126 @@
+
+ return {
+ "artifacts": [
+ {"id": ident, **row, "stale": stale(ident)}
+ for ident, row in rows.items()
+ if phase is None or row["phase"] == phase
</file context>
Suggested change
| {"id": ident, **row, "stale": stale(ident)} | |
| {**row, "id": ident, "stale": stale(ident)} |
| yy = round(pad + y - min(0, bend) - glyph.height / 2) | ||
| patch = out.crop((x, yy, x + glyph.width, yy + glyph.height)) | ||
| out.paste(ImageChops.lighter(patch, glyph), (x, yy)) | ||
| return out.crop(out.getbbox()) |
There was a problem hiding this comment.
P3: When a curved line is empty or all whitespace, the glyph loop skips every character, out.getbbox() returns None, and crop(None) returns the entire zero-filled canvas instead of raising. The result is a silent invisible card (full-size layer with alpha 0) rather than the "caption has no visible glyphs" error that text_mask raises for the straight-line path. Raise when no ink was placed so a bad manifest fails loudly and consistently.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/cards.py, line 108:
<comment>When a curved line is empty or all whitespace, the glyph loop skips every character, `out.getbbox()` returns None, and `crop(None)` returns the entire zero-filled canvas instead of raising. The result is a silent invisible card (full-size layer with alpha 0) rather than the "caption has no visible glyphs" error that `text_mask` raises for the straight-line path. Raise when no ink was placed so a bad manifest fails loudly and consistently.</comment>
<file context>
@@ -0,0 +1,482 @@
+ yy = round(pad + y - min(0, bend) - glyph.height / 2)
+ patch = out.crop((x, yy, x + glyph.width, yy + glyph.height))
+ out.paste(ImageChops.lighter(patch, glyph), (x, yy))
+ return out.crop(out.getbbox())
+
+
</file context>
Suggested change
| return out.crop(out.getbbox()) | |
| ink = out.getbbox() | |
| if ink is None: | |
| raise ValueError("caption has no visible glyphs") | |
| return out.crop(ink) |
|
|
||
| # format output seconds on the SRT millisecond clock | ||
| def srt_timestamp(seconds: float) -> str: | ||
| total_ms = int(round(max(0.0, seconds) * 1000)) |
There was a problem hiding this comment.
P3: SRT stamps are the millisecond clock this helper guarantees, but round() rounds halves to even, so 1.0025 s formats as ,002 instead of ,003 — the exact banker's rounding the edit_clock.nearest() added in this same PR deliberately avoids. Use half-up rounding: int(max(0.0, seconds) * 1000 + 0.5).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/caption_raster.py, line 46:
<comment>SRT stamps are the millisecond clock this helper guarantees, but `round()` rounds halves to even, so 1.0025 s formats as `,002` instead of `,003` — the exact banker's rounding the `edit_clock.nearest()` added in this same PR deliberately avoids. Use half-up rounding: `int(max(0.0, seconds) * 1000 + 0.5)`.</comment>
<file context>
@@ -0,0 +1,510 @@
+
+# format output seconds on the SRT millisecond clock
+def srt_timestamp(seconds: float) -> str:
+ total_ms = int(round(max(0.0, seconds) * 1000))
+ hours, remainder = divmod(total_ms, 3_600_000)
+ minutes, remainder = divmod(remainder, 60_000)
</file context>
Suggested change
| total_ms = int(round(max(0.0, seconds) * 1000)) | |
| total_ms = int(max(0.0, seconds) * 1000 + 0.5) |
| "pipe:1", | ||
| ], | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.DEVNULL, |
There was a problem hiding this comment.
P3: The decoding subprocess discards ffmpeg's stderr, so any real ffmpeg failure (invalid filter expression, undecodable stream, codec error) surfaces only as the generic "source ended before native frame N" message, with no diagnostics. Capture stderr and include its tail when the read-size check fails; this matters for an agent-facing helper where the failure cause is otherwise invisible.
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 96:
<comment>The decoding subprocess discards ffmpeg's stderr, so any real ffmpeg failure (invalid filter expression, undecodable stream, codec error) surfaces only as the generic "source ended before native frame N" message, with no diagnostics. Capture stderr and include its tail when the read-size check fails; this matters for an agent-facing helper where the failure cause is otherwise invisible.</comment>
<file context>
@@ -0,0 +1,151 @@
+ "pipe:1",
+ ],
+ stdout=subprocess.PIPE,
+ stderr=subprocess.DEVNULL,
+ )
+ try:
</file context>
| def catalog(path): | ||
| """Record native presentation timestamps and the source file fingerprint.""" | ||
| data = probe(path) | ||
| stream = next(s for s in data["streams"] if s["codec_type"] == "video") |
There was a problem hiding this comment.
P3: catalog() raises a bare StopIteration when the input file has no video stream (e.g., an audio-only file), because next(...) has no default. The same pattern is duplicated in selected_frames(). Raise a descriptive ValueError naming the input instead.
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 16:
<comment>`catalog()` raises a bare `StopIteration` when the input file has no video stream (e.g., an audio-only file), because `next(...)` has no default. The same pattern is duplicated in `selected_frames()`. Raise a descriptive ValueError naming the input instead.</comment>
<file context>
@@ -0,0 +1,151 @@
+def catalog(path):
+ """Record native presentation timestamps and the source file fingerprint."""
+ data = probe(path)
+ stream = next(s for s in data["streams"] if s["codec_type"] == "video")
+ frames = json.loads(
+ run(
</file context>
| ] | ||
| ).stdout | ||
| )["frames"] | ||
| pts = [float(f["best_effort_timestamp_time"]) for f in frames] |
There was a problem hiding this comment.
P3: catalog() raises a bare KeyError for any frame that ffprobe does not emit best_effort_timestamp_time for (frames without a decodable best-effort PTS, common with some containers/raw captures where ffprobe omits the key). Guard with f.get(...) and raise a ValueError naming the offending frame so the failure is attributable.
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 33:
<comment>`catalog()` raises a bare `KeyError` for any frame that ffprobe does not emit `best_effort_timestamp_time` for (frames without a decodable best-effort PTS, common with some containers/raw captures where ffprobe omits the key). Guard with `f.get(...)` and raise a ValueError naming the offending frame so the failure is attributable.</comment>
<file context>
@@ -0,0 +1,151 @@
+ ]
+ ).stdout
+ )["frames"]
+ pts = [float(f["best_effort_timestamp_time"]) for f in frames]
+ if not pts or any(b <= a for a, b in zip(pts, pts[1:])):
+ raise ValueError(
</file context>
Suggested change
| pts = [float(f["best_effort_timestamp_time"]) for f in frames] | |
| pts = [] | |
| for i, f in enumerate(frames): | |
| if "best_effort_timestamp_time" not in f: | |
| raise ValueError(f"frame {i} lacks a best-effort presentation timestamp") | |
| pts.append(float(f["best_effort_timestamp_time"])) |
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
Captions should be easy to read and appear at the right time. These helpers let the agent style captions and animated word cards, check that the text fits, and export transparent layers for a video.
Builds on #164 for shared media and timing helpers.
Changes
assets/fonts/. Protect existing outputs and document both helpers in five focused commits.Limits
Alfa Slab One is the default font. Inter Tight is not bundled; projects naming it must select another font or supply their own appropriately licensed file. The legacy
@skill/assets/fonts/alias remains supported for retained bundled fonts.These are standalone rendering helpers. Wiring them into the main renderer and the ASS caption module from #147 is deferred to the Rendering PR. They do not transcribe speech or create subject masks.
Overlapping SRT cues are handled sequentially. Source-to-edit offsets still use declared clip durations, so this does not resolve #161. Successful RAQM text shaping needs a separate equipped environment; its missing-dependency behavior is tested here.
Styling and raster-caption capabilities overlap #159, #15, #103, #120 and #63. Their caption interfaces still need coordination before the renderer integration.