Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -3689,10 +3689,10 @@ def bundle_tests() -> None:
toggle_packages(libva_ENABLED, "xpra.codecs.libva")
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")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per above, x11 bits should only be enabled if x11_ENABLED.

if libva_decoder_ENABLED:
ace("xpra.codecs.libva.decoder,xpra/codecs/libva/va_decode.c",
"libva,libva-win32" if WIN32 else "libva,libva-drm")
"libva,libva-win32" if WIN32 else "libva,libva-drm,libva-x11,x11")
toggle_packages(gstreamer_ENABLED, "xpra.gstreamer")
toggle_packages(remote_encoder_ENABLED, "xpra.codecs.remote")

Expand Down
46 changes: 40 additions & 6 deletions xpra/client/gui/window/backing.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,16 +125,21 @@ def rgba_text(text: str, width: int = 64, height: int = 32, x: int = 20, y: int


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),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

# scaled by the spec's runtime factor - the same mechanism the
# server's pipeline scoring uses (video_scoring.py): a spec at its
# `max_instances` capacity returns 0 and loses to any available
# alternative, but remains eligible if there is nothing else
scores: dict[float, list[int]] = {}
for index, decoder_spec in enumerate(decoders_for_cs):
cost = decoder_spec.setup_cost
if cost > max_setup_cost:
continue
scores.setdefault(cost, []).append(index)
score = (100 - cost) * decoder_spec.get_runtime_factor()
scores.setdefault(score, []).append(index)
if not scores:
raise RuntimeError("no decoders available!")
best_score = sorted(scores)[0]
best_score = sorted(scores)[-1]
options_for_score = scores[best_score]
# if multiple decoders have the same score, just use the first one:
chosen = decoders_for_cs[options_for_score[0]]
Expand Down Expand Up @@ -820,20 +825,49 @@ def restart(msg: str, *args) -> None:
if not all_decoders_for_cs:
raise RuntimeError(f"no video decoders for {coding!r} and {input_colorspace!r}")
decoders_for_cs = list(all_decoders_for_cs)

# filter out decoders whose hard limits this stream
# exceeds (min/max dimensions, max_pixels area, ie:
# driver decode envelopes), so that such streams fall
# back to another decoder instead of poisoning the
# shared spec's setup_cost via guaranteed init_context
# failures (which at >100 delists the decoder for the
# entire session).
# (`max_instances` capacity is handled by
# choose_decoder() via get_runtime_factor, like the
# server's pipeline scoring - and dimension masks are
# deliberately NOT checked here: real decoders handle
# padded+cropped sizes regardless of their alignment
# masks, and pre-filtering on them can leave an
# at-capacity decoder as the only candidate)
def within_limits(ds) -> bool:
return (ds.min_w <= enc_width <= ds.max_w
and ds.min_h <= enc_height <= ds.max_h
and (ds.max_pixels <= 0 or enc_width * enc_height <= ds.max_pixels))

hard_ok = [ds for ds in decoders_for_cs if within_limits(ds)]
if hard_ok:
decoders_for_cs = hard_ok
else:
videolog.warn(f"Warning: no decoder is rated for {enc_width}x{enc_height} {coding} frames")
videolog.warn(" trying anyway with: " + csv(d.codec_type for d in decoders_for_cs))
while not self._video_decoder:
decoder_spec = choose_decoder(decoders_for_cs)
videolog("paint_with_video_decoder: new %s%s",
decoder_spec.codec_type, (coding, enc_width, enc_height, input_colorspace))
try:
vd = decoder_spec.codec_class()
# make_instance (not codec_class()) so the spec's
# instance WeakSet tracks live decoders — that is
# what max_instances gating counts
vd = decoder_spec.make_instance()
vd.init_context(coding, enc_width, enc_height, input_colorspace, options)
self._video_decoder = vd
break
except TransientCodecException 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 += 10
except RuntimeError as e:
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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This chunk is very useful and can be merged as-is.
(I just never hit those limits myself)

