[Qemu-devel] gtk UI doesn't correctly byte swap 32-bit framebuffer on qemu-system-ppc little-endian host

[Qemu-devel] gtk UI doesn't correctly byte swap 32-bit framebuffer on qemu-system-ppc little-endian host

[Mark Cave-Ayland]

Hi all,

I've just been testing some more OpenBIOS images with the new gtk UI and 
found that if you specify a 32-bit depth framebuffer on qemu-system-ppc 
running on a little-endian host then the RGB -> BGR byteswap doesn't 
take place.

Good:
./qemu-system-ppc -g 1024x768x32 -vnc :1
./qemu-system-ppc -g 1024x768x32 -sdl

Bad:
./qemu-system-ppc -g 1024x768x32

I've quickly confirmed this with git bisect which simply returns:

commit 15546425c5527ebb08ede399373b705866f1ff84
Author: Anthony Liguori <aliguori@us.ibm.com>
Date:   Wed Feb 20 07:43:25 2013 -0600

     gtk: make default UI (v5)


Many thanks,

Mark.

Re: [Qemu-devel] gtk UI doesn't correctly byte swap 32-bit framebuffer on qemu-system-ppc little-endian host

[Anthony Liguori]

Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:

> Hi all,
>
> I've just been testing some more OpenBIOS images with the new gtk UI and 
> found that if you specify a 32-bit depth framebuffer on qemu-system-ppc 
> running on a little-endian host then the RGB -> BGR byteswap doesn't 
> take place.
>
> Good:
> ./qemu-system-ppc -g 1024x768x32 -vnc :1
> ./qemu-system-ppc -g 1024x768x32 -sdl
>
> Bad:
> ./qemu-system-ppc -g 1024x768x32

cairo has a pretty limited number of modes that it supports.

I guess we could use pixman to do the conversion.  Gerd, is that the
right approach?  Is there a way I can force the display API to do the
conversion for me?

Regards,

Anthony Liguori

>
> I've quickly confirmed this with git bisect which simply returns:
>
> commit 15546425c5527ebb08ede399373b705866f1ff84
> Author: Anthony Liguori <aliguori@us.ibm.com>
> Date:   Wed Feb 20 07:43:25 2013 -0600
>
>      gtk: make default UI (v5)
>
>
> Many thanks,
>
> Mark.

Re: [Qemu-devel] gtk UI doesn't correctly byte swap 32-bit framebuffer on qemu-system-ppc little-endian host

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]

On 06/04/13 01:19, Anthony Liguori wrote:
> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:
> 
>> Hi all,
>>
>> I've just been testing some more OpenBIOS images with the new gtk UI and 
>> found that if you specify a 32-bit depth framebuffer on qemu-system-ppc 
>> running on a little-endian host then the RGB -> BGR byteswap doesn't 
>> take place.
>>
>> Good:
>> ./qemu-system-ppc -g 1024x768x32 -vnc :1
>> ./qemu-system-ppc -g 1024x768x32 -sdl
>>
>> Bad:
>> ./qemu-system-ppc -g 1024x768x32
> 
> cairo has a pretty limited number of modes that it supports.
> 
> I guess we could use pixman to do the conversion.  Gerd, is that the
> right approach?

Hmm, looks like we can't pass pixman format values to cairo, even though
cairo uses pixman under the hood.  So, yes, we must convert and using
pixman is the easiest way to get the job done.

> Is there a way I can force the display API to do the
> conversion for me?

No, but asking pixman to do it is easy, see attached patch.

cheers,
  Gerd



