fix: repair local zero-key rendering (Remotion asset staging, resolveAsset, Piper status)#238
fix: repair local zero-key rendering (Remotion asset staging, resolveAsset, Piper status)#2380xDevNinja wants to merge 8 commits into
Conversation
calesthio
left a comment
There was a problem hiding this comment.
Thanks for digging into this and for the clear repro in #237. The direction is right, and the focused Piper status tests are useful. I’m requesting changes because I found two remaining local-rendering blockers in the PR branch.
-
CinematicRendereraudio is not staged._remotion_render()now rewritesaudio.narration.srcandaudio.music.src, butcinematic-trailer/documentary-montageroute toCinematicRenderer, whose audio props are top-levelsoundtrack.srcandmusic.src. I verified those absolute paths remain unchanged, so local cinematic/documentary Remotion renders can still hit the same local-audio failure described in #237. -
Staged asset filenames only hash the source path, and existing staged files are not refreshed. If a generated narration/image/video is rewritten at the same path, the next render silently uses the old copy in
public/_om_assets. I reproduced this with two_remotion_render()calls against the same source path: after changing the source bytes, the staged file still contained the first render’s bytes.
One smaller follow-up: Piper status currently reports AVAILABLE if any .onnx voice exists, even when the default/requested en_US-lessac-medium model is absent. That can still make preflight look ready while the default execute path fails later.
Verification I ran:
python -m pytest tests/tools/test_piper_tts_status.py -q-> 4 passedpython -m py_compile tools\video\video_compose.py tools\audio\piper_tts.py-> passed- Direct
_remotion_render()probes for the cinematic prop gap and stale staged-asset behavior
|
Thanks — all three are fair and the direct 1. CinematicRenderer audio (and scenes) not staged. You're right, the staging pass only knew the Explainer shape. Broadened it to cover the Cinematic shape too: 2. Stale staged copies. Fixed. The staged filename still keys on the source path, but the copy now refreshes when the source 3. Piper AVAILABLE with default absent. Fixed. New/added tests:
Verification:
Ready for another look when you have a moment. |
|
Thanks for the follow-up and for addressing the three review points. The implementation changes look aligned with the prior feedback. One thing to fix before this can be cleared: please pull/rebase onto the latest Once the branch is updated and the merge-ref CI is green, I can take another look. |
|
Thanks — fixed and rebased onto latest main. The failure was in the test stub, not the render code: it parsed only the separate Rebased the branch onto current main and reran locally:
Should be green on merge-ref now — ready for another look. |
23029c7 to
6edd315
Compare
calesthio
left a comment
There was a problem hiding this comment.
Thanks for rebasing and fixing the merge-ref test stub. The Remotion staging coverage now looks good: the focused staging/Piper tests pass locally and CI is green.
I found one remaining blocker in the Piper path. execute() now checks _find_voice(model) against the current directory / PIPER_VOICE_DIR / home search dirs, but _generate() still invokes Piper with the original model string rather than the resolved .onnx path:
"--model", inputs.get("model", self.default_voice)So if the default voice is installed in PIPER_VOICE_DIR or ~/.piper/models, preflight reports AVAILABLE and execute() passes the gate, but the actual subprocess is still called as piper --model en_US-lessac-medium from the repo cwd instead of piper --model <resolved search-dir path>/en_US-lessac-medium.onnx. That reintroduces the same shape of issue this PR is fixing: status says the local TTS path is ready, but the default execution can fail at render time depending on where the voice model is installed.
I verified this with a subprocess-capture probe: _find_voice('en_US-lessac-medium') returned the model under a patched search dir, but the captured command was still:
['piper', '--model', 'en_US-lessac-medium', ...]
Please plumb the resolved path into _generate() (and cover it with a test that installs the fake voice outside cwd and asserts the --model argument is that resolved .onnx path).
Checks run:
python -m pytest tests/tools/test_remotion_staging.py tests/tools/test_piper_tts_status.py -q-> 8 passedpython -m py_compile tools/video/video_compose.py tools/audio/piper_tts.py-> passed- targeted Piper subprocess-capture probe for search-dir voice resolution -> command still used bare model name
|
Good catch — you're right, Fixed: Added a regression test that installs the fake voice in a search dir outside cwd and asserts the captured Checks: |
calesthio
left a comment
There was a problem hiding this comment.
I re-reviewed the latest commits after the prior feedback. The previously requested Piper/Remotion fixes look addressed in the branch: focused local checks pass, including the test that asserts Piper receives the resolved .onnx path for a voice found outside cwd.
I am not clearing this yet because the PR is currently CONFLICTING against main. Please rebase/update the branch and rerun CI so the review applies to the actual merge result.
Checks I ran on the PR head:
- python -m pytest tests/tools/test_remotion_staging.py tests/tools/test_piper_tts_status.py -q -> 9 passed
- python -m pytest tests/contracts/test_phase0_contracts.py -q -> 35 passed
- python -m py_compile tools/video/video_compose.py tools/audio/piper_tts.py -> passed
- git diff --check -> passed
c6012c5 to
34863c8
Compare
|
Rebased onto current main — the conflict was isolated to Re-ran locally after the rebase:
The three prior review points (cinematic audio staging, stale staged-asset refresh, Piper resolved-path) are unchanged from the commits you last cleared. Ready for a re-review on the merge result. |
calesthio
left a comment
There was a problem hiding this comment.
Thanks for the continued follow-up on the local rendering fixes. The Remotion asset staging and resolved Piper voice-path fixes look valuable and the focused new tests pass, but I’m requesting changes because the current branch still fails the merge-ref CI and I can reproduce the failing contract locally.\n\nFailing check:\n- tests/contracts/test_phase3_contracts.py::TestPiperTTS::test_status_requires_piper_executable_even_if_python_package_imports\n\nLocal validation on the PR head:\n- python -m pytest tests/tools/test_remotion_staging.py tests/tools/test_piper_tts_status.py tests/contracts/test_phase3_contracts.py::TestPiperTTS::test_status_requires_piper_executable_even_if_python_package_imports -q -> 9 passed, 1 failed\n\nThe regression is that _piper_installed() returns true when the Python piper package imports even if shutil.which('piper') is absent. This tool shells out to the piper executable, so importing the package is not enough for execution readiness. In that state get_status() should remain UNAVAILABLE, not DEGRADED; otherwise preflight still implies the local TTS path is installed when the binary command cannot run.\n\nPlease make executable availability the install gate again, then keep the voice-model check as the AVAILABLE vs DEGRADED distinction once the binary exists. After that, the existing staging/Piper tests plus this contract should be green and the PR should be in good shape.
resolveAsset stripped 'file:///' including the leading slash, so 'file:///abs' became the relative 'abs' (then mis-served from public/) and a bare '/abs' became 'file:////abs' (four slashes). Strip only the 'file://' scheme and build the URI without doubling the slash: unix '/abs' -> 'file:///abs', windows 'C:/abs' -> 'file:///C:/abs', relative -> staticFile(). Refs calesthio#237
Remotion's renderer only loads http(s) URLs or files served from the composer's public/ directory (via staticFile) — file:// URIs are rejected at render time. The remotion_render path passed local cut sources through as file:// and never converted audio paths at all, so local images, video, narration, and music failed to load (e.g. 'Can only download URLs starting with http:// or https://'). Stage every local asset (cuts[].source and audio.narration/music.src) into remotion-composer/public/_om_assets/ under a content-hashed name and rewrite the prop to the staticFile-relative path. public/* is already gitignored. Refs calesthio#237
get_status() returned AVAILABLE whenever the piper module imported, but Piper
ships no voice model, so execute() then failed ('Unable to find voice ...') and
preflight wrongly reported TTS ready. Now report DEGRADED when installed but no
voice model is discoverable (cwd, PIPER_VOICE_DIR, ~/.local/share/piper,
~/.piper/models), give an actionable download hint on execute, and fix
install_instructions to the current 'python -m piper.download_voices' command.
Tests cover unavailable/degraded/available and the execute hint. Closes calesthio#237
…opies The Remotion asset-staging pass only handled the Explainer prop shape (cuts[].source, audio.narration/music.src). Cinematic compositions (cinematic-trailer, documentary-montage) route to CinematicRenderer, whose assets live at scenes[].src and top-level soundtrack.src / music.src — those local absolute paths were left unstaged and still hit the file:// render rejection described in calesthio#237. Also key the staged copy on source size+mtime, not path alone: a regenerated asset written back to the same path previously left the earlier render's bytes in public/_om_assets and was served stale. copy2 preserves mtime, so an unchanged source still skips the copy while a changed one refreshes. Adds tests covering cinematic scene/audio staging and stale-copy refresh. Refs calesthio#237
get_status() reported AVAILABLE when any .onnx voice existed, so preflight looked ready even when the default en_US-lessac-medium was absent and the default execute path would fail. Report AVAILABLE only when the default voice is present, DEGRADED otherwise. execute() now gates on the voice the call will actually use (inputs['model'] or the default) rather than the coarse status, so a caller may still run an explicitly-provided installed voice, and a missing-model error names the requested model instead of always the default. Refs calesthio#237
The staging test stub parsed only the separate `--props <path>` form, but the composer invokes Remotion with the `--props=<path>` equals form, so the merged code path failed the test in merge-ref CI. Handle both forms. Refs calesthio#237
… name get_status()/execute() resolve the voice via _find_voice() across cwd, PIPER_VOICE_DIR, and ~/.piper, but _generate() still invoked piper with the bare model string. Piper only searches the current directory for a bare name, so a voice installed in PIPER_VOICE_DIR/~/.piper passed the availability gate yet failed to load at run time from a different cwd — the same false-ready shape this PR fixes for status. Plumb the resolved .onnx path into _generate and report it in the result. Adds a test that installs the voice outside cwd and asserts the piper --model argument is the resolved .onnx path. Refs calesthio#237
…package
get_status()/execute() checked `_piper_installed()`, which returned True when
the Python `piper` package merely imports even if the `piper` binary is absent
from PATH. This tool shells out to the `piper` executable, so a package-only
install cannot actually run — preflight must report UNAVAILABLE, not DEGRADED,
in that state. Gate solely on `shutil.which("piper")`; the voice-model check
still distinguishes AVAILABLE vs DEGRADED once the binary exists.
Fixes the phase3 contract
test_status_requires_piper_executable_even_if_python_package_imports.
34863c8 to
57a2777
Compare
|
Good catch — fixed. Also rebased onto current main. Local run on the new head:
|
Summary
Fixes the three bugs from #237 that block fully local, zero-API-key rendering (Piper narration + Remotion component scenes). Verified end to end by rendering a real Explainer composition with a Piper-generated narration track at an absolute path — it now produces a 28s 1080p MP4 with audio, no manual workaround.
Closes #237
Changes
1.
tools/video/video_compose.py— stage local assets intopublic/(the real fix).Remotion's renderer only loads
http(s)URLs or files served from the composer'spublic/dir viastaticFile—file://URIs are rejected at render time (Can only download URLs starting with http:// or https://)._remotion_renderpreviously rewrotecuts[].sourcetofile://and never touched audio paths at all. Now every local asset (cuts[].sourceandaudio.narration/music.src) is copied intoremotion-composer/public/_om_assets/<hash>and the prop rewritten to thestaticFile-relative path.public/*is already gitignored.2.
remotion-composer/src/Explainer.tsx— fixresolveAsset(defense in depth).It stripped
file:///including the leading slash, sofile:///absbecame the relativeabs(mis-served frompublic/) and a bare/absbecamefile:////abs(four slashes). Now strips only thefile://scheme: unix/abs→file:///abs, windowsC:/abs→file:///C:/abs, relative →staticFile().3.
tools/audio/piper_tts.py— honest status.get_status()returnedAVAILABLEwhenever the module imported, but Piper ships no voice model, soexecute()failed withUnable to find voice ...and preflight wrongly reported TTS ready. Now reportsDEGRADEDwhen installed but no voice model is discoverable (cwd,PIPER_VOICE_DIR,~/.local/share/piper,~/.piper/models), gives an actionable download hint onexecute, and fixesinstall_instructionsto the currentpython -m piper.download_voicescommand.Testing
Clean venv (
requirements-dev.txt+ numpy):tests/tools/test_piper_tts_status.py→ 4 passed (unavailable / degraded / available / actionable execute error).video_composeremotion_renderwithaudio.narration.srcat an absolute path → renders 28s 1080p MP4 with audio; asset auto-staged topublic/_om_assets/. (Before: 404 /file://rejected.)pytest tests/tools tests/contracts→ only the 2 pre-existing unrelated failures (test_runtime_presentation_contract[character-animation],test_phase3_contracts::...test_registry_catalog_views).Checklist
piper_ttsinstall_instructions).