libva: X11 VADisplay support, size/capacity-aware decoder selection, H.264 multi-reference DPB#4964
libva: X11 VADisplay support, size/capacity-aware decoder selection, H.264 multi-reference DPB#4964Wenri wants to merge 3 commits into
Conversation
The libva codec module opened VA displays via vaGetDisplayDRM exclusively, which cannot work with X11-only VA drivers (e.g. the VDPAU-backed VA driver: VDPAU's sole constructor is vdp_device_create_x11, and pre-DRM NVIDIA drivers expose no render node at all). - va_common.h: libva_open_display() accepts device "x11" or "x11:<name>" and opens the display via XOpenDisplay+vaGetDisplay; a small per-TU registry pairs each VADisplay with its X Display so teardown can XCloseDisplay it (VADisplay does not own it). - va_decode.c / va_encode.c: call libva_x11_close() at teardown; the decoder device scan falls back to "x11" when no render node works. - va_decode.c: get_output_frame() falls back from vaDeriveImage to vaCreateImage+vaGetImage over the full aligned surface for drivers whose surfaces have no linear CPU view (OPERATION_FAILED) - the same pattern ffmpeg's hwcontext_vaapi uses. - codecs/video.py: register dec_libva in CODEC_TO_MODULE and add "libva" to ALL_VIDEO_DECODER_OPTIONS - the module and enc_libva were shipped but the decoder was not selectable (--video-decoders=libva warned 'unknown'). - setup.py: link libva-x11 + x11 for the codec extensions. Tested on NVIDIA 340.xx (GeForce GT 130, no render node) through the VDPAU-backed VA driver: vainfo-equivalent probing, decode, and teardown all work over the X11 display path.
Hardware decoders have real limits that the decoder selection did not
model, with two bad failure modes observed in production (VDPAU-class
hardware behind the libva decoder):
1. a stream a decoder can NEVER handle (too large, wrong parity)
still got selected; every guaranteed init_context failure bumped
the shared spec's setup_cost until the decoder was delisted for
the entire session - one oversized window permanently demoted the
hardware decoder for all windows ('openh264 forever').
2. decoders whose hardware corrupts concurrent streams (measured on
VDPAU feature-set-A: two interleaved decode streams damage each
other's luma) had no way to express a concurrency budget on the
client: max_instances existed in CodecSpec but the client decoder
path neither tracked instances nor consulted the field.
Changes:
- constants.py: new CodecSpec.max_pixels (0 = unlimited) - hardware
decode envelopes are often AREA limits that max_w/max_h cannot
express (e.g. VDPAU feature set A: 8192 macroblocks but 2048 per
dimension).
- backing.py: filter decoder candidates by min/max dimensions and
max_pixels BEFORE instantiation, so out-of-envelope streams fall
back to another decoder without poisoning setup_cost; dimension
masks are deliberately not filtered (real decoders handle
padded+cropped sizes regardless of their alignment masks).
- backing.py: create decoders through spec.make_instance() (tracking
the instance WeakSet, as the server's video pipeline does) and rank
candidates by (100 - setup_cost) * spec.get_runtime_factor() in
choose_decoder() - the same capacity mechanism video_scoring.py
applies to encoders/CSC: a spec at max_instances scores 0 and
loses to any available alternative, but stays usable when nothing
else remains. Also catch ValueError from init_context like
RuntimeError (same setup_cost accounting) - an uncaught size
rejection previously escaped the fallback loop entirely.
- libva decoder: declare the envelope (max size knobs overridable via
XPRA_LIBVA_MAX_WIDTH/HEIGHT/PIXELS), max_instances=1 (VDPAU
cross-stream corruption), setup_cost=0 (rank hardware decode by
the user's --video-decoders order instead of never selecting it),
and accept odd display sizes: h264 streams are coded at the padded
even size (SPS crop carries the true size), the NV12 copy-out now
even-rounds the chroma stride, so odd-sized windows keep hardware
decode instead of raising per-paint.
The H.264 decoder kept a single reference frame (ReferenceFrames[0], 4 surfaces, every decoded frame overwrote 'the reference' - even non-reference frames). Real encoder output is multi-reference whenever speed drops: x264's faster/fast/medium presets default to ref=2/2/3, NVENC emits max_num_ref_frames=3 at quality presets - and multi-ref slices decoded against the wrong reference produce whole-window motion-compensation garbage. Implement a proper decoded picture buffer (progressive streams): - 16-entry DPB + 17-surface pool (surface chosen by DPB-liveness scan); vp8/vp9 paths untouched (they keep their 4 surfaces) - picture order count per spec 8.2.1, types 0 and 2 (type 2 is what x264 emits with bframes=0; type 1 is cleanly rejected - no mainstream encoder emits it) - dec_ref_pic_marking parsed and applied: sliding-window eviction by FrameNumWrap and MMCO operations 1-6 (including MMCO5 numbering reset), IDR flush, long_term_reference_flag - only nal_ref_idc != 0 pictures enter the DPB - ref_pic_list_modification parsed; RefPicList0 filled with the spec-default P list (short-term by descending PicNum, then long-term by ascending LongTermFrameIdx) with modifications applied - B slices are rejected at slice-header parse (this decoder has no display-order reordering; rejecting cleanly lets the paint code fall back instead of decoding garbage) - decompress_image raises CodecStateException instead of RuntimeError on mid-stream decode errors: the paint code restarts the decoder on that type, whereas RuntimeError propagated and left a stale decoder in place Validation (NVIDIA GT 130 / driver 340.108 via the VDPAU-backed VA driver, X11 display path): x264 IPP streams at ref=1/2/3/16 (1280x720) and ref=3 (1920x1080), 120 frames each, all bit-exact (per-frame NV12 MD5) against ffmpeg software decode; odd display size (1397x785 view of a 1398x786 stream) byte-identical to the cropped even-size decode; a VDPAU call interceptor confirmed the rotating multi-entry referenceFrames reaching VdpDecoderRender and decoder creation with max_references=3. Live xpra session against an NVENC nrf=3 stream: sustained hardware decode, zero errors.
There was a problem hiding this comment.
Pull request overview
This PR improves Xpra’s VA-API (libva) client-side decoding path by (1) making libva usable on systems where VA drivers only support X11 displays (no DRM render node), (2) making client decoder selection capacity- and size-aware to avoid global “poisoning” of decoder specs, and (3) implementing a proper H.264 multi-reference DPB to avoid decode corruption on multi-ref streams.
Changes:
- Register and enable the libva video decoder option, add X11 VADisplay open/teardown, and add a safer surface readback fallback (
vaDeriveImage→vaCreateImage+vaGetImage). - Update client decoder selection to consider decoder capacity (
max_instances) and hard stream envelopes (max_pixels, min/max dimensions), and track live instances viaCodecSpec.make_instance(). - Implement H.264 multi-reference DPB handling in the libva decoder, including reference marking/MMCO handling and explicit rejection of unsupported stream features (eg. B-slices, POC type 1).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| xpra/codecs/video.py | Registers dec_libva module and adds libva to the decoder option list. |
| xpra/codecs/libva/va_encode.c | Ensures X11 display teardown is performed after vaTerminate(). |
| xpra/codecs/libva/va_decode.c | Adds X11 fallback device probing, output image fallback path, odd-width NV12 handling, and H.264 multi-reference DPB implementation. |
| xpra/codecs/libva/va_common.h | Adds X11 display open/close support and a small registry to pair VADisplay with X11 Display teardown. |
| xpra/codecs/libva/decoder.pyx | Exposes libva decoder limits/config knobs, relaxes odd-size handling, and raises CodecStateException on mid-stream decode errors. |
| xpra/codecs/constants.py | Adds CodecSpec.max_pixels and uses instances for max_instances runtime-capacity gating. |
| xpra/client/gui/window/backing.py | Implements size/capacity-aware decoder selection and uses make_instance() for live instance tracking. |
| setup.py | Links libva encoder/decoder against libva-x11 and X11 in addition to DRM on non-Windows platforms. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pic.CurrPic.picture_id = surface; | ||
| pic.CurrPic.frame_idx = (uint32_t)first->frame_num; | ||
| pic.CurrPic.flags = VA_PICTURE_H264_SHORT_TERM_REFERENCE; | ||
| pic.CurrPic.TopFieldOrderCnt = first->poc_lsb; | ||
| pic.CurrPic.BottomFieldOrderCnt = first->poc_lsb; | ||
| pic.CurrPic.flags = first->nal_ref_idc ? VA_PICTURE_H264_SHORT_TERM_REFERENCE : 0; | ||
| pic.CurrPic.TopFieldOrderCnt = top_foc; | ||
| pic.CurrPic.BottomFieldOrderCnt = bottom_foc; |
There was a problem hiding this comment.
This copilot comment is over my head, but looks legit.
| #include <va/va_drm.h> | ||
| #include <va/va_x11.h> | ||
| #include <X11/Xlib.h> |
There was a problem hiding this comment.
This is a valid concern - xpra can be built --without-x11 and codecs should not link to X11 in that case.
#ifdefs are a pain in Cython, alternatively you could move the x11 glue code to a file and only include it in the setup.py compilation when x11_ENABLED is True. (and have an empty shim for building without)
| if width > MAX_WIDTH or height > MAX_HEIGHT or (MAX_PIXELS > 0 and width * height > MAX_PIXELS): | ||
| # belt-and-braces for callers that bypass the size-aware | ||
| # decoder selection: fail here (cleanly, before creating a | ||
| # decoder that would error on every submit) | ||
| raise RuntimeError("%ix%i exceeds this device's decode limits" % (width, height)) |
There was a problem hiding this comment.
This copilot suggestion is overkill: we don't currently support the huge picture sizes that could ever overflow this.
But I guess the extra care is cheap.
totaam
left a comment
There was a problem hiding this comment.
Please try to split this PR so it is more manageable.
ie: the validation / size mask stuff should be easy to merge.
The x11 hooks are harder.
| def choose_decoder(decoders_for_cs: list[CodecSpec], max_setup_cost=100) -> CodecSpec: | ||
| # for now, just rank by setup-cost, so slow-to-initialize decoders come last: | ||
| scores: dict[int, list[int]] = {} | ||
| # rank by setup-cost (so slow-to-initialize decoders come last), |
There was a problem hiding this comment.
Note: That's not necessarily what we want here.
The server does this, but eventually settles on efficient encoders after a while, even sending non-video frames during the encoder's warmup.
So we may have to change this part too...
| except (RuntimeError, ValueError) as e: | ||
| log("%s.init_context(..)", vd, exc_info=True) | ||
| log.warn(f"Warning: failed to initialize decoder {decoder_spec.codec_type}: {e}") | ||
| decoder_spec.setup_cost += 50 |
There was a problem hiding this comment.
This chunk is very useful and can be merged as-is.
(I just never hit those limits myself)
| MAX_WIDTH, MAX_HEIGHT = 8192, 8192 | ||
| # driver limits, overridable for hardware whose VA driver cannot | ||
| # express them (ie: the VDPAU bridge on feature-set-A NVIDIA: H264 | ||
| # maxes at 2048x2048 AND 8192 macroblocks = 2097152 pixels): |
| # NEVER be selected when openh264 is present. With | ||
| # equal cost the tie-break is list order = the user's | ||
| # --video-decoders order, which is the right authority. | ||
| setup_cost=0, |
There was a problem hiding this comment.
I don't think it's right to lower the setup-cost to allow this encoder to be selected.
libva decoders are slower to initialize than openh264.
Maybe we should select based on cpu-cost / gpu-cost instead.
| if width > MAX_WIDTH or height > MAX_HEIGHT or (MAX_PIXELS > 0 and width * height > MAX_PIXELS): | ||
| # belt-and-braces for callers that bypass the size-aware | ||
| # decoder selection: fail here (cleanly, before creating a | ||
| # decoder that would error on every submit) | ||
| raise RuntimeError("%ix%i exceeds this device's decode limits" % (width, height)) |
There was a problem hiding this comment.
This copilot suggestion is overkill: we don't currently support the huge picture sizes that could ever overflow this.
But I guess the extra care is cheap.
| *display_out = display; | ||
| return 1; | ||
| } | ||
| fd = open(device, O_RDWR | O_CLOEXEC); |
There was a problem hiding this comment.
This chunk could easily be separated out.
| int cw = (w + 1) & ~1; | ||
| size_t ysize = (size_t)w * h; | ||
| size_t uvsize = (size_t)w * ((h + 1) / 2); | ||
| size_t uvsize = (size_t)cw * ((h + 1) / 2); |
There was a problem hiding this comment.
This looks reasonable and ties with the width_mask change, right?
| pic.CurrPic.picture_id = surface; | ||
| pic.CurrPic.frame_idx = (uint32_t)first->frame_num; | ||
| pic.CurrPic.flags = VA_PICTURE_H264_SHORT_TERM_REFERENCE; | ||
| pic.CurrPic.TopFieldOrderCnt = first->poc_lsb; | ||
| pic.CurrPic.BottomFieldOrderCnt = first->poc_lsb; | ||
| pic.CurrPic.flags = first->nal_ref_idc ? VA_PICTURE_H264_SHORT_TERM_REFERENCE : 0; | ||
| pic.CurrPic.TopFieldOrderCnt = top_foc; | ||
| pic.CurrPic.BottomFieldOrderCnt = bottom_foc; |
There was a problem hiding this comment.
This copilot comment is over my head, but looks legit.
| HARDWARE_ENCODER_OPTIONS: Sequence[str] = ("nvenc", "vpl", "libva", "nvjpeg", "vt") | ||
| ALL_CSC_MODULE_OPTIONS: Sequence[str] = ("cython", "libyuv") | ||
| ALL_VIDEO_DECODER_OPTIONS: Sequence[str] = ("vpl", "mf", "nvdec", "openh264", "vpx", "aom", "de265") | ||
| ALL_VIDEO_DECODER_OPTIONS: Sequence[str] = ("vpl", "mf", "nvdec", "libva", "openh264", "vpx", "aom", "de265") |
There was a problem hiding this comment.
As per above, I don't think that the list order is the right way of choosing a decoder, spec details are - that's what they're for.
| if libva_encoder_ENABLED: | ||
| ace("xpra.codecs.libva.encoder,xpra/codecs/libva/va_encode.c", | ||
| "libva,libva-win32" if WIN32 else "libva,libva-drm") | ||
| "libva,libva-win32" if WIN32 else "libva,libva-drm,libva-x11,x11") |
There was a problem hiding this comment.
As per above, x11 bits should only be enabled if x11_ENABLED.
This series makes the libva client decoder usable on X11-only VA drivers and implements a proper H.264 multi-reference DPB, replacing the current single-reference behaviour. Developed and validated on a pre-DRM NVIDIA setup (GeForce GT 130, driver 340.xx — no render node) through the VDPAU-backed VA driver, but every change is generic.
1.
libva: X11 VADisplay support + register the video decoderlibva_open_display()accepts devicex11[:<name>](XOpenDisplay +vaGetDisplay, with symmetric teardown) and the decoder device scan falls back tox11when no render node works. VDPAU-backed VA has no DRM constructor at all, so DRM-only display opening made the module unusable there.get_output_frame()falls back fromvaDeriveImagetovaCreateImage+vaGetImage(the ffmpeg hwcontext pattern) for drivers whose surfaces have no linear CPU view.dec_libvais registered inCODEC_TO_MODULE/ALL_VIDEO_DECODER_OPTIONS— the module shipped without the decoder being selectable (--video-decoders=libvawarned "unknown").2.
client paint: size- and capacity-aware video decoder selectionTwo production failure modes motivated this:
init_contextfailure bumped the shared spec'ssetup_costuntil the decoder was delisted session-wide — one oversized window permanently demoted the hardware decoder for every window.max_instancesexisted inCodecSpecbut the client decoder path neither tracked instances nor read it.So: new
CodecSpec.max_pixels(area limits thatmax_w/max_hcan't express, e.g. VDPAU feature set A = 8192 macroblocks but 2048/side), a hard envelope filter before instantiation, decoder creation throughmake_instance(), andchoose_decoder()ranking by(100 - setup_cost) × get_runtime_factor()— the same capacity mechanismvideo_scoring.pyalready applies to encoders/CSC. The libva spec declares its envelope (env-tunable),max_instances=1,setup_cost=0, and accepts odd display sizes (streams are coded even + SPS crop; the NV12 copy-out now even-rounds the chroma stride).Note on
setup_cost=0: with the current cost-only ranking, libva's 50 vs openh264's 0 meant the hardware decoder could never win. Zero makes the tie-break the user's--video-decodersorder. Happy to adjust if you'd rather express this differently.3.
libva: H.264 multi-reference DPBThe decoder kept a single reference (
ReferenceFrames[0], 4 surfaces, every frame — even non-references — overwrote it). Real encoder output is multi-ref whenever speed drops (x264 faster/fast/medium default ref=2/2/3; NVENC emitsmax_num_ref_frames=3at quality presets), and multi-ref slices against the wrong reference produce whole-window MC garbage.Implemented for progressive streams: 16-entry DPB + 17-surface pool, POC types 0/2 (type 1 cleanly rejected),
dec_ref_pic_markingparsed and applied (sliding window + MMCO 1–6 incl. MMCO5, IDR flush, long-term),nal_ref_idcgating,ref_pic_list_modificationparsed with the spec-default P list + modifications fillingRefPicList0. B slices are rejected at slice-header parse (no display-order reordering here; clean rejection lets the paint code fall back instead of decoding garbage).decompress_imagenow raisesCodecStateExceptionon mid-stream errors so the paint code restarts the decoder (aRuntimeErrorpropagates and leaves a stale decoder in place).Validation
ref=1/2/3/16@1280×720 andref=3@1920×1080, 120 frames each: bit-exact (per-frame NV12 MD5) vs ffmpeg software decode, through the VDPAU-backed VA driver on the X11 display path.referenceFramesreachingVdpDecoderRenderand decoder creation withmax_references=3.nrf=3stream: sustained hardware decode, zero errors.Happy to split this into separate PRs if preferred.