From 7b829b46c12297b73b689c00ae22471d7f1ddd2b Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Fri, 27 Sep 2024 18:20:04 -0700 Subject: [PATCH] GUACAMOLE-377: Migrate RDP to default render thread. --- src/protocols/rdp/client.h | 29 +++++------------ src/protocols/rdp/gdi.c | 12 +++---- src/protocols/rdp/pointer.c | 6 ++-- src/protocols/rdp/rdp.c | 61 +++++++++--------------------------- src/protocols/rdp/rdp.h | 13 ++------ src/protocols/rdp/settings.c | 4 +++ 6 files changed, 35 insertions(+), 90 deletions(-) diff --git a/src/protocols/rdp/client.h b/src/protocols/rdp/client.h index 9b307878b..7955db364 100644 --- a/src/protocols/rdp/client.h +++ b/src/protocols/rdp/client.h @@ -23,29 +23,14 @@ #include /** - * The maximum duration of a frame in milliseconds. This ensures we at least - * meet a reasonable minimum framerate in the case that the RDP server provides - * no frame boundaries and streams data continuously enough that frame - * boundaries are not discernable through timing. + * The amount of time to wait for new messages from the RDP server before + * moving on to internal matters, in milliseconds. This value must be kept + * reasonably small such that a slow RDP server will not prevent external + * events from being handled (such as the stop signal from guac_client_stop()), + * but large enough that the message handling loop does not eat up CPU + * spinning. */ -#define GUAC_RDP_MAX_FRAME_DURATION 33 - -/** - * The minimum duration of a frame in milliseconds. This ensures we don't start - * flushing a ton of tiny frames if an RDP server provides no frame boundaries - * and streams data inconsistently enough that timing would suggest frame - * boundaries in the middle of a frame. - */ -#define GUAC_RDP_MIN_FRAME_DURATION 10 - -/** - * The amount of time to wait for a new message from the RDP server when - * beginning a new frame, in milliseconds. This value must be kept reasonably - * small such that a slow RDP server will not prevent external events from - * being handled (such as the stop signal from guac_client_stop()), but large - * enough that the message handling loop does not eat up CPU spinning. - */ -#define GUAC_RDP_FRAME_START_TIMEOUT 250 +#define GUAC_RDP_MESSAGE_CHECK_INTERVAL 1000 /** * The native resolution of most RDP connections. As Windows and other systems diff --git a/src/protocols/rdp/gdi.c b/src/protocols/rdp/gdi.c index 8a9869595..69373faf9 100644 --- a/src/protocols/rdp/gdi.c +++ b/src/protocols/rdp/gdi.c @@ -39,15 +39,9 @@ void guac_rdp_gdi_mark_frame(rdpContext* context, int starting) { guac_client* client = ((rdp_freerdp_context*) context)->client; guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; - /* A new frame is beginning */ - if (starting) { - rdp_client->in_frame = 1; - return; - } - /* A new frame has been received from the RDP server and processed */ - rdp_client->in_frame = 0; - rdp_client->frames_received++; + if (!starting) + guac_display_render_thread_notify_frame(rdp_client->render_thread); } @@ -133,6 +127,8 @@ BOOL guac_rdp_gdi_end_paint(rdpContext* context) { guac_rect_constrain(&dst_rect, ¤t_context->bounds); guac_rect_extend(¤t_context->dirty, &dst_rect); + guac_display_render_thread_notify_modified(rdp_client->render_thread); + paint_complete: /* There will be no further drawing operations */ diff --git a/src/protocols/rdp/pointer.c b/src/protocols/rdp/pointer.c index 1a4f3f807..7387b46b8 100644 --- a/src/protocols/rdp/pointer.c +++ b/src/protocols/rdp/pointer.c @@ -103,7 +103,7 @@ BOOL guac_rdp_pointer_set(rdpContext* context, POINTER_SET_CONST rdpPointer* poi guac_display_layer_close_raw(cursor_layer, dst_context); guac_display_layer_close_raw(src_layer, src_context); - guac_display_end_mouse_frame(rdp_client->display); + guac_display_render_thread_notify_modified(rdp_client->render_thread); return TRUE; } @@ -128,7 +128,7 @@ BOOL guac_rdp_pointer_set_null(rdpContext* context) { /* Set cursor to empty/blank graphic */ guac_display_set_cursor(rdp_client->display, GUAC_DISPLAY_CURSOR_NONE); - guac_display_end_mouse_frame(rdp_client->display); + guac_display_render_thread_notify_modified(rdp_client->render_thread); return TRUE; } @@ -141,6 +141,6 @@ BOOL guac_rdp_pointer_set_default(rdpContext* context) { /* Set cursor to embedded pointer */ guac_display_set_cursor(rdp_client->display, GUAC_DISPLAY_CURSOR_POINTER); - guac_display_end_mouse_frame(rdp_client->display); + guac_display_render_thread_notify_modified(rdp_client->render_thread); return TRUE; } diff --git a/src/protocols/rdp/rdp.c b/src/protocols/rdp/rdp.c index 74f2d266b..fc74e65d2 100644 --- a/src/protocols/rdp/rdp.c +++ b/src/protocols/rdp/rdp.c @@ -597,6 +597,8 @@ static int guac_rdp_handle_connection(guac_client* client) { guac_rwlock_release_lock(&(rdp_client->lock)); + rdp_client->render_thread = guac_display_render_thread_create(rdp_client->display); + /* Handle messages from RDP server while client is running */ while (client->state == GUAC_CLIENT_RUNNING && !guac_rdp_disp_reconnect_needed(rdp_client->disp)) { @@ -605,48 +607,16 @@ static int guac_rdp_handle_connection(guac_client* client) { guac_rdp_disp_update_size(rdp_client->disp, settings, rdp_inst); /* Wait for data and construct a reasonable frame */ - int wait_result = rdp_guac_client_wait_for_messages(client, GUAC_RDP_FRAME_START_TIMEOUT); - if (wait_result > 0) { - - /* Read server messages until frame is built */ - guac_timestamp frame_start = guac_timestamp_current(); - int frames_at_start = rdp_client->frames_received; - do { - - /* Handle any queued FreeRDP events (this may result in RDP - * messages being sent), aborting if FreeRDP event handling - * fails */ - if (!guac_rdp_handle_events(rdp_client)) { - wait_result = -1; - break; - } - - int frame_duration = guac_timestamp_current() - frame_start; - - if (!rdp_client->in_frame) { - - /* Flush frame if at least one frame has been produced */ - if (rdp_client->frames_received > frames_at_start) - break; - - /* Continue processing messages for up to a reasonable - * minimum framerate without an explicit frame boundary - * indicating that the frame is not yet complete */ - if (frame_duration > GUAC_RDP_MAX_FRAME_DURATION) - break; - - } - /* Do not exceed a reasonable maximum framerate without an - * explicit frame boundary terminating the frame early */ - int allowed_wait = GUAC_RDP_MIN_FRAME_DURATION - frame_duration; - if (allowed_wait < 0) - allowed_wait = 0; - - wait_result = rdp_guac_client_wait_for_messages(client, allowed_wait); - - } while (wait_result > 0); + int wait_result = rdp_guac_client_wait_for_messages(client, GUAC_RDP_MESSAGE_CHECK_INTERVAL); + if (wait_result < 0) + break; + /* Handle any queued FreeRDP events (this may result in RDP messages + * being sent), aborting if FreeRDP event handling fails */ + if (!guac_rdp_handle_events(rdp_client)) { + wait_result = -1; + break; } /* Test whether the RDP server is closing the connection */ @@ -666,13 +636,6 @@ static int guac_rdp_handle_connection(guac_client* client) { guac_client_abort(client, GUAC_PROTOCOL_STATUS_UPSTREAM_UNAVAILABLE, "Connection closed."); - /* Flush frame only if successful and an RDP frame is not known to be - * in progress */ - else if (!rdp_client->in_frame) { - guac_display_end_multiple_frames(rdp_client->display, rdp_client->frames_received); - rdp_client->frames_received = 0; - } - } guac_rwlock_acquire_write_lock(&(rdp_client->lock)); @@ -688,6 +651,10 @@ static int guac_rdp_handle_connection(guac_client* client) { freerdp_disconnect(rdp_inst); pthread_mutex_unlock(&(rdp_client->message_lock)); + /* Stop render loop */ + guac_display_render_thread_destroy(rdp_client->render_thread); + rdp_client->render_thread = NULL; + /* Remove reference to FreeRDP's GDI buffer so that it can be safely freed * prior to freeing the guac_display */ guac_display_layer_raw_context* context = guac_display_layer_open_raw(default_layer); diff --git a/src/protocols/rdp/rdp.h b/src/protocols/rdp/rdp.h index 5d723c30a..76ca18ca3 100644 --- a/src/protocols/rdp/rdp.h +++ b/src/protocols/rdp/rdp.h @@ -115,17 +115,10 @@ typedef struct guac_rdp_client { guac_display_layer_raw_context* current_context; /** - * Whether the RDP server has reported that a new frame is in progress, and - * we are now receiving updates relevant to that frame. + * The current instance of the guac_display render thread. If the thread + * has not yet been started, this will be NULL. */ - int in_frame; - - /** - * The number of distinct frames received from the RDP server since last - * flush, if the RDP server supports reporting frame boundaries. If the RDP - * server does not support tracking frames, this will be zero. - */ - int frames_received; + guac_display_render_thread* render_thread; /** * The current state of the keyboard with respect to the RDP session. diff --git a/src/protocols/rdp/settings.c b/src/protocols/rdp/settings.c index 47320dfcc..f47b1cff8 100644 --- a/src/protocols/rdp/settings.c +++ b/src/protocols/rdp/settings.c @@ -1774,6 +1774,10 @@ void guac_rdp_push_settings(guac_client* client, rdp_settings->FrameMarkerCommandEnabled = TRUE; rdp_settings->SurfaceFrameMarkerEnabled = TRUE; + /* Always handle input events asynchronously (rather than synchronously + * with the rest of FreeRDP's event loop, including graphics) */ + rdp_settings->AsyncInput = TRUE; + /* Enable RemoteFX / Graphics Pipeline */ if (guac_settings->enable_gfx) {