[RFC 0/6] RFC: hw/display/virtio-gpu: problems with coloured cursors

[RFC 0/6] RFC: hw/display/virtio-gpu: problems with coloured cursors

[Daniel P. Berrangé]

Help needed ! This is not for merge, just demo.

I started working on a simple bug in the GTK-VNC 'alpha cursor' encoding
support and in testing that uncovered what appear to be several bugs in
QEMU related to cursor handling across different components.

The two core behaviours are seen:

 * In some display backends (SDL, GTK), areas of the cursor
   with partial alpha are less saturated than they ought to be.

   The cursor data virtio-gpu is receiving from the guest has
   had the alpha channel pre-multiplied into the RGB components.

   This is not apparent in the VNC backend, since the VNC server
   is *supposed* to pre-multiply alpha but forgot to do so. Thus
   the virtio-gpu issue is hiding the VNC server issue.

 * In some display backends (GTK, VNC), the cursor RGB components
   are reversed.

   The cursor data virtio-gpu is receiving from the guest appears
   to be in BGRA8888 format, while GTK/VNC both expect the data to
   be in RGBA8888 format.

   This is not apparent in the SDL backend, since that is telling
   SDL to use BGRA8888 format when loading the cursor.

This series has patches to virtio-gpu which reverse the RGB
components and un-multiply the alpha channel.

I am not especially confident this is correct though.

The virtio spec is light on details:

  https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-3200007

but says

   "In 2D mode the virtio-gpu device provides support
    for ARGB Hardware cursors and multiple scanouts
    (aka heads)."

I'm unclear whether its reference to "ARGB" here implies that
seeing BGRA8888 data is intentional, or a guest kernel bug ?

The spec says nothing at all about alpha pre-multiplication.
I kind of think this is more likely to be a guest kernel bug,
but its possible the spec just forgot to mention this ?

Meanwhile I've absolutely no clue what impact endianness will
have on this mess. All my testing thus far has been x86_64
host (QEMU git HEAD) with x86_64 guest (Fedora 41).

The problem isn't obvious since guests usually have monochrome
cursors by default. To aid in testing, on *both* your host and
your guest OS do:

  git clone https://gitlab.com/berrange/cursordemo
  cd cursordemo
  ./setup.sh
  gnome-tweaks

Navigate to Apperance, and select "RGB" or "Rainbow" as the
cursor theme.

Now compare the guest OS rendered cursor to what you see on
the host OS. They should obviously match 100% since they're
using the same theme.

Daniel P. Berrangé (6):
  ui: add more cursor helper methods
  hw/display/virtio-gpu.c: reverse alpha pre-multiplication
  hw/display/virtio-gpu: fix pixel ordering from BGRA8888 to RGBA8888
  ui/vnc: pre-multiply alpha with alpha cursor
  ui/sdl: load cursor in RGBA8888 format not BGRA8888
  ui: add ability to dump the raw cursor bytes

 hw/display/virtio-gpu.c |  3 ++
 include/ui/console.h    |  7 +++++
 ui/cursor.c             | 70 +++++++++++++++++++++++++++++++++++++++++
 ui/sdl2.c               |  2 +-
 ui/vnc.c                |  7 ++++-
 5 files changed, 87 insertions(+), 2 deletions(-)

-- 
2.47.1

[RFC 1/6] ui: add more cursor helper methods

[Daniel P. Berrangé]

This adds helper methods to the QEMUCursor APIs for multiplying /
unmultiplying the alpha channel into the RGB components; for swapping
the RGB component order; for copying cursor objects; auto-freeing
cursor objects.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/ui/console.h |  6 ++++++
 ui/cursor.c          | 49 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

diff --git a/include/ui/console.h b/include/ui/console.h
index 46b3128185..e5eb903feb 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -175,6 +175,12 @@ void cursor_set_mono(QEMUCursor *c,
                      int transparent, uint8_t *mask);
 void cursor_get_mono_mask(QEMUCursor *c, int transparent, uint8_t *mask);
 
+void cursor_multiply_alpha(QEMUCursor *c);
+void cursor_unmultiply_alpha(QEMUCursor *c);
+void cursor_swap_rgb(QEMUCursor *c);
+QEMUCursor *cursor_copy(QEMUCursor *c);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(QEMUCursor, cursor_unref);
+
 typedef void *QEMUGLContext;
 typedef struct QEMUGLParams QEMUGLParams;
 
