GUACAMOLE-2123: Fix remote app render issues. Use per-window guac_dis…#673
GUACAMOLE-2123: Fix remote app render issues. Use per-window guac_dis…#673jjheinon wants to merge 1 commit into
Conversation
|
great work |
necouchman
left a comment
necouchman
left a comment
There was a problem hiding this comment.
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
left a comment
necouchman
left a comment
There was a problem hiding this comment.
@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.
jjheinon
force-pushed
the
guacamole-2123-remote-app-windows-gfx
branch
3 times, most recently
from
b062650 to
735cd68
Compare
necouchman
left a comment
necouchman
left a comment
There was a problem hiding this comment.
Looking pretty good to me. Just one other change for an explicit type-cast at the moment.
| * finish, leaving subsequent input ignored after dragging a RemoteApp | ||
| * window. */ | ||
| RAIL_CLIENT_STATUS_ORDER client_status = { | ||
| .flags = |
There was a problem hiding this comment.
Cannot use TS_RAIL_CLIENTSTATUS_ALLOWLOCALMOVESIZE, or we might lose mouse input. We obey positioning info from server instead, which fixes the issue.
| * @return | ||
| * CHANNEL_RC_OK (zero) as the order has been ignored successfully. | ||
| */ | ||
| static UINT guac_rdp_rail_local_move_size(RailClientContext* rail, |
There was a problem hiding this comment.
Just in case server sends the request, we answer with CHANNEL_RC_OK.
|
|
||
| /* RDPGFX window surfaces are copied as opaque layer content. We don't | ||
| * want holes on our RemoteApp windows. */ | ||
| UINT32 dst_format = guac_rdp_get_native_pixel_format(FALSE); |
There was a problem hiding this comment.
GFX windows might have alpha channel, we don't want that when copying the layers to Guacamole side. (managed to get holes in paint app, where there should have been black instead). This fixes the issue.
jjheinon
force-pushed
the
guacamole-2123-remote-app-windows-gfx
branch
from
061ba6f to
b8aa4f4
Compare
jjheinon
force-pushed
the
guacamole-2123-remote-app-windows-gfx
branch
from
b8aa4f4 to
cdd4ade
Compare
necouchman
left a comment
necouchman
left a comment
There was a problem hiding this comment.
One more change, and I think it'll be ready to merge.
| guac_display_layer_set_opacity(rail_window->layer, opacity); | ||
| } | ||
|
|
||
| static void guac_rdp_rail_paint_background(guac_rdp_client* rdp_client) { |
There was a problem hiding this comment.
Moved the blackground painting here, as earlier we rendered the black background for RAIL apps too soon before the login was done. This caused screen to remain black if login failed.
| guac_client* client = (guac_client*) context->custom; | ||
| guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; | ||
|
|
||
| if (execResult->execResult == RAIL_EXEC_E_NOT_IN_ALLOWLIST |
There was a problem hiding this comment.
If server tells us RAIL_EXEC_E_NOT_IN_ALLOWLIST after rendering is already working and windows being drawn, just ignore it.
There was a problem hiding this comment.
Why would we just ignore it? If the application is not in the allow list, the server shouldn't actually provide the application, and we shouldn't get anything, correct?
There was a problem hiding this comment.
This was an interesting edge case. Apparently it's possible for Windows RemoteApp to launch another process, which does not belong to allowed app listing for the RemoteApp session.
In this case, the requested RAIL app launches properly (and renders the app window), but server sends RAIL_EXEC_E_NOT_IN_ALLOWLIST when it prevents launching the other app process.
Not sure what should be the optimal behavior in this case, but removed the modification and reverted back to the original behavior (connection fails), but added log message about it so the Windows admin can make the requested changes on their environment.
| * disabled. | ||
| */ | ||
|
|
||
| #if defined(HAVE_RDPGFX_SURFACE_AREA_UPDATE) \ |
There was a problem hiding this comment.
This was changed a bit, we kept on using UpdateSurfaceArea even when newer UpdateWindowFromSurface was available. Now it works better.
| * into its own primary buffer. | ||
| */ | ||
| if (rdp_client->settings->remote_app == NULL | ||
| || !rdp_client->settings->enable_gfx |
There was a problem hiding this comment.
Use primary buffer only if RAIL is disabled, GFX is disabled or both are enabled, but we're not yet received first paint event. This fixes the issue where invalid credentials ends up to Windows login screen on non-NLA mode, but login screen is not visible. Earlier we rendered black screen in that case.
|
|
||
| guac_display_layer_close_raw(rail_window->layer, layer_context); | ||
| if (!copied) { | ||
| guac_common_list_unlock(rdp_client->rail_windows); |
There was a problem hiding this comment.
Let's not render background before window rendering has started and we have received app data.
| guac_common_list_unlock(rdp_client->rail_windows); | ||
| return CHANNEL_RC_OK; | ||
| } | ||
| guac_rdp_rail_paint_background(rdp_client); |
There was a problem hiding this comment.
Render only after data has been received. This prevents us blocking Windows login dialog on non-NLA logins, if credentials are incorrect.
| rail_window->has_rail_order = 1; | ||
| int needs_raise = 0; | ||
|
|
||
| if (fieldFlags & WINDOW_ORDER_FIELD_OWNER) { |
There was a problem hiding this comment.
A window order includes the owner field. Make sure it's shown above the owner window.
| for (guac_common_list_element* current = rdp_client->rail_windows->head; | ||
| current != NULL; current = current->next) { | ||
| guac_rdp_rail_window* rail_window = current->data; | ||
| rail_window->in_monitored_desktop_zorder = 0; |
There was a problem hiding this comment.
We got the official z-index for this window from the server, so we don't need to handle it separately anymore.
jjheinon
force-pushed
the
guacamole-2123-remote-app-windows-gfx
branch
from
10d806b to
50b8f58
Compare
necouchman
left a comment
necouchman
left a comment
There was a problem hiding this comment.
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?
| * @return | ||
| * CHANNEL_RC_OK if the surface mapping was handled successfully. |
There was a problem hiding this comment.
...or what, if the mapping was not handled correctly?
| * @return | ||
| * CHANNEL_RC_OK if the surface unmapping was handled successfully. |
There was a problem hiding this comment.
...or what, if the unmapping was not handled successfully?
| * @return | ||
| * CHANNEL_RC_OK if the surface was painted successfully, or an error code. |
There was a problem hiding this comment.
What type of error code? FreeRDP? Guacamole? Generic negative/positive integer?
| * @return | ||
| * CHANNEL_RC_OK if the surface was painted successfully, or an error code. |
There was a problem hiding this comment.
What type of error code?
| guac_client_log(client, GUAC_LOG_WARNING, "Rendering backend for RDPGFX " | ||
| "channel could not be loaded. Graphics may not render at all!"); | ||
| else | ||
| } else { |
There was a problem hiding this comment.
Please don't cuddle braces.
| current != NULL; current = current->next) { | ||
| guac_rdp_rail_window* rail_window = current->data; | ||
|
|
||
| if (rail_window->z_order >= z) |
There was a problem hiding this comment.
And, here...check to make sure rail_window is not null.
| while (current != NULL) { | ||
| guac_rdp_rail_window* rail_window = current->data; | ||
|
|
||
| if (rail_window->owner_window_id == owner_window_id) { |
| guac_client* client = (guac_client*) context->custom; | ||
| guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; | ||
|
|
||
| if (execResult->execResult == RAIL_EXEC_E_NOT_IN_ALLOWLIST |
There was a problem hiding this comment.
Why would we just ignore it? If the application is not in the allow list, the server shouldn't actually provide the application, and we shouldn't get anything, correct?
| * @return | ||
| * CHANNEL_RC_OK if the surface was painted successfully, or an error code. |
There was a problem hiding this comment.
What kind of error code?
jjheinon
force-pushed
the
guacamole-2123-remote-app-windows-gfx
branch
4 times, most recently
from
7aa7b43 to
a308e7a
Compare
…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
force-pushed
the
guacamole-2123-remote-app-windows-gfx
branch
from
a308e7a to
534b89a
Compare
4 participants
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, also built with 2.11.7.