Conversation
Rule 5 computes a cue's output time as `word.start - segment_start + segment_offset`, and `segment_offset` has to be where the segment actually starts in the concatenated output. `build_master_srt` accumulated it as `seg_offset += (end - start)` from the EDL. But `extract_segment` trims with `-ss`/`-t` and the result is quantised to whole frames, so every clip is a fraction of a frame longer than that arithmetic says. The error is one-directional and accumulates: on a 30-segment, 3m34s edit the captions ran 0.598s early by the final cue (0.000s at the first, growing monotonically). Two things make it easy to miss: the first cue is always correct, so a spot check at the top of a video looks fine; and a short test EDL never accumulates past a frame or two. `build_master_srt` now takes the extracted clips and measures them with ffprobe. They already exist when it is called — it runs after the concat — so this costs one ffprobe per segment and no extra encoding. If the argument is omitted, the clip count disagrees with the range count, or a probe fails, it falls back to the previous float arithmetic and says so. Verified: on a real 30-segment EDL, with no `segment_paths` the generated SRT is byte-identical to the current implementation (305 cues). Four new tests cover the measured path, the EDL path, and both fallbacks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 2 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/render.py">
<violation number="1" location="helpers/render.py:450">
P3: When `segment_paths=[]` is supplied for an EDL containing ranges, this truthiness check skips the count-mismatch warning and silently falls back to EDL durations. Check for `None` so an omitted argument remains distinct from an empty, mismatched list.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| # that error: on a 30-segment, 3m34s edit the captions ran 0.598s early by the | ||
| # final cue. Measure the rendered clips when they are available. | ||
| measured: list[float] | None = None | ||
| if segment_paths: |
There was a problem hiding this comment.
P3: When segment_paths=[] is supplied for an EDL containing ranges, this truthiness check skips the count-mismatch warning and silently falls back to EDL durations. Check for None so an omitted argument remains distinct from an empty, mismatched list.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/render.py, line 450:
<comment>When `segment_paths=[]` is supplied for an EDL containing ranges, this truthiness check skips the count-mismatch warning and silently falls back to EDL durations. Check for `None` so an omitted argument remains distinct from an empty, mismatched list.</comment>
<file context>
@@ -416,24 +426,49 @@ def _words_in_range(transcript: dict, t_start: float, t_end: float) -> list[dict
+ # that error: on a 30-segment, 3m34s edit the captions ran 0.598s early by the
+ # final cue. Measure the rendered clips when they are available.
+ measured: list[float] | None = None
+ if segment_paths:
+ if len(segment_paths) != len(edl["ranges"]):
+ print(f" warning: {len(segment_paths)} clips for {len(edl['ranges'])} ranges;"
</file context>
Suggested change
| if segment_paths: | |
| if segment_paths is not None: |
shoaib90
added a commit
to shoaib90/video-use
that referenced
this pull request
Sep 8, 2026
…t files `balance` ran after the greedy loop had already capped every chunk at `words_per_chunk`, so `ceil(len(run)/words_per_chunk)` was always 1 and the pass did nothing. The caption improvement measured on the YT1 edit came entirely from `break_on` and `min_words`. It now splits the punctuation-delimited runs before the cap is applied, which is what the docstring claimed. On that edit it changes 83 cues (38 of them pinned at the 8-word cap, tail out to 11 words) into 95 cues with a 5-7 word spread. Caught by a unit test written for the upstream PR — the two test files from browser-use#159 and browser-use#161 are brought over here too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 17, 2026
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
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.
The bug
Hard Rule 5 computes a cue's output time as
word.start - segment_start + segment_offset, andsegment_offsethas to be where the segment actually starts in the concatenated output.build_master_srtaccumulates it from the EDL instead:But
extract_segmenttrims with-ss/-t, and the result is quantised to whole frames — every clip comes out a fraction of a frame longer than that arithmetic says. The error is one-directional, so it accumulates.On a real 30-segment, 3m34s talking-head edit the captions ran 0.598s early by the final cue:
Two things make this easy to miss:
The fix
build_master_srttakes the extracted clips and measures them withffprobe. They already exist when it is called — it runs after the concat — so this costs oneffprobeper segment and no extra encoding.It degrades safely. If
segment_pathsis omitted, the clip count disagrees with the range count, or a probe fails, it falls back to the previous arithmetic and prints why.Verification
build_master_srtwithoutsegment_pathsproduces an SRT that is byte-identical to the current implementation (305 cues). Existing callers are unaffected.tests/test_render_caption_offsets.pycover the measured path, the EDL path, a clip/range count mismatch, and a failed probe.Note on overlap
This touches
build_master_srt, which #159 also changes. They are independent — this is the offset arithmetic, #159 is chunking and style — but whichever lands second will need a trivial rebase.Summary by cubic
Fixes caption offset drift in
build_master_srtby measuring the rendered segments with ffprobe instead of trusting EDL durations. Each extracted clip is quantized to whole frames and comes out slightly longer than the EDL'send - start, so the one-directional error accumulates—captions ran 0.598s early by the final cue on a 30-segment, 3m34s edit.Details
build_master_srttakes an optionalsegment_pathsargument; probes each clip for its real duration, costing one ffprobe per segment and no re-encoding.segment_paths, output is byte-identical to the previous implementation; existing callers are unaffected.Written for commit 97c35b5. Summary will update on new commits.