From b7e0e080da3846be55fe99c2ffb96639463c7b49 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Fri, 23 Sep 2016 13:47:09 -0700 Subject: [PATCH 1/2] GUACAMOLE-180: Make common surface threadsafe. --- src/common/common/surface.h | 29 ++---- src/common/surface.c | 188 +++++++++++++++++++++++++++++------- 2 files changed, 163 insertions(+), 54 deletions(-) diff --git a/src/common/common/surface.h b/src/common/common/surface.h index edc7af8e..f92c0685 100644 --- a/src/common/common/surface.h +++ b/src/common/common/surface.h @@ -29,6 +29,8 @@ #include #include +#include + /** * The maximum number of updates to allow within the bitmap queue. */ @@ -226,6 +228,13 @@ typedef struct guac_common_surface { */ guac_common_surface_heat_cell* heat_map; + /** + * Mutex which is locked internally when access to the surface must be + * synchronized. All public functions of guac_common_surface should be + * considered threadsafe. + */ + pthread_mutex_t _lock; + } guac_common_surface; /** @@ -418,16 +427,6 @@ void guac_common_surface_set_parent(guac_common_surface* surface, */ void guac_common_surface_set_opacity(guac_common_surface* surface, int opacity); -/** - * Flushes only the properties of the given surface, such as layer location or - * opacity. Image state is not flushed. If the surface represents a buffer or - * the default layer, this function has no effect. - * - * @param surface - * The surface to flush. - */ -void guac_common_surface_flush_properties(guac_common_surface* surface); - /** * Flushes the given surface, including any applicable properties, drawing any * pending operations on the remote display. @@ -437,16 +436,6 @@ void guac_common_surface_flush_properties(guac_common_surface* surface); */ void guac_common_surface_flush(guac_common_surface* surface); -/** - * Schedules a deferred flush of the given surface. This will not immediately - * flush the surface to the client. Instead, the result of the flush is - * added to a queue which is reinspected and combined (if possible) with other - * deferred flushes during the call to guac_common_surface_flush(). - * - * @param surface The surface to flush. - */ -void guac_common_surface_flush_deferred(guac_common_surface* surface); - /** * Duplicates the contents of the current surface to the given socket. Pending * changes are not flushed. diff --git a/src/common/surface.c b/src/common/surface.c index 318e7d63..99e91c06 100644 --- a/src/common/surface.c +++ b/src/common/surface.c @@ -29,6 +29,7 @@ #include #include +#include #include #include #include @@ -117,26 +118,50 @@ #define GUAC_SURFACE_WEBP_BLOCK_SIZE 8 void guac_common_surface_move(guac_common_surface* surface, int x, int y) { + + pthread_mutex_lock(&surface->_lock); + surface->x = x; surface->y = y; surface->location_dirty = 1; + + pthread_mutex_unlock(&surface->_lock); + } void guac_common_surface_stack(guac_common_surface* surface, int z) { + + pthread_mutex_lock(&surface->_lock); + surface->z = z; surface->location_dirty = 1; + + pthread_mutex_unlock(&surface->_lock); + } void guac_common_surface_set_parent(guac_common_surface* surface, const guac_layer* parent) { + + pthread_mutex_lock(&surface->_lock); + surface->parent = parent; surface->location_dirty = 1; + + pthread_mutex_unlock(&surface->_lock); + } void guac_common_surface_set_opacity(guac_common_surface* surface, int opacity) { + + pthread_mutex_lock(&surface->_lock); + surface->opacity = opacity; surface->opacity_dirty = 1; + + pthread_mutex_unlock(&surface->_lock); + } /** @@ -586,16 +611,34 @@ static void __guac_common_surface_flush_to_queue(guac_common_surface* surface) { } -void guac_common_surface_flush_deferred(guac_common_surface* surface) { +/** + * Flushes the given surface, drawing any pending operations on the remote + * display. Surface properties are not flushed. + * + * @param surface + * The surface to flush. + */ +static void __guac_common_surface_flush(guac_common_surface* surface); + +/** + * Schedules a deferred flush of the given surface. This will not immediately + * flush the surface to the client. Instead, the result of the flush is + * added to a queue which is reinspected and combined (if possible) with other + * deferred flushes during the call to guac_common_surface_flush(). + * + * @param surface The surface to flush. + */ +static void __guac_common_surface_flush_deferred(guac_common_surface* surface) { /* Do not flush if not dirty */ if (!surface->dirty) return; - /* Flush if queue size has reached maximum (space is reserved for the final dirty rect, - * as guac_common_surface_flush() MAY add an additional rect to the queue */ + /* Flush if queue size has reached maximum (space is reserved for the final + * dirty rect, as __guac_common_surface_flush() MAY add an additional rect + * to the queue */ if (surface->bitmap_queue_length == GUAC_COMMON_SURFACE_QUEUE_SIZE-1) - guac_common_surface_flush(surface); + __guac_common_surface_flush(surface); /* Append dirty rect to queue */ __guac_common_surface_flush_to_queue(surface); @@ -1017,6 +1060,8 @@ guac_common_surface* guac_common_surface_alloc(guac_client* client, surface->width = w; surface->height = h; + pthread_mutex_init(&surface->_lock, NULL); + /* Create corresponding Cairo surface */ surface->stride = cairo_format_stride_for_width(CAIRO_FORMAT_RGB24, w); surface->buffer = calloc(h, surface->stride); @@ -1047,6 +1092,8 @@ void guac_common_surface_free(guac_common_surface* surface) { if (surface->realized) guac_protocol_send_dispose(surface->socket, surface->layer); + pthread_mutex_destroy(&surface->_lock); + free(surface->heat_map); free(surface->buffer); free(surface); @@ -1055,6 +1102,8 @@ void guac_common_surface_free(guac_common_surface* surface) { void guac_common_surface_resize(guac_common_surface* surface, int w, int h) { + pthread_mutex_lock(&surface->_lock); + guac_socket* socket = surface->socket; const guac_layer* layer = surface->layer; @@ -1104,10 +1153,14 @@ void guac_common_surface_resize(guac_common_surface* surface, int w, int h) { if (surface->realized) guac_protocol_send_size(socket, layer, w, h); + pthread_mutex_unlock(&surface->_lock); + } void guac_common_surface_draw(guac_common_surface* surface, int x, int y, cairo_surface_t* src) { + pthread_mutex_lock(&surface->_lock); + unsigned char* buffer = cairo_image_surface_get_data(src); cairo_format_t format = cairo_image_surface_get_format(src); int stride = cairo_image_surface_get_stride(src); @@ -1123,12 +1176,12 @@ void guac_common_surface_draw(guac_common_surface* surface, int x, int y, cairo_ /* Clip operation */ __guac_common_clip_rect(surface, &rect, &sx, &sy); if (rect.width <= 0 || rect.height <= 0) - return; + goto complete; /* Update backing surface */ __guac_common_surface_put(buffer, stride, &sx, &sy, surface, &rect, format != CAIRO_FORMAT_ARGB32); if (rect.width <= 0 || rect.height <= 0) - return; + goto complete; /* Update the heat map for the update rectangle. */ guac_timestamp time = guac_timestamp_current(); @@ -1136,15 +1189,20 @@ void guac_common_surface_draw(guac_common_surface* surface, int x, int y, cairo_ /* Flush if not combining */ if (!__guac_common_should_combine(surface, &rect, 0)) - guac_common_surface_flush_deferred(surface); + __guac_common_surface_flush_deferred(surface); /* Always defer draws */ __guac_common_mark_dirty(surface, &rect); +complete: + pthread_mutex_unlock(&surface->_lock); + } -void guac_common_surface_paint(guac_common_surface* surface, int x, int y, cairo_surface_t* src, - int red, int green, int blue) { +void guac_common_surface_paint(guac_common_surface* surface, int x, int y, + cairo_surface_t* src, int red, int green, int blue) { + + pthread_mutex_lock(&surface->_lock); unsigned char* buffer = cairo_image_surface_get_data(src); int stride = cairo_image_surface_get_stride(src); @@ -1160,22 +1218,30 @@ void guac_common_surface_paint(guac_common_surface* surface, int x, int y, cairo /* Clip operation */ __guac_common_clip_rect(surface, &rect, &sx, &sy); if (rect.width <= 0 || rect.height <= 0) - return; + goto complete; /* Update backing surface */ __guac_common_surface_fill_mask(buffer, stride, sx, sy, surface, &rect, red, green, blue); /* Flush if not combining */ if (!__guac_common_should_combine(surface, &rect, 0)) - guac_common_surface_flush_deferred(surface); + __guac_common_surface_flush_deferred(surface); /* Always defer draws */ __guac_common_mark_dirty(surface, &rect); +complete: + pthread_mutex_unlock(&surface->_lock); + } -void guac_common_surface_copy(guac_common_surface* src, int sx, int sy, int w, int h, - guac_common_surface* dst, int dx, int dy) { +void guac_common_surface_copy(guac_common_surface* src, int sx, int sy, + int w, int h, guac_common_surface* dst, int dx, int dy) { + + /* Lock both surfaces */ + pthread_mutex_lock(&dst->_lock); + if (src != dst) + pthread_mutex_lock(&src->_lock); guac_socket* socket = dst->socket; const guac_layer* src_layer = src->layer; @@ -1187,13 +1253,13 @@ void guac_common_surface_copy(guac_common_surface* src, int sx, int sy, int w, i /* Clip operation */ __guac_common_clip_rect(dst, &rect, &sx, &sy); if (rect.width <= 0 || rect.height <= 0) - return; + goto complete; /* Update backing surface first only if destination rect cannot intersect source rect */ if (src != dst) { __guac_common_surface_transfer(src, &sx, &sy, GUAC_TRANSFER_BINARY_SRC, dst, &rect); if (rect.width <= 0 || rect.height <= 0) - return; + goto complete; } /* Defer if combining */ @@ -1202,8 +1268,8 @@ void guac_common_surface_copy(guac_common_surface* src, int sx, int sy, int w, i /* Otherwise, flush and draw immediately */ else { - guac_common_surface_flush(dst); - guac_common_surface_flush(src); + __guac_common_surface_flush(dst); + __guac_common_surface_flush(src); guac_protocol_send_copy(socket, src_layer, sx, sy, rect.width, rect.height, GUAC_COMP_OVER, dst_layer, rect.x, rect.y); dst->realized = 1; @@ -1213,11 +1279,23 @@ void guac_common_surface_copy(guac_common_surface* src, int sx, int sy, int w, i if (src == dst) __guac_common_surface_transfer(src, &sx, &sy, GUAC_TRANSFER_BINARY_SRC, dst, &rect); +complete: + + /* Unlock both surfaces */ + pthread_mutex_unlock(&dst->_lock); + if (src != dst) + pthread_mutex_unlock(&src->_lock); + } void guac_common_surface_transfer(guac_common_surface* src, int sx, int sy, int w, int h, guac_transfer_function op, guac_common_surface* dst, int dx, int dy) { + /* Lock both surfaces */ + pthread_mutex_lock(&dst->_lock); + if (src != dst) + pthread_mutex_lock(&src->_lock); + guac_socket* socket = dst->socket; const guac_layer* src_layer = src->layer; const guac_layer* dst_layer = dst->layer; @@ -1228,13 +1306,13 @@ void guac_common_surface_transfer(guac_common_surface* src, int sx, int sy, int /* Clip operation */ __guac_common_clip_rect(dst, &rect, &sx, &sy); if (rect.width <= 0 || rect.height <= 0) - return; + goto complete; /* Update backing surface first only if destination rect cannot intersect source rect */ if (src != dst) { __guac_common_surface_transfer(src, &sx, &sy, op, dst, &rect); if (rect.width <= 0 || rect.height <= 0) - return; + goto complete; } /* Defer if combining */ @@ -1243,9 +1321,10 @@ void guac_common_surface_transfer(guac_common_surface* src, int sx, int sy, int /* Otherwise, flush and draw immediately */ else { - guac_common_surface_flush(dst); - guac_common_surface_flush(src); - guac_protocol_send_transfer(socket, src_layer, sx, sy, rect.width, rect.height, op, dst_layer, rect.x, rect.y); + __guac_common_surface_flush(dst); + __guac_common_surface_flush(src); + guac_protocol_send_transfer(socket, src_layer, sx, sy, rect.width, + rect.height, op, dst_layer, rect.x, rect.y); dst->realized = 1; } @@ -1253,11 +1332,19 @@ void guac_common_surface_transfer(guac_common_surface* src, int sx, int sy, int if (src == dst) __guac_common_surface_transfer(src, &sx, &sy, op, dst, &rect); +complete: + + /* Unlock both surfaces */ + pthread_mutex_unlock(&dst->_lock); + if (src != dst) + pthread_mutex_unlock(&src->_lock); + } void guac_common_surface_rect(guac_common_surface* surface, - int x, int y, int w, int h, - int red, int green, int blue) { + int x, int y, int w, int h, int red, int green, int blue) { + + pthread_mutex_lock(&surface->_lock); guac_socket* socket = surface->socket; const guac_layer* layer = surface->layer; @@ -1268,12 +1355,12 @@ void guac_common_surface_rect(guac_common_surface* surface, /* Clip operation */ __guac_common_clip_rect(surface, &rect, NULL, NULL); if (rect.width <= 0 || rect.height <= 0) - return; + goto complete; /* Update backing surface */ __guac_common_surface_rect(surface, &rect, red, green, blue); if (rect.width <= 0 || rect.height <= 0) - return; + goto complete; /* Defer if combining */ if (__guac_common_should_combine(surface, &rect, 1)) @@ -1281,16 +1368,21 @@ void guac_common_surface_rect(guac_common_surface* surface, /* Otherwise, flush and draw immediately */ else { - guac_common_surface_flush(surface); + __guac_common_surface_flush(surface); guac_protocol_send_rect(socket, layer, rect.x, rect.y, rect.width, rect.height); guac_protocol_send_cfill(socket, GUAC_COMP_OVER, layer, red, green, blue, 0xFF); surface->realized = 1; } +complete: + pthread_mutex_unlock(&surface->_lock); + } void guac_common_surface_clip(guac_common_surface* surface, int x, int y, int w, int h) { + pthread_mutex_lock(&surface->_lock); + guac_common_rect clip; /* Init clipping rectangle if clipping not already applied */ @@ -1302,10 +1394,14 @@ void guac_common_surface_clip(guac_common_surface* surface, int x, int y, int w, guac_common_rect_init(&clip, x, y, w, h); guac_common_rect_constrain(&surface->clip_rect, &clip); + pthread_mutex_unlock(&surface->_lock); + } void guac_common_surface_reset_clip(guac_common_surface* surface) { + pthread_mutex_lock(&surface->_lock); surface->clipped = 0; + pthread_mutex_unlock(&surface->_lock); } /** @@ -1443,7 +1539,6 @@ static void __guac_common_surface_flush_to_webp(guac_common_surface* surface) { } - /** * Comparator for instances of guac_common_surface_bitmap_rect, the elements * which make up a surface's bitmap buffer. @@ -1467,7 +1562,16 @@ static int __guac_common_surface_bitmap_rect_compare(const void* a, const void* } -void guac_common_surface_flush_properties(guac_common_surface* surface) { +/** + * Flushes only the properties of the given surface, such as layer location or + * opacity. Image state is not flushed. If the surface represents a buffer or + * the default layer, this function has no effect. + * + * @param surface + * The surface to flush. + */ +static void __guac_common_surface_flush_properties( + guac_common_surface* surface) { guac_socket* socket = surface->socket; @@ -1490,7 +1594,7 @@ void guac_common_surface_flush_properties(guac_common_surface* surface) { } -void guac_common_surface_flush(guac_common_surface* surface) { +static void __guac_common_surface_flush(guac_common_surface* surface) { /* Flush final dirty rectangle to queue. */ __guac_common_surface_flush_to_queue(surface); @@ -1570,20 +1674,33 @@ void guac_common_surface_flush(guac_common_surface* surface) { } - /* Flush any applicable layer properties */ - guac_common_surface_flush_properties(surface); - /* Flush complete */ surface->bitmap_queue_length = 0; } +void guac_common_surface_flush(guac_common_surface* surface) { + + pthread_mutex_lock(&surface->_lock); + + /* Flush any applicable layer properties */ + __guac_common_surface_flush_properties(surface); + + /* Flush surface contents */ + __guac_common_surface_flush(surface); + + pthread_mutex_unlock(&surface->_lock); + +} + void guac_common_surface_dup(guac_common_surface* surface, guac_user* user, guac_socket* socket) { + pthread_mutex_lock(&surface->_lock); + /* Do nothing if not realized */ if (!surface->realized) - return; + goto complete; /* Synchronize layer-specific properties if applicable */ if (surface->layer->index > 0) { @@ -1611,5 +1728,8 @@ void guac_common_surface_dup(guac_common_surface* surface, guac_user* user, 0, 0, rect); cairo_surface_destroy(rect); +complete: + pthread_mutex_unlock(&surface->_lock); + } From 5d1de67a0c63556f62d844d16d9839411260a475 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Mon, 26 Sep 2016 13:06:56 -0700 Subject: [PATCH 2/2] GUACAMOLE-180: Make common display threadsafe. --- src/common/common/display.h | 9 ++++++++ src/common/display.c | 46 +++++++++++++++++++++++++++++-------- 2 files changed, 45 insertions(+), 10 deletions(-) diff --git a/src/common/common/display.h b/src/common/common/display.h index 0b54338c..6707bcb1 100644 --- a/src/common/common/display.h +++ b/src/common/common/display.h @@ -26,6 +26,8 @@ #include #include +#include + /** * A list element representing a pairing of a Guacamole layer with a * corresponding guac_common_surface which wraps that layer. Adjacent layers @@ -97,6 +99,13 @@ typedef struct guac_common_display { */ guac_common_display_layer* buffers; + /** + * Mutex which is locked internally when access to the display must be + * synchronized. All public functions of guac_common_display should be + * considered threadsafe. + */ + pthread_mutex_t _lock; + } guac_common_display; /** diff --git a/src/common/display.c b/src/common/display.c index 03c8da43..09bf67e7 100644 --- a/src/common/display.c +++ b/src/common/display.c @@ -24,6 +24,7 @@ #include #include +#include #include #include @@ -106,6 +107,8 @@ guac_common_display* guac_common_display_alloc(guac_client* client, if (display == NULL) return NULL; + pthread_mutex_init(&display->_lock, NULL); + /* Associate display with given client */ display->client = client; @@ -135,6 +138,7 @@ void guac_common_display_free(guac_common_display* display) { guac_common_display_free_layers(display->buffers, display->client); guac_common_display_free_layers(display->layers, display->client); + pthread_mutex_destroy(&display->_lock); free(display); } @@ -142,6 +146,8 @@ void guac_common_display_free(guac_common_display* display) { void guac_common_display_dup(guac_common_display* display, guac_user* user, guac_socket* socket) { + pthread_mutex_lock(&display->_lock); + /* Sunchronize shared cursor */ guac_common_cursor_dup(display->cursor, user, socket); @@ -152,10 +158,14 @@ void guac_common_display_dup(guac_common_display* display, guac_user* user, guac_common_display_dup_layers(display->layers, user, socket); guac_common_display_dup_layers(display->buffers, user, socket); + pthread_mutex_unlock(&display->_lock); + } void guac_common_display_flush(guac_common_display* display) { + pthread_mutex_lock(&display->_lock); + guac_common_display_layer* current = display->layers; /* Flush all surfaces */ @@ -166,6 +176,8 @@ void guac_common_display_flush(guac_common_display* display) { guac_common_surface_flush(display->default_surface); + pthread_mutex_unlock(&display->_lock); + } /** @@ -246,42 +258,50 @@ static void guac_common_display_remove_layer(guac_common_display_layer** head, guac_common_display_layer* guac_common_display_alloc_layer( guac_common_display* display, int width, int height) { - guac_layer* layer; - guac_common_surface* surface; + pthread_mutex_lock(&display->_lock); /* Allocate Guacamole layer */ - layer = guac_client_alloc_layer(display->client); + guac_layer* layer = guac_client_alloc_layer(display->client); /* Allocate corresponding surface */ - surface = guac_common_surface_alloc(display->client, + guac_common_surface* surface = guac_common_surface_alloc(display->client, display->client->socket, layer, width, height); /* Add layer and surface to list */ - return guac_common_display_add_layer(&display->layers, layer, surface); + guac_common_display_layer* display_layer = + guac_common_display_add_layer(&display->layers, layer, surface); + + pthread_mutex_unlock(&display->_lock); + return display_layer; } guac_common_display_layer* guac_common_display_alloc_buffer( guac_common_display* display, int width, int height) { - guac_layer* buffer; - guac_common_surface* surface; + pthread_mutex_lock(&display->_lock); /* Allocate Guacamole buffer */ - buffer = guac_client_alloc_buffer(display->client); + guac_layer* buffer = guac_client_alloc_buffer(display->client); /* Allocate corresponding surface */ - surface = guac_common_surface_alloc(display->client, + guac_common_surface* surface = guac_common_surface_alloc(display->client, display->client->socket, buffer, width, height); /* Add buffer and surface to list */ - return guac_common_display_add_layer(&display->buffers, buffer, surface); + guac_common_display_layer* display_layer = + guac_common_display_add_layer(&display->buffers, buffer, surface); + + pthread_mutex_unlock(&display->_lock); + return display_layer; } void guac_common_display_free_layer(guac_common_display* display, guac_common_display_layer* display_layer) { + pthread_mutex_lock(&display->_lock); + /* Remove list element from list */ guac_common_display_remove_layer(&display->layers, display_layer); @@ -292,11 +312,15 @@ void guac_common_display_free_layer(guac_common_display* display, /* Free list element */ free(display_layer); + pthread_mutex_unlock(&display->_lock); + } void guac_common_display_free_buffer(guac_common_display* display, guac_common_display_layer* display_buffer) { + pthread_mutex_lock(&display->_lock); + /* Remove list element from list */ guac_common_display_remove_layer(&display->buffers, display_buffer); @@ -307,5 +331,7 @@ void guac_common_display_free_buffer(guac_common_display* display, /* Free list element */ free(display_buffer); + pthread_mutex_unlock(&display->_lock); + }