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

[Gerd Hoffmann <kraxel@redhat.com>]

[-- 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