[Spice-devel,spice-server,1/2] RFC: Join drawables to improve rhel7 behavior

Submitted by Frediano Ziglio on Dec. 6, 2016, 12:28 p.m.

Details

Message ID 20161206122833.30944-2-fziglio@redhat.com
State New
Headers show
Series "RHEL7 improvements" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio Dec. 6, 2016, 12:28 p.m.
Due to the way RHEL7 works the images came out from guest using multiple
commands. This increase the commands to the client and cause the
video code to create and handle multiple streams creating some
visual glitches.
This patch attempt to detect and join the multiple commands to
avoid these issues.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 server/red-parse-qxl.h |   1 +
 server/red-worker.c    | 181 +++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 175 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
index 86a2d93..56cc906 100644
--- a/server/red-parse-qxl.h
+++ b/server/red-parse-qxl.h
@@ -57,6 +57,7 @@  typedef struct RedDrawable {
         SpiceWhiteness whiteness;
         SpiceComposite composite;
     } u;
+    struct RedDrawable *joined;
 } RedDrawable;
 
 static inline RedDrawable *red_drawable_ref(RedDrawable *drawable)
diff --git a/server/red-worker.c b/server/red-worker.c
index 79e459f..b841782 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -84,6 +84,8 @@  struct RedWorker {
 
     RedRecord *record;
     GMainLoop *loop;
+
+    RedDrawable *joinable_drawable;
 };
 
 static int display_is_connected(RedWorker *worker)
@@ -94,12 +96,16 @@  static int display_is_connected(RedWorker *worker)
 
 void red_drawable_unref(RedDrawable *red_drawable)
 {
-    if (--red_drawable->refs) {
-        return;
+    while (red_drawable) {
+        if (--red_drawable->refs) {
+            return;
+        }
+        red_qxl_release_resource(red_drawable->qxl, red_drawable->release_info_ext);
+        red_put_drawable(red_drawable);
+        RedDrawable *next = red_drawable->joined;
+        free(red_drawable);
+        red_drawable = next;
     }
-    red_qxl_release_resource(red_drawable->qxl, red_drawable->release_info_ext);
-    red_put_drawable(red_drawable);
-    free(red_drawable);
 }
 
 static int red_process_cursor(RedWorker *worker, int *ring_is_empty)
@@ -163,6 +169,144 @@  static RedDrawable *red_drawable_new(QXLInstance *qxl)
     return red;
 }
 
+/* Check that a given drawable it's a simple copy that can be
+ * possibly be joined to next one.
+ * This is used to undo some engine which split images into
+ * chunks causing more commands and creating multiple streams.
+ * One example is RHEL 7.
+ */
+static gboolean red_drawable_joinable(const RedDrawable *drawable)
+{
+    /* these 3 initial tests are arranged to minimize checks as
+     * they are in reverse order of occurrences */
+    if (drawable->clip.type != SPICE_CLIP_TYPE_NONE) {
+        return FALSE;
+    }
+    if (drawable->type != QXL_DRAW_COPY) {
+        return FALSE;
+    }
+    if (drawable->effect != QXL_EFFECT_OPAQUE) {
+        return FALSE;
+    }
+    if (drawable->self_bitmap) {
+        return FALSE;
+    }
+    if (drawable->surface_deps[0] != -1 ||
+        drawable->surface_deps[1] != -1 ||
+        drawable->surface_deps[2] != -1) {
+        return FALSE;
+    }
+
+    const SpiceCopy *copy = &drawable->u.copy;
+    if (copy->src_bitmap == NULL) {
+        return FALSE;
+    }
+    if (copy->rop_descriptor != SPICE_ROPD_OP_PUT) {
+        return FALSE;
+    }
+    if (copy->mask.bitmap != NULL) {
+        return FALSE;
+    }
+
+    const SpiceImage *image = copy->src_bitmap;
+    if (image->descriptor.type != SPICE_IMAGE_TYPE_BITMAP) {
+        return FALSE;
+    }
+    const SpiceBitmap *bitmap = &image->u.bitmap;
+    if (bitmap->format != SPICE_BITMAP_FMT_RGBA && bitmap->format != SPICE_BITMAP_FMT_32BIT) {
+        return FALSE;
+    }
+    if (bitmap->flags != SPICE_BITMAP_FLAGS_TOP_DOWN) {
+        return FALSE;
+    }
+    if (bitmap->palette != NULL) {
+        return FALSE;
+    }
+    if (bitmap->data == NULL) {
+        return FALSE;
+    }
+    if (bitmap->data->flags & SPICE_CHUNKS_FLAGS_UNSTABLE) {
+        return FALSE;
+    }
+    if (bitmap->x != image->descriptor.width ||
+        bitmap->y != image->descriptor.height) {
+        return FALSE;
+    }
+    /* area should specify all image */
+    if (copy->src_area.left != 0 || copy->src_area.top != 0 ||
+        copy->src_area.right != bitmap->x || copy->src_area.bottom != bitmap->y) {
+        return FALSE;
+    }
+    if (drawable->bbox.right - drawable->bbox.left != bitmap->x ||
+        drawable->bbox.bottom - drawable->bbox.top != bitmap->y) {
+        return FALSE;
+    }
+
+    return TRUE;
+}
+
+static gboolean red_drawable_can_join(const RedDrawable *first, const RedDrawable *second)
+{
+    if (!red_drawable_joinable(first) || !red_drawable_joinable(second)) {
+        return FALSE;
+    }
+    if (first->surface_id != second->surface_id) {
+        return FALSE;
+    }
+    if (first->u.copy.src_bitmap->u.bitmap.format != second->u.copy.src_bitmap->u.bitmap.format) {
+        return FALSE;
+    }
+    // they must have the same width
+    if (first->u.copy.src_bitmap->u.bitmap.x != second->u.copy.src_bitmap->u.bitmap.x ||
+        first->u.copy.src_bitmap->u.bitmap.stride != second->u.copy.src_bitmap->u.bitmap.stride) {
+        return FALSE;
+    }
+    // check that second is exactly under the first
+    if (first->bbox.left != second->bbox.left) {
+        return FALSE;
+    }
+    if (first->bbox.bottom != second->bbox.top) {
+        return FALSE;
+    }
+    // check we can join the chunks
+    if (first->u.copy.src_bitmap->u.bitmap.data->flags !=
+        second->u.copy.src_bitmap->u.bitmap.data->flags) {
+        return FALSE;
+    }
+    return TRUE;
+}
+
+static SpiceChunks *chunks_join(const SpiceChunks *first, const SpiceChunks *second)
+{
+    // TODO use realloc to optimize
+    SpiceChunks *new_chunks = spice_chunks_new(first->num_chunks + second->num_chunks);
+    new_chunks->flags = first->flags;
+    new_chunks->data_size = first->data_size + second->data_size;
+    memcpy(new_chunks->chunk, first->chunk, sizeof(first->chunk[0]) * first->num_chunks);
+    memcpy(new_chunks->chunk + first->num_chunks, second->chunk, sizeof(second->chunk[0]) * second->num_chunks);
+    return new_chunks;
+}
+
+static RedDrawable *red_drawable_join(RedDrawable *first, RedDrawable *second)
+{
+    uint32_t first_height = first->u.copy.src_bitmap->u.bitmap.y;
+    second->u.copy.src_bitmap->descriptor.flags &= ~SPICE_IMAGE_FLAGS_CACHE_ME;
+    second->bbox.top = first->bbox.top;
+    second->u.copy.src_bitmap->descriptor.height += first_height;
+    second->u.copy.src_bitmap->u.bitmap.y += first_height;
+    second->u.copy.src_area.bottom += first_height;
+    // join chunks
+    SpiceChunks *new_chunks = chunks_join(first->u.copy.src_bitmap->u.bitmap.data, second->u.copy.src_bitmap->u.bitmap.data);
+    // prevent to free chunks copied in new structure
+    second->u.copy.src_bitmap->u.bitmap.data->num_chunks = 0;
+    first->u.copy.src_bitmap->u.bitmap.data->num_chunks = 0;
+    // replace second one
+    spice_chunks_destroy(second->u.copy.src_bitmap->u.bitmap.data);
+    second->u.copy.src_bitmap->u.bitmap.data = new_chunks;
+    second->joined = first;
+    return second;
+}
+
 static int red_process_display(RedWorker *worker, int *ring_is_empty)
 {
     QXLCommandExt ext_cmd;
@@ -201,8 +345,31 @@  static int red_process_display(RedWorker *worker, int *ring_is_empty)
 
             if (!red_get_drawable(&worker->mem_slots, ext_cmd.group_id,
                                  red_drawable, ext_cmd.cmd.data, ext_cmd.flags)) {
-                display_channel_process_draw(worker->display_channel, red_drawable,
-                                             worker->process_display_generation);
+                if (!red_drawable_joinable(red_drawable)) {
+                    // not joinable, process all
+                    if (worker->joinable_drawable) {
+                        display_channel_process_draw(worker->display_channel, worker->joinable_drawable,
+                                                     worker->process_display_generation);
+                        red_drawable_unref(worker->joinable_drawable);
+                        worker->joinable_drawable = NULL;
+                    }
+                    display_channel_process_draw(worker->display_channel, red_drawable,
+                                                 worker->process_display_generation);
+                    red_drawable_unref(red_drawable);
+                    break;
+                } else if (worker->joinable_drawable == NULL) {
+                    // try to join with next one
+                } else if (red_drawable_can_join(worker->joinable_drawable, red_drawable)) {
+                    red_drawable = red_drawable_join(worker->joinable_drawable, red_drawable);
+                } else {
+                    // they can't be joined
+                    display_channel_process_draw(worker->display_channel, worker->joinable_drawable,
+                                                 worker->process_display_generation);
+                    red_drawable_unref(worker->joinable_drawable);
+                }
+                // try to join with next one
+                worker->joinable_drawable = red_drawable;
+                break;
             }
             // release the red_drawable
             red_drawable_unref(red_drawable);