GUACAMOLE-377: Do not allow "img" instructions to break "rect" and "c…

…fill" pairs.

Other instructions that occur between "rect" and "cfill" may disrupt the
path set by "rect", resulting in "cfill" having no effect, resulting in
graphical artifacts.

This also has the side effect of reducing thread contention by flushing
the simple operations early (it is now less likely that multiple worker
threads will be free for further tasks at nearly exactly the same time).

src/libguac/display-plan.c

@@ -20,9 +20,12 @@
 #include "display-plan.h"
 #include "display-priv.h"
 #include "guacamole/assert.h"
+#include "guacamole/client.h"
 #include "guacamole/display.h"
 #include "guacamole/fifo.h"
 #include "guacamole/mem.h"
+#include "guacamole/protocol.h"
+#include "guacamole/socket.h"
 #include "guacamole/timestamp.h"
 
 #include <string.h>
@@ -365,10 +368,65 @@ void guac_display_plan_free(guac_display_plan* plan) {
 void guac_display_plan_apply(guac_display_plan* plan) {
 
     guac_display* display = plan->display;
+    guac_client* client = display->client;
     guac_display_plan_operation* op = plan->ops;
 
+    /* Do not allow worker threads to move forward with image encoding until
+     * AFTER the non-image instructions have finished being written */
+    guac_fifo_lock(&display->ops);
+
+    /* Immediately send instructions for all updates that do not involve
+     * significant processing (do not involve encoding anything). This allows
+     * us to use the worker threads solely for encoding, reducing contention
+     * between the threads. */
     for (int i = 0; i < plan->length; i++) {
-        guac_fifo_enqueue(&display->ops, op++);
+
+        guac_display_layer* display_layer = op->layer;
+        switch (op->type) {
+
+            case GUAC_DISPLAY_PLAN_OPERATION_COPY:
+                guac_protocol_send_copy(client->socket, op->src.layer_rect.layer,
+                        op->src.layer_rect.rect.left, op->src.layer_rect.rect.top,
+                        guac_rect_width(&op->src.layer_rect.rect), guac_rect_height(&op->src.layer_rect.rect),
+                        GUAC_COMP_OVER, display_layer->layer, op->dest.left, op->dest.top);
+                break;
+
+            case GUAC_DISPLAY_PLAN_OPERATION_RECT:
+
+                guac_protocol_send_rect(client->socket, display_layer->layer,
+                        op->dest.left, op->dest.top, guac_rect_width(&op->dest), guac_rect_height(&op->dest));
+
+                int alpha = (op->src.color & 0xFF000000) >> 24;
+                int red   = (op->src.color & 0x00FF0000) >> 16;
+                int green = (op->src.color & 0x0000FF00) >> 8;
+                int blue  = (op->src.color & 0x000000FF);
+
+                /* Clear before drawing if layer is not opaque (transparency
+                 * will not be copied correctly otherwise) */
+                if (!display_layer->opaque)
+                    guac_protocol_send_cfill(client->socket, GUAC_COMP_ROUT, display_layer->layer,
+                            0x00, 0x00, 0x00, 0xFF);
+
+                guac_protocol_send_cfill(client->socket, GUAC_COMP_OVER, display_layer->layer,
+                        red, green, blue, alpha);
+
+                break;
+
+            /* Simply ignore and drop NOP */
+            case GUAC_DISPLAY_PLAN_OPERATION_NOP:
+                break;
+
+            /* All other operations should be handled by the workers */
+            default:
+                guac_fifo_enqueue(&display->ops, op);
+                break;
+
+        }
+
+        op++;
+
     }
 
+    guac_fifo_unlock(&display->ops);
+
 }

src/libguac/display-priv.h

@@ -92,7 +92,6 @@
  * 2) last_frame.lock
  * 3) ops
  * 4) render_state
- * 5) op_path_lock
  *
  * Acquiring these locks in any other order risks deadlock. Don't do it.
  */
