fix: include ffmpeg stderr in transcode error messages#79
Conversation
ravirajsinh45
left a comment
There was a problem hiding this comment.
The _run helper and surfacing stderr is a real improvement — the old CalledProcessError messages were useless. One regression to fix before merge, plus a couple of smaller points.
Regression: text=True can fail successful transcodes
_run hardcodes text=True with no errors=. Three of the migrated call sites (generate_thumbnails, the HLS ffmpeg_cmd, and thumb_cmd) previously ran in bytes mode. subprocess.run decodes the entire stderr with strict UTF-8 before the return code is checked, and ffmpeg echoes the input's container/ID3 metadata (often Latin-1/Shift-JIS) verbatim — so a transcode that exited 0 raises UnicodeDecodeError, gets caught by the broad except, and the version is marked failed after burning retries. Reproduces in python:3.11-slim on a file whose title tag contains byte 0xe9. Fix: pass errors='replace' (or capture bytes and decode only when building the error message).
Smaller
- Both ffprobe calls use
-v quiet, soresult.stderris always empty and_runraises"ffprobe exited N: no stderr output"— which defeats the PR's goal for the ffprobe paths.-v errorkeeps JSON on stdout while emitting real errors to stderr. - The
transcode()ffprobe result is unused (dead code), but_runnow makes a non-zero exit fatal where it was previously ignored. Minor and arguably fine as fail-fast, just noting it since the metadata isn't consumed. - No transcoder tests cover
_run's non-zero branch or the decode behavior, which is why the above could ship unnoticed — a couple ofsubprocess.run-mocked unit tests would help.
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
02490ba to
7a1ae5f
Compare
|
Thanks for the review @ravirajsinh45. All feedback addressed: REGRESSION fix:
ffprobe verbosity:
Dead code note:
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
All
subprocess.run(call, check=True, capture_output=True)calls in the transcoder swallowed ffmpeg/ffprobe stderr on failure. Python'sCalledProcessError.__str__only shows the command string, not the captured stderr, producing useless errors like:Command '['ffmpeg', '-y', '-i', 'http://...']' returned non-zero exit status 8Fix
_run()helper method that checks return code and raisesRuntimeErrorwith stderr included in the messageget_video_metadata,generate_thumbnails, and 3 intranscode) with_run()This is a pure error reporting improvement — no behavioral change.