[Spice-devel] [PATCH v3 1/2] display-channel: Join drawables to improve rhel7 behaviour
Frediano Ziglio
fziglio at redhat.com
Fri Mar 3 11:46:26 UTC 2017
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 at redhat.com>
---
server/display-channel-private.h | 2 +-
server/display-channel.c | 220 +++++++++++++++++++++++++++++++-
server/red-parse-qxl.h | 1 +-
server/red-worker.c | 14 +-
4 files changed, 230 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 67a77ef..e95741f 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -78,6 +78,8 @@ display_channel_finalize(GObject *object)
{
DisplayChannel *self = DISPLAY_CHANNEL(object);
+ red_drawable_unref(self->priv->joinable_drawable);
+
display_channel_destroy_surfaces(self);
image_cache_reset(&self->priv->image_cache);
monitors_config_unref(self->priv->monitors_config);
@@ -1176,8 +1178,190 @@ 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 is a simple copy that can
+ * possibly be joined to next one.
+ * 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.
+ * One example is RHEL 7.
+ * Some of the check in this function are just to check that the
+ * operation is a opaque bitmap copy operations, some other just
+ * avoid to make the joining code more complicated just to support
+ * unseen commands.
+ */
+static bool red_drawable_joinable(const RedDrawable *drawable)
+{
+ // 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;
+ }
+ // Never seen, avoid to support during join
+ if (drawable->self_bitmap) {
+ return false;
+ }
+ // Consistency, a copy should not have dependencies
+ if (drawable->surface_deps[0] != -1 ||
+ drawable->surface_deps[1] != -1 ||
+ drawable->surface_deps[2] != -1) {
+ return false;
+ }
+
+ const SpiceCopy *copy = &drawable->u.copy;
+ // We need a bitmap
+ if (copy->src_bitmap == NULL) {
+ return false;
+ }
+ if (copy->rop_descriptor != SPICE_ROPD_OP_PUT) {
+ return false;
+ }
+ // Never seen, avoid to support during join
+ if (copy->mask.bitmap != NULL) {
+ return false;
+ }
+
+ // Limit image support to 32bit bitmap
+ 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;
+ }
+ // Never seen anything else, avoid to support during join.
+ // We could not easily join bitmap with different orientations
+ if (bitmap->flags != SPICE_BITMAP_FLAGS_TOP_DOWN) {
+ return false;
+ }
+ // Consistency, a 32bit image have no reason to have a palette
+ if (bitmap->palette != NULL) {
+ return false;
+ }
+ if (bitmap->data == NULL) {
+ return false;
+ }
+ // Never seen, avoid to support during join.
+ // Wouldn't be too hard to implement but looking at driver code this flag
+ // is not used so no worth implementing it.
+ if (bitmap->data->flags & SPICE_CHUNKS_FLAGS_UNSTABLE) {
+ return false;
+ }
+ // Consistency, the bitmap inside the image should fill the image
+ if (bitmap->x != image->descriptor.width ||
+ bitmap->y != image->descriptor.height) {
+ return false;
+ }
+ // Area should specify all image. If not we would have to
+ // adjust the memory chunks to take into account this or make other
+ // complicated cropping.
+ 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 bool 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;
+ }
+ // We can't join 2 different format
+ 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;
+}
+
+/* Create a new set of chunks containing the chunks of first+second set.
+ * Note that the resulting chunks will point to original data so
+ * you should pay attention in order to avoid double free or other
+ * corruptions.
+ * For instance you could set to zero the number of chunks of first and second
+ * just after calling this function or free the two sets paying attention
+ * they don't release the data.
+ */
+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;
+}
+
+/* Join the first drawable with the second.
+ * The resulting drawable is returned.
+ * Once joined you should avoid to use first and second drawables.
+ */
+static RedDrawable *red_drawable_join(RedDrawable *first, RedDrawable *second)
+{
+ // Join drawing areas. The first is exactly on top of the 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 set of chunks with the sum computed above
+ spice_chunks_destroy(second->u.copy.src_bitmap->u.bitmap.data);
+ second->u.copy.src_bitmap->u.bitmap.data = new_chunks;
+
+ // Create a chain of drawables. This is necessary to retain needed memory
+ // resources.
+ 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,
@@ -1192,6 +1376,38 @@ 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;
+ }
+ // Drawable is joinable
+
+ if (display->priv->joinable_drawable != NULL) {
+ 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 c88034b..897566e 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)
--
git-series 0.9.1
More information about the Spice-devel
mailing list