diff --git a/ui/cursor.c b/ui/cursor.c
index 6e23244fbe..536e022548 100644
--- a/ui/cursor.c
+++ b/ui/cursor.c
@@ -225,3 +225,52 @@ void cursor_get_mono_mask(QEMUCursor *c, int transparent, uint8_t *mask)
         mask += bpl;
     }
 }
+
+void cursor_multiply_alpha(QEMUCursor *c)
+{
+    int i;
+
+    for (i = 0 ; i < (c->width * c->height); i++) {
+        guint8 *pixels = (guint8 *)c->data + (i * 4);
+        pixels[0] = (unsigned)pixels[0] * pixels[3] / 255;
+        pixels[1] = (unsigned)pixels[1] * pixels[3] / 255;
+        pixels[2] = (unsigned)pixels[2] * pixels[3] / 255;
+    }
+}
+
+void cursor_unmultiply_alpha(QEMUCursor *c)
+{
+    int i;
+
+    for (i = 0 ; i < (c->width * c->height); i++) {
+        guint8 *pixels = (guint8 *)c->data + (i * 4);
+        guint8 alpha = pixels[3] ? pixels[3] : 1;
+        pixels[0] = (unsigned)pixels[0] * 255 / alpha;
+        pixels[1] = (unsigned)pixels[1] * 255 / alpha;
+        pixels[2] = (unsigned)pixels[2] * 255 / alpha;
+    }
+}
+
+void cursor_swap_rgb(QEMUCursor *c)
+{
+    int i;
+
+    for (i = 0 ; i < (c->width * c->height); i++) {
+        guint8 *pixels = (guint8 *)c->data + (i * 4);
+        pixels[0] ^= pixels[2];
+        pixels[2] ^= pixels[0];
+        pixels[0] ^= pixels[2];
+    }
+}
+
+QEMUCursor *cursor_copy(QEMUCursor *c)
+{
+    QEMUCursor *ret = cursor_alloc(c->width, c->height);
+
+    ret->hot_x = c->hot_x;
+    ret->hot_y = c->hot_y;
+
+    memcpy(ret->data, c->data, c->width * c->height * 4);
+
+    return ret;
+}
-- 
2.47.1

[RFC 2/6] hw/display/virtio-gpu.c: reverse alpha pre-multiplication

[Daniel P. Berrangé]

The cursor data we receive from the guest has had the alpha channel
pre-multiplied into the RGB components. Reverse this before passing
the cursor onto the backends.

Without doing this, areas of the cursor with alpha are much less
saturated than they are intended to be. This effect is visible with
the SDL and GTK backends, but was masked with the VNC backend since
that forget to apply pre-multiplication with its alpha cursor
encoding thus cancelling out the virtio-gpu problem.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/display/virtio-gpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 11a7a85750..156d4e0b9b 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -102,6 +102,7 @@ static void update_cursor(VirtIOGPU *g, struct virtio_gpu_update_cursor *cursor)
 
         if (cursor->resource_id > 0) {
             vgc->update_cursor_data(g, s, cursor->resource_id);
+            cursor_unmultiply_alpha(s->current_cursor);
         }
         dpy_cursor_define(s->con, s->current_cursor);
 
-- 
2.47.1

[RFC 3/6] hw/display/virtio-gpu: fix pixel ordering from BGRA8888 to RGBA8888

[Daniel P. Berrangé]

Currently both the VNC and GTK displays are rendering coloured cursors
with RGB components reversed. This originates with the data received
from the guest, so virtio-gpu must reverse this again to get it back
to the natural RGBA8888 order expected by backends.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/display/virtio-gpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 156d4e0b9b..9952658df2 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -103,6 +103,7 @@ static void update_cursor(VirtIOGPU *g, struct virtio_gpu_update_cursor *cursor)
         if (cursor->resource_id > 0) {
             vgc->update_cursor_data(g, s, cursor->resource_id);
             cursor_unmultiply_alpha(s->current_cursor);
+            cursor_swap_rgb(s->current_cursor);
         }
         dpy_cursor_define(s->con, s->current_cursor);
 
-- 
2.47.1

[RFC 4/6] ui/vnc: pre-multiply alpha with alpha cursor

[Daniel P. Berrangé]

