fix(export): run the app's own ffmpeg for the video watermark - #2192
Conversation
Two call sites still reached for the bare name `ffmpeg` instead of
services.ffmpeg_utils.find_ffmpeg(), which every other call site uses. The
bare name only resolves a system install: imageio-ffmpeg -- the app's
default source, and a locked dependency -- ships its binary as
`ffmpeg-<platform>-v<version>`, and ensure_media_tools_on_path() publishes
that directory on PATH without giving the file an `ffmpeg` name. So on a
host with no separate system ffmpeg, which is most installs:
- /export dropped the visible video watermark. is_visible_video_enabled()
defaults to ON, the spawn raised FileNotFoundError, and the except arm
quietly plain-copied the file -- the user asked for a watermarked export
and got an unmarked one with no error anywhere.
- video_context._extract_keyframes gated on shutil.which("ffmpeg") and
logged "ffmpeg not found, skipping frame extraction", so the dubbing
director's visual context was empty while the app's own ffmpeg sat on
disk, resolvable. Same shape as debpalash#1256.
The export also no longer spawns anything when nothing resolves: it goes
straight to the plain copy instead of failing a subprocess to get there.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe PR updates video watermark overlays and keyframe extraction to use the resolved FFmpeg executable. It preserves plain-copy and no-executable fallback behavior and adds tests for resolved and missing FFmpeg paths. ChangesFFmpeg resolution
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The documented FFmpeg-resolution updates have no supported unresolved defect in the supplied evidence, so no merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 7 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (7 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 30.77% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 4 files. (1 skipped: 1 unsupported.) Full details: Description checkExplanation The description explains the main changes, fallback behavior, testing, documentation, and related pull requests. It does not follow the required template structure and omits explicit Type and Checklist sections, including confirmation of the regression fixture requirement. Resolution Rewrite the description using the repository template. Add the required Summary, Changes, Type, Testing, Checklist, and Release cadence sections. Mark the applicable Type and Checklist items, and explicitly confirm the smoke-matrix regression fixture status or explain why it is not applicable.
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 |
 The implementation appears functionally safe, but the explicit changelog placement requirement must be satisfied before merging.
 |
|
Heads-up on the CHANGELOG: |
The module's Pillow-floor check read pyproject.toml with a bare read_text(), i.e. in the locale code page, so the whole module errored out on a Chinese, Japanese or Korean Windows and the tests added here could not be run there. TOML is UTF-8; name it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| ### Fixed | ||
|
|
||
| - Video watermark exports and dubbing keyframes use the bundled FFmpeg without requiring a system install (#2192) — thanks @kevin9327! |
There was a problem hiding this comment.
Fixed, but the repository requires Unreleased entries in the short Highlights list. Move this single-line credited entry to Highlights; this requirement must be satisfied before merging.
Context Used: Review as a panel of senior domain experts (ML inference, audio DSP, desktop systems). Comment ONLY on findings that would change what gets merged: a concrete bug, a violated house rule from CLAUDE.md, a real security/data risk. Per finding: at most ... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
2 participants
Video watermark exports and dubbing keyframe extraction use the shared FFmpeg resolver, including the bundled executable, instead of requiring a system binary named ffmpeg. Existing missing-tool and failed-overlay fallbacks remain unchanged.
Current main is merged, the changelog conflict is resolved with contributor credit, and dubbing documentation describes bundled-tool support. CodeRabbit and Greptile reviews were checked.
Validation: 109 offline export, video-context, router, watermark and changelog tests passed. Resolver stubs keep tests isolated from the real executable-probe cache.
Included in #2191 with contributor history preserved. The combined PR runs full CI before landing and will absorb this PR; do not merge this separately.
Review disposition: the changelog entries stay under
### Fixed, which CLAUDE.md explicitly permits beneath a short Highlights list. The request to move every entry into Highlights conflicts with that governing rule; the deterministic changelog tests pass.