fix: handle transcode for videos without audio track#78
Conversation
ravirajsinh45
left a comment
There was a problem hiding this comment.
The conditional -map a:0 / var_stream_map logic for audio-less inputs is correct and a genuinely useful fix. A few things before merge:
- Coordinate with #79 (overlapping change). This PR and #79 both edit the same region of
packages/transcoder/ffmpeg_transcoder.pyand neither is merged yet. GitHub reports each as individually mergeable because it only checks againstmain, not against each other — whichever lands second will need a rebase. - The probe should fail safe. The new audio probe uses
subprocess.run(...)+json.loads(stdout)unchecked. On a flaky/partial probe (truncated upload, S3 stall, expired presigned URL), ffprobe exits non-zero but still prints{}, sohas_audiobecomesFalseand audio is silently dropped from a file that has it while the job reports success. Recommendcheck=True(a genuine no-audio file still exits 0 with{"streams": []}, so the happy path is unaffected). If #79 lands first, route this through its_runhelper instead. - Tests.
packages/transcoder/has no tests, so the newhas_audio=Falsebranch is unverified. A unit test asserting the constructed argv (no-map a:0,var_stream_map=v:0 v:1 v:2) for both audio/no-audio inputs would lock it in. - Minor. This adds a second full ffprobe round-trip over the presigned URL; a single
-show_streamscall can yield both metadata and audio presence.
PR Techiebutler#76 (HLS Playback): - Prefix relative stream URLs with API_URL before hls.loadSource() - Add streamLoading to useEffect dependency array - Lazy-import hls.js (no top-level import) - Add Hls.Events.ERROR handler + video onError for fatal-error state - Add canPlayType guard for native .m3u8 Safari fallback PR Techiebutler#77 (Comment Crash): - Replace guest_name/guest_email with guest_author.name as primary - Fallback chain: guest_author?.name || author?.name || 'Unknown' - Add referrerPolicy=no-referrer on avatar <img> - Add onError fallback to initials on failed avatar load - Use || instead of ?? for empty-string fallthrough PR Techiebutler#78 (No-Audio Transcode): - Add tests asserting constructed argv for has_audio=True/False - ffprobe audio probe uses _run() with fail-fast error handling PR Techiebutler#79 (Stderr in Error Messages): - Add errors='replace' to subprocess.run() for Latin-1/Shift-JIS stderr - Change ffprobe -v quiet to -v error to preserve stderr content PR Techiebutler#80 (Clipboard Fallback): - Extract copyToClipboard() helper into lib/utils.ts - iOS Safari: contentEditable + setSelectionRange for execCommand - Add aria-live region for success/failure screen reader announcements - Add 'Failed' visual state with XCircle icon - Use copyToClipboard in both share-dialog and share-create-dialog
0ec29c1 to
7a1ae5f
Compare
|
Thanks for the review @ravirajsinh45. Feedback addressed: Probe fail-safe:
Tests:
All 5 tests pass ( |
|
Thanks for splitting this up, @rubenxyz — I can see you took the "separate PRs" suggestion to heart. 🙏 One thing to flag: #76, #77, #78, #79, and #80 currently contain the exact same diff (identical To keep the review in one place, I'm consolidating on #76 (which already has the full review) and closing this one as a duplicate — nothing is lost, all the changes live on #76. If you'd like to genuinely split them later, the key is that each branch should contain only its own change. A clean way from …repeated per change, so each PR is independently reviewable. But that's optional — happy to take it all as one PR on #76 for now. Let's continue there! |
Description
Videos without an audio track (e.g. screen recordings, animations, silent clips) cause the transcoder to fail with
Stream map 'a:0' matches no streamsbecause-map a:0was applied unconditionally.Fix
ffprobe -select_streams abefore building the ffmpeg command-map a:0andvar_stream_mapaudio entries (v:{i},a:{i}) whenhas_audio=True