feat(render): mixed-orientation canvas + music-bed montage support - #153
Open
Pcecil21 wants to merge 5 commits into
Open
feat(render): mixed-orientation canvas + music-bed montage support#153Pcecil21 wants to merge 5 commits into
Pcecil21 wants to merge 5 commits into
Conversation
render.py scaled portrait sources by height (-2:1920) and landscape by
width (1920:-2), so a montage mixing phone verticals with landscape
footage produced segments of different dimensions — which the `-c copy`
concat in Rule 2 cannot join. There was also no music-bed support, so a
montage whose spine is a song had no path through this renderer.
Two opt-in EDL fields, both off by default so existing EDLs are unchanged:
"canvas": "fill" — render every segment onto one fixed landscape
canvas: a zoomed blurred copy fills it edge-to-edge
with the sharp aspect-correct frame centred on top.
Portrait gets a blurred pillarbox, not black bars,
and every segment shares dimensions so the lossless
concat still works.
"music": {...} — mix a bed under the concatenated natural audio,
looped if short and cut to length if long. Mixed
after concat with -c:v copy, so it costs no extra
video encode. amix uses normalize=0, without which
amix rescales by 1/n and the explicit gains stop
meaning anything.
Per-range "mute": true silences a segment's natural audio while keeping
the stream, so a music bed can cover it.
Also fixes a latent bug this surfaced: a source with no audio stream (an
exported photo — the most common ingredient in a music montage) crashed
the render, because the mix had no [0:a] to duck under. Silence is now
synthesized via anullsrc for any source lacking audio, which also closes
a hole where a silent clip among audio clips left gaps in the
concatenated audio timeline.
Verified end-to-end, not just by mocks: a 1920x1080 + 608x1080 EDL with a
looped bed renders all segments to 1920x1080; a muted segment measures
-91.0 dB without music and -19.5 dB with it; and the unchanged non-canvas
path reproduces its prior loudnorm measurement exactly (I=-22.64 LUFS,
TP=-8.74, LRA=3.20).
Tests: 28 -> 52.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CqL3Pwe5Xy2rmrsSzRU9Zg
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
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:930">
P3: The `_music.mp4` intermediate (and the original `base.mp4`) are left behind after the render. Every other intermediate (`_concat.txt`, `.prenorm.mp4`) is explicitly cleaned up, so this is inconsistent and leaves stale large files in `edit_dir/` on every music render. Unlink `mixed_path` after `base_path` is switched, matching the existing cleanup pattern.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| # Video is stream-copied here, so this costs no extra video encode. | ||
| music_cfg = edl.get("music") | ||
| if music_cfg and not args.no_music: | ||
| mixed_path = base_path.with_name(base_path.stem + "_music.mp4") |
There was a problem hiding this comment.
P3: The _music.mp4 intermediate (and the original base.mp4) are left behind after the render. Every other intermediate (_concat.txt, .prenorm.mp4) is explicitly cleaned up, so this is inconsistent and leaves stale large files in edit_dir/ on every music render. Unlink mixed_path after base_path is switched, matching the existing cleanup pattern.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/render.py, line 930:
<comment>The `_music.mp4` intermediate (and the original `base.mp4`) are left behind after the render. Every other intermediate (`_concat.txt`, `.prenorm.mp4`) is explicitly cleaned up, so this is inconsistent and leaves stale large files in `edit_dir/` on every music render. Unlink `mixed_path` after `base_path` is switched, matching the existing cleanup pattern.</comment>
<file context>
@@ -738,6 +923,15 @@ def main() -> None:
+ # Video is stream-copied here, so this costs no extra video encode.
+ music_cfg = edl.get("music")
+ if music_cfg and not args.no_music:
+ mixed_path = base_path.with_name(base_path.stem + "_music.mp4")
+ mix_music_bed(base_path, music_cfg, mixed_path, edit_dir)
+ if mixed_path.exists():
</file context>
Three defects that only surfaced once real footage went through the montage path — synthetic clips cut from a single source share codec parameters and are always moving video, so none of them appeared. 1. A mono source collapsed the whole reel to mono. The concat demuxer takes its parameters from the FIRST segment, so one mono clip (a LiveBarn arena export) silently downgraded the entire timeline — including the stereo music bed mixed on afterwards. render.py already forced a uniform sample rate but never a channel count; now -ac 2. 2. Still images produced a segment with no video. A JPEG decodes as a single frame, so without -loop 1 the segment reported a duration borrowed from its synthesized audio while containing no real video. A 2s + 3s montage came out 9.42s instead of 5.0s. Stills are now looped, and -ss is skipped for them (seeking into one frame yields nothing). 3. Stills carry no audio, so they route through the synthesized-silence path already added for silent video sources. Verified with a real 1920x1080 mono LiveBarn export and real iPhone stills (4032x3024, EXIF orientation 6): segments now come out uniformly 1920x1080 stereo, the photo segment holds 90 frames over 3.009s, and the montage totals 5.10s as specified. ffmpeg applies EXIF orientation to stills on its own, so portrait photos land upright on the canvas. Tests: 52 -> 61. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CqL3Pwe5Xy2rmrsSzRU9Zg
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
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_montage.py">
<violation number="1" location="tests/test_render_montage.py:303">
P2: The two new test classes are defined after the `if __name__ == "__main__": unittest.main()` block, so running `python tests/test_render_montage.py` executes only the 24 earlier tests and prints a green "OK" while the 9 new regression tests never run (they only run under `unittest discover`). A direct run therefore gives false confidence that the real-footage regressions are covered. Move the `if __name__ == "__main__": unittest.main()` guard to the end of the file, after both new classes.</violation>
</file>
<file name="helpers/render.py">
<violation number="1" location="helpers/render.py:225">
P2: is_still_image() keys off codec_name alone, but ffprobe reports "mjpeg" for both a JPEG still and a Motion JPEG video stream, and "gif" for animated GIFs. A real MJPEG/.avi or animated-GIF source is therefore treated as a still: extract_segment drops `-ss`, never decodes the actual clip, and emits `-loop 1` for a non-image demuxer — silently corrupting that segment or failing the render. Distinguish single-frame stills by probing frame count or the image2 format instead of codec name alone.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| unittest.main() | ||
|
|
||
|
|
||
| class RealFootageRegressionTests(unittest.TestCase): |
There was a problem hiding this comment.
P2: The two new test classes are defined after the if __name__ == "__main__": unittest.main() block, so running python tests/test_render_montage.py executes only the 24 earlier tests and prints a green "OK" while the 9 new regression tests never run (they only run under unittest discover). A direct run therefore gives false confidence that the real-footage regressions are covered. Move the if __name__ == "__main__": unittest.main() guard to the end of the file, after both new classes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_render_montage.py, line 303:
<comment>The two new test classes are defined after the `if __name__ == "__main__": unittest.main()` block, so running `python tests/test_render_montage.py` executes only the 24 earlier tests and prints a green "OK" while the 9 new regression tests never run (they only run under `unittest discover`). A direct run therefore gives false confidence that the real-footage regressions are covered. Move the `if __name__ == "__main__": unittest.main()` guard to the end of the file, after both new classes.</comment>
<file context>
@@ -297,3 +298,88 @@ def test_per_range_mute_is_passed_through(self):
unittest.main()
+
+
+class RealFootageRegressionTests(unittest.TestCase):
+ """Three defects found only by feeding real footage through the renderer.
+
</file context>
| "-show_entries", "stream=codec_name", "-of", "csv=p=0", str(path)], | ||
| check=True, capture_output=True, text=True, | ||
| ).stdout.strip().lower() | ||
| return out in STILL_IMAGE_CODECS |
There was a problem hiding this comment.
P2: is_still_image() keys off codec_name alone, but ffprobe reports "mjpeg" for both a JPEG still and a Motion JPEG video stream, and "gif" for animated GIFs. A real MJPEG/.avi or animated-GIF source is therefore treated as a still: extract_segment drops -ss, never decodes the actual clip, and emits -loop 1 for a non-image demuxer — silently corrupting that segment or failing the render. Distinguish single-frame stills by probing frame count or the image2 format instead of codec name alone.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/render.py, line 225:
<comment>is_still_image() keys off codec_name alone, but ffprobe reports "mjpeg" for both a JPEG still and a Motion JPEG video stream, and "gif" for animated GIFs. A real MJPEG/.avi or animated-GIF source is therefore treated as a still: extract_segment drops `-ss`, never decodes the actual clip, and emits `-loop 1` for a non-image demuxer — silently corrupting that segment or failing the render. Distinguish single-frame stills by probing frame count or the image2 format instead of codec name alone.</comment>
<file context>
@@ -204,6 +204,29 @@ def blurred_fill_chain(width: int, height: int, sigma: int = CANVAS_BLUR_SIGMA)
+ "-show_entries", "stream=codec_name", "-of", "csv=p=0", str(path)],
+ check=True, capture_output=True, text=True,
+ ).stdout.strip().lower()
+ return out in STILL_IMAGE_CODECS
+ except (subprocess.CalledProcessError, FileNotFoundError):
+ return False
</file context>
Two silent-wrong defects in the music-bed path, both raised by automated review of the montage PR. Each renders successfully and only reveals itself when you listen to the end. 1. A source whose audio stream is shorter than its video range left the tail of the reel unscored. amix's `duration=first` ended the mixed output at the natural-audio endpoint, so the picture kept running in silence. Reproduced with a 6s clip carrying 2s of audio: the output had 6.0s of video and 2.03s of audio, the last four seconds silent. Fixed at the root — every segment's audio is now padded with `apad` and bounded by `-t`, so audio always runs exactly as long as its video. That also closes a hole this left in the concatenated audio timeline. Defence in depth on top: the mix uses `duration=longest`, with `-shortest` (already required, since the bed loops indefinitely) bounding the output at the video's length. 2. A missing bed source on a rerender silently scored the reel with the PREVIOUS render's music. mix_music_bed writes nothing when the source is absent, but the caller then tested `mixed_path.exists()` — which a stale `*_music.mp4` from an earlier run satisfies. The stale file is now cleared before mixing, and mix_music_bed returns whether it actually wrote an output so the caller gates on the result rather than on a filesystem check. Verified against both repros: the short-audio clip now yields 6.0s video / 6.02s audio with the tail scored, and a missing bed with a stale artifact present correctly renders unscored. Two existing assertions pinned the old behaviour (`duration=first`, and no `-shortest` on the mixed path) and were updated to the corrected contract. Tests: 61 -> 65. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CqL3Pwe5Xy2rmrsSzRU9Zg
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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:587">
P3: The `mix_music_bed` docstring still says the bed is "cut to video length if longer (`duration=first`)", but this change switched the mix to `duration=longest` and made `-shortest` the thing that bounds the output. That sentence now describes behavior the code no longer has and misleads anyone reading the EDL shape documentation.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| filt = ( | ||
| f"[0:a]volume={natural}[nat];" | ||
| f"[1:a]{music_chain}[bed];" | ||
| f"[nat][bed]amix=inputs=2:duration=longest:dropout_transition=0:normalize=0[aout]" |
There was a problem hiding this comment.
P3: The mix_music_bed docstring still says the bed is "cut to video length if longer (duration=first)", but this change switched the mix to duration=longest and made -shortest the thing that bounds the output. That sentence now describes behavior the code no longer has and misleads anyone reading the EDL shape documentation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/render.py, line 587:
<comment>The `mix_music_bed` docstring still says the bed is "cut to video length if longer (`duration=first`)", but this change switched the mix to `duration=longest` and made `-shortest` the thing that bounds the output. That sentence now describes behavior the code no longer has and misleads anyone reading the EDL shape documentation.</comment>
<file context>
@@ -568,14 +575,16 @@ def mix_music_bed(
f"[0:a]volume={natural}[nat];"
f"[1:a]{music_chain}[bed];"
- f"[nat][bed]amix=inputs=2:duration=first:dropout_transition=0:normalize=0[aout]"
+ f"[nat][bed]amix=inputs=2:duration=longest:dropout_transition=0:normalize=0[aout]"
)
natural_note = f"natural {natural}"
</file context>
… range Three defects found by cutting a real montage from photographs and arena footage. All three succeed loudly and produce wrong output quietly. 1. Frame rate was resolved from the FIRST range's source. ffprobe reports a nominal 25 fps for a JPEG, so a montage opening on a photo forced 25 fps onto 30 fps footage and resampled the motion. The rate now comes from the first moving source, falling back only when every source is a still. 2. grade.py could not analyze still images at all. _sample_frame_stats built `-ss <start> -i <file> -t <duration>`; seeking into a single-frame input yields ZERO frames, the signalstats metadata came back empty, and the analysis fell through to its neutral defaults. Every photo in a montage therefore got an identical canned correction regardless of exposure, and `grade: "auto"` produced luma byte-identical to no grading at all. Measured before: two visually different stills both reported y_mean 0.5 while a video reported 0.8. After: the same stills report 0.52 and 0.58. 3. No per-range grade existed. A montage cuts between sources with very different exposure — blown-white ice against a dim room — which one global grade cannot reconcile, while the auto-grade is deliberately bounded to +/-8% and too gentle to try. A range-level "grade" now overrides the EDL-wide one, and accepts "auto" to resolve per segment. Effect on a real 46s montage: adjacent-shot luma spread fell from 98 to 68 and the worst cut-to-cut jump from 98 to 67, with the remaining large step being a motivated dark-exterior to bright-rink transition. Each fix is pinned by a test that was verified to FAIL against the pre-fix code, not merely to pass against the fixed code. Tests: 65 -> 79. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CqL3Pwe5Xy2rmrsSzRU9Zg
The fake ffmpeg run wrote its metadata file relative to a cwd kwarg, which only exists where the metadata target is passed as a bare filename. Accept either an absolute target or a cwd-relative one so the test asserts the behaviour (stills get analyzed differently) rather than the plumbing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CqL3Pwe5Xy2rmrsSzRU9Zg
There was a problem hiding this comment.
1 existing issue remains and 1 new issue found across 4 files (changes from recent commits).
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/grade.py">
<violation number="1" location="helpers/grade.py:85">
P3: grade.py introduces an exact duplicate of render.py's `STILL_IMAGE_CODECS` constant and `is_still_image()` probe helper (identical codec set and identical ffprobe call). Keep one shared implementation so the still-codec set can't diverge between the analysis and extraction paths.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| STILL_IMAGE_CODECS = {"mjpeg", "png", "bmp", "tiff", "webp", "gif", "jpeg2000", "ppm"} | ||
|
|
||
|
|
||
| def _is_still_image(path: Path) -> bool: |
There was a problem hiding this comment.
P3: grade.py introduces an exact duplicate of render.py's STILL_IMAGE_CODECS constant and is_still_image() probe helper (identical codec set and identical ffprobe call). Keep one shared implementation so the still-codec set can't diverge between the analysis and extraction paths.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/grade.py, line 85:
<comment>grade.py introduces an exact duplicate of render.py's `STILL_IMAGE_CODECS` constant and `is_still_image()` probe helper (identical codec set and identical ffprobe call). Keep one shared implementation so the still-codec set can't diverge between the analysis and extraction paths.</comment>
<file context>
@@ -75,6 +75,25 @@ def get_preset(name: str) -> str:
+STILL_IMAGE_CODECS = {"mjpeg", "png", "bmp", "tiff", "webp", "gif", "jpeg2000", "ppm"}
+
+
+def _is_still_image(path: Path) -> bool:
+ try:
+ out = subprocess.run(
</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.
Motivation
render.pycurrently has no path for a music-bed montage that mixes orientations — the two gaps compound:-2:1920), landscape by width (1920:-2), so segments come out with different dimensions. Rule 2's-c copyconcat cannot join them, and there is no way to reach a montage that mixes phone verticals with landscape footage.amix, no ducking. For a montage whose spine is a song rather than speech, the renderer has nothing to offer.The talking-head model the renderer is built around doesn't hit either of these. A montage hits both immediately.
What this adds
Two opt-in EDL fields. Both default off, so every existing EDL renders byte-identically.
{ "canvas": "fill", "music": { "source": "bed.mp3", "gain": 0.85, "natural_gain": 0.5, "fade_out": 3.0 }, "ranges": [ { "source": "phone", "start": 0, "end": 3, "mute": true } ] }"canvas": "fill"renders every segment onto one fixed landscape canvas: a zoomed blurred copy fills it edge-to-edge with the sharp aspect-correct frame centred on top. Landscape fills completely; portrait gets a blurred pillarbox rather than black bars. Every segment shares dimensions, so the lossless concat still works. Draft mode uses a 1280x720 canvas."music": {...}mixes a bed under the concatenated natural audio — looped if short, cut to length if long. Mixed after concat with-c:v copy, so it costs no extra video encode."mute": truesilences a segment's natural audio while keeping the stream, so the bed can cover it.Two details that are easy to get wrong and are pinned by tests:
amixusesnormalize=0. Without it amix rescales every input by1/nand the explicit gains stop meaning anything.-shortest, becauseamix'sduration=firstis otherwise the only thing stopping an infinitely looped bed.Incidental fix: sources with no audio stream
Building this surfaced a pre-existing crash. A source with no audio stream — an exported photo, the most common ingredient in a music montage — produced a segment with no audio, so the mix had no
[0:a]to duck under and the render died.Silence is now synthesized with
anullsrcfor any source lacking audio. That also closes a quieter hole: a silent clip among audio clips previously left gaps in the concatenated audio timeline.Verification
Beyond the unit tests, rendered end-to-end from a 3-segment EDL mixing a 1920x1080 source, a 608x1080 source, and a silent source, with a looped bed:
1920x1080with audio streams (the concat precondition)I=-22.64 LUFS, TP=-8.74, LRA=3.20)Tests: 16 -> 40 on this base.
Risks and caveats
gblurat sigma 20 costs encode time on the canvas path. Not measured against a long timeline.extracting N segment(s) →print withUnicodeEncodeError: 'charmap' codec can't encode '→', before reaching any code in this PR. fix: make CLI helpers run on Windows (cp1252 console + ffmpeg path) #78 fixes that. I tested here withPYTHONIOENCODING=utf-8to work around it. Not a dependency for the merge itself, only for a Windows user exercising the feature..MOVcarryingmebxtimed-metadata streams. The implementation should be safe by construction —has_audio_streamuses-select_streams a, which ignores data streams, and the silent path maps explicitly rather than with-map 0:a?— but that is reasoning, not evidence.🤖 Generated with Claude Code
https://claude.ai/code/session_01CqL3Pwe5Xy2rmrsSzRU9Zg
Summary by cubic
Adds opt-in EDL fields for mixed-orientation montages and music beds, and fixes how the renderer handles silent, mono, and still-image sources. Both features default off.
New Features
"canvas": "fill"renders every segment onto one fixed canvas so portrait and landscape sources stay losslessly concatenable; portrait gets a blurred pillarbox instead of black bars."music": {...}mixes a bed under the natural audio after the concat with-c:v copy, andamixusesnormalize=0so explicit gains still mean anything."mute"silences a segment's natural audio while keeping the stream, and per-range"grade"overrides the EDL-wide grade, including"auto".Bug Fixes
anullsrc; mono sources are forced to stereo; stills are looped instead of seeked; short audio is padded to video length.amixusesduration=longest, and stale*_music.mp4output is cleared before mixing.The blurred-fill look is a taste call,
gblurat sigma 20 adds encode time, and this path is verified on Windows only.Written for commit 2af93c9. Summary will update on new commits.