@@ -667,15 +666,6 @@ struct guac_display {
      */
     guac_fifo ops;
 
-    /**
-     * Lock which ensures instructions that make use of a layer's current path
-     * (such as "rect" and "cfill") do not get inadvertently interleaved.
-     * Interleaving of path instructions can result in those paths not matching
-     * the expectatiosn of subsequent instructions, causing graphical
-     * artifacts.
-     */
-    pthread_mutex_t op_path_lock;
-
     /**
      * Storage for any items within the ops fifo.
      */

src/libguac/display-worker.c

@@ -104,16 +104,12 @@ static void guac_display_layer_clear_non_opaque(guac_display_layer* display_laye
      * being non-opaque */
     if (!display_layer->opaque) {
 
-        pthread_mutex_lock(&display->op_path_lock);
-
         guac_protocol_send_rect(socket, layer, dirty->left, dirty->top,
                 guac_rect_width(dirty), guac_rect_height(dirty));
 
         guac_protocol_send_cfill(socket, GUAC_COMP_ROUT, layer,
                 0x00, 0x00, 0x00, 0xFF);
 
-        pthread_mutex_unlock(&display->op_path_lock);
-
     }
 
 }
@@ -371,36 +367,14 @@ void* guac_display_worker_thread(void* data) {
                 break;
 
             case GUAC_DISPLAY_PLAN_OPERATION_COPY:
-                guac_protocol_send_copy(client->socket, op.src.layer_rect.layer,
-                        op.src.layer_rect.rect.left, op.src.layer_rect.rect.top,
-                        guac_rect_width(&op.src.layer_rect.rect), guac_rect_height(&op.src.layer_rect.rect),
-                        GUAC_COMP_OVER, display_layer->layer, op.dest.left, op.dest.top);
-                break;
-
             case GUAC_DISPLAY_PLAN_OPERATION_RECT:
-
-                pthread_mutex_lock(&display->op_path_lock);
-                guac_protocol_send_rect(client->socket, display_layer->layer,
-                        op.dest.left, op.dest.top, guac_rect_width(&op.dest), guac_rect_height(&op.dest));
-
-                int alpha = (op.src.color & 0xFF000000) >> 24;
-                int red   = (op.src.color & 0x00FF0000) >> 16;
-                int green = (op.src.color & 0x0000FF00) >> 8;
-                int blue  = (op.src.color & 0x000000FF);
-
-                /* Clear before drawing if layer is not opaque (transparency
-                 * will not be copied correctly otherwise) */
-                if (!display_layer->opaque)
-                    guac_protocol_send_cfill(client->socket, GUAC_COMP_ROUT, display_layer->layer,
-                            0x00, 0x00, 0x00, 0xFF);
-
-                guac_protocol_send_cfill(client->socket, GUAC_COMP_OVER, display_layer->layer,
-                        red, green, blue, alpha);
-
-                pthread_mutex_unlock(&display->op_path_lock);
-                break;
-
             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, "
+                        "single-threaded flush step. This is likely a bug.",
+                        op.type);
                 break;
 
             case GUAC_DISPLAY_PLAN_END_FRAME:

src/libguac/display.c

@@ -137,9 +137,6 @@ guac_display* guac_display_alloc(guac_client* client) {
     guac_flag_init(&display->render_state);
     guac_flag_set(&display->render_state, GUAC_DISPLAY_RENDER_STATE_FRAME_NOT_IN_PROGRESS);
 
-    /* Init lock specific to the GUAC_DISPLAY_PLAN_OPERATION_RECT operation */
-    pthread_mutex_init(&display->op_path_lock, NULL);
-
     int cpu_count = guac_display_nproc();
     if (cpu_count <= 0) {
         guac_client_log(client, GUAC_LOG_WARNING, "Number of available "
@@ -176,7 +173,6 @@ void guac_display_free(guac_display* display) {
         pthread_join(display->worker_threads[i], NULL);
 
     /* All locks, FIFOs, etc. are now unused and can be safely destroyed */
-    pthread_mutex_destroy(&display->op_path_lock);
     guac_flag_destroy(&display->render_state);
     guac_fifo_destroy(&display->ops);
     guac_rwlock_destroy(&display->last_frame.lock);