[-- Attachment #2: 0001-gtk-add-support-for-surface-conversion.patch --]
[-- Type: text/plain, Size: 4426 bytes --]

>From 488a5fa74c6112364885c4ab4c369cb1ed096c64 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Tue, 4 Jun 2013 09:36:52 +0200
Subject: [PATCH] gtk: add support for surface conversion

Also use CAIRO_FORMAT_RGB24 unconditionally.  DisplaySurfaces will never
ever see 8bpp surfaces.  And using CAIRO_FORMAT_RGB16_565 for the 16bpp
case doesn't seem to be a good idea too.

<quote src="/usr/include/cairo/cairo.h">
 * @CAIRO_FORMAT_RGB16_565: This format value is deprecated. It has
 *   never been properly implemented in cairo and should not be used
 *   by applications. (since 1.2)
</quote>

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/gtk.c |   63 +++++++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 40 insertions(+), 23 deletions(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index 52c3f95..40adb05 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -147,6 +147,7 @@ typedef struct GtkDisplayState
     GtkWidget *notebook;
     GtkWidget *drawing_area;
     cairo_surface_t *surface;
+    pixman_image_t *convert;
     DisplayChangeListener dcl;
     DisplaySurface *ds;
     int button_mask;
@@ -303,6 +304,11 @@ static void gd_update(DisplayChangeListener *dcl,
 
     DPRINTF("update(x=%d, y=%d, w=%d, h=%d)\n", x, y, w, h);
 
+    if (s->convert) {
+        pixman_image_composite(PIXMAN_OP_SRC, s->ds->image, NULL, s->convert,
+                               x, y, 0, 0, x, y, w, h);
+    }
+
     x1 = floor(x * s->scale_x);
     y1 = floor(y * s->scale_y);
 
@@ -384,9 +390,7 @@ static void gd_switch(DisplayChangeListener *dcl,
                       DisplaySurface *surface)
 {
     GtkDisplayState *s = container_of(dcl, GtkDisplayState, dcl);
-    cairo_format_t kind;
     bool resized = true;
-    int stride;
 
     DPRINTF("resize(width=%d, height=%d)\n",
             surface_width(surface), surface_height(surface));
@@ -401,29 +405,42 @@ static void gd_switch(DisplayChangeListener *dcl,
         resized = false;
     }
     s->ds = surface;
-    switch (surface_bits_per_pixel(surface)) {
-    case 8:
-        kind = CAIRO_FORMAT_A8;
-        break;
-    case 16:
-        kind = CAIRO_FORMAT_RGB16_565;
-        break;
-    case 32:
-        kind = CAIRO_FORMAT_RGB24;
-        break;
-    default:
-        g_assert_not_reached();
-        break;
-    }
 
-    stride = cairo_format_stride_for_width(kind, surface_width(surface));
-    g_assert(surface_stride(surface) == stride);
+    if (s->convert) {
+        pixman_image_unref(s->convert);
+        s->convert = NULL;
+    }
 
-    s->surface = cairo_image_surface_create_for_data(surface_data(surface),
-                                                     kind,
-                                                     surface_width(surface),
-                                                     surface_height(surface),
-                                                     surface_stride(surface));
+    if (surface->format == PIXMAN_x8r8g8b8) {
+        /*
+         * PIXMAN_x8r8g8b8 == CAIRO_FORMAT_RGB24
+         *
+         * No need to convert, use surface directly.  Should be the
+         * common case as this is qemu_default_pixelformat(32) too.
+         */
+        s->surface = cairo_image_surface_create_for_data
+            (surface_data(surface),
+             CAIRO_FORMAT_RGB24,
+             surface_width(surface),
+             surface_height(surface),
+             surface_stride(surface));
+    } else {
+        /* Must convert surface, use pixman to do it. */
+        s->convert = pixman_image_create_bits(PIXMAN_x8r8g8b8,
+                                              surface_width(surface),
+                                              surface_height(surface),
+                                              NULL, 0);
+        s->surface = cairo_image_surface_create_for_data
+            ((void *)pixman_image_get_data(s->convert),
+             CAIRO_FORMAT_RGB24,
+             pixman_image_get_width(s->convert),
+             pixman_image_get_height(s->convert),
+             pixman_image_get_stride(s->convert));
+        pixman_image_composite(PIXMAN_OP_SRC, s->ds->image, NULL, s->convert,
+                               0, 0, 0, 0, 0, 0,
+                               pixman_image_get_width(s->convert),
+                               pixman_image_get_height(s->convert));
+    }
 
     if (resized) {
         gd_update_windowsize(s);
-- 
1.7.9.7

Re: [Qemu-devel] gtk UI doesn't correctly byte swap 32-bit framebuffer on qemu-system-ppc little-endian host

[Mark Cave-Ayland]

On 04/06/13 08:50, Gerd Hoffmann wrote:

> On 06/04/13 01:19, Anthony Liguori wrote:
>> Mark Cave-Ayland<mark.cave-ayland@ilande.co.uk>  writes:
>>
>>> Hi all,
>>>
>>> I've just been testing some more OpenBIOS images with the new gtk UI and
>>> found that if you specify a 32-bit depth framebuffer on qemu-system-ppc
>>> running on a little-endian host then the RGB ->  BGR byteswap doesn't
>>> take place.
>>>
>>> Good:
>>> ./qemu-system-ppc -g 1024x768x32 -vnc :1
>>> ./qemu-system-ppc -g 1024x768x32 -sdl
>>>
>>> Bad:
>>> ./qemu-system-ppc -g 1024x768x32
>>
>> cairo has a pretty limited number of modes that it supports.
>>
>> I guess we could use pixman to do the conversion.  Gerd, is that the
>> right approach?
>
> Hmm, looks like we can't pass pixman format values to cairo, even though
> cairo uses pixman under the hood.  So, yes, we must convert and using
> pixman is the easiest way to get the job done.
>
>> Is there a way I can force the display API to do the
>> conversion for me?
>
> No, but asking pixman to do it is easy, see attached patch.

Hi Gerd,

A quick test of the patch shows that it solves the problem here - thanks 
a lot!


ATB,

Mark.