The RFB specification for the alpha cursor encoding requires that the
alpha channel is pre-multiplied into the RGB components. This worked
by luck previously since the virtio-gpu device was not reversing the
pre-multiplication on data received from the guest. Now virtio-gpu is
fixed, the VNC server must apply pre-multiplication itself.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 ui/vnc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 9241caaad9..5ffb50109d 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -996,6 +996,7 @@ static int vnc_cursor_define(VncState *vs)
     }
 
     if (vnc_has_feature(vs, VNC_FEATURE_ALPHA_CURSOR)) {
+        g_autoptr(QEMUCursor) tmpc = cursor_copy(c);
         vnc_lock_output(vs);
         vnc_write_u8(vs,  VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
         vnc_write_u8(vs,  0);  /*  padding     */
@@ -1003,7 +1004,11 @@ static int vnc_cursor_define(VncState *vs)
         vnc_framebuffer_update(vs, c->hot_x, c->hot_y, c->width, c->height,
                                VNC_ENCODING_ALPHA_CURSOR);
         vnc_write_s32(vs, VNC_ENCODING_RAW);
-        vnc_write(vs, c->data, c->width * c->height * 4);
+
+        // Alpha is required to be pre-multiplied into RGB components
+        cursor_multiply_alpha(tmpc);
+
+        vnc_write(vs, tmpc->data, c->width * c->height * 4);
         vnc_unlock_output(vs);
         return 0;
     }
-- 
2.47.1

[RFC 5/6] ui/sdl: load cursor in RGBA8888 format not BGRA8888

[Daniel P. Berrangé]

The SDL code was loading cursor data in BGRA8888 format which masked the
problem with virtio-gpu not supplying data in RGBA8888 format. Now that
virtio-gpu is fixed, the SDL code needs to be fixed to match.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 ui/sdl2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui/sdl2.c b/ui/sdl2.c
index 445eb1dd9f..2d56b4d174 100644
--- a/ui/sdl2.c
+++ b/ui/sdl2.c
@@ -772,7 +772,7 @@ static void sdl_mouse_define(DisplayChangeListener *dcl,
 
     guest_sprite_surface =
         SDL_CreateRGBSurfaceFrom(c->data, c->width, c->height, 32, c->width * 4,
-                                 0xff0000, 0x00ff00, 0xff, 0xff000000);
+                                 0xff, 0x00ff00, 0xff0000, 0xff000000);
 
     if (!guest_sprite_surface) {
         fprintf(stderr, "Failed to make rgb surface from %p\n", c);
-- 
2.47.1

[RFC 6/6] ui: add ability to dump the raw cursor bytes

[Daniel P. Berrangé]

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/display/virtio-gpu.c |  1 +
 include/ui/console.h    |  1 +
 ui/cursor.c             | 21 +++++++++++++++++++++
 3 files changed, 23 insertions(+)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 9952658df2..84836f52c4 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -102,6 +102,7 @@ static void update_cursor(VirtIOGPU *g, struct virtio_gpu_update_cursor *cursor)
 
         if (cursor->resource_id > 0) {
             vgc->update_cursor_data(g, s, cursor->resource_id);
+            cursor_dump_hex(s->current_cursor, "", 24, 24);
             cursor_unmultiply_alpha(s->current_cursor);
             cursor_swap_rgb(s->current_cursor);
         }
diff --git a/include/ui/console.h b/include/ui/console.h
index e5eb903feb..95b57b0e45 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -169,6 +169,7 @@ void cursor_unref(QEMUCursor *c);
 QEMUCursor *cursor_builtin_hidden(void);
 QEMUCursor *cursor_builtin_left_ptr(void);
 void cursor_print_ascii_art(QEMUCursor *c, const char *prefix);
+void cursor_dump_hex(QEMUCursor *c, const char *prefix, int maxw, int maxh);
 int cursor_get_mono_bpl(QEMUCursor *c);
 void cursor_set_mono(QEMUCursor *c,
                      uint32_t foreground, uint32_t background, uint8_t *image,
diff --git a/ui/cursor.c b/ui/cursor.c
index 536e022548..bc96307f3f 100644
--- a/ui/cursor.c
+++ b/ui/cursor.c
@@ -80,6 +80,27 @@ void cursor_print_ascii_art(QEMUCursor *c, const char *prefix)
     }
 }
 
+void cursor_dump_hex(QEMUCursor *c, const char *prefix, int maxw, int maxh)
+{
+    uint8_t *data = (uint8_t *) c->data;
+    int x,y,v;
+
+    maxw = MIN(maxw, c->width);
+    maxh = MIN(maxh, c->height);
+
+    for (y = 0; y < maxh; y++) {
+        fprintf(stderr, "%s: %2d: |", prefix, y);
+        for (x = 0; x < maxw; x++) {
+            for (v = 0; v < 4 ; v++, data++) {
+                fprintf(stderr, "%02x", *data);
+            }
+            fprintf(stderr, " ");
+        }
+        data += (c->width - maxw) * 4;
+        fprintf(stderr, "|\n");
+    }
+}
+
 QEMUCursor *cursor_builtin_hidden(void)
 {
     return cursor_parse_xpm(cursor_hidden_xpm);
-- 
2.47.1

Re: [RFC 0/6] RFC: hw/display/virtio-gpu: problems with coloured cursors

[Gerd Hoffmann]

  Hi,

>    The cursor data virtio-gpu is receiving from the guest has
>    had the alpha channel pre-multiplied into the RGB components.

The kernel driver simply passes through whatever it gets from userspace.

Not sure what userspace passes to the kernel, I suspect it is whatever
typical GPUs can use unmodified as cursor sprite.

>    The cursor data virtio-gpu is receiving from the guest appears
>    to be in BGRA8888 format, while GTK/VNC both expect the data to
>    be in RGBA8888 format.

The format used by virtio-gpu is DRM_FORMAT_ARGB8888
(VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM) on little endian guests.  Byteswapped
(DRM_FORMAT_BGRA8888 / VIRTIO_GPU_FORMAT_A8R8G8B8_UNORM) on bigendian
guests.

> This series has patches to virtio-gpu which reverse the RGB
> components and un-multiply the alpha channel.

I'd tend to simply pass the cursor format information to the ui and
leave it to the ui to convert if needed, or just use the data as-is if
possible (like SDL does).  Same approach we are using for the
framebuffer data.

IIRC the cursor code predates the adoption of pixman in qemu, maybe it
makes sense to redesign this around pixman images.

> I'm unclear whether its reference to "ARGB" here implies that
> seeing BGRA8888 data is intentional, or a guest kernel bug ?

Intentional.

> The spec says nothing at all about alpha pre-multiplication.
> I kind of think this is more likely to be a guest kernel bug,
> but its possible the spec just forgot to mention this ?

Not sure, missing in the spec I'd guess.

> Meanwhile I've absolutely no clue what impact endianness will
> have on this mess. All my testing thus far has been x86_64
> host (QEMU git HEAD) with x86_64 guest (Fedora 41).

Looking at virtio_gpu_update_cursor_data() my guess would be it's
broken (or working by pure luck and bug compatibility).  The
function goes lookup the resource containing the cursor data,
checks size, but doesn't even look at the format.

HTH & take care,
  Gerd

Re: [RFC 0/6] RFC: hw/display/virtio-gpu: problems with coloured cursors

[Daniel P. Berrangé]

On Fri, Jan 24, 2025 at 11:00:33AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> >    The cursor data virtio-gpu is receiving from the guest has
> >    had the alpha channel pre-multiplied into the RGB components.
> 
> The kernel driver simply passes through whatever it gets from userspace.
> 
> Not sure what userspace passes to the kernel, I suspect it is whatever
> typical GPUs can use unmodified as cursor sprite.
> 
> >    The cursor data virtio-gpu is receiving from the guest appears
> >    to be in BGRA8888 format, while GTK/VNC both expect the data to
> >    be in RGBA8888 format.
> 
> The format used by virtio-gpu is DRM_FORMAT_ARGB8888
> (VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM) on little endian guests.  Byteswapped
> (DRM_FORMAT_BGRA8888 / VIRTIO_GPU_FORMAT_A8R8G8B8_UNORM) on bigendian
> guests.
> 
> > This series has patches to virtio-gpu which reverse the RGB
> > components and un-multiply the alpha channel.
> 
> I'd tend to simply pass the cursor format information to the ui and
> leave it to the ui to convert if needed, or just use the data as-is if
> possible (like SDL does).  Same approach we are using for the
> framebuffer data.
> 
> IIRC the cursor code predates the adoption of pixman in qemu, maybe it
> makes sense to redesign this around pixman images.

Yeah that all makes sense, but trying it I hit something odd. The
virtio_gpu_resource_create_2d *always* seems to report the format
as VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM rather than
VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM for the cursor resources.

IOW, the guest is incorrectly telling us there's no alpha channel
data, so if I honour the guest format, the cursor is filled in
black wherever there is supposed to be 100% alpha :-(

I can see a place in linux.git virtiogpu_gem.c that has harcoded
the DRM_FORMAT_HOST_XRGB8888 format for resources, which I guess
is where its going wrong :-(

I could try to workaround this in QEMU, and blindly assume that
guests always intend to have an alpha channel with cursors, as
it makes no sense to not do so.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|