diff --git a/src/libguac/display-plan-combine.c b/src/libguac/display-plan-combine.c index 32542a1b4..2a04151dd 100644 --- a/src/libguac/display-plan-combine.c +++ b/src/libguac/display-plan-combine.c @@ -23,31 +23,38 @@ #include "guacamole/rect.h" /** - * Returns whether the given operation can be combined with others. Only - * operations of certain types can be combined, and only up to a certain size - * (to favor parallelism). + * Returns whether the given rectangle crosses the boundaries of any two + * adjacent cells in a grid, where each cell in the grid is + * 2^GUAC_DISPLAY_MAX_COMBINED_SIZE pixels on each side. * - * @param op - * The operation to test. + * This function exists because combination of adjacent image updates is + * intentionally limited to a certain size in order to favor parallelism. + * Greedily combining in the horizontal direction works, but in practice tends + * to produce a vertical series of strips that are offset from each other to + * the point that they cannot be further combined. Anchoring combined image + * updates to a grid helps prevent ths. + * + * @param rect + * The rectangle to test. * * @return - * Non-zero if the operation can be combined with others, zero otherwise. + * Non-zero if the rectangle crosses the boundary of any adjacent pair of + * cells in a grid, where each cell is 2^GUAC_DISPLAY_MAX_COMBINED_SIZE + * pixels on each side, zero otherwise. */ -static int guac_display_plan_is_combinable(const guac_display_plan_operation* op) { - switch (op->type) { +static int guac_display_plan_rect_crosses_boundary(const guac_rect* rect) { - case GUAC_DISPLAY_PLAN_OPERATION_IMG: - return guac_rect_width(&op->dest) < GUAC_DISPLAY_MAX_COMBINED_WIDTH - && guac_rect_height(&op->dest) < GUAC_DISPLAY_MAX_COMBINED_HEIGHT; + /* A particular rectangle crosses a grid boundary if and only if expanding + * that rectangle to fit the grid would mean increasing the size of that + * rectangle beyond a single grid cell */ - case GUAC_DISPLAY_PLAN_OPERATION_RECT: - case GUAC_DISPLAY_PLAN_OPERATION_COPY: - return 1; + guac_rect rect_copy = *rect; + guac_rect_align(&rect_copy, GUAC_DISPLAY_MAX_COMBINED_SIZE); - default: - return 0; + const int max_size_pixels = 1 << GUAC_DISPLAY_MAX_COMBINED_SIZE; + return guac_rect_width(&rect_copy) > max_size_pixels + || guac_rect_height(&rect_copy) > max_size_pixels; - } } /** @@ -109,16 +116,14 @@ static int guac_display_plan_has_common_edge(const guac_display_plan_operation* static int guac_display_plan_should_combine(const guac_display_plan_operation* op_a, const guac_display_plan_operation* op_b) { - /* Consider only operations that have combinable types (draw to a - * particular rectangle in the layer) */ - if (!guac_display_plan_is_combinable(op_a) - || !guac_display_plan_is_combinable(op_b)) - return 0; - /* Operations can only be combined within the same layer */ if (op_a->layer != op_b->layer) return 0; + /* Simulate combination */ + guac_rect combined = op_a->dest; + guac_rect_extend(&combined, &op_b->dest); + /* Operations of the same type can be trivially unified under specific * circumstances */ if (op_a->type == op_b->type) { @@ -137,7 +142,8 @@ static int guac_display_plan_should_combine(const guac_display_plan_operation* o int delta_yb = op_b->dest.top - op_b->src.layer_rect.rect.top; return delta_xa == delta_xb - && delta_ya == delta_yb; + && delta_ya == delta_yb + && !guac_display_plan_rect_crosses_boundary(&combined); } break; @@ -147,7 +153,14 @@ static int guac_display_plan_should_combine(const guac_display_plan_operation* o * color */ case GUAC_DISPLAY_PLAN_OPERATION_RECT: return op_a->src.color == op_b->src.color - && guac_display_plan_has_common_edge(op_a, op_b); + && guac_display_plan_has_common_edge(op_a, op_b) + && !guac_display_plan_rect_crosses_boundary(&combined); + + /* Image-drawing operations can be combined if doing so wouldn't + * exceed the size limits for images (we enforce size limits here + * to promote parallelism) */ + case GUAC_DISPLAY_PLAN_OPERATION_IMG: + return !guac_display_plan_rect_crosses_boundary(&combined); /* Other combinations require more complex logic... (see below) */ default: @@ -156,10 +169,6 @@ static int guac_display_plan_should_combine(const guac_display_plan_operation* o } } - /* Simulate combination */ - guac_rect combined = op_a->dest; - guac_rect_extend(&combined, &op_b->dest); - /* Combine if result is still small */ int combined_width = guac_rect_width(&combined); int combined_height = guac_rect_height(&combined); diff --git a/src/libguac/display-plan.h b/src/libguac/display-plan.h index 76fecd0e2..d1a520b7f 100644 --- a/src/libguac/display-plan.h +++ b/src/libguac/display-plan.h @@ -47,22 +47,16 @@ #define GUAC_DISPLAY_DATA_FACTOR 128 /** - * The maximum width to allow when combining any pair of rendering operations - * into a single operation, in pixels. This value is intended to be large - * enough to avoid unnecessarily increasing the number of drawing operations, - * yet also small enough to allow larger updates to be easily parallelized via - * the worker threads. - */ -#define GUAC_DISPLAY_MAX_COMBINED_WIDTH 512 - -/** - * The maximum height to allow when combining any pair of rendering operations - * into a single operation, in pixels. This value is intended to be large - * enough to avoid unnecessarily increasing the number of drawing operations, - * yet also small enough to allow larger updates to be easily parallelized via - * the worker threads. + * The maximum width or height to allow when combining any pair of rendering + * operations into a single operation, in pixels, as the exponent of a power of + * two. This value is intended to be large enough to avoid unnecessarily + * increasing the number of drawing operations, yet also small enough to allow + * larger updates to be easily parallelized via the worker threads. + * + * The current value of 9 means that each encoded image will be no larger than + * 512x512 pixels. */ -#define GUAC_DISPLAY_MAX_COMBINED_HEIGHT 512 +#define GUAC_DISPLAY_MAX_COMBINED_SIZE 9 /** * The base cost of every update. Each update should be considered to have