3.1.0: review the cut on a virtual timeline, not a render - #10
Merged
Conversation
Reviewing a cut no longer means waiting for an encode. All of this came out of one real 20-minute 4K project whose gate-2 preview took 22 minutes to render, twice, because the first one failed validation at the finish line. New edl_to_ffconcat.py writes the EDL as a virtual timeline (ffconcat, and optionally an mpv EDL) that plays the cut with no encode at all. On a 379-segment 16-minute cut it resolves in 33ms against 22 minutes for the equivalent render, verified pixel-identical to source ground truth at four points including random seeks. mc-cut now builds that at gate 2 and renders no file; a file is rendered only when something needs a file. Preview proxies are now all-intra, which is a correctness fix rather than a speed tweak: the concat demuxer can only cut on keyframes, so against a long-GOP proxy the timeline silently plays the wrong frames while reporting the exact right duration. Measured at an 8.333s GOP against segments averaging 2.51s, every segment began up to 8.3s early. Proxy paths and sidecars now carry the encode family and a recipe so an older proxy can never be reused as if it were intra. Two EDL bugs that only appear on long cuts are now cutting rules: boundaries must be quantized to the source frame grid (sub-frame times accumulate past the output gate's tolerance beyond ~150 segments), and a trailing silence must be truncated rather than tightened (which otherwise scrapes a junk segment off the end of the file). Applying approved cuts now says out loud that everything derived from the EDL is stale, including beat anchors once the beats stage has run, with an explicit regeneration order and a required anchor re-check before graphics continue. A concat stream copy renders the same frames ~470x faster but is deliberately not the preview path: it emits duplicate DTS on sub-frame-length segments and fails render_preview's own decode validation. It is kept, tested and documented for callers that accept the caveat.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
WalkthroughThe cut pipeline now creates all-intra proxies and ffconcat virtual timelines for gate-2 review, validates keyframe compatibility, tracks proxy recipes, and documents regeneration and EDL correctness rules. ChangesCut review pipeline
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CutWorkflow
participant render_preview.py
participant composite_core.py
participant edl_to_ffconcat.py
participant ffplay
CutWorkflow->>render_preview.py: request proxy-only preview
render_preview.py->>composite_core.py: build or reuse all-intra proxies
CutWorkflow->>edl_to_ffconcat.py: convert cut/edl.json
edl_to_ffconcat.py-->>CutWorkflow: write virtual timeline
CutWorkflow->>ffplay: review ffconcat playlist
Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR replaces the gate-2 rendered preview with an ffconcat virtual timeline (
Confidence Score: 4/5Safe to merge once the CLAUDE.md invariant block is updated and the optional path-quoting and flag-conflict fixes are applied. The core virtual-timeline path and the all-intra proxy correctness fix are well-reasoned and well-tested. The CLAUDE.md invariant block still describes 'render-first' while SKILL.md and the scripts now implement 'virtual-timeline first, render only when needed' — a future contributor following the module's binding context would conflict with what the pipeline actually does. Two smaller issues: --proxy-only combined with --no-proxy exits 0 with an empty proxy map (misleading success), and ffconcat_lines single-quote path wrapping would produce a malformed playlist for any source path containing a single quote. Files Needing Attention: CLAUDE.md (design invariant section); skills/mc-cut/scripts/edl_to_ffconcat.py (ffconcat_lines path quoting); skills/mc-cut/scripts/render_preview.py (proxy-only + no-proxy guard)
|
| Filename | Overview |
|---|---|
| skills/mc-cut/scripts/edl_to_ffconcat.py | New script generating ffconcat and mpv EDL virtual timelines; logic is sound, but single-quote path escaping is missing in ffconcat_lines |
| skills/mc-cut/scripts/render_preview.py | Adds --proxy-only mode and lane_mode tracking; --proxy-only silently succeeds when combined with --no-proxy, returning an empty proxy map |
| skills/mc-cut/scripts/composite_core.py | Adds all-intra proxy support, recipe-based sidecar validation, and build_streamcopy_command; changes are well-documented and the path naming prevents intra/long-GOP collision |
| skills/mc-cut/scripts/tests/test-edl_to_ffconcat.py | 17 well-structured tests covering pure builders, byte-length mpv quoting, all-intra guard by behavior, and error paths; no issues |
| skills/mc-cut/scripts/tests/test-render_preview.py | Adds coverage for intra flags, path separation, stream-copy command, and stale-recipe invalidation; tests are accurate and match new composite_core API |
| skills/mc-cut/SKILL.md | Correctly updated to reflect virtual-timeline-first workflow, EDL-change invalidation section, and updated cutting rules; prose is clear and operator-directed |
| CLAUDE.md | Design invariant section still reads 'render-first' despite PR replacing gate-2 renders with a virtual timeline; creates a contradiction with SKILL.md for future contributors |
| skills/mc-cut/references/rendering.md | Updated to document the virtual-timeline workflow, all-intra correctness requirement, and stream-copy caveat; accurate and consistent with script changes |
Sequence Diagram
sequenceDiagram
participant MC as mc-cut skill
participant RP as render_preview.py
participant EF as edl_to_ffconcat.py
participant CC as composite_core.py
participant FP as ffplay/mpv
MC->>RP: --proxy-only (build all-intra proxy)
RP->>CC: "build_proxy_command(intra=True)"
CC-->>RP: ffmpeg -g 1 -keyint_min 1 -sc_threshold 0
RP->>CC: "write_proxy_sidecar(recipe=intra-v1)"
RP-->>MC: ok proxy-only
MC->>EF: edl.json -o preview.ffconcat --source proxy-intra.mp4
EF->>EF: is_all_intra(proxy) → True
EF->>EF: ffconcat_lines + mpv_edl_lines
EF-->>MC: "ok segments all_intra=true"
MC->>FP: ffplay -f concat -safe 0 -i preview.ffconcat
FP-->>MC: plays cut (no encode, frame-exact)
Note over MC,FP: Render a file only when needed
MC->>RP: edl.json -o preview.mp4 (full render)
RP->>CC: build_command / build_lane_composite_command
RP->>CC: validate_render + publish_render (atomic)
RP-->>MC: "validated lane=encode"
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
skills/mc-cut/scripts/render_preview.py:276-280
**`--proxy-only` silently succeeds with `--no-proxy`, building nothing**
When both `--proxy-only` and `--no-proxy` are passed together, `proxy_map` remains `{}` (the `ensure_proxies` block is skipped), and the script prints `{"ok": true, "mode": "proxy-only", "proxies": {}}` and exits 0. The caller sees a success response with an empty proxy map and has no indication that no proxy was actually built. Adding an early guard makes the conflict explicit.
```suggestion
if args.proxy_only:
if args.no_proxy:
print("error: --proxy-only and --no-proxy are mutually exclusive",
file=sys.stderr)
return 2
print(json.dumps({"ok": True, "mode": "proxy-only",
"proxies": {k: str(v) for k, v in
proxy_map.items()}}, indent=2))
return 0
```
### Issue 2 of 2
skills/mc-cut/scripts/edl_to_ffconcat.py:69-74
**Single quotes in source paths produce a malformed ffconcat file**
`ffconcat_lines` wraps every source path in single quotes: `file '/path/to/source.mp4'`. If a resolved path contains a single quote (e.g., `/footage/it's live.mp4`), ffmpeg's concat demuxer receives an unterminated quoted string and rejects the playlist. The mpv EDL uses length-prefix quoting (`%len%path`) which handles arbitrary characters correctly; the ffconcat path needs a similar escape.
```suggestion
def ffconcat_lines(segments, source_override=None):
"""The ffconcat v1.0 body for these segments (pure)."""
out = ["ffconcat version 1.0"]
for seg in segments:
src = source_override or seg["source"]
escaped = str(src).replace("'", "\\'")
out.append(f"file '{escaped}'")
```
Reviews (1): Last reviewed commit: "Release 3.1.0: review the cut on a virtu..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+276
to
+280
| if args.proxy_only: | ||
| print(json.dumps({"ok": True, "mode": "proxy-only", | ||
| "proxies": {k: str(v) for k, v in | ||
| proxy_map.items()}}, indent=2)) | ||
| return 0 |
There was a problem hiding this comment.
--proxy-only silently succeeds with --no-proxy, building nothing
When both --proxy-only and --no-proxy are passed together, proxy_map remains {} (the ensure_proxies block is skipped), and the script prints {"ok": true, "mode": "proxy-only", "proxies": {}} and exits 0. The caller sees a success response with an empty proxy map and has no indication that no proxy was actually built. Adding an early guard makes the conflict explicit.
Suggested change
| if args.proxy_only: | |
| print(json.dumps({"ok": True, "mode": "proxy-only", | |
| "proxies": {k: str(v) for k, v in | |
| proxy_map.items()}}, indent=2)) | |
| return 0 | |
| if args.proxy_only: | |
| if args.no_proxy: | |
| print("error: --proxy-only and --no-proxy are mutually exclusive", | |
| file=sys.stderr) | |
| return 2 | |
| print(json.dumps({"ok": True, "mode": "proxy-only", | |
| "proxies": {k: str(v) for k, v in | |
| proxy_map.items()}}, indent=2)) | |
| return 0 |
Prompt To Fix With AI
This is a comment left during a code review.
Path: skills/mc-cut/scripts/render_preview.py
Line: 276-280
Comment:
**`--proxy-only` silently succeeds with `--no-proxy`, building nothing**
When both `--proxy-only` and `--no-proxy` are passed together, `proxy_map` remains `{}` (the `ensure_proxies` block is skipped), and the script prints `{"ok": true, "mode": "proxy-only", "proxies": {}}` and exits 0. The caller sees a success response with an empty proxy map and has no indication that no proxy was actually built. Adding an early guard makes the conflict explicit.
```suggestion
if args.proxy_only:
if args.no_proxy:
print("error: --proxy-only and --no-proxy are mutually exclusive",
file=sys.stderr)
return 2
print(json.dumps({"ok": True, "mode": "proxy-only",
"proxies": {k: str(v) for k, v in
proxy_map.items()}}, indent=2))
return 0
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+69
to
+74
| def ffconcat_lines(segments, source_override=None): | ||
| """The ffconcat v1.0 body for these segments (pure).""" | ||
| out = ["ffconcat version 1.0"] | ||
| for seg in segments: | ||
| src = source_override or seg["source"] | ||
| out.append(f"file '{src}'") |
There was a problem hiding this comment.
ffconcat_lines wraps every source path in single quotes: file '/path/to/source.mp4'. If a resolved path contains a single quote (e.g., /footage/it's live.mp4), ffmpeg's concat demuxer receives an unterminated quoted string and rejects the playlist. The mpv EDL uses length-prefix quoting (%len%path) which handles arbitrary characters correctly; the ffconcat path needs a similar escape.
Suggested change
| def ffconcat_lines(segments, source_override=None): | |
| """The ffconcat v1.0 body for these segments (pure).""" | |
| out = ["ffconcat version 1.0"] | |
| for seg in segments: | |
| src = source_override or seg["source"] | |
| out.append(f"file '{src}'") | |
| def ffconcat_lines(segments, source_override=None): | |
| """The ffconcat v1.0 body for these segments (pure).""" | |
| out = ["ffconcat version 1.0"] | |
| for seg in segments: | |
| src = source_override or seg["source"] | |
| escaped = str(src).replace("'", "\\'") | |
| out.append(f"file '{escaped}'") |
Prompt To Fix With AI
This is a comment left during a code review.
Path: skills/mc-cut/scripts/edl_to_ffconcat.py
Line: 69-74
Comment:
**Single quotes in source paths produce a malformed ffconcat file**
`ffconcat_lines` wraps every source path in single quotes: `file '/path/to/source.mp4'`. If a resolved path contains a single quote (e.g., `/footage/it's live.mp4`), ffmpeg's concat demuxer receives an unterminated quoted string and rejects the playlist. The mpv EDL uses length-prefix quoting (`%len%path`) which handles arbitrary characters correctly; the ffconcat path needs a similar escape.
```suggestion
def ffconcat_lines(segments, source_override=None):
"""The ffconcat v1.0 body for these segments (pure)."""
out = ["ffconcat version 1.0"]
for seg in segments:
src = source_override or seg["source"]
escaped = str(src).replace("'", "\\'")
out.append(f"file '{escaped}'")
```
How can I resolve this? If you propose a fix, please make it concise.
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.
Reviewing a cut no longer means waiting for an encode.
All of this came out of one real 20-minute 4K project (
fourth-wave-of-software, 379 segments) whose gate-2 preview took 22 minutes to render — twice, because the first one failed validation at the finish line. Every number below is measured on that project, not estimated.You don't render a file to review a cut any more
New
edl_to_ffconcat.pywrites the EDL as a virtual timeline — an ffconcat playlist (and optionally an mpv EDL) that plays the cut with no encode at all:It carries video and audio, so A/V sync is reviewable — which an audio-only preview can't show.
mc-cutnow builds this at gate 2 and renders no file. A file gets rendered only when something actually needs one: a composited preview once overlays exist, something to share, or the gate-4 final.render_preview.py --proxy-onlybuilds the proxy without rendering.All-intra proxies — a correctness fix, not a speed tweak
The concat demuxer can only cut on keyframes. Against a long-GOP proxy the virtual timeline silently plays the wrong frames while reporting the exact right duration — measured at an 8.333s GOP against segments averaging 2.51s (163 of 379 under 2s), every segment began up to 8.3s early. That passes every check except a frame comparison.
So proxies are now
-g 1 -keyint_min 1 -sc_threshold 0, proxy paths carry the encode family (<stem>-720p-intra.mp4), and sidecars record a recipe so an older proxy can never be reused as if it were intra.edl_to_ffconcat.pyrefuses a non-intra source unless you pass--allow-long-gop.Cost: ~10x the disk (698MB vs 65MB for 20 min at 720p), built once per source.
Two EDL bugs that only show up on long cuts
trimto a frame and the error accumulates. This cut rendered 954.766s against an expected 952.460s and the output gate correctly refused to publish. Past ~150 segments it happens every time.source_durationonce aligned.Both are now cutting rules.
Applying approved cuts invalidates everything downstream — loudly
The moment
cut/edl.jsonis rewritten, every derived artifact is stale: the timeline, the FCPXML, the edited transcript, the preview, the boundary frames, and every beat anchor inbeats/beats.md. Nothing on disk announces this — a stale FCPXML imports cleanly, and a stale beat table renders overlays that land off their phrase, discovered after the graphics are paid for.mc-cutnow spells out the regeneration order, runsverify_edl.pyfirst with its non-zero exit stopping the rest, and requires the beat anchor check to re-run and be reported before graphics continue.Deliberately not shipped
A concat stream copy off the intra proxy renders the same frames ~470x faster (2.8s vs 22min, 340x realtime). It is not the preview path: it emits duplicate DTS wherever a segment runs a frame or two, and the file fails
render_preview.py's own decode validation — 125 errors, and 3 with every timestamp remedy applied (+genpts,avoid_negative_ts,video_track_timescale,fps_mode passthrough,muxdelay 0), never zero, from just 2 sub-0.1s segments.Publishing it would mean loosening a correctness gate to buy a speed number.
composite_core.build_streamcopy_commandkeeps it — tested and documented — for a caller that accepts the caveat. The speed moved to the virtual timeline instead, which is both faster and correct.Testing
14 test suites pass. New
test-edl_to_ffconcat.py(17 tests) covers the pure builders, mpv byte-length path quoting, and the all-intra guard by behaviour — including that a refused run leaves no file behind.test-render_preview.pygains coverage for the intra flags, intra/long-GOP path separation, the stream-copy command, and stale-recipe invalidation.Verified end to end on the live project: proxy reuse, timeline generation, the long-GOP guard (exit 1, no file written), the
--allow-long-gopescape hatch, and frame accuracy against source ground truth.Summary by CodeRabbit
New Features
Bug Fixes
Documentation