Skip to content

video_compose: cuts→scenes adapter for CinematicRenderer + dict profile#41

Open
chantskevin wants to merge 1 commit into
calesthio:mainfrom
chantskevin:fix/video-compose-llm-adapter
Open

video_compose: cuts→scenes adapter for CinematicRenderer + dict profile#41
chantskevin wants to merge 1 commit into
calesthio:mainfrom
chantskevin:fix/video-compose-llm-adapter

Conversation

@chantskevin

@chantskevin chantskevin commented Apr 20, 2026

Copy link
Copy Markdown

Summary

Three small fixes to video_compose._remotion_render for LLM-driven callers:

  1. cuts → scenes adapter for CinematicRenderer — the agent emits a uniform
    edit_decisions.cuts[] shape across every renderer_family, but
    CinematicRenderer reads props.scenes[] with a different field layout.
    Without this, renderer_family=cinematic-trailer / documentary-montage
    silently renders 20–30s of pure black — scenes defaults to [] in
    Root.tsx, no error signal.

  2. File-URI rewrite applied to scenes[] too — companion to README video attachment upload #1; the
    existing loop only rewrote cuts[].source.

  3. Accept dict-form profile — schema documents profile as a string,
    but LLM callers frequently pass {"width": 1280, "height": 720, "fps": 30}.
    get_profile(name) then crashes with TypeError: unhashable type: 'dict'.
    Accept both: dict → extract width/height directly; string → existing
    lookup.

Notes

Remotion's Chromium still rejects file:/// URLs for local resources.
Fix #2 is a code-consistency fix; the broader "serve local files over HTTP"
story is unchanged by this patch.

All three fixes are additive; no behavior change for schema-compliant callers.

Test plan

  • Render with renderer_family="cinematic-trailer" and cuts[] — produces video (not black)
  • Render with profile={"width": 1280, "height": 720, "fps": 30} — does not crash
  • Render with profile="youtube_landscape" — existing behavior preserved

🤖 Generated with Claude Code

@chantskevin @claude
video_compose: cuts→scenes adapter for CinematicRenderer + dict profile
07983fd 
Three small fixes that matter for LLM-driven callers:

1. cuts → scenes adapter for CinematicRenderer
   The agent emits edit_decisions.cuts[] with a single consistent
   shape across every renderer_family. But the two Remotion compositions
   the tool targets read different props:
     - Explainer             reads props.cuts[]   {source, in_seconds, ...}
     - CinematicRenderer     reads props.scenes[] {src, kind:"video",
                                                   startSeconds,
                                                   durationSeconds, ...}
   When renderer_family=cinematic-trailer / documentary-montage and
   props.cuts[] is present without props.scenes[], _remotion_render
   now transforms cuts→scenes in place (cumulative startSeconds,
   durationSeconds = out-in, in_seconds→trimBeforeSeconds, optional
   tone/filter/fade hints preserved).
   Without this, Cinematic compositions render 20–30s of pure black
   video with no error signal — scenes[] just defaults to [].

2. File-URI rewrite applied to scenes[] too
   The existing loop that converts absolute paths to file:// URIs only
   ran against cuts[]. After the adapter fires, scene sources need the
   same treatment.

3. Accept dict-form profile as well as string name
   `profile` is documented as a string key from media_profiles.py
   (e.g. "youtube_landscape"), but LLM callers frequently pass
   {"width": 1280, "height": 720, "fps": 30}. get_profile(name) then
   raises TypeError: unhashable type: 'dict' with no caller recourse.
   Accept both: dict → extract width/height directly; string → the
   existing get_profile lookup path.

Note on the broader file-path limitation:
  Remotion's Chromium refuses `file:///` URLs for local-resource
  security ("Not allowed to load local resource"). Callers that pass
  absolute local paths still hit this — not regressed by this patch,
  just not solved. The long-term fix is serving local files over HTTP
  before invoking Remotion. In the meantime, callers with local files
  should front them with an HTTP server and pass the URL in cut.source.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@chantskevin chantskevin requested a review from calesthio as a code owner April 20, 2026 08:30
chantskevin added a commit to chantskevin/OpenMontage that referenced this pull request Apr 24, 2026
@chantskevin @claude
tests/renders: build operation-appropriate inputs per cell
bc77f7a 
The matrix-full caught two bugs the first time it actually ran:

### Bug 1: Remotion couldn't load testsrc sources

