chore(ffmpeg): say that the encoder list is reported, not verified - #579
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe FFmpeg fetch script renames its encoder list to ChangesEncoder reporting
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This clarifies that reported FFmpeg encoder presence is not runtime verification without changing encoder selection or behavior. No current merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the technical change and motivation, but it does not follow the required template. It omits the required section headings, leaves the related issue as incomplete "Fixes #", provides no selected change type, release impact, or desktop impact, and does not describe testing. Resolution Reformat the description using the repository template. Add the Summary, Related issue with a valid reference such as "Refs ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
`WANTED_ENCODERS` drove a line reading `hardware encoders: h264_vaapi` on Linux. That is true in the only sense the code checks — the name appears in `-encoders` — and misleading in the sense a reader takes it: that the encoder works here. It does not follow. The vendored builds list `h264_vaapi`, and on any host with libva < 2.21 (Ubuntu 24.04 LTS included) reaching it through the CPU upload path used to abort the process with SIGABRT rather than return an error, because the implib trampoline `abort()`s on a symbol it cannot resolve. A build that "has" the encoder and a machine that can use it are separate questions. Renamed to `REPORTED_ENCODERS`, the log line now says "present (not verified)", and the comment records why the distinction matters, with the libva case and pointers to #552 and #576. No behaviour change: this was already non-fatal and already only a log. What changes is that it no longer implies a guarantee it never made. The only evidence an encoder works is a frame going through it — `vaapi_encodes_from_an_exported_dmabuf` is that evidence for the Linux hardware path, and the export falls back to software whenever it is absent.
efbd03e to
0f8bb3e
Compare
1 participant
Follow-up to #576, which observed that the vendor-time encoder check asserts something it does not test.
Slightly narrower than the issue framed it: the check is not a gate, it is already non-fatal and only logs. The problem is the naming and the wording, which promise more than is verified —
hardware encoders: h264_vaapireads as "this works here", when all that was tested is that the name appears in-encoders.That gap is not hypothetical. The vendored builds list
h264_vaapi, and on any host with libva < 2.21 — Ubuntu 24.04 LTS included — reaching it through the CPU upload path aborts the process withSIGABRTrather than returning an error, because the implib trampolineabort()s on a symbol it cannot resolve.So:
REPORTED_ENCODERS, a log line that sayspresent (not verified), and a comment recording why, pointing at #552 and #576.No behaviour change — same list, same non-fatal path, same output structure. What changes is that it stops implying a guarantee it never made.
For the record on the other half of #576: the hardware ladder it warns about has since shipped (#559) and runs on exactly that configuration, because it uses
av_hwframe_mapand neverav_hwframe_transfer_data— the abort lives in the upload path only. Details in #576 (comment).Summary by CodeRabbit
Documentation
Bug Fixes