feat(export): pick the video encoder from the host's hardware, with a libx264 fallback - #161
Merged
EtienneLescot merged 3 commits intoJul 27, 2026
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
EtienneLescot
force-pushed
the
release/1.8.0
branch
from
July 26, 2026 15:36
b43adda to
92fbe07
Compare
EtienneLescot
force-pushed
the
feat/native-encoder-vendor-selection
branch
from
July 26, 2026 15:36
7ea22fd to
f01df9e
Compare
EtienneLescot
force-pushed
the
release/1.8.0
branch
from
July 27, 2026 06:28
2bb4058 to
f7a020d
Compare
… software fallback pipeline.rs hard-coded h264_amf in three places and bailed when it was missing. AMF is AMD-only, so on an NVIDIA or Intel host - or any machine without a hardware H.264 encoder - native export could not open an encoder at all. Development happens on an AMD GPU, which is why this stayed invisible. ExportCodec::candidates() now returns an ordered list of (encoder name, input pixel format). VideoEncoder::open takes the first one avcodec_open2 actually accepts, logs which it kept and which it refused with their errors, and VideoEncoder::send hides the rest: D3D11 candidates get the compositor's texture unchanged, the others get an av_hwframe_transfer_data download, plus an NV12 -> YUV420P de-interleave for the software encoders. All three call sites route through it, the C0/C1 bench paths included - they carried the identical bail. Three things the scoping issue could not know: libx264 is not in the shipped ffmpeg. The vendored build is LGPL (--disable-libx264 --disable-libx265). The software encoders that do exist are libopenh264 and libkvazaar, so those are the last resort. A test asserts it, so the fallback cannot quietly become a name that is not there. electron/media/ffmpegCapabilities.ts is gone, deleted in 9d6e7e1 with the web MP4 pipeline. There is no TS-side discovery left to consult; this list is the only one. h264_mf is listed NV12-only. It advertises d3d11 and opens in d3d11, then dies on the first send with "Failed to set D3D manager: 80004001" once MediaFoundation has resolved the software MFT. Since the choice is committed at open time, that form would fail the export instead of sliding to the next candidate. A probe frame was built and removed rather than kept. C0 passes the decoder's pool, and an arbitrary buffer from it is not a real decoded frame: AMF refused it and the run fell through to h264_mf, costing 179 -> 60 fps on a path that worked. Every remaining candidate fails cleanly at avcodec_open2 (nvenc "Operation not permitted", qsv "Unknown error"), so opening is a sufficient probe now that h264_mf/d3d11 is out. try_open carries the reasoning and the upgrade path. Verified by forcing each tier to the head of the list, at 1920x1080 on the frozen fixture: h264_amf zero-copy 141.6 fps (reference), h264_mf via NV12 58.0 fps at PSNR y 39.5 / u 46.8 / v 46.5, libopenh264 41.0 fps at y 39.0 / u 45.7 / v 46.2. Chroma PSNR above luma in both is what correct chroma looks like - a U/V swap would land near 15 dB. Shipping path unregressed: C0 238.9 fps, C1 141.6 fps, 2.7% spread. 76/76 crate tests pass, and the ordering test was confirmed to fail on a deliberately broken order.
EtienneLescot
force-pushed
the
feat/native-encoder-vendor-selection
branch
from
July 27, 2026 19:02
705cf30 to
1eb3f25
Compare
EtienneLescot
marked this pull request as ready for review
July 27, 2026 19:09
…well-formed Follow-ups from review of the candidate list. None of them change which encoder a normal run picks. OPENSCREEN_EXPORT_ENCODER=<name> restricts VideoEncoder::open to that one candidate. The non-AMD paths were verified by temporarily hoisting each tier to the head of the list, which works but needs a source edit and a rebuild, so it cannot be asked of a user reporting a bad export nor run against a packaged build. The forcing deliberately falls back to nothing: a silent slide to AMF would make a check look like it passed. It follows OPENSCREEN_WGC_PREFER_SOFTWARE_ENCODER, which does the same for capture. AV_CODEC_FLAG_GLOBAL_HEADER is now set on every encoder context. All three sites mux MP4, which wants SPS/PPS - and VPS for HEVC - in the extradata rather than in-band. AMF emitted it regardless; libopenh264 and libkvazaar do not without the flag, and the mov muxer recovering parameter sets from the first packet is a fallback, not a contract. ffprobe reports extradata_size 30 on both the AMF and the libopenh264 output. A failed run no longer leaves its output file behind. Each *_inner exits through some thirty `?`, every one of which abandoned an MP4 with no moov under exactly the name the user believes they exported. The guard sits on the three public facades rather than on each exit. The ffmpeg contexts do still leak on those paths; that wants a RAII guard per pointer and is marked where it belongs. nv12_to_yuv420p uses div_ceil for the chroma extents. An odd width or height carries one more chroma column and row than half, which truncating division left as av_frame_get_buffer returned them - a green edge. D3D11 refuses odd NV12 textures so the case is unreachable today, but being right costs nothing here. The test runs 5x3 beside 4x4, and was confirmed to fail on the truncating form before the fix went in. The ordering test is renamed. It asserts D3D11 before system memory, and h264_qsv and h264_mf are hardware as well - the input format separates them, not the silicon. Re-measured on the frozen fixture at 1920x1080: C0 235.5 fps on h264_amf unchanged, C0 forced to libopenh264 54.7 fps, PSNR between the two y 38.1 / u 41.8 / v 42.1. 76/76 crate tests pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
crates/compositor/src/pipeline.rshard-coded the AMD encoder in three places and refused to continue without it:AMF is AMD-only. On an NVIDIA or Intel host, or on any machine without a hardware H.264 encoder, that path could not open an encoder at all — native export failed outright rather than degrading. Development happens on an AMD GPU, which is exactly why this stayed invisible.
What shipped
ExportCodec::candidates()returns an ordered list of(encoder name, input pixel format).VideoEncoder::openkeeps the first oneavcodec_open2actually accepts, logs which it kept and which it refused with their errors, andVideoEncoder::sendhides the difference: D3D11 candidates take the compositor's texture unchanged, the rest get anav_hwframe_transfer_datadownload plus an NV12 → YUV420P de-interleave for the software encoders. All three call sites route through it, C0/C1 bench paths included — they carried the identicalbail.h264_amfhevc_amfh264_nvenchevc_nvench264_qsvhevc_qsvh264_mfhevc_mflibopenh264libkvazaarSelection is automatic and derived from the host. There is no user-facing selector.
Three things the scoping issue got wrong
libx264is not in the shipped ffmpeg. The vendored build is LGPL (--disable-libx264 --disable-libx265). The software encoders that do exist arelibopenh264andlibkvazaar, so those are the last resort. A test asserts it, so the fallback cannot quietly become a name that is not there.electron/media/ffmpegCapabilities.tsis gone, deleted in9d6e7e1ewith the web MP4 pipeline. There is no TS-side discovery left to consult; this list is the only one.h264_mfis listed NV12-only. It advertisesd3d11and opens ind3d11, then dies on the first send withFailed to set D3D manager: 80004001once MediaFoundation resolves the software MFT. Since the choice is committed at open time, that form would fail the export instead of sliding to the next candidate.Testing the paths this machine does not have
OPENSCREEN_EXPORT_ENCODER=<name>restricts the search to a single candidate — the only way to exercise the non-AMD paths from an AMD host without editing the source and rebuilding, and the only form that works against a packaged build or a user's machine. It deliberately falls back to nothing: a silent slide to AMF would make a check look like it passed. FollowsOPENSCREEN_WGC_PREFER_SOFTWARE_ENCODER, which does the same for capture.Acceptance
libopenh264: 360 frames, 54.7 fps, valid MP4. Unforced on the same machine:h264_amf, 235.5 fps, unregressed.[pipeline] encodeur vidéo : libopenh264 (frames système), with the refused candidates and their errors appended when there were any.AV_CODEC_FLAG_GLOBAL_HEADERis set on every encoder context — all three sites mux MP4, and unlike AMF the software encoders emit no extradata without it.ffprobereportsextradata_size=30on both outputs.A failed run now deletes its own output file instead of leaving a
moov-less MP4 under the name the user believes they exported.Out of scope
preferSoftwareEncoderoverride for export — separate PR.avcodec_open2. Opening is a sufficient probe for every candidate that remains, but it is not a strong one;try_opencarries the reasoning and the upgrade path.cargois never invoked by any workflow, so these tests only run locally.