The cuts→scenes adapter (calesthio#41 / cherry-pick ef16bba) converts
cuts[].source from absolute paths to file:// URIs for Remotion.
Remotion's Chromium then proxies file:// requests through its
local dev server at /public/<path>. Since the bundle's public/ dir
doesn't contain the test source, every Remotion render returned
404 → render exited non-zero.

Fix: cherry-picked d820d3b (serve Remotion assets via
--public-dir) and da218ed (harden public-dir selection +
regression test). Both are upstream-quality bug fixes that lived
on the snapshot branch but weren't yet on main. With them, the
bundle server's --public-dir is set to the directory containing
the assets, so file:// URIs resolve correctly via the
http://localhost:3000/public/<rel-path> proxy.

The audit doc had categorized these as Bucket 2 (BOS-integration
artifacts) but they're actually Bucket 1 (real upstream bugs) —
needed by ANY caller using local files with Remotion. The matrix
test surfaced this.

### Bug 2: test cell built ID-shaped sources for low-level entry

operation="render" / "compose" are high-level orchestrators that
take asset_manifest and resolve cuts[].source IDs internally.
operation="remotion_render" is the LOW-LEVEL direct entry — it
expects cuts with pre-resolved file paths and ignores
asset_manifest.

The matrix's `_build_inputs` was building ID-shaped sources for
ALL cells. The cinematic-trailer__remotion-render cell hit
Remotion with `source: "a0"` literally, which proxied to
`/public/a0` and 404'd.

Fix: `_build_inputs` now builds operation-appropriate inputs.
remotion_render cells get literal paths in cuts[].source and no
asset_manifest. render/compose cells continue to get IDs +
manifest.

This also documents the contract: each operation's expected input
shape lives in the cell-builder. If a future operation expects
something different, this is the obvious place to add it.

### Result

12/12 cells pass in 32s, well under the 10-min nightly budget.
The full matrix is now a real safety net.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chantskevin added a commit to chantskevin/OpenMontage that referenced this pull request Apr 24, 2026
@chantskevin @claude
tests/renders: build operation-appropriate inputs per cell
2a00533 
The matrix-full caught two bugs the first time it actually ran:

### Bug 1: Remotion couldn't load testsrc sources

The cuts→scenes adapter (calesthio#41 / cherry-pick ef16bba) converts
cuts[].source from absolute paths to file:// URIs for Remotion.
Remotion's Chromium then proxies file:// requests through its
local dev server at /public/<path>. Since the bundle's public/ dir
doesn't contain the test source, every Remotion render returned
404 → render exited non-zero.

Fix: cherry-picked d820d3b (serve Remotion assets via
--public-dir) and da218ed (harden public-dir selection +
regression test). Both are upstream-quality bug fixes that lived
on the snapshot branch but weren't yet on main. With them, the
bundle server's --public-dir is set to the directory containing
the assets, so file:// URIs resolve correctly via the
http://localhost:3000/public/<rel-path> proxy.

The audit doc had categorized these as Bucket 2 (BOS-integration
artifacts) but they're actually Bucket 1 (real upstream bugs) —
needed by ANY caller using local files with Remotion. The matrix
test surfaced this.

### Bug 2: test cell built ID-shaped sources for low-level entry

operation="render" / "compose" are high-level orchestrators that
take asset_manifest and resolve cuts[].source IDs internally.
operation="remotion_render" is the LOW-LEVEL direct entry — it
expects cuts with pre-resolved file paths and ignores
asset_manifest.

The matrix's `_build_inputs` was building ID-shaped sources for
ALL cells. The cinematic-trailer__remotion-render cell hit
Remotion with `source: "a0"` literally, which proxied to
`/public/a0` and 404'd.

Fix: `_build_inputs` now builds operation-appropriate inputs.
remotion_render cells get literal paths in cuts[].source and no
asset_manifest. render/compose cells continue to get IDs +
manifest.

This also documents the contract: each operation's expected input
shape lives in the cell-builder. If a future operation expects
something different, this is the obvious place to add it.

### Result

12/12 cells pass in 32s, well under the 10-min nightly budget.
The full matrix is now a real safety net.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
calesthio
calesthio requested changes Jul 6, 2026
View reviewed changes

@calesthio calesthio left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for identifying the CinematicRenderer cuts -> scenes gap. The problem is real: documentary/cinematic renderers consume scenes[], while pipeline edit decisions commonly produce cuts[], so a missing adapter can render black output.\n\nI’m requesting changes before merge because this patch is stale/partial relative to the current render work:\n\n- It has no regression tests for the cuts→scenes adapter or dict-form profile handling. The PR body test plan is unchecked, and this path has already proven easy to regress silently.\n- It still rewrites local scene sources to ile:// URIs, while the newer local-rendering work shows Remotion render needs public-directory staging/staticFile-relative paths for local assets.\n- It overlaps with #242 and #238. Please rebase on the current �ideo_compose.py, keep the adapter as a focused tested change, and avoid reintroducing the older file-URI path behavior.\n\nThe idea is worth keeping, but the current branch should not merge as-is because it would land untested behavior into a fragile render path and conflict with the newer local asset staging fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@calesthio calesthio calesthio requested changes
+1 more reviewer
@8az8az12-lab 8az8az12-lab 8az8az12-lab approved these changes
Reviewers whose approvals may not affect merge requirements

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@chantskevin @calesthio @8az8az12-lab