mirror of
https://github.com/sol1/rustguac.git
synced 2026-10-24 20:52:24 +00:00
fix(rdp): reinstall the RDPGFX wrappers after a channel reconnect
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.
This commit is contained in:
@@ -992,7 +992,7 @@ index 327e7c2b..c49737d7 100644
|
||||
/**
|
||||
* Callback which associates handlers specific to Guacamole with the
|
||||
* RdpgfxClientContext instance allocated by FreeRDP to deal with received
|
||||
@@ -63,13 +438,45 @@ static void guac_rdp_rdpgfx_channel_connected(rdpContext* context,
|
||||
@@ -63,13 +438,68 @@ static void guac_rdp_rdpgfx_channel_connected(rdpContext* context,
|
||||
RdpgfxClientContext* rdpgfx = (RdpgfxClientContext*) args->pInterface;
|
||||
rdpGdi* gdi = context->gdi;
|
||||
|
||||
@@ -1009,27 +1009,50 @@ index 327e7c2b..c49737d7 100644
|
||||
+ guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
|
||||
+
|
||||
+ /* Installed only for a connection that asked for H.264, and only
|
||||
+ * once. This handler runs again whenever the RDPGFX channel
|
||||
+ * reconnects -- the display resize xrdp performs at login triggers
|
||||
+ * exactly that -- and a second install reads back the wrapper the
|
||||
+ * first one left behind, so orig_surface_command ends up pointing at
|
||||
+ * guac_rdp_gfx_surface_command itself. Every surface command that is
|
||||
+ * not H.264 then calls the wrapper from inside the wrapper and
|
||||
+ * recurses until the stack is gone. H.264 commands take the
|
||||
+ * onto a handler that is not already ours. This runs again every
|
||||
+ * time the RDPGFX channel reconnects -- the display resize xrdp
|
||||
+ * performs at login triggers exactly that -- and the two ways of
|
||||
+ * getting it wrong fail in opposite directions.
|
||||
+ *
|
||||
+ * Reinstalling unconditionally is fatal: the second pass reads back
|
||||
+ * the wrapper the first one left, orig_surface_command ends up
|
||||
+ * pointing at guac_rdp_gfx_surface_command itself, and every surface
|
||||
+ * command that is not H.264 calls the wrapper from inside the
|
||||
+ * wrapper until the stack is gone. H.264 commands take the
|
||||
+ * skip_decode path and never make that call, so a server sending
|
||||
+ * nothing but H.264 survives it, while one mixing in planar or
|
||||
+ * progressive surfaces dies on the first of them. */
|
||||
+ if (rdp_client->settings->enable_h264
|
||||
+ && rdp_client->orig_surface_command == NULL) {
|
||||
+ * nothing but H.264 survives it while one mixing in planar or
|
||||
+ * progressive surfaces dies on the first of them.
|
||||
+ *
|
||||
+ * Installing only once is quieter and just as broken.
|
||||
+ * gdi_graphics_pipeline_uninit() sets SurfaceCommand to NULL and
|
||||
+ * gdi_graphics_pipeline_init() puts gdi_SurfaceCommand back, so a
|
||||
+ * reconnect leaves FreeRDP's own handler installed while a
|
||||
+ * once-only guard declines to wrap it again. Passthrough then stops
|
||||
+ * for the rest of the session -- silently, since the frames still
|
||||
+ * render via GDI and guacd simply re-encodes them.
|
||||
+ *
|
||||
+ * Testing what is actually installed, per callback, covers both. */
|
||||
+ if (rdp_client->settings->enable_h264) {
|
||||
+
|
||||
+ /* Wrap SurfaceCommand to intercept H.264 data after GDI decode.
|
||||
+ * Store the original callback per-connection to avoid clobbering
|
||||
+ * when multiple RDP sessions are active concurrently. */
|
||||
+ rdp_client->orig_surface_command = (void*) rdpgfx->SurfaceCommand;
|
||||
+ rdpgfx->SurfaceCommand = guac_rdp_gfx_surface_command;
|
||||
+ if (rdpgfx->SurfaceCommand != guac_rdp_gfx_surface_command) {
|
||||
+ rdp_client->orig_surface_command = (void*) rdpgfx->SurfaceCommand;
|
||||
+ rdpgfx->SurfaceCommand = guac_rdp_gfx_surface_command;
|
||||
+ }
|
||||
+
|
||||
+ rdp_client->orig_caps_confirm = (void*) rdpgfx->CapsConfirm;
|
||||
+ rdpgfx->CapsConfirm = guac_rdp_gfx_caps_confirm;
|
||||
+ /* Guarded separately, because the two callbacks 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. One shared guard therefore re-reads a wrapper
|
||||
+ * we installed ourselves and points orig_caps_confirm at
|
||||
+ * guac_rdp_gfx_caps_confirm. */
|
||||
+ if (rdpgfx->CapsConfirm != guac_rdp_gfx_caps_confirm) {
|
||||
+ rdp_client->orig_caps_confirm = (void*) rdpgfx->CapsConfirm;
|
||||
+ rdpgfx->CapsConfirm = guac_rdp_gfx_caps_confirm;
|
||||
+ }
|
||||
+
|
||||
+ guac_client_log(client, GUAC_LOG_INFO,
|
||||
+ "H.264 passthrough enabled for RDPGFX channel.");
|
||||
|
||||
Reference in New Issue
Block a user