Conversation
H.264 passthrough had two independent defects. On Windows hosts it
negotiated no H.264 at all, and where it did engage it delivered about
half the saving it should.
GfxAVC444 = FALSE was set to make the client advertise AVC420-only,
which is not what the combination does. FreeRDP gates the entire RDPGFX
V10+ capability block on
!GfxH264 || GfxAVC444
(rdpgfx_main.c:254, rdpgfx_send_caps_advertise_pdu), so with GfxH264 set
and GfxAVC444 clear no V10 capability set is sent at all -- the client
advertises only V8 and V8.1. Windows will not offer H.264 at V8.1: it
falls back to CLEARCODEC and CAPROGRESSIVE, which guacd then decodes and
re-encodes as JPEG/WebP, exactly the cost the feature exists to avoid.
The display stays correct, so the regression is invisible without
measuring CPU or counting codecs on the wire. Over a minute of 1080p
video on Windows 11 Pro: 4050 CLEARCODEC + 1447 CAPROGRESSIVE + zero
H.264 frames before, all frames codec 15 after. xrdp is unaffected,
which is why this went unnoticed -- it accepts AVC420 at V8.1 and never
needed the V10 capsets.
AVC444 was disabled because forwarding it was broken, not because
AVC420 was wanted: only bitstream[0] reached the client, rendering as a
green/magenta split. Forwarding one view cannot work -- both bitstreams
decode through the same H264_CONTEXT in FreeRDP's avc444_decompress, so
they are one H.264 sequence with alternating views. Dropping either
breaks reference frames for the other, and the decoder reports no error
because what arrives is well formed. Both views are now forwarded,
tagged with a view field on the h264 instruction; the client decodes
both, draws only view 0, and combines the auxiliary view's chroma into
full 4:4:4 in a WebGL2 pass, falling back to 4:2:0 if it cannot.
The GDI decode is now skipped for captured H.264 commands. Previously
the wrapper always called through to the original handler, so only the
re-encode was saved. RemoteFX, planar and progressive still decode
normally, so a server mixing codecs within a frame stays correct.
guacd CPU with 1080p video playing, sampled per-thread over 30s:
xrdp (AVC420) ~100% of a core -> 2.0%
Windows 11 (AVC444) 90.6% of a core -> 2.1%
Configuration is unchanged: the per-connection enable-h264 argument is
the only switch, and there are no environment variables.
Recordings capture the raw protocol stream, so a recording of an H.264 session contains h264 instructions. recordings.html loaded neither H264Decoder.js nor Yuv444.js, so playback of those sessions rendered nothing.
Review notes from the upstream issue thread: - The auxiliary-view snapshot was sized to the auxiliary frame's own dimensions. The v1 chroma layout pads that view to a multiple of 16 rows, so the snapshot could be taller than the picture the renderer draws; with no rects the whole thing is blitted, painting a blank strip below the image. Size it from the rendered canvas instead. - The per-frame watchdog was cleared before combineFrame() ran, so the 4:4:4 path had no timeout despite the comment saying otherwise: a copyTo() that never settled would hold the ordered display queue with nothing able to release it. Clear it once the frame is done with instead -- in the finally for the synchronous paths, and after the copy settles for the combine. - Drop the unreachable view != 0 branch after the combine hand-off. - '0' no longer coerces to false in override(), so ?h264ChromaFilter=0 sets threshold zero rather than switching the filter off. Boolean overrides are unaffected: they coerce, and 0 is falsy. - Drop the stale stats() reference from client.html; the decoder has no such method.
|
I think CI is failing on main, not on this branch — git diff origin/main HEAD -- src Cargo.toml Cargo.lock .github is empty, so there's no Rust in this PR at all. Two pre-existing issues, both time-based: clippy 1.98 flags needless_late_init at tunnel.rs:259/273 and result_large_err at api.rs:2143 (CI installs unpinned stable), and cargo audit hits RUSTSEC-2026-0258 on h2 0.4.15, published five days after v1.9.9. |
|
I built this branch this evening and can confirm it seems to work as intended. On the previous 1.9.9 Rustguac release, a single RDP session to a Windows 11 box doing AVC444 I was seeing the Rustguac server hit 40-50% CPU load. On the client machine I was running Edge, and dev tools confirms WebCodecs::VideoDecoder was showing a h264 session running. Video playback through the Rustguac session was buttery smooth, and no video artifacts present in the session so it appears the two bitstreams are being combined correctly. This looks like a nice win. |
|
I am running a custom patched versions of xrdp and xorgxrdp-glamor-gbm. I'll stand up a standard release versions on a VM and try to reproduce over the next few days. Unfortunately, I am traveling for work at the moment and have limited time until the coming weekend. |
guac_rdp_rdpgfx_channel_connected() installed the SurfaceCommand and
CapsConfirm wrappers unconditionally, every time it ran. It runs again when
the RDPGFX channel reconnects, which xrdp provokes with the display resize it
performs at login, and the second pass saved the wrapper the first pass had
installed:
rdp_client->orig_surface_command = (void*) rdpgfx->SurfaceCommand;
rdpgfx->SurfaceCommand = guac_rdp_gfx_surface_command;
leaving orig_surface_command pointing at guac_rdp_gfx_surface_command itself.
Any surface command that is not H.264 falls through to orig(context, cmd),
re-enters the wrapper, and recurses until the stack is exhausted and the guacd
child dies -- a white screen in the browser, with no "Client terminated
successfully" in the log and the segfault visible only outside a container.
H.264 commands take skip_decode and never reach that call, so a server sending
nothing but H.264 is unaffected. A stock xrdp is the opposite case: one trace
showed 7440 planar and 12 progressive surface commands against 437 AVC420, so
it dies on the first frame. Distro xrdp packages (0.10.1 on Ubuntu) are built
--enable-rfxcodec with no H.264 encoder at all, so they send planar for
everything and hit this immediately.
The wrappers were also installed for every GFX session regardless of
enable_h264, so this reproduced with RFX alone and H.264 unticked -- no
H.264-capable server required.
Both halves are fixed: install only when the connection asked for H.264, and
only when nothing is installed yet. guac_rdp_client is guac_mem_zalloc'd, so
the NULL check is sound on first entry.
|
Found and fixed the white screen in latest commit — it was a crash in the passthrough, not a rendering Root cause
rdp_client->orig_surface_command = (void*) rdpgfx->SurfaceCommand; // already ours
rdpgfx->SurfaceCommand = guac_rdp_gfx_surface_command;so Since the wrappers were installed for every GFX session and Repro without any H.264 server Ubuntu's xrdp 0.10.1 is built I don't currently have a compiled version of stock xrdp with --enable-x264 compiled in. Please give |
|
I compiled a stock version of xrdp v0.10.6 with h264 support on Xubuntu 26.04 for testing today. Edit: 2449aa5 stopped the RDPGFX handler from wrapping its own wrapper, which fixed the recursion crash — but it guarded on orig_surface_command == NULL. That field lives on guac_rdp_client, so it's per-connection, while the handler it protects is per-channel-instance. FreeRDP's gdi_graphics_pipeline_uninit() sets SurfaceCommand to NULL and gdi_graphics_pipeline_init() assigns gdi_SurfaceCommand back (libfreerdp/gdi/gfx.c:1864, :1911), so when the channel reconnects FreeRDP's own handler is restored — and the once-only guard, seeing a non-NULL orig_surface_command from the first install, declines to wrap it again. xrdp reconnects the channel at login, for the same display resize 2449aa5 identified, so this fired on essentially every xrdp session. And it was silent: surface commands still reach FreeRDP's GDI decoder and guacd re-encodes them as JPEG/WebP, so the desktop renders correctly at ~100% of a core while no H.264 ever reaches the browser. On xrdp 0.10.6 I traced one keyframe forwarded, the channel unloading 52ms later, and no H.264 passthrough enabled line after the reconnect which initially fooled me. The fix guards each callback on its own pointer. That part matters: gdi_graphics_pipeline_init() assigns SurfaceCommand but never touches CapsConfirm, so after a reconnect CapsConfirm is still ours while SurfaceCommand is FreeRDP's again. A single guard keyed on SurfaceCommand passes and then points orig_caps_confirm at guac_rdp_gfx_caps_confirm itself — the same stack exhaustion, reached by the other callback. I hit exactly that before landing this. Same host after the fix: 807 AVC420 deltas and 3 keyframes over 60s of desktop use, guacd at 3.3% of a core. To confirm passthrough is actually live, you want two of these per session — one either side of the channel reconnect: journalctl -u guacd --since "2 min ago" One H.264 passthrough enabled for RDPGFX channel. line means the reconnect ate it. |
The previous commit stopped guac_rdp_rdpgfx_channel_connected() from wrapping
its own wrapper, which fixed the recursion crash, but it guarded on
orig_surface_command being NULL:
if (rdp_client->settings->enable_h264
&& rdp_client->orig_surface_command == NULL) {
orig_surface_command lives on guac_rdp_client, so it is per-connection, while
the handler it protects is per-channel-instance. FreeRDP's
gdi_graphics_pipeline_uninit() sets SurfaceCommand to NULL and
gdi_graphics_pipeline_init() assigns gdi_SurfaceCommand back
(libfreerdp/gdi/gfx.c:1864, :1911), so a reconnect restores FreeRDP's own
handler -- and the once-only guard, seeing a non-NULL orig_surface_command
from the first install, declines to wrap it again. Passthrough is then dead
for the rest of the session.
xrdp reconnects the channel at login, for the display resize the previous
commit already identified, so this fired on essentially every xrdp session.
It was silent: the surface commands still reached FreeRDP's GDI decoder and
guacd re-encoded them as JPEG/WebP, so the desktop rendered correctly at
~100% of a core while no H.264 reached the browser. A trace on xrdp 0.10.6
showed one keyframe forwarded, the channel unloading 52ms later, and no
"H.264 passthrough enabled" line after the reconnect that followed.
Guard on what is actually installed, per callback. The two are restored on
different schedules -- gdi_graphics_pipeline_init() assigns SurfaceCommand
but never touches CapsConfirm -- so after a reconnect CapsConfirm is still
ours while SurfaceCommand is FreeRDP's again. A single guard keyed on
SurfaceCommand would pass and then point orig_caps_confirm at
guac_rdp_gfx_caps_confirm itself, reaching the same stack exhaustion by the
other callback.
With this, the same host forwards 807 AVC420 deltas and 3 keyframes over 60s
of desktop use, with guacd at 3.3% of a core.
|
I built b3b2cce this evening and gave it a go. No regressions I could find on my Windows boxen. Thumbs up for Windows at least from me. |
The combining shader hardcoded BT.709 at full range, citing the coefficients FreeRDP decodes these streams with. But in passthrough guacd never decodes -- the browser does, and the 4:2:0 path never reaches the shader at all: that VideoFrame is drawn by the browser, which applies the frame's own colour space including the limited-range expansion of 16-235 to 0-255. So the two paths converted differently on the same session. An H.264 stream carrying no VUI signalling is limited range, and treating it as full leaves black at RGB 16 and white at 235, with chroma unscaled to match: the combined 4:4:4 picture renders visibly flatter than the 4:2:0 one beside it, depending only on which codec the server chose. Take the range and matrix from the decoded frame's colorSpace instead, read once from the first main view, and default to limited range BT.709 when the decoder reports nothing -- which is what the absence of signalling means. VideoFrame.colorSpace was available in scope and never consulted.
The colour-space change introduced "float y" for the scaled luma, but y is already the int pixel row in that scope. GLSL rejected the redefinition and then every arithmetic line that mixed the two, so the shader failed to compile and the renderer fell back to 4:2:0 for the whole session: ERROR: 0:89: 'y' : redefinition ERROR: 0:93: '+' : no operation exists that takes highp int and highp float Name it luma. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
waitForPending() holds the Guacamole sync response while pendingDecodes exceeds MAX_PIPELINE_DEPTH, but resolveIfIdle() only fired its resolvers at pendingDecodes <= 0. So once the backlog crossed the depth it had to drain completely before a sync could go out, and a session decoding continuously never reaches zero. Every sync therefore waited out its full timeout, logging "sync wait timeout" and reporting that much processing lag however fast the client was really going. guacd paces frames against the client's processing lag, so the session runs slow while the client is not in fact behind. The function's own documentation already said the callback fires "when the backlog is within the allowed pipeline depth"; only the implementation disagreed. It shows up first on AVC444, where a picture is two access units and the backlog therefore runs twice as deep for the same frame rate, making it far less likely to touch zero between frames. That presents as AVC444 being expensive rather than as a threshold mismatch, so an A/B that varies the server between AVC444 and AVC420 while holding the client constant cannot tell the two apart.
…ace" This reverts 8988165, restoring the unconditional full-range BT.709 conversion. The commit it undoes identified a real inconsistency and resolved it toward the wrong side. Its reasoning was that the 4:2:0 path never reaches the shader, so the combined 4:4:4 picture converted differently from the 4:2:0 one beside it, and that adopting the decoded frame's colour space would make the two agree. The two did then agree -- as limited range, on hosts that send full range. Before it, 4:4:4 was correct and 4:2:0 was crushed; after it, both were crushed. The symptom it describes, "the combined 4:4:4 picture renders visibly flatter", was the correct picture beside a crushed one. What every RDP host actually sends is now read out of the SPS rather than assumed. Windows writes video_full_range_flag=1 with no colour description. The xrdp fork writes it with a complete BT.709 description. Stock xrdp 0.10.6 omits the video_signal_type block altogether -- it passes x264 no VUI parameters -- while naming its own conversion XRDP_yuv444_709fr. All three encode full-range BT.709, which is what MS-RDPEGFX defines. What the browser reports for those streams is a different question. Chrome's hardware decoder ignores a bare range flag with no description beside it and reports limited for a host that plainly declared full, so a shader that adopts the frame's colour space expands 16-235 to 0-255 on full-range samples: blacks crushed to zero, whites clipped, chroma over-saturated by 255/224. The inconsistency this reinstates is real and is not being dismissed: a session can render 4:4:4 correctly and 4:2:0 crushed. One path right beats both wrong, and the repair belongs upstream of the browser -- completing the host's SPS on the wire, so that neither path has to guess and third-party clients benefit too. That follows separately; it is server-side and does not belong in this PR. The coefficients are exact BT.709 rather than the 8-bit approximations the original cited from FreeRDP's prim_internal.h. Under passthrough FreeRDP never decodes these streams, so matching its rounding was never the point. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013Rxg4cT87G9czw4nUxKhHG
|
I've pushed a revert of 8988165 (the color-space adoption), restoring the Short version: that commit identified a real inconsistency between the 4:4:4 I've spent a while since then on color-space handling and on AVC444 decode
Chrome's hardware decoder ignores a bare range flag with no color description beside it and reports Two consequences for this PR:
Separately, on performance: the 4:4:4 combine costs roughly 1.37ms per Unless I find something fatal, I am done tinkering with this PR for now. Let me know if you |
3 participants
Fixes the two defects from #208 : the GfxAVC444 gate that leaves Windows hosts negotiating no H.264 at all, and the server-side decode that ran on every frame despite being unused.
Review notes from the issue thread are folded into f16eed7. Built and running against FreeRDP 3 on Debian 13; guacd CPU with 1080p video goes from ~100% of a core to ~2%.
The v1 AVC444 chroma layout is untested — neither xrdp nor the Windows hosts here emit it.