[Spice-devel,v2,1/2] display-channel: Join drawables to improve rhel7 behaviour

Submitted by Frediano Ziglio on March 2, 2017, 11:34 a.m.

Details

Message ID 827bbd64c0ba9f0c8cbf37a814781bd1513eae72.1488454452.git-series.fziglio@redhat.com
State New
Headers show
Series "RHEL7 improvements" ( rev: 3 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio March 2, 2017, 11:34 a.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/display-channel-private.h |   2 +-
 server/display-channel.c         | 173 +++++++++++++++++++++++++++++++-
 server/red-parse-qxl.h           |   1 +-
 server/red-worker.c              |  14 +--
 4 files changed, 183 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/display-channel-private.h b/server/display-channel-private.h
index da807d1..62e03b6 100644
--- a/server/display-channel-private.h
+++ b/server/display-channel-private.h
@@ -69,6 +69,8 @@  struct DisplayChannelPrivate
 
     int gl_draw_async_count;
 
+    RedDrawable *joinable_drawable;
+
 /* TODO: some day unify this, make it more runtime.. */
     stat_info_t add_stat;
     stat_info_t exclude_stat;
diff --git a/server/display-channel.c b/server/display-channel.c
index fa2b281..37d1703 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -1175,8 +1175,147 @@  static void display_channel_add_drawable(DisplayChannel *display, Drawable *draw
 #endif
 }
 
-void display_channel_process_draw(DisplayChannel *display, RedDrawable *red_drawable,
-                                  uint32_t process_commands_generation)
+/* 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 void
+display_channel_process_draw_single(DisplayChannel *display, RedDrawable *red_drawable,
+                                    uint32_t process_commands_generation)
 {
     Drawable *drawable =
         display_channel_get_drawable(display, red_drawable->effect, red_drawable,
@@ -1191,6 +1330,36 @@  void display_channel_process_draw(DisplayChannel *display, RedDrawable *red_draw
     drawable_unref(drawable);
 }
 
+void display_channel_process_draw(DisplayChannel *display, RedDrawable *red_drawable,
+                                  uint32_t process_commands_generation)
+{
+    if (!red_drawable_joinable(red_drawable)) {
+        // not joinable, process all
+        if (display->priv->joinable_drawable) {
+            display_channel_process_draw_single(display, display->priv->joinable_drawable,
+                                                process_commands_generation);
+            red_drawable_unref(display->priv->joinable_drawable);
+            display->priv->joinable_drawable = NULL;
+        }
+        display_channel_process_draw_single(display, red_drawable, process_commands_generation);
+        return;
+    }
+
+    if (display->priv->joinable_drawable == NULL) {
+        // try to join with next one
+    } else if (red_drawable_can_join(display->priv->joinable_drawable, red_drawable)) {
+        red_drawable = red_drawable_join(display->priv->joinable_drawable, red_drawable);
+    } else {
+        // they can't be joined
+        display_channel_process_draw_single(display, display->priv->joinable_drawable,
+                                            process_commands_generation);
+        red_drawable_unref(display->priv->joinable_drawable);
+    }
+    // try to join with next one
+    red_drawable_ref(red_drawable);
+    display->priv->joinable_drawable = red_drawable;
+}
+
 int display_channel_wait_for_migrate_data(DisplayChannel *display)
 {
     uint64_t end_time = spice_get_monotonic_time_ns() + DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT;
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 8735cd1..b0d2955 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -94,12 +94,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 gboolean red_process_cursor_cmd(RedWorker *worker, const QXLCommandExt *ext)

Comments

On Thu, 2017-03-02 at 11:34 +0000, Frediano Ziglio wrote:
> 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/display-channel-private.h |   2 +-
>  server/display-channel.c         | 173
> +++++++++++++++++++++++++++++++-
>  server/red-parse-qxl.h           |   1 +-
>  server/red-worker.c              |  14 +--
>  4 files changed, 183 insertions(+), 7 deletions(-)
> 
> diff --git a/server/display-channel-private.h b/server/display-
> channel-private.h
> index da807d1..62e03b6 100644
> --- a/server/display-channel-private.h
> +++ b/server/display-channel-private.h
> @@ -69,6 +69,8 @@ struct DisplayChannelPrivate
>  
>      int gl_draw_async_count;
>  
> +    RedDrawable *joinable_drawable;
> +
>  /* TODO: some day unify this, make it more runtime.. */
>      stat_info_t add_stat;
>      stat_info_t exclude_stat;
> diff --git a/server/display-channel.c b/server/display-channel.c
> index fa2b281..37d1703 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -1175,8 +1175,147 @@ static void
> display_channel_add_drawable(DisplayChannel *display, Drawable *draw
>  #endif
>  }
>  
> -void display_channel_process_draw(DisplayChannel *display,
> RedDrawable *red_drawable,
> -                                  uint32_t
> process_commands_generation)
> +/* Check that a given drawable it's a simple copy that can be

- it's -> is
- remove the extra 'be' at the end of the line above

> + * 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.

maybe reword this:

"This is used to work around some drivers which split larger image
updates into smaller chunks. These chunks are generally horizontal
strips. This can cause our stream-detection heuristics to generate
multiple streams instead of a single stream, and results in more
commands being sent to the client."

> + */
> +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 */

"reverse order of occurrence" implies to me that the first one is the
least likely to occur. Is that what you intended? I think it should be
the opposite, right?

(also: occurrences should just be occurrence (singular))

> +    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;
> +    }