Expand Down
5 changes: 5 additions & 0 deletions xpra/codecs/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,11 @@ class CodecSpec:
min_h : int = 1
max_w : int = 4 * 1024
max_h : int = 4 * 1024
# maximum total pixels per frame (0 = no limit): hardware decoders
# often have an AREA limit that max_w/max_h alone cannot express,
# ie: VDPAU feature-set-A caps H264 at 8192 macroblocks (2097152
# pixels) while allowing 2048 in each dimension
max_pixels : int = 0
can_scale : bool = False
score_boost : int = 0
width_mask : int = 0xFFFF
Expand Down
47 changes: 40 additions & 7 deletions xpra/codecs/libva/decoder.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,12 @@ from time import monotonic
from typing import Any, Dict, Tuple
from collections.abc import Sequence

from xpra.codecs.constants import VideoSpec, EncodingNotSupported
from xpra.codecs.constants import VideoSpec, EncodingNotSupported, CodecStateException
from xpra.codecs.vacommon import config_libva_logging
from xpra.codecs.image import ImageWrapper
from xpra.common import SizedBuffer
from xpra.util.objects import typedict
from xpra.util.env import envint
from xpra.log import Logger

log = Logger("decoder", "libva")
Expand Down Expand Up @@ -155,7 +156,12 @@ cdef bint supports(str encoding, str colorspace):
return bool(libva_decode_supports(enc, cs))


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):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would you use VDPAU with nvidia when we have nvjpeg (#3504) and nvdec (#3703) ?
These should be more reliable.

MAX_WIDTH = envint("XPRA_LIBVA_MAX_WIDTH", 8192)
MAX_HEIGHT = envint("XPRA_LIBVA_MAX_HEIGHT", 8192)
MAX_PIXELS = envint("XPRA_LIBVA_MAX_PIXELS", 0)


def get_specs() -> Sequence[VideoSpec]:
Expand All @@ -174,10 +180,25 @@ def get_specs() -> Sequence[VideoSpec]:
codec_type=get_type(),
quality=80, speed=90,
size_efficiency=70,
setup_cost=50,
# setup_cost 0 (was 50): choose_decoder() ranks by
# setup_cost alone and openh264 declares 0, so any
# nonzero value here means the hardware decoder can
# 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,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# ONE live hardware decode stream at a time: on the
# VDPAU bridge (NVIDIA VP2 era) two concurrent decode
# streams corrupt each other's luma via the shared video
# engine's per-channel state (measured: whole-frame Y
# damage on 13-41% of frames, chroma bit-exact). The
# selection layer routes additional streams to the next
# decoder (ie: openh264) while one is live.
max_instances=1,
min_w=64, min_h=64,
width_mask=0xFFFE, height_mask=0xFFFE,
width_mask=0xFFFF, height_mask=0xFFFF,
max_w=MAX_WIDTH, max_h=MAX_HEIGHT,
max_pixels=MAX_PIXELS,
cpu_cost=10,
gpu_cost=80,
))
Expand All @@ -203,8 +224,15 @@ cdef class Decoder:
log("libva.decoder.init_context%s", (encoding, width, height, colorspace, options))
assert encoding in ENCODINGS, "unsupported encoding: %s" % encoding
assert colorspace in COLORSPACES[encoding], "invalid colorspace %s for %s" % (colorspace, encoding)
if width & 1 or height & 1:
raise ValueError("invalid odd width %i or height %i" % (width, height))
# odd display sizes are fine: the bitstream is coded at the
# padded even size (SPS crop carries the display size) and the
# NV12 copy-out handles odd width/height (ceil'd chroma rows,
# even-rounded chroma stride)
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))
Comment on lines +231 to +235

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self.encoding = encoding
self.colorspace = colorspace
self.width = width
Expand Down Expand Up @@ -298,7 +326,12 @@ cdef class Decoder:
if status != LIBVA_DEC_OK:
detail = libva_decoder_get_last_error(self.context).decode("utf-8", "replace")
last_sts = libva_decoder_get_last_status(self.context)
raise RuntimeError("libva decode error: %s (detail: %s, sts=%d)" % (
# CodecStateException (not RuntimeError): the paint code
# restarts the decoder on it (backing.py), whereas other
# exception types propagate and leave a stale decoder in
# place; a mid-stream failure here always invalidates the
# decoder state (DPB / reference chain)
raise CodecStateException("libva decode error: %s (detail: %s, sts=%d)" % (

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there other places where we end up leaving a stale decoder behind?

libva_decode_status_str(status).decode("latin-1"), detail, last_sts))

pixels = tuple(frame.planes[i][:frame.sizes[i]] for i in range(frame.nplanes))
Expand Down
62 changes: 61 additions & 1 deletion xpra/codecs/libva/va_common.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
#include <va/va_win32.h>
#else
#include <va/va_drm.h>
#include <va/va_x11.h>
#include <X11/Xlib.h>
Comment on lines 14 to +16

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

#include <fcntl.h>
#include <unistd.h>
#endif
Expand Down Expand Up @@ -167,17 +169,75 @@ static inline int libva_open_display(const char *device, int *fd_out, VADisplay
return 1;
}
#else
/* X11 VADisplays (device "x11" or "x11:<name>") need their Display
* closed at teardown; VADisplay itself does not own it. Small per-TU
* registry, looked up by libva_x11_close() which no-ops for DRM
* displays. Rationale: VDPAU-backed VA drivers (nvidia-340 era) have
* no DRM path at all - vdp_device_create_x11 is VDPAU's only
* constructor - and such systems may have no render nodes either. */
#define LIBVA_X11_DISPLAYS_MAX 8
static Display *libva_x11_dpys[LIBVA_X11_DISPLAYS_MAX];
static VADisplay libva_x11_vadpys[LIBVA_X11_DISPLAYS_MAX];
static inline void libva_x11_register(VADisplay va, Display *dpy) {
for (int i = 0; i < LIBVA_X11_DISPLAYS_MAX; i++) {
if (!libva_x11_vadpys[i]) {
libva_x11_vadpys[i] = va;
libva_x11_dpys[i] = dpy;
return;
}
}
/* table full: the X connection is leaked on close */
}
static inline void libva_x11_close(VADisplay va) {
for (int i = 0; i < LIBVA_X11_DISPLAYS_MAX; i++) {
if (libva_x11_vadpys[i] == va) {
XCloseDisplay(libva_x11_dpys[i]);
libva_x11_vadpys[i] = NULL;
libva_x11_dpys[i] = NULL;
return;
}
}
}
static inline int libva_open_display(const char *device, int *fd_out, VADisplay *display_out,
int *major_out, int *minor_out,
char *vendor, size_t vendor_size,
char *error, size_t error_size) {
int fd = open(device, O_RDWR | O_CLOEXEC);
int fd;
VADisplay display;
VAStatus status;
const char *vstr;

*fd_out = -1;
*display_out = NULL;
if (strncmp(device, "x11", 3) == 0 && (device[3] == 0 || device[3] == ':')) {
/* "x11" opens $DISPLAY, "x11:<name>" a specific X display */
const char *dpy_name = (device[3] == ':') ? device + 4 : NULL;
Display *dpy = XOpenDisplay(dpy_name);
if (!dpy) {
snprintf(error, error_size, "XOpenDisplay failed for %.160s",
dpy_name ? dpy_name : "$DISPLAY");
return 0;
}
display = vaGetDisplay(dpy);
if (!display) {
snprintf(error, error_size, "vaGetDisplay failed for X11 display");
XCloseDisplay(dpy);
return 0;
}
status = vaInitialize(display, major_out, minor_out);
if (status != VA_STATUS_SUCCESS) {
snprintf(error, error_size, "vaInitialize failed for X11 display: %s (%d)",
vaErrorStr(status), (int)status);
XCloseDisplay(dpy);
return 0;
}
vstr = vaQueryVendorString(display);
snprintf(vendor, vendor_size, "%s", vstr ? vstr : "");
libva_x11_register(display, dpy);
*display_out = display;
return 1;
}
fd = open(device, O_RDWR | O_CLOEXEC);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This chunk could easily be separated out.

if (fd < 0)
return 0;
display = vaGetDisplayDRM(fd);
Expand Down
Loading