From 11eaa378304d7f2ae6accbca74c7bbbd21590a7c Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Tue, 6 May 2025 14:35:22 -0700 Subject: [PATCH] GUACAMOLE-2061: Report mouse movement as part of frame via render thread. Doing this decouples reporting of mouse movement from the display locks and display processing, ensuring that any delays from processing the pending frame do not cause delays in processing further instructions (like "sync"). If delays in processing "sync" are tied to server-side processing, the assumptions behind the client processing lag calculations cause a feedback loop of further delays. Measurable delays in "sync" need to be purely due to client-side processing. --- src/libguac/display-priv.h | 43 +++++++++++++++++++++++++++++ src/libguac/display-render-thread.c | 29 +++++++++++++++++++ src/libguac/guacamole/display.h | 43 +++++++++++++++++++++++++++++ src/protocols/rdp/input.c | 2 +- src/protocols/vnc/input.c | 2 +- 5 files changed, 117 insertions(+), 2 deletions(-) diff --git a/src/libguac/display-priv.h b/src/libguac/display-priv.h index d66020fa..67eb08cd 100644 --- a/src/libguac/display-priv.h +++ b/src/libguac/display-priv.h @@ -237,6 +237,40 @@ */ #define GUAC_DISPLAY_RENDER_THREAD_STATE_FRAME_READY 4 +/** + * The state of the mouse cursor, as independently tracked by the render + * thread. The mouse cursor state may be reported by + * guac_display_render_thread_notify_user_moved_mouse() to avoid unnecessarily + * locking the display within instruction handlers (which can otherwise result + * in delays in handling critical instructions like "sync"). + */ +typedef struct guac_display_render_thread_cursor_state { + + /** + * The user that moved or clicked the mouse. + * + * NOTE: This user is NOT guaranteed to still exist in memory. This may be + * a dangling pointer and must be validated before deferencing. + */ + guac_user* user; + + /** + * The X coordinate of the mouse cursor. + */ + int x; + + /** + * The Y coordinate of the mouse cursor. + */ + int y; + + /** + * The mask representing the states of all mouse buttons. + */ + int mask; + +} guac_display_render_thread_cursor_state; + struct guac_display_render_thread { /** @@ -260,6 +294,12 @@ struct guac_display_render_thread { */ guac_flag state; + /** + * The current mouse cursor state, as reported by + * guac_display_render_thread_notify_user_moved_mouse(). + */ + guac_display_render_thread_cursor_state cursor_state; + /** * The number of frames that have been explicitly marked as ready since the * last frame sent. This will be zero if explicit frame boundaries are not @@ -617,6 +657,9 @@ typedef struct guac_display_state { * The user that moved or clicked the mouse. This is used to ensure we * don't attempt to synchronize an out-of-date mouse position to the user * that is actively moving the mouse. + * + * NOTE: This user is NOT guaranteed to still exist in memory. This may be + * a dangling pointer and must be validated before deferencing. */ guac_user* cursor_user; diff --git a/src/libguac/display-render-thread.c b/src/libguac/display-render-thread.c index ac4a3889..4cfcc602 100644 --- a/src/libguac/display-render-thread.c +++ b/src/libguac/display-render-thread.c @@ -65,6 +65,8 @@ static void* guac_display_render_loop(void* data) { for (;;) { + guac_display_render_thread_cursor_state cursor_state = render_thread->cursor_state; + /* Wait indefinitely for any change to the frame state */ guac_flag_wait_and_lock(&render_thread->state, GUAC_DISPLAY_RENDER_THREAD_STATE_STOPPING @@ -109,6 +111,12 @@ static void* guac_display_render_loop(void* data) { } + /* Copy cursor state for later flushing with final frame, + * regardless of whether it's changed (there's really no need to + * compare here - that will be done by the actual guac_display + * frame flush) */ + cursor_state = render_thread->cursor_state; + /* Do not exceed a reasonable maximum framerate without an * explicit frame boundary terminating the frame early */ allowed_wait = GUAC_DISPLAY_RENDER_THREAD_MIN_FRAME_DURATION - frame_duration; @@ -125,6 +133,14 @@ static void* guac_display_render_loop(void* data) { | GUAC_DISPLAY_RENDER_THREAD_STATE_FRAME_READY | GUAC_DISPLAY_RENDER_THREAD_STATE_FRAME_MODIFIED, allowed_wait)); + /* Pass on cursor state for consumption by guac_display frame flush */ + guac_rwlock_acquire_write_lock(&display->pending_frame.lock); + display->pending_frame.cursor_user = cursor_state.user; + display->pending_frame.cursor_x = cursor_state.x; + display->pending_frame.cursor_y = cursor_state.y; + display->pending_frame.cursor_mask = cursor_state.mask; + guac_rwlock_release_lock(&display->pending_frame.lock); + guac_display_end_multiple_frames(display, rendered_frames); } @@ -140,6 +156,7 @@ guac_display_render_thread* guac_display_render_thread_create(guac_display* disp guac_flag_init(&render_thread->state); render_thread->display = display; render_thread->frames = 0; + render_thread->cursor_state = (guac_display_render_thread_cursor_state) { 0 }; /* Start render thread (this will immediately begin blocking until frame * modification or readiness is signalled) */ @@ -159,6 +176,18 @@ void guac_display_render_thread_notify_frame(guac_display_render_thread* render_ guac_flag_unlock(&render_thread->state); } +void guac_display_render_thread_notify_user_moved_mouse(guac_display_render_thread* render_thread, + guac_user* user, int x, int y, int mask) { + + guac_flag_set_and_lock(&render_thread->state, GUAC_DISPLAY_RENDER_THREAD_STATE_FRAME_MODIFIED); + render_thread->cursor_state.user = user; + render_thread->cursor_state.x = x; + render_thread->cursor_state.y = y; + render_thread->cursor_state.mask = mask; + guac_flag_unlock(&render_thread->state); + +} + void guac_display_render_thread_destroy(guac_display_render_thread* render_thread) { /* Clean up render thread after signalling it to stop */ diff --git a/src/libguac/guacamole/display.h b/src/libguac/guacamole/display.h index 84c4c908..baf768d4 100644 --- a/src/libguac/guacamole/display.h +++ b/src/libguac/guacamole/display.h @@ -254,6 +254,13 @@ void guac_display_notify_user_left(guac_display* display, guac_user* user); * mouse button. This function automatically invokes * guac_display_end_mouse_frame(). * + * NOTE: If using guac_display_render_thread, the + * guac_display_render_thread_notify_user_moved_mouse() function should be used + * instead. If NOT using guac_display_render_thread, care should be taken in + * calling this function directly within a mouse_handler, as doing so + * inherently must lock the guac_display, which can cause delays in handling + * other received instructions like "sync". + * * @param display * The guac_display to notify. * @@ -762,6 +769,42 @@ void guac_display_render_thread_notify_modified(guac_display_render_thread* rend */ void guac_display_render_thread_notify_frame(guac_display_render_thread* render_thread); +/** + * Notifies the given render thread that a specific user has changed the state + * of the mouse, such as through moving the pointer or pressing/releasing a + * mouse button. + * + * When using guac_display_render_thread, this function should be preferred to + * manually invoking guac_display_notify_user_moved_mouse(). + * + * @param render_thread + * The guac_display_render_thread to notify. + * + * @param user + * The user that moved the mouse or pressed/released a mouse button. + * + * @param x + * The X position of the mouse, in pixels. + * + * @param y + * The Y position of the mouse, in pixels. + * + * @param mask + * An integer value representing the current state of each button, where + * the Nth bit within the integer is set to 1 if and only if the Nth mouse + * button is currently pressed. The lowest-order bit is the left mouse + * button, followed by the middle button, right button, and finally the up + * and down buttons of the scroll wheel. + * + * @see GUAC_CLIENT_MOUSE_LEFT + * @see GUAC_CLIENT_MOUSE_MIDDLE + * @see GUAC_CLIENT_MOUSE_RIGHT + * @see GUAC_CLIENT_MOUSE_SCROLL_UP + * @see GUAC_CLIENT_MOUSE_SCROLL_DOWN + */ +void guac_display_render_thread_notify_user_moved_mouse(guac_display_render_thread* render_thread, + guac_user* user, int x, int y, int mask); + /** * Safely stops and frees all resources associated with the given render * thread. The provided pointer to the render thread is no longer valid after a diff --git a/src/protocols/rdp/input.c b/src/protocols/rdp/input.c index d8890571..d74a9c79 100644 --- a/src/protocols/rdp/input.c +++ b/src/protocols/rdp/input.c @@ -48,7 +48,7 @@ int guac_rdp_user_mouse_handler(guac_user* user, int x, int y, int mask) { goto complete; /* Store current mouse location/state */ - guac_display_notify_user_moved_mouse(rdp_client->display, user, x, y, mask); + guac_display_render_thread_notify_user_moved_mouse(rdp_client->render_thread, user, x, y, mask); /* Report mouse position within recording */ if (rdp_client->recording != NULL) diff --git a/src/protocols/vnc/input.c b/src/protocols/vnc/input.c index bfb2546d..5c4700b7 100644 --- a/src/protocols/vnc/input.c +++ b/src/protocols/vnc/input.c @@ -34,7 +34,7 @@ int guac_vnc_user_mouse_handler(guac_user* user, int x, int y, int mask) { rfbClient* rfb_client = vnc_client->rfb_client; /* Store current mouse location/state */ - guac_display_notify_user_moved_mouse(vnc_client->display, user, x, y, mask); + guac_display_render_thread_notify_user_moved_mouse(vnc_client->render_thread, user, x, y, mask); /* Report mouse position within recording */ if (vnc_client->recording != NULL)