GUACAMOLE-2123: Fix remote app render issues. Use per-window guac_dis… by jjheinon · Pull Request #673 · apache/guacamole-server

jjheinon · 2026-05-14T11:08:00Z

Fixing remote app render issues with this patch, using per-window guac_display_layer for remote apps. Also introducing a fix for stale desktop background on RDPGFX.

Using per-window guac_display_layer for remote apps and also prevent dirty-region propagation to desktop background. Added support for UpdateSurfaceArea (FreeRDP 2.x) and UpdateWindowFromSurface (FreeRDP 3.x)

When Guacamole's RDP plugin is configured with both RemoteApp and the RDPGFX pipeline enabled, remote application windows fail to render.
With GFX active, FreeRDP does not update RAIL window contents via BeginPaint/Endpaint, but via RDPGFX surfaces using windowId and callbacks.
For FreeRDP 3.x: UpdateWindowFromSurface is called per window-mapped surface with its accumulated invalid region.
For FreeRDP 2.x: UpdateSurfaceArea is called per surface with a rect list.

This patch has a dedicated guac_display_layer for each remote app and derives visibility via showState. Surface content painted with RDPGFX callbacks and scaled to correct size.
Layer is kept hidden until both window position is known and at least one surface paint request has arrived, avoiding flashes and race conditions.

Tested remote apps with FreeRDP 3.26.0 with the latest staging/1.6.1 branch.

stephzero1 · 2026-05-14T12:25:30Z

great work

necouchman

Thanks @jjheinon, this is great - I had started to work on this some time back but hit a couple of walls that I couldn't get past, and it looks like you've figured out the issues I couldn't!

The code looks pretty good to me, though I need to take time to more thoroughly review it - most of the things that need to be changed so far deal with style, particularly the comment blocks of the functions you've implemented.

necouchman

@jjheinon:
A few more changes and a couple of questions. Also, one of your commits lacks the required GUACAMOLE-2123: tag.

Overall looking pretty good and just about ready for merge.

necouchman

Looking pretty good to me. Just one other change for an explicit type-cast at the moment.

necouchman

One more change, and I think it'll be ready to merge.

necouchman

This code is looking really good - a handful of changes, mostly to comments, but overall I'm liking it.

@jjheinon Are you good with it? I see a lot of comments and pushes coming in - do we need to convert it to a draft at this point, or are you good with it?

necouchman · 2026-06-20T12:57:48Z

@jjheinon I've marked this as a draft for now as it looks like you're still pretty actively working through things and making changes. When you're happy that it's ready for a final review and merge, just hit that Ready for review button.

…ng by tracking RAIL windows as independent Guacamole layers. Synchronize RDPGFX surface updates with RAIL window metadata. Track surface-to-window mappings, window ownership, visibility, geometry, activation, and z-order to render RemoteApp windows independently of the hidden desktop. Add support for both FreeRDP 2.x UpdateSurfaceArea and FreeRDP 3.x UpdateWindowFromSurface, including HiDPI scaling. Improved input responsiveness. Preserve the hidden desktop framebuffer and black background during RemoteApp sessions.

jjheinon · 2026-07-02T11:10:24Z

Quite happy with it now, not aware of any major issues or bugs (tested on FreeRDP 3.26.0).
For future reference, noticed that the memory usage for RDPGFX is quite massive compared to non-GFX, so something to optimize in the future. This is not just a RAIL issue but an overall optimization.

Also, the GfxProgressive codec is enabled for both non-RAIL and RAIL paths, but making additional codecs user configurable would be a nice improvement.
For example: FreeRDP_GfxProgressiveV2, FreeRDP_GfxH264, FreeRDP_GfxAVC444, FreeRDP_GfxAVC444v2

Will probably create a separate ticket about these in the future.

necouchman · 2026-07-02T21:07:31Z

Thanks @jjheinon, I'll try to get it reviewed here today or tomorrow.

necouchman

A few more items:

Overall, there are some large blocks of code, particularly in rail.c, where some inline comments to document the code flow, particularly the "why", would be useful.
In data structures, each variable should be documented independently - I understand the desire to document, e.g. x and y with a single comment block as they necessarily go together, but 1) it doesn't follow established style in the code, and 2) I think there are actually some reasons why we don't do things that way related to automatic generation of documentation.

necouchman · 2026-07-05T11:16:46Z

+    int rdp_x = x;
+    int rdp_y = y;


I'm looking through the code, here, and I'm not seeing a reason why we need to make copies of the x and y variables in this function? Does one of the other functions where they are passed as an argument through here potentially update them? Or is there some other need to duplicate them?

necouchman · 2026-07-05T11:21:55Z

+ *     The server-side ID of the RAIL window that was unmapped.
+ *
+ * @return
+ *     CHANNEL_RC_OK is always returned. Surface unmapping failures are ignored as the window might not be tracked.


If you could wrap this line at 80 columns that'd be nice :-).

necouchman · 2026-07-05T11:30:19Z

+    guac_display_layer_raw_context* current_context = NULL;
+
+    /* Full desktop sessions share the resized GDI buffer with the default
+     * layer, and must hold an open raw context while the external buffer
+     * details change. */
+    if (rdp_client->settings->remote_app == NULL
+            || !rdp_client->settings->enable_gfx
+            || !rdp_client->rail_background_painted)
+        current_context = guac_display_layer_open_raw(default_layer);


This there some reason why this particular operation - opening the raw display layer - needs to happen prior to the gdi_resize() call just below this? It looks like there are two identical if statements, here, and I'm just wondering if this guac_display_layer_open_raw() call could be moved inside the if statement down on line 224, or if it needs to happen prior to the FreeRDP GDI buffer resize?

necouchman · 2026-07-05T11:45:07Z


    /* Send client status */
-    guac_client_log(client, GUAC_LOG_TRACE, "Sending RAIL client status.");
+    guac_client_log(client, GUAC_LOG_DEBUG,


Is there a reason why this should be DEBUG level and not remain TRACE? This seems like pretty low-level log messages, particularly including the actual flags, so I'm thinking TRACE is more appropriate?

necouchman · 2026-07-05T11:46:38Z

    status = rail->ClientExecute(rail, &exec);
    pthread_mutex_unlock(&(rdp_client->message_lock));

+    guac_client_log(client, GUAC_LOG_DEBUG,


Since the log message above for the Executing remote application... log is TRACE, this should probably also be TRACE rather than DEBUG.

necouchman · 2026-07-05T11:52:52Z

+    int x;
+    int y;


This is a bit of a nit, but, in keeping with style used throughout the rest of the code, I think these variables need to be documented individually. I also think there's a technical reason behind it - I believe there are some automated processes that use the documentation to generate things like API documentation (e.g. https://guacamole.apache.org/doc/libguac/), and that documenting multiple variables with a single comment may cause some problems with that.

There are several places throughout the code, here, where this needs to be tweaked.

necouchman · 2026-07-05T12:00:06Z

+    for (guac_common_list_element* current = rdp_client->rail_windows->head;
+            current != NULL; current = current->next) {
+
+        guac_rdp_rail_window* rail_window = current->data;
+        if (rail_window == NULL)
+            continue;
+
+        if (rail_window->visible && rail_window->has_rail_order
+                && !rail_window->has_surface) {
+
+            if (!has_no_surface_window
+                    || rail_window->z_order > highest_no_surface_z) {
+                highest_no_surface_z = rail_window->z_order;
+                has_no_surface_window = 1;
+            }
+
+            continue;
+        }
+
+        if (!rail_window->visible || !rail_window->has_surface
+                || !rail_window->has_rail_order)
+            continue;
+
+        if (x < rail_window->x || y < rail_window->y
+                || x >= rail_window->x + rail_window->layer_width
+                || y >= rail_window->y + rail_window->layer_height)
+            continue;
+
+        if (clicked_window == NULL || rail_window->z_order > clicked_z) {
+            clicked_window = rail_window;
+            clicked_z = rail_window->z_order;
+        }
+
+    }


Might be nice to have a few comments in here to document the code flow in-line.

necouchman · 2026-07-05T12:01:04Z

+    if (rdp_client->mouse_button_mask != 0
+            && rdp_client->rail_mouse_window_id != 0)
+        target_window = guac_rdp_rail_get_window(rdp_client,
+                rdp_client->rail_mouse_window_id);
+
+    if (target_window == NULL) {
+
+        int target_z = 0;
+
+        for (guac_common_list_element* current = rdp_client->rail_windows->head;
+                current != NULL; current = current->next) {
+
+            guac_rdp_rail_window* rail_window = current->data;
+            if (rail_window == NULL)
+                continue;
+
+            if (!rail_window->visible || !rail_window->has_surface
+                    || !rail_window->has_rail_order)
+                continue;
+
+            if (x < rail_window->x || y < rail_window->y
+                    || x >= rail_window->x + rail_window->layer_width
+                    || y >= rail_window->y + rail_window->layer_height)
+                continue;
+
+            if (target_window == NULL || rail_window->z_order > target_z) {
+                target_window = rail_window;
+                target_z = rail_window->z_order;
+            }
+
+        }
+
+    }
+
+    if (target_window != NULL
+            && rdp_client->mouse_button_mask == 0 && mask != 0)
+        rdp_client->rail_mouse_window_id = target_window->window_id;
+
+    if (mask == 0)
+        rdp_client->rail_mouse_window_id = 0;
+
+    guac_common_list_unlock(rdp_client->rail_windows);


Similar to above, a handful of comments in here would be helpful.

necouchman · 2026-07-05T17:55:45Z

Note that the build currently fails when using FreeRDP on Rocky9 (2.11.7-7.el9_8.3):

channels/rail.c: In function ‘guac_rdp_rail_repaint_surface’:
channels/rail.c:1089:47: error: ‘guac_rdp_client’ has no member named ‘rdpgfx_interface’; did you mean ‘rail_interface’?
 1089 |     RdpgfxClientContext* rdpgfx = rdp_client->rdpgfx_interface;
      |                                               ^~~~~~~~~~~~~~~~
      |                                               rail_interface
channels/rail.c: In function ‘guac_rdp_rail_window_update’:
channels/rail.c:1353:31: error: implicit declaration of function ‘ConvertWCharNToUtf8Alloc’ [-Werror=implicit-function-declaration]
 1353 |                 char* title = ConvertWCharNToUtf8Alloc(title_string,
      |                               ^~~~~~~~~~~~~~~~~~~~~~~~
channels/rail.c:1353:31: error: initialization of ‘char *’ from ‘int’ makes pointer from integer without a cast [-Werror=int-conversion]
channels/rail.c: In function ‘guac_rdp_rail_paint_surface’:
channels/rail.c:1921:23: error: implicit declaration of function ‘freerdp_image_copy_no_overlap’ [-Werror=implicit-function-declaration]
 1921 |             success = freerdp_image_copy_no_overlap(
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[4]: *** [Makefile:1146: channels/libguac_client_rdp_la-rail.lo] Error 1
make[4]: Leaving directory '/devpool/guacamole/guacamole-server/src/protocols/rdp'
make[3]: *** [Makefile:1473: all-recursive] Error 1
make[3]: Leaving directory '/devpool/guacamole/guacamole-server/src/protocols/rdp'
make[2]: *** [Makefile:763: all] Error 2
make[2]: Leaving directory '/devpool/guacamole/guacamole-server/src/protocols/rdp'
make[1]: *** [Makefile:544: all-recursive] Error 1
make[1]: Leaving directory '/devpool/guacamole/guacamole-server'
make: *** [Makefile:464: all] Error 2

necouchman requested changes May 14, 2026

View reviewed changes

necouchman requested changes May 16, 2026

View reviewed changes

jjheinon force-pushed the guacamole-2123-remote-app-windows-gfx branch 3 times, most recently from b062650 to 735cd68 Compare May 16, 2026 13:02

jjheinon commented May 16, 2026

View reviewed changes

Comment thread src/protocols/rdp/rdp.c Outdated

jjheinon commented May 16, 2026

View reviewed changes

Comment thread src/protocols/rdp/gdi.c

jjheinon commented May 16, 2026

View reviewed changes

Comment thread src/protocols/rdp/gdi.c

jjheinon commented May 16, 2026

View reviewed changes

Comment thread src/protocols/rdp/gdi.c

necouchman requested changes May 16, 2026

View reviewed changes

Comment thread src/protocols/rdp/channels/rail.c

jjheinon commented May 17, 2026

View reviewed changes

Comment thread src/protocols/rdp/channels/rail.c

jjheinon commented May 17, 2026

View reviewed changes

Comment thread src/protocols/rdp/channels/rail.c

jjheinon commented May 17, 2026

View reviewed changes

Comment thread src/protocols/rdp/channels/rail.c

jjheinon commented May 17, 2026

View reviewed changes

Comment thread src/protocols/rdp/channels/rail.c

jjheinon commented May 17, 2026

View reviewed changes

Comment thread src/protocols/rdp/channels/rail.c

jjheinon force-pushed the guacamole-2123-remote-app-windows-gfx branch from 061ba6f to b8aa4f4 Compare May 17, 2026 08:55

jjheinon force-pushed the guacamole-2123-remote-app-windows-gfx branch from b8aa4f4 to cdd4ade Compare June 15, 2026 07:43

necouchman requested changes Jun 15, 2026

View reviewed changes

Comment thread src/protocols/rdp/channels/rail.c

jjheinon commented Jun 16, 2026

View reviewed changes

Comment thread src/protocols/rdp/channels/rail.c

jjheinon commented Jun 16, 2026

View reviewed changes

Comment thread src/protocols/rdp/channels/rail.c Outdated

jjheinon commented Jun 16, 2026

View reviewed changes

Comment thread src/protocols/rdp/channels/rdpgfx.c Outdated

jjheinon commented Jun 16, 2026

View reviewed changes

Comment thread src/protocols/rdp/gdi.c

jjheinon commented Jun 16, 2026

View reviewed changes

Comment thread src/protocols/rdp/channels/rail.c

jjheinon commented Jun 16, 2026

View reviewed changes

Comment thread src/protocols/rdp/channels/rail.c Outdated

jjheinon commented Jun 17, 2026

View reviewed changes

Comment thread src/protocols/rdp/channels/rail.c

jjheinon commented Jun 17, 2026

View reviewed changes

Comment thread src/protocols/rdp/channels/rail.c

jjheinon commented Jun 17, 2026

View reviewed changes

Comment thread src/protocols/rdp/channels/rail.c Outdated

jjheinon force-pushed the guacamole-2123-remote-app-windows-gfx branch from 10d806b to 50b8f58 Compare June 17, 2026 09:11

necouchman requested changes Jun 18, 2026

View reviewed changes

jjheinon force-pushed the guacamole-2123-remote-app-windows-gfx branch 5 times, most recently from a308e7a to 534b89a Compare June 19, 2026 11:56

jjheinon commented Jun 20, 2026

View reviewed changes

Comment thread src/protocols/rdp/channels/rail.c Outdated

necouchman marked this pull request as draft June 20, 2026 12:56

jjheinon force-pushed the guacamole-2123-remote-app-windows-gfx branch 4 times, most recently from 3fedd19 to 40556be Compare June 26, 2026 13:07

jjheinon force-pushed the guacamole-2123-remote-app-windows-gfx branch from 40556be to f860d9e Compare June 30, 2026 08:07

jjheinon marked this pull request as ready for review June 30, 2026 11:24

necouchman requested changes Jul 5, 2026

View reviewed changes

Uh oh!

Conversation

jjheinon commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephzero1 commented May 14, 2026

Uh oh!

necouchman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

necouchman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

necouchman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

necouchman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

necouchman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

necouchman commented Jun 20, 2026

Uh oh!

jjheinon commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

necouchman commented Jul 2, 2026

Uh oh!

necouchman left a comment

Choose a reason for hiding this comment

jjheinon commented May 14, 2026 •

edited

Loading

jjheinon commented Jul 2, 2026 •

edited

Loading