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 3 commits 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.
| /** | ||
| * The next RAIL window in the list. | ||
| */ | ||
| guac_rdp_rail_window* next; |
There was a problem hiding this comment.
It looks like this and several of the functions you implement are implementing a linked list. There's already a generic linked-list implementation provided within the guacd code:
https://github.com/apache/guacamole-server/blob/main/src/common/common/list.h
It might be worth at least considering re-using that rather than re-implementing all the linked list functions separately, here.
| /** | ||
| * Callback which is invoked when one or more areas of an RDPGFX surface have | ||
| * been updated. | ||
| * | ||
| * @param context | ||
| * The RDPGFX client context associated with the surface update. | ||
| * | ||
| * @param surface_id | ||
| * The server-side ID of the updated RDPGFX surface. | ||
| * | ||
| * @param rect_count | ||
| * The number of rectangles within the rects array. | ||
| * | ||
| * @param rects | ||
| * The updated rectangular regions of the surface. | ||
| * | ||
| * @return | ||
| * CHANNEL_RC_OK if the surface was painted successfully, or an error code. | ||
| */ |
There was a problem hiding this comment.
Since this is defined in rail.h, there's no need to document it, here.
| /** | ||
| * Frees all tracked RAIL windows associated with the given RDP client. | ||
| * | ||
| * @param rdp_client | ||
| * The RDP client whose tracked RAIL windows should be freed. | ||
| */ |
There was a problem hiding this comment.
This function also can be documented in rail.h and not, here.
jjheinon
force-pushed
the
guacamole-2123-remote-app-windows-gfx
branch
2 times, most recently
from
dd00012 to
b062650
Compare
…play_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)
jjheinon
force-pushed
the
guacamole-2123-remote-app-windows-gfx
branch
from
b062650 to
735cd68
Compare
…DI's hidden desktop framebuffer, preserving the black background during resizes
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.
| */ | ||
| static void guac_rdp_rail_free_window_data(void* data) { | ||
|
|
||
| guac_rdp_rail_window* rail_window = data; |
There was a problem hiding this comment.
This may work as-is, but shouldn't this be explicitly type-cast?
guac_rdp_rail_window* rail_window = (guac_rdp_rail_window*) data;
There was a problem hiding this comment.
No, void* implicitly converts to any pointer type. That would be required in C++ though, but not in C.
| * The RemoteApp window container layer, or NULL if it could not be | ||
| * allocated. | ||
| */ | ||
| static guac_display_layer* guac_rdp_rail_get_window_layer( |
There was a problem hiding this comment.
Remote App window ordering was broken and fixing that causes mouse layer to be rendered under the app windows. Attempt to fix that with negative zIndexes was not allowed/wrong approach.
So instead, we render all rail windows in the same container (in correct order), that stays under the cursor layer.
| * 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.
| * @return | ||
| * TRUE if the desktop order was handled successfully, otherwise FALSE. | ||
| */ | ||
| static BOOL guac_rdp_rail_monitored_desktop(rdpContext* context, |
There was a problem hiding this comment.
The window ordering was broken earlier, now we obey server sent z-order for the windows. We also set active window according to server orders.
Guacamole side does not do any window manager stuff, we just do what server says.
|
|
||
| /* 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.
…, opacity/alpha handling, z-order, and input responsiveness by rendering windows inside a shared RAIL container. Follow server z-order only.
jjheinon
force-pushed
the
guacamole-2123-remote-app-windows-gfx
branch
from
061ba6f to
b8aa4f4
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.