[Spice-devel] [PATCH spice-server v2] red-parse-qxl: Avoid invalid flag usage

[Frediano Ziglio]

self_bitmap flag is used for some complex drawing not possible
by QXL_DRAW_COPY commands. Having this flag set causes
spice-server do draw part of the screen, copy that part on new
allocated image and reduce network optimisations with no visual
changes.
Some drivers (like Windows 10 DOD) set this flag by mistake for
this command so reset it.

More details follow.

The self_bitmap flag is used for some drawing command requiring to mix
the frame buffer with some other image. For this specific
QXL_DRAW_COPY command self_bitmap is used by spice-server code during
cachine/sending (the reason for the cache is to cache images sent to
client so the relationship between the two parts of the code).
However the self_bitmap_image (an image created in spice-server if
this flags is set) is used only if src_bitmap of SpiceCopy structure
(the structure used to store the QXL_DRAW_COPY command inside
spice-server) is NULL. But in red_get_copy_ptr (red-parse-qxl.c, the
function that parse the QXL_DRAW_COPY command form the QXL device)
not having a src_bitmap is considered an error so the
self_bitmap_image won't be used.

Why this flag affects network performance?
When spice-server see this flag it update the frame buffer according
to the pending commands (commands to be sent or still to be drawn on
frame buffer). spice-server maintain a tree of commands used to reduce
rendering and command to send. More or less if a command is covering
other commands (for instance filling the entire screen with a single
color) the pending commands can be removed from the queue and not sent
to the client. However when an update of the frame buffer is requested
spice-server update the frame buffer removing the commands from the
tree but not from the client queue.

Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
---
 server/red-parse-qxl.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

Changes since v1:
- handle all QXL_DRAW_COPY paths parsing the command;
- more long explanation.

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index 4a45c0f7..ca9b27a3 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -677,8 +677,15 @@ static void red_put_opaque(SpiceOpaque *red)
 }
 
 static bool red_get_copy_ptr(RedMemSlotInfo *slots, int group_id,
-                             SpiceCopy *red, QXLCopy *qxl, uint32_t flags)
+                             RedDrawable *red_drawable, QXLCopy *qxl, uint32_t flags)
 {
+    /* there's no sense to have this true, this will just waste CPU and reduce optimizations
+     * for this command. Due to some bugs however some driver set self_bitmap field for this
+     * command so reset it. */
+    red_drawable->self_bitmap = false;
+
+    SpiceCopy *red = &red_drawable->u.copy;
+
     red->src_bitmap      = red_get_image(slots, group_id, qxl->src_bitmap, flags, false);
     if (!red->src_bitmap) {
         return false;
@@ -1049,9 +1056,9 @@ static bool red_get_native_drawable(RedMemSlotInfo *slots, int group_id,
                               &red->u.blackness, &qxl->u.blackness, flags);
         break;
     case QXL_DRAW_BLEND:
-        return red_get_blend_ptr(slots, group_id, &red->u.blend, &qxl->u.blend, flags);
+        return red_get_blend_ptr(slots, group_id, red, &qxl->u.blend, flags);
     case QXL_DRAW_COPY:
-        return red_get_copy_ptr(slots, group_id, &red->u.copy, &qxl->u.copy, flags);
+        return red_get_copy_ptr(slots, group_id, red, &qxl->u.copy, flags);
     case QXL_COPY_BITS:
         red_get_point_ptr(&red->u.copy_bits.src_pos, &qxl->u.copy_bits.src_pos);
         break;
@@ -1128,9 +1135,9 @@ static bool red_get_compat_drawable(RedMemSlotInfo *slots, int group_id,
                               &red->u.blackness, &qxl->u.blackness, flags);
         break;
     case QXL_DRAW_BLEND:
-        return red_get_blend_ptr(slots, group_id, &red->u.blend, &qxl->u.blend, flags);
+        return red_get_blend_ptr(slots, group_id, red, &qxl->u.blend, flags);
     case QXL_DRAW_COPY:
-        return red_get_copy_ptr(slots, group_id, &red->u.copy, &qxl->u.copy, flags);
+        return red_get_copy_ptr(slots, group_id, red, &qxl->u.copy, flags);
     case QXL_COPY_BITS:
         red_get_point_ptr(&red->u.copy_bits.src_pos, &qxl->u.copy_bits.src_pos);
         red->surface_deps[0] = 0;
-- 
2.17.1