From 0cce3ceb55a30f8e2e11e3aa0f28636f659ef404 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Thu, 1 May 2025 15:31:43 -0700 Subject: [PATCH] GUACAMOLE-377: Remove use of END_FRAME operation from guac_display. The END_FRAME operation was previously used to notify workers that the frame has ended, but since the receiving worker needs to check and push that operation back onto the queue if other workers are still busy, it's essentially unnecessary _and_ results in several workers spinning as they pass END_FRAME around until all others are done. It's sufficient to simply check whether the operation queue is empty and no other workers are active. --- src/libguac/display-flush.c | 7 +- src/libguac/display-plan.c | 8 +- src/libguac/display-plan.h | 7 +- src/libguac/display-worker.c | 229 +++++++++++++++++------------------ 4 files changed, 114 insertions(+), 137 deletions(-) diff --git a/src/libguac/display-flush.c b/src/libguac/display-flush.c index 4816576f..7968bf70 100644 --- a/src/libguac/display-flush.c +++ b/src/libguac/display-flush.c @@ -379,11 +379,12 @@ void guac_display_end_multiple_frames(guac_display* display, int frames) { guac_rwlock_release_lock(&display->last_frame.lock); /* Not all frames are graphical. If we end up with a frame containing - * nothing but layer property changes, then we must still send a frame - * boundary even though there is no display plan to optimize. */ + * nothing but layer property changes, then we must still send at least one + * operation to awaken the workers and flush layer changes, even though + * there is no display plan to optimize. */ if (plan == NULL && frame_nonempty) { guac_display_plan_operation end_frame_op = { - .type = GUAC_DISPLAY_PLAN_END_FRAME + .type = GUAC_DISPLAY_PLAN_OPERATION_NOP }; guac_fifo_enqueue(&display->ops, &end_frame_op); } diff --git a/src/libguac/display-plan.c b/src/libguac/display-plan.c index 212d9346..6cd726bb 100644 --- a/src/libguac/display-plan.c +++ b/src/libguac/display-plan.c @@ -305,7 +305,7 @@ guac_display_plan* PFW_LFR_guac_display_plan_create(guac_display* display) { guac_display_plan* plan = guac_mem_alloc(sizeof(guac_display_plan)); plan->display = display; plan->frame_end = frame_end; - plan->length = guac_mem_ckd_add_or_die(op_count, 1); + plan->length = op_count; plan->ops = guac_mem_alloc(plan->length, sizeof(guac_display_plan_operation)); /* Convert the dirty rectangles stored in each layer's cells to individual @@ -359,12 +359,6 @@ guac_display_plan* PFW_LFR_guac_display_plan_create(guac_display* display) { * predicted quantity */ GUAC_ASSERT(added_ops == op_count); - /* Worker threads must be aware of end-of-frame to know when to send sync, - * etc. Noticing that the operation queue is empty is insufficient, as the - * queue may become empty while a frame is in progress if the worker - * threads happen to be processing things quickly. */ - current_op->type = GUAC_DISPLAY_PLAN_END_FRAME; - return plan; } diff --git a/src/libguac/display-plan.h b/src/libguac/display-plan.h index d1a520b7..d39d3958 100644 --- a/src/libguac/display-plan.h +++ b/src/libguac/display-plan.h @@ -141,12 +141,7 @@ typedef enum guac_display_plan_operation_type { /** * Draw arbitrary image data to the destination rect. */ - GUAC_DISPLAY_PLAN_OPERATION_IMG, - - /** - * Finish the frame, sending the frame boundary to all connected users. - */ - GUAC_DISPLAY_PLAN_END_FRAME + GUAC_DISPLAY_PLAN_OPERATION_IMG } guac_display_plan_operation_type; diff --git a/src/libguac/display-worker.c b/src/libguac/display-worker.c index a9bfb3be..6c785d88 100644 --- a/src/libguac/display-worker.c +++ b/src/libguac/display-worker.c @@ -368,142 +368,129 @@ void* guac_display_worker_thread(void* data) { case GUAC_DISPLAY_PLAN_OPERATION_COPY: case GUAC_DISPLAY_PLAN_OPERATION_RECT: - case GUAC_DISPLAY_PLAN_OPERATION_NOP: guac_client_log(client, GUAC_LOG_DEBUG, "Operation type %i " "should NOT be present in the set of operations given " "to guac_display worker thread. All operations except " - "IMG and END_FRAME are handled during the initial, " + "IMG and NOP are handled during the initial, " "single-threaded flush step. This is likely a bug.", op.type); break; - case GUAC_DISPLAY_PLAN_END_FRAME: - - guac_fifo_lock(&display->ops); - int other_workers_busy = (display->active_workers > 1); - guac_fifo_unlock(&display->ops); - - /* If other workers are still busy, push the frame boundary - * back on the queue so that it's picked up by one of those - * workers */ - if (other_workers_busy) { - guac_fifo_enqueue(&display->ops, &op); - } - - /* Otherwise, we've reached the end of the frame, and this is - * the worker that will be sending that boundary to connected - * users */ - else { - - /* Update the mouse cursor if it's been changed since the - * last frame */ - guac_display_layer* cursor = display->cursor_buffer; - if (!guac_rect_is_empty(&cursor->last_frame.dirty)) { - guac_protocol_send_cursor(client->socket, - display->last_frame.cursor_hotspot_x, - display->last_frame.cursor_hotspot_y, - cursor->layer, 0, 0, - cursor->last_frame.width, - cursor->last_frame.height); - } - - /* Use the amount of time that the client has been waiting - * for a frame vs. the amount of time that it took the - * client to process the most recently acknowledged frame - * to calculate the amount of additional delay required to - * allow the client to catch up. This value is used later, - * after everything else related to the frame has been - * finalized. */ - int time_since_last_frame = guac_timestamp_current() - client->last_sent_timestamp; - int processing_lag = guac_client_get_processing_lag(client); - int required_wait = processing_lag - time_since_last_frame; - - /* Allow connected clients to move forward with rendering */ - guac_client_end_multiple_frames(client, display->last_frame.frames); - - /* While connected clients moves forward with rendering, - * commit any changed contents to client-side backing buffer */ - guac_display_layer* current = display->last_frame.layers; - while (current != NULL) { - - /* Save a copy of the changed region if the layer has - * been modified since the last frame */ - guac_rect* dirty = ¤t->last_frame.dirty; - if (!guac_rect_is_empty(dirty)) { - - int x = dirty->left; - int y = dirty->top; - int width = guac_rect_width(dirty); - int height = guac_rect_height(dirty); - - /* Ensure destination region is cleared out first if the alpha channel need be considered, - * as GUAC_COMP_OVER is significantly faster than GUAC_COMP_SRC on the browser side */ - if (!current->opaque) { - guac_protocol_send_rect(client->socket, current->last_frame_buffer, x, y, width, height); - guac_protocol_send_cfill(client->socket, GUAC_COMP_RATOP, current->last_frame_buffer, - 0x00, 0x00, 0x00, 0x00); - } - - guac_protocol_send_copy(client->socket, - current->layer, x, y, width, height, - GUAC_COMP_OVER, current->last_frame_buffer, x, y); - - } - - current = current->last_frame.next; - - } - - /* This is now absolutely everything for the current frame, - * and it's safe to flush any outstanding data */ - guac_socket_flush(client->socket); - - /* Notify any watchers of render_state that a frame is no longer in progress */ - guac_flag_set_and_lock(&display->render_state, GUAC_DISPLAY_RENDER_STATE_FRAME_NOT_IN_PROGRESS); - guac_flag_clear(&display->render_state, GUAC_DISPLAY_RENDER_STATE_FRAME_IN_PROGRESS); - guac_flag_unlock(&display->render_state); - - /* Exclude local, server-side frame processing latency from - * waiting period */ - int latency = (int) (guac_timestamp_current() - display->last_frame.timestamp); - if (latency >= 0) { - guac_client_log(display->client, GUAC_LOG_TRACE, - "Rendering latency: %ims (%i:1 frame)\n", - latency, display->last_frame.frames); - required_wait -= latency; - } - - /* Ensure we don't wait without bound when compensating for - * client-side processing delays */ - if (required_wait > GUAC_DISPLAY_MAX_LAG_COMPENSATION) - required_wait = GUAC_DISPLAY_MAX_LAG_COMPENSATION; - - /* Allow connected clients to catch up if they're taking - * longer to process frames than the server is taking to - * generate them */ - if (required_wait > 0) { - guac_client_log(display->client, GUAC_LOG_TRACE, - "Waiting %ims to compensate for client-side " - "processing delays.\n", required_wait); - guac_timestamp_msleep(required_wait); - } - - guac_fifo_lock(&display->ops); - has_outstanding_frames = display->frame_deferred; - guac_fifo_unlock(&display->ops); - - } - + case GUAC_DISPLAY_PLAN_OPERATION_NOP: + /* Do nothing */ break; } - guac_rwlock_release_lock(&display->last_frame.lock); - guac_fifo_lock(&display->ops); + + /* If we're the only active worker and there are no further operations + * pending, we've reached the end of the frame, and this is the worker + * that will be sending that boundary to connected users */ + if (!(display->ops.state.value & GUAC_FIFO_STATE_NONEMPTY) && display->active_workers == 1) { + + /* Update the mouse cursor if it's been changed since the + * last frame */ + guac_display_layer* cursor = display->cursor_buffer; + if (!guac_rect_is_empty(&cursor->last_frame.dirty)) { + guac_protocol_send_cursor(client->socket, + display->last_frame.cursor_hotspot_x, + display->last_frame.cursor_hotspot_y, + cursor->layer, 0, 0, + cursor->last_frame.width, + cursor->last_frame.height); + } + + /* Use the amount of time that the client has been waiting + * for a frame vs. the amount of time that it took the + * client to process the most recently acknowledged frame + * to calculate the amount of additional delay required to + * allow the client to catch up. This value is used later, + * after everything else related to the frame has been + * finalized. */ + int time_since_last_frame = guac_timestamp_current() - client->last_sent_timestamp; + int processing_lag = guac_client_get_processing_lag(client); + int required_wait = processing_lag - time_since_last_frame; + + /* Allow connected clients to move forward with rendering */ + guac_client_end_multiple_frames(client, display->last_frame.frames); + + /* While connected clients moves forward with rendering, + * commit any changed contents to client-side backing buffer */ + guac_display_layer* current = display->last_frame.layers; + while (current != NULL) { + + /* Save a copy of the changed region if the layer has + * been modified since the last frame */ + guac_rect* dirty = ¤t->last_frame.dirty; + if (!guac_rect_is_empty(dirty)) { + + int x = dirty->left; + int y = dirty->top; + int width = guac_rect_width(dirty); + int height = guac_rect_height(dirty); + + /* Ensure destination region is cleared out first if the alpha channel need be considered, + * as GUAC_COMP_OVER is significantly faster than GUAC_COMP_SRC on the browser side */ + if (!current->opaque) { + guac_protocol_send_rect(client->socket, current->last_frame_buffer, x, y, width, height); + guac_protocol_send_cfill(client->socket, GUAC_COMP_RATOP, current->last_frame_buffer, + 0x00, 0x00, 0x00, 0x00); + } + + guac_protocol_send_copy(client->socket, + current->layer, x, y, width, height, + GUAC_COMP_OVER, current->last_frame_buffer, x, y); + + } + + current = current->last_frame.next; + + } + + /* This is now absolutely everything for the current frame, + * and it's safe to flush any outstanding data */ + guac_socket_flush(client->socket); + + /* Notify any watchers of render_state that a frame is no longer in progress */ + guac_flag_set_and_lock(&display->render_state, GUAC_DISPLAY_RENDER_STATE_FRAME_NOT_IN_PROGRESS); + guac_flag_clear(&display->render_state, GUAC_DISPLAY_RENDER_STATE_FRAME_IN_PROGRESS); + guac_flag_unlock(&display->render_state); + + /* Exclude local, server-side frame processing latency from + * waiting period */ + int latency = (int) (guac_timestamp_current() - display->last_frame.timestamp); + if (latency >= 0) { + guac_client_log(display->client, GUAC_LOG_TRACE, + "Rendering latency: %ims (%i:1 frame)\n", + latency, display->last_frame.frames); + required_wait -= latency; + } + + /* Ensure we don't wait without bound when compensating for + * client-side processing delays */ + if (required_wait > GUAC_DISPLAY_MAX_LAG_COMPENSATION) + required_wait = GUAC_DISPLAY_MAX_LAG_COMPENSATION; + + /* Allow connected clients to catch up if they're taking + * longer to process frames than the server is taking to + * generate them */ + if (required_wait > 0) { + guac_client_log(display->client, GUAC_LOG_TRACE, + "Waiting %ims to compensate for client-side " + "processing delays.\n", required_wait); + guac_timestamp_msleep(required_wait); + } + + has_outstanding_frames = display->frame_deferred; + + } + display->active_workers--; guac_fifo_unlock(&display->ops); + guac_rwlock_release_lock(&display->last_frame.lock); + /* Trigger additional flush if frames were completed while we were * still processing the previous frame */ if (has_outstanding_frames) {