It's not really clear to me why all of these checks are necessary, or
whether there are any checks missing. How did you decide which checks
were required? Can any comments be added?


> +
> +    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;
> +}
> +

I think that the following function deserves a few comments because
it's a pretty easy function to misuse. First of all, the returned
object will contain references to data held by the two arguments, so
care must be taken to avoid double-freeing that data. Also, I think
it's useful to indicate in a comment that a return value is newly-
allocated and must be freed, and that the data from 'second' will be
appended to 'first'. 
> +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;
> +}
> +


This function also could use some basic documentation. It doesn't
allocate and return a new drawable like the previous _join() function.
Rather it copies all of the data into the second one, links them
together, and then returns the second one.
> +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;

So this stores a reference from the most recent drawable to the (empty)
drawable that it was just joined with. That one may have links to
earlier (empty) joined drawables. This link is necessary only to be
able to free that drawable later. I suppose it's not possible to simply
free it now because that would release some resources within the driver
and cause problems?

> +    return second;
> +}
> +
> +static void
> +display_channel_process_draw_single(DisplayChannel *display,
> RedDrawable *red_drawable,
> +                                    uint32_t
> process_commands_generation)
>  {
>      Drawable *drawable =
>          display_channel_get_drawable(display, red_drawable->effect,
> red_drawable,
> @@ -1191,6 +1330,36 @@ void
> display_channel_process_draw(DisplayChannel *display, RedDrawable
> *red_draw
>      drawable_unref(drawable);
>  }
>  
> +void display_channel_process_draw(DisplayChannel *display,
> RedDrawable *red_drawable,
> +                                  uint32_t
> process_commands_generation)
> +{
> +    if (!red_drawable_joinable(red_drawable)) {
> +        // not joinable, process all
> +        if (display->priv->joinable_drawable) {
> +            display_channel_process_draw_single(display, display-
> >priv->joinable_drawable,
> +                                                process_commands_gen
> eration);

Isn't it possible that the earlier invocation of
display_channel_proces_draw() (when we saved priv->joinable_drawable)
may have used a different value for process_commands_generation? Should
that value be saved with the joinable_drawable and passed to this
function call? Or are you certain that it's OK to use the current value
here? 

> +            red_drawable_unref(display->priv->joinable_drawable);
> +            display->priv->joinable_drawable = NULL;
> +        }
> +        display_channel_process_draw_single(display, red_drawable,
> process_commands_generation);
> +        return;
> +    }
> +

add comment:
// drawable is joinable
> +    if (display->priv->joinable_drawable == NULL) {
> +        // try to join with next one
> +    } else if (red_drawable_can_join(display->priv-
> >joinable_drawable, red_drawable)) {
> +        red_drawable = red_drawable_join(display->priv-
> >joinable_drawable, red_drawable);
> +    } else {
> +        // they can't be joined
> +        display_channel_process_draw_single(display, display->priv-
> >joinable_drawable,
> +                                            process_commands_generat
> ion);
> +        red_drawable_unref(display->priv->joinable_drawable);
> +    }
> +    // try to join with next one
> +    red_drawable_ref(red_drawable);
> +    display->priv->joinable_drawable = red_drawable;
> +}
> +

I find the above section a little bit confusing. I think it would be a
bit easier to follow like this:


if (display->priv->joinable_drawable != NULL) {
    if (red_drawable_can_join(...)) {
        red_drawable = red_drawable_join(...);
    } else {
        // the can't be joined
        display_channel_process_draw_single(...);
        red_drawable_unref(display->priv->joinable_drawable);
    }
}

// try to join with next one
red_drawable_ref()
display->priv->joinable_drawable = red_drawable;



>  int display_channel_wait_for_migrate_data(DisplayChannel *display)
>  {
>      uint64_t end_time = spice_get_monotonic_time_ns() +
> DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT;
> 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 8735cd1..b0d2955 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -94,12 +94,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 gboolean red_process_cursor_cmd(RedWorker *worker, const
> QXLCommandExt *ext)
> 
> On Thu, 2017-03-02 at 11:34 +0000, Frediano Ziglio wrote:
> > 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/display-channel-private.h |   2 +-
> >  server/display-channel.c         | 173
> > +++++++++++++++++++++++++++++++-
> >  server/red-parse-qxl.h           |   1 +-
> >  server/red-worker.c              |  14 +--
> >  4 files changed, 183 insertions(+), 7 deletions(-)
> > 
> > diff --git a/server/display-channel-private.h b/server/display-
> > channel-private.h
> > index da807d1..62e03b6 100644
> > --- a/server/display-channel-private.h
> > +++ b/server/display-channel-private.h
> > @@ -69,6 +69,8 @@ struct DisplayChannelPrivate
> >  
> >      int gl_draw_async_count;
> >  
> > +    RedDrawable *joinable_drawable;
> > +
> >  /* TODO: some day unify this, make it more runtime.. */
> >      stat_info_t add_stat;
> >      stat_info_t exclude_stat;
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index fa2b281..37d1703 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -1175,8 +1175,147 @@ static void
> > display_channel_add_drawable(DisplayChannel *display, Drawable *draw
> >  #endif
> >  }
> >  
> > -void display_channel_process_draw(DisplayChannel *display,
> > RedDrawable *red_drawable,
> > -                                  uint32_t
> > process_commands_generation)
> > +/* Check that a given drawable it's a simple copy that can be
> 
> - it's -> is
> - remove the extra 'be' at the end of the line above
> 
> > + * 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.
> 
> maybe reword this:
> 
> "This is used to work around some drivers which split larger image
> updates into smaller chunks. These chunks are generally horizontal
> strips. This can cause our stream-detection heuristics to generate
> multiple streams instead of a single stream, and results in more
> commands being sent to the client."
> 
> > + */
> > +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 */
> 
> "reverse order of occurrence" implies to me that the first one is the
> least likely to occur. Is that what you intended? I think it should be
> the opposite, right?
> 
> (also: occurrences should just be occurrence (singular))
> 

First is more probable.
I probably needs some help with the comment. Maybe:

"these 3 initial tests are arranged to minimize checks as
 they are arranged from more probable to less probable."

> > +    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;
> > +    }
> 
> It's not really clear to me why all of these checks are necessary, or
> whether there are any checks missing. How did you decide which checks
> were required? Can any comments be added?
> 

Yes

> 
> > +
> > +    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;
> > +}
> > +
> 
> I think that the following function deserves a few comments because
> it's a pretty easy function to misuse. First of all, the returned
> object will contain references to data held by the two arguments, so
> care must be taken to avoid double-freeing that data. Also, I think
> it's useful to indicate in a comment that a return value is newly-
> allocated and must be freed, and that the data from 'second' will be
> appended to 'first'.
> > +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;
> > +}
> > +
> 
> 
> This function also could use some basic documentation. It doesn't
> allocate and return a new drawable like the previous _join() function.
> Rather it copies all of the data into the second one, links them
> together, and then returns the second one.
> > +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;
> 
> So this stores a reference from the most recent drawable to the (empty)
> drawable that it was just joined with. That one may have links to
> earlier (empty) joined drawables. This link is necessary only to be
> able to free that drawable later. I suppose it's not possible to simply
> free it now because that would release some resources within the driver
> and cause problems?
> 
> > +    return second;
> > +}
> > +
> > +static void
> > +display_channel_process_draw_single(DisplayChannel *display,
> > RedDrawable *red_drawable,
> > +                                    uint32_t
> > process_commands_generation)
> >  {
> >      Drawable *drawable =
> >          display_channel_get_drawable(display, red_drawable->effect,
> > red_drawable,
> > @@ -1191,6 +1330,36 @@ void
> > display_channel_process_draw(DisplayChannel *display, RedDrawable
> > *red_draw
> >      drawable_unref(drawable);
> >  }
> >  
> > +void display_channel_process_draw(DisplayChannel *display,
> > RedDrawable *red_drawable,
> > +                                  uint32_t
> > process_commands_generation)
> > +{
> > +    if (!red_drawable_joinable(red_drawable)) {
> > +        // not joinable, process all
> > +        if (display->priv->joinable_drawable) {
> > +            display_channel_process_draw_single(display, display-
> > >priv->joinable_drawable,
> > +                                                process_commands_gen
> > eration);
> 
> Isn't it possible that the earlier invocation of
> display_channel_proces_draw() (when we saved priv->joinable_drawable)
> may have used a different value for process_commands_generation? Should
> that value be saved with the joinable_drawable and passed to this
> function call? Or are you certain that it's OK to use the current value
> here?
> 
> > +            red_drawable_unref(display->priv->joinable_drawable);
> > +            display->priv->joinable_drawable = NULL;
> > +        }
> > +        display_channel_process_draw_single(display, red_drawable,
> > process_commands_generation);
> > +        return;
> > +    }
> > +
> 
> add comment:
> // drawable is joinable
> > +    if (display->priv->joinable_drawable == NULL) {
> > +        // try to join with next one
> > +    } else if (red_drawable_can_join(display->priv-
> > >joinable_drawable, red_drawable)) {
> > +        red_drawable = red_drawable_join(display->priv-
> > >joinable_drawable, red_drawable);
> > +    } else {
> > +        // they can't be joined
> > +        display_channel_process_draw_single(display, display->priv-
> > >joinable_drawable,
> > +                                            process_commands_generat
> > ion);
> > +        red_drawable_unref(display->priv->joinable_drawable);
> > +    }
> > +    // try to join with next one
> > +    red_drawable_ref(red_drawable);
> > +    display->priv->joinable_drawable = red_drawable;
> > +}
> > +
> 
> I find the above section a little bit confusing. I think it would be a
> bit easier to follow like this:
> 
> 
> if (display->priv->joinable_drawable != NULL) {
>     if (red_drawable_can_join(...)) {
>         red_drawable = red_drawable_join(...);
>     } else {
>         // the can't be joined
>         display_channel_process_draw_single(...);
>         red_drawable_unref(display->priv->joinable_drawable);
>     }
> }
> 
> // try to join with next one
> red_drawable_ref()
> display->priv->joinable_drawable = red_drawable;
> 
> 
> 
> >  int display_channel_wait_for_migrate_data(DisplayChannel *display)
> >  {
> >      uint64_t end_time = spice_get_monotonic_time_ns() +
> > DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT;
> > 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 8735cd1..b0d2955 100644
> > --- a/server/red-worker.c
> > +++ b/server/red-worker.c
> > @@ -94,12 +94,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 gboolean red_process_cursor_cmd(RedWorker *worker, const
> > QXLCommandExt *ext)
> 

Thanks for reviewing this.

Frediano