[Qemu-devel] [PATCH 0/4] console: more pixman conversion work

[Qemu-devel] [PATCH 0/4] console: more pixman conversion work

[Gerd Hoffmann]

  Hi,

This patch series makes the console core code stop using the qemu
PixelFormat.  We use pixman format codes everywhere instead.

It is a preparation for making use of pixman in display device
emulation.  qemu behavior should not change with this patch series.

please review and test,
  Gerd

Gerd Hoffmann (4):
  console: add qemu_pixelformat_from_pixman
  console: add qemu_default_pixman_format
  console: reimplement qemu_default_pixelformat
  console: stop using PixelFormat

 hw/display/qxl-render.c  |   7 ++-
 hw/display/vga.c         |  12 ++--
 hw/display/vmware_vga.c  |   6 +-
 include/ui/console.h     |  14 ++---
 include/ui/qemu-pixman.h |   2 +
 trace-events             |   2 +-
 ui/console.c             | 150 +++++------------------------------------------
 ui/qemu-pixman.c         |  80 +++++++++++++++++++++++++
 ui/sdl.c                 |   5 +-
 9 files changed, 123 insertions(+), 155 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/4] console: add qemu_pixelformat_from_pixman

[Gerd Hoffmann]

Function to convert pixman format to qemu PixelFormat.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/ui/qemu-pixman.h |  1 +
 ui/qemu-pixman.c         | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/include/ui/qemu-pixman.h b/include/ui/qemu-pixman.h
index ba970f8..090a3e2 100644
--- a/include/ui/qemu-pixman.h
+++ b/include/ui/qemu-pixman.h
@@ -33,6 +33,7 @@
 
 /* -------------------------------------------------------------------- */
 
+PixelFormat qemu_pixelformat_from_pixman(pixman_format_code_t format);
 int qemu_pixman_get_type(int rshift, int gshift, int bshift);
 pixman_format_code_t qemu_pixman_get_format(PixelFormat *pf);
 
diff --git a/ui/qemu-pixman.c b/ui/qemu-pixman.c
index 254bd8c..bcddc04 100644
--- a/ui/qemu-pixman.c
+++ b/ui/qemu-pixman.c
@@ -6,6 +6,61 @@
 #include "qemu-common.h"
 #include "ui/console.h"
 
+PixelFormat qemu_pixelformat_from_pixman(pixman_format_code_t format)
+{
+    PixelFormat pf;
+
+    pf.bits_per_pixel = PIXMAN_FORMAT_BPP(format);
+    pf.bytes_per_pixel = PIXMAN_FORMAT_BPP(format) / 8;
+    pf.depth = PIXMAN_FORMAT_DEPTH(format);
+
+    pf.abits = PIXMAN_FORMAT_A(format);
+    pf.rbits = PIXMAN_FORMAT_R(format);
+    pf.gbits = PIXMAN_FORMAT_G(format);
+    pf.bbits = PIXMAN_FORMAT_B(format);
+
+    switch (PIXMAN_FORMAT_TYPE(format)) {
+    case PIXMAN_TYPE_ARGB:
+        pf.ashift = pf.bbits + pf.gbits + pf.rbits;
+        pf.rshift = pf.bbits + pf.gbits;
+        pf.gshift = pf.bbits;
+        pf.bshift = 0;
+        break;
+    case PIXMAN_TYPE_ABGR:
+        pf.ashift = pf.rbits + pf.gbits + pf.bbits;
+        pf.bshift = pf.rbits + pf.gbits;
+        pf.gshift = pf.rbits;
+        pf.rshift = 0;
+        break;
+    case PIXMAN_TYPE_BGRA:
+        pf.bshift = pf.abits + pf.rbits + pf.gbits;
+        pf.gshift = pf.abits + pf.rbits;
+        pf.rshift = pf.abits;
+        pf.ashift = 0;
+        break;
+    case PIXMAN_TYPE_RGBA:
+        pf.rshift = pf.abits + pf.bbits + pf.gbits;
+        pf.gshift = pf.abits + pf.bbits;
+        pf.bshift = pf.abits;
+        pf.ashift = 0;
+        break;
+    default:
+        g_assert_not_reached();
+        break;
+    }
+
+    pf.amax = (1 << pf.abits) - 1;
+    pf.rmax = (1 << pf.abits) - 1;
+    pf.gmax = (1 << pf.gbits) - 1;
+    pf.bmax = (1 << pf.bbits) - 1;
+    pf.amask = pf.amax << pf.ashift;
+    pf.rmask = pf.rmax << pf.rshift;
+    pf.gmask = pf.gmax << pf.gshift;
+    pf.bmask = pf.bmax << pf.bshift;
+
+    return pf;
+}
+
 int qemu_pixman_get_type(int rshift, int gshift, int bshift)
 {
     int type = PIXMAN_TYPE_OTHER;
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/4] console: add qemu_default_pixman_format

[Gerd Hoffmann]

Function returning the default pixman format for a given depth.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/ui/qemu-pixman.h |  1 +
 ui/qemu-pixman.c         | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/include/ui/qemu-pixman.h b/include/ui/qemu-pixman.h
index 090a3e2..80ed94a 100644
--- a/include/ui/qemu-pixman.h
+++ b/include/ui/qemu-pixman.h
@@ -34,6 +34,7 @@
 /* -------------------------------------------------------------------- */
 
 PixelFormat qemu_pixelformat_from_pixman(pixman_format_code_t format);
+pixman_format_code_t qemu_default_pixman_format(int bpp, bool native_endian);
 int qemu_pixman_get_type(int rshift, int gshift, int bshift);
 pixman_format_code_t qemu_pixman_get_format(PixelFormat *pf);
 
diff --git a/ui/qemu-pixman.c b/ui/qemu-pixman.c
index bcddc04..20af365 100644
--- a/ui/qemu-pixman.c
+++ b/ui/qemu-pixman.c
@@ -61,6 +61,31 @@ PixelFormat qemu_pixelformat_from_pixman(pixman_format_code_t format)
     return pf;
 }
 
+pixman_format_code_t qemu_default_pixman_format(int bpp, bool native_endian)
+{
+    if (native_endian) {
+        switch (bpp) {
+        case 15:
+            return PIXMAN_x1r5g5b5;
+        case 16:
+            return PIXMAN_r5g6b5;
+        case 24:
+            return PIXMAN_r8g8b8;
+        case 32:
+            return PIXMAN_x8r8g8b8;
+        }
+    } else {
+        switch (bpp) {
+        case 24:
+            return PIXMAN_b8g8r8;
+        case 32:
+            return PIXMAN_b8g8r8a8;
+        break;
+        }
+    }
+    g_assert_not_reached();
+}
+
 int qemu_pixman_get_type(int rshift, int gshift, int bshift)
 {
     int type = PIXMAN_TYPE_OTHER;
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/4] console: reimplement qemu_default_pixelformat

[Gerd Hoffmann]

Use the new qemu_pixelformat_from_pixman and qemu_default_pixman_format
functions to reimplement qemu_default_pixelformat
(qemu_different_endianness_pixelformat too).

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/console.c | 117 ++---------------------------------------------------------
 1 file changed, 4 insertions(+), 113 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index 7dc4c14..a614e0b 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1902,124 +1902,15 @@ DisplayState *qemu_console_displaystate(QemuConsole *console)
 
 PixelFormat qemu_different_endianness_pixelformat(int bpp)
 {
-    PixelFormat pf;
-
-    memset(&pf, 0x00, sizeof(PixelFormat));
-
-    pf.bits_per_pixel = bpp;
-    pf.bytes_per_pixel = DIV_ROUND_UP(bpp, 8);
-    pf.depth = bpp == 32 ? 24 : bpp;
-
-    switch (bpp) {
-        case 24:
-            pf.rmask = 0x000000FF;
-            pf.gmask = 0x0000FF00;
-            pf.bmask = 0x00FF0000;
-            pf.rmax = 255;
-            pf.gmax = 255;
-            pf.bmax = 255;
-            pf.rshift = 0;
-            pf.gshift = 8;
-            pf.bshift = 16;
-            pf.rbits = 8;
-            pf.gbits = 8;
-            pf.bbits = 8;
-            break;
-        case 32:
-            pf.rmask = 0x0000FF00;
-            pf.gmask = 0x00FF0000;
-            pf.bmask = 0xFF000000;
-            pf.amask = 0x00000000;
-            pf.amax = 255;
-            pf.rmax = 255;
-            pf.gmax = 255;
-            pf.bmax = 255;
-            pf.ashift = 0;
-            pf.rshift = 8;
-            pf.gshift = 16;
-            pf.bshift = 24;
-            pf.rbits = 8;
-            pf.gbits = 8;
-            pf.bbits = 8;
-            pf.abits = 8;
-            break;
-        default:
-            break;
-    }
+    pixman_format_code_t fmt = qemu_default_pixman_format(bpp, false);
+    PixelFormat pf = qemu_pixelformat_from_pixman(fmt);
     return pf;
 }
 
 PixelFormat qemu_default_pixelformat(int bpp)
 {
-    PixelFormat pf;
-
-    memset(&pf, 0x00, sizeof(PixelFormat));
-
-    pf.bits_per_pixel = bpp;
-    pf.bytes_per_pixel = DIV_ROUND_UP(bpp, 8);
-    pf.depth = bpp == 32 ? 24 : bpp;
-
-    switch (bpp) {
-        case 15:
-            pf.bits_per_pixel = 16;
-            pf.rmask = 0x00007c00;
-            pf.gmask = 0x000003E0;
-            pf.bmask = 0x0000001F;
-            pf.rmax = 31;
-            pf.gmax = 31;
-            pf.bmax = 31;
-            pf.rshift = 10;
-            pf.gshift = 5;
-            pf.bshift = 0;
-            pf.rbits = 5;
-            pf.gbits = 5;
-            pf.bbits = 5;
-            break;
-        case 16:
-            pf.rmask = 0x0000F800;
-            pf.gmask = 0x000007E0;
-            pf.bmask = 0x0000001F;
-            pf.rmax = 31;
-            pf.gmax = 63;
-            pf.bmax = 31;
-            pf.rshift = 11;
-            pf.gshift = 5;
-            pf.bshift = 0;
-            pf.rbits = 5;
-            pf.gbits = 6;
-            pf.bbits = 5;
-            break;
-        case 24:
-            pf.rmask = 0x00FF0000;
-            pf.gmask = 0x0000FF00;
-            pf.bmask = 0x000000FF;
-            pf.rmax = 255;
-            pf.gmax = 255;
-            pf.bmax = 255;
-            pf.rshift = 16;
-            pf.gshift = 8;
-            pf.bshift = 0;
-            pf.rbits = 8;
-            pf.gbits = 8;
-            pf.bbits = 8;
-            break;
-        case 32:
-            pf.rmask = 0x00FF0000;
-            pf.gmask = 0x0000FF00;
-            pf.bmask = 0x000000FF;
-            pf.rmax = 255;
-            pf.gmax = 255;
-            pf.bmax = 255;
-            pf.rshift = 16;
-            pf.gshift = 8;
-            pf.bshift = 0;
-            pf.rbits = 8;
-            pf.gbits = 8;
-            pf.bbits = 8;
-            break;
-        default:
-            break;
-    }
+    pixman_format_code_t fmt = qemu_default_pixman_format(bpp, true);
+    PixelFormat pf = qemu_pixelformat_from_pixman(fmt);
     return pf;
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/4] console: stop using PixelFormat

[Gerd Hoffmann]

With this patch the qemu console core stops using PixelFormat and pixman
format codes side-by-side, pixman format code is the primary way to
specify the DisplaySurface format:

 * DisplaySurface stops carrying a PixelFormat field.
 * qemu_create_displaysurface_from() expects a pixman format now.

Functions to convert PixelFormat to pixman_format_code_t (and back)
exist for those who still use PixelFormat.   As PixelFormat allows
easy access to masks and shifts it will probably continue to exist.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/qxl-render.c |  7 ++++---
 hw/display/vga.c        | 12 ++++++++----
 hw/display/vmware_vga.c |  6 ++++--
 include/ui/console.h    | 14 +++++++-------
 trace-events            |  2 +-
 ui/console.c            | 33 ++++++++++-----------------------
 ui/sdl.c                |  5 +++--
 7 files changed, 37 insertions(+), 42 deletions(-)

diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
index cc2c2b1..d79f5a3 100644
--- a/hw/display/qxl-render.c
+++ b/hw/display/qxl-render.c
@@ -116,13 +116,14 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice *qxl)
                qxl->guest_primary.bytes_pp,
                qxl->guest_primary.bits_pp);
         if (qxl->guest_primary.qxl_stride > 0) {
+            pixman_format_code_t format =
+                qemu_default_pixman_format(qxl->guest_primary.bits_pp, true);
             surface = qemu_create_displaysurface_from
                 (qxl->guest_primary.surface.width,
                  qxl->guest_primary.surface.height,
-                 qxl->guest_primary.bits_pp,
+                 format,
                  qxl->guest_primary.abs_stride,
-                 qxl->guest_primary.data,
-                 false);
+                 qxl->guest_primary.data);
         } else {
             surface = qemu_create_displaysurface
                 (qxl->guest_primary.surface.width,
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 4b089a3..ff49f57 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1693,9 +1693,11 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
         height != s->last_height ||
         s->last_depth != depth) {
         if (depth == 32 || (depth == 16 && !byteswap)) {
+            pixman_format_code_t format =
+                qemu_default_pixman_format(depth, !byteswap);
             surface = qemu_create_displaysurface_from(disp_width,
-                    height, depth, s->line_offset,
-                    s->vram_ptr + (s->start_addr * 4), byteswap);
+                    height, format, s->line_offset,
+                    s->vram_ptr + (s->start_addr * 4));
             dpy_gfx_replace_surface(s->con, surface);
         } else {
             qemu_console_resize(s->con, disp_width, height);
@@ -1711,9 +1713,11 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
     } else if (is_buffer_shared(surface) &&
                (full_update || surface_data(surface) != s->vram_ptr
                 + (s->start_addr * 4))) {
+        pixman_format_code_t format =
+            qemu_default_pixman_format(depth, !byteswap);
         surface = qemu_create_displaysurface_from(disp_width,
-                height, depth, s->line_offset,
-                s->vram_ptr + (s->start_addr * 4), byteswap);
+                height, format, s->line_offset,
+                s->vram_ptr + (s->start_addr * 4));
         dpy_gfx_replace_surface(s->con, surface);
     }
 
diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 591b645..b8901d0 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -1052,10 +1052,12 @@ static inline void vmsvga_check_size(struct vmsvga_state_s *s)
         s->new_height != surface_height(surface) ||
         s->new_depth != surface_bits_per_pixel(surface)) {
         int stride = (s->new_depth * s->new_width) / 8;
+        pixman_format_code_t format =
+            qemu_default_pixman_format(s->new_depth, true);
         trace_vmware_setmode(s->new_width, s->new_height, s->new_depth);
         surface = qemu_create_displaysurface_from(s->new_width, s->new_height,
-                                                  s->new_depth, stride,
-                                                  s->vga.vram_ptr, false);
+                                                  format, stride,
+                                                  s->vga.vram_ptr);
         dpy_gfx_replace_surface(s->vga.con, surface);
         s->invalidated = 1;
     }
diff --git a/include/ui/console.h b/include/ui/console.h
index edbaa9b..0a59b8d 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -119,8 +119,6 @@ struct DisplaySurface {
     pixman_format_code_t format;
     pixman_image_t *image;
     uint8_t flags;
-
-    struct PixelFormat pf;
 };
 
 typedef struct QemuUIInfo {
@@ -188,9 +186,9 @@ struct DisplayChangeListener {
 };
 
 DisplayState *init_displaystate(void);
-DisplaySurface* qemu_create_displaysurface_from(int width, int height, int bpp,
-                                                int linesize, uint8_t *data,
-                                                bool byteswap);
+DisplaySurface *qemu_create_displaysurface_from(int width, int height,
+                                                pixman_format_code_t format,
+                                                int linesize, uint8_t *data);
 PixelFormat qemu_different_endianness_pixelformat(int bpp);
 PixelFormat qemu_default_pixelformat(int bpp);
 
@@ -199,10 +197,12 @@ void qemu_free_displaysurface(DisplaySurface *surface);
 
 static inline int is_surface_bgr(DisplaySurface *surface)
 {
-    if (surface->pf.bits_per_pixel == 32 && surface->pf.rshift == 0)
+    if (PIXMAN_FORMAT_BPP(surface->format) == 32 &&
+        PIXMAN_FORMAT_TYPE(surface->format) == PIXMAN_TYPE_ABGR) {
         return 1;
-    else
+    } else {
         return 0;
+    }
 }
 
 static inline int is_buffer_shared(DisplaySurface *surface)
diff --git a/trace-events b/trace-events
index f8dff48..a4411bc 100644
--- a/trace-events
+++ b/trace-events
@@ -1035,7 +1035,7 @@ console_txt_new(int w, int h) "%dx%d"
 console_select(int nr) "%d"
 console_refresh(int interval) "interval %d ms"
 displaysurface_create(void *display_surface, int w, int h) "surface=%p, %dx%d"
-displaysurface_create_from(void *display_surface, int w, int h, int bpp, int swap) "surface=%p, %dx%d, bpp %d, bswap %d"
+displaysurface_create_from(void *display_surface, int w, int h, uint32_t format) "surface=%p, %dx%d, format 0x%x"
 displaysurface_free(void *display_surface) "surface=%p"
 displaychangelistener_register(void *dcl, const char *name) "%p [ %s ]"
 displaychangelistener_unregister(void *dcl, const char *name) "%p [ %s ]"
diff --git a/ui/console.c b/ui/console.c
index a614e0b..bb304fc 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1224,22 +1224,18 @@ static QemuConsole *new_console(DisplayState *ds, console_type_t console_type,
     return s;
 }
 
-static void qemu_alloc_display(DisplaySurface *surface, int width, int height,
-                               int linesize, PixelFormat pf, int newflags)
+static void qemu_alloc_display(DisplaySurface *surface, int width, int height)
 {
-    surface->pf = pf;
-
     qemu_pixman_image_unref(surface->image);
     surface->image = NULL;
 
-    surface->format = qemu_pixman_get_format(&pf);
-    assert(surface->format != 0);
+    surface->format = PIXMAN_x8r8g8b8;
     surface->image = pixman_image_create_bits(surface->format,
                                               width, height,
-                                              NULL, linesize);
+                                              NULL, width * 4);
     assert(surface->image != NULL);
 
-    surface->flags = newflags | QEMU_ALLOCATED_FLAG;
+    surface->flags = QEMU_ALLOCATED_FLAG;
 #ifdef HOST_WORDS_BIGENDIAN
     surface->flags |= QEMU_BIG_ENDIAN_FLAG;
 #endif
@@ -1248,29 +1244,20 @@ static void qemu_alloc_display(DisplaySurface *surface, int width, int height,
 DisplaySurface *qemu_create_displaysurface(int width, int height)
 {
     DisplaySurface *surface = g_new0(DisplaySurface, 1);
-    int linesize = width * 4;
 
     trace_displaysurface_create(surface, width, height);
-    qemu_alloc_display(surface, width, height, linesize,
-                       qemu_default_pixelformat(32), 0);
+    qemu_alloc_display(surface, width, height);
     return surface;
 }
 
-DisplaySurface *qemu_create_displaysurface_from(int width, int height, int bpp,
-                                                int linesize, uint8_t *data,
-                                                bool byteswap)
+DisplaySurface *qemu_create_displaysurface_from(int width, int height,
+                                                pixman_format_code_t format,
+                                                int linesize, uint8_t *data)
 {
     DisplaySurface *surface = g_new0(DisplaySurface, 1);
 
-    trace_displaysurface_create_from(surface, width, height, bpp, byteswap);
-    if (byteswap) {
-        surface->pf = qemu_different_endianness_pixelformat(bpp);
-    } else {
-        surface->pf = qemu_default_pixelformat(bpp);
-    }
-
-    surface->format = qemu_pixman_get_format(&surface->pf);
-    assert(surface->format != 0);
+    trace_displaysurface_create_from(surface, width, height, format);
+    surface->format = format;
     surface->image = pixman_image_create_bits(surface->format,
                                               width, height,
                                               (void *)data, linesize);
diff --git a/ui/sdl.c b/ui/sdl.c
index 4e7f920..94c1d9d 100644
--- a/ui/sdl.c
+++ b/ui/sdl.c
@@ -127,6 +127,7 @@ static void do_sdl_resize(int width, int height, int bpp)
 static void sdl_switch(DisplayChangeListener *dcl,
                        DisplaySurface *new_surface)
 {
+    PixelFormat pf = qemu_pixelformat_from_pixman(new_surface->format);
 
     /* temporary hack: allows to call sdl_switch to handle scaling changes */
     if (new_surface) {
@@ -148,8 +149,8 @@ static void sdl_switch(DisplayChangeListener *dcl,
         (surface_data(surface),
          surface_width(surface), surface_height(surface),
          surface_bits_per_pixel(surface), surface_stride(surface),
-         surface->pf.rmask, surface->pf.gmask,
-         surface->pf.bmask, surface->pf.amask);
+         pf.rmask, pf.gmask,
+         pf.bmask, pf.amask);
 }
 
 /* generic keyboard conversion */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 4/4] console: stop using PixelFormat

[Benjamin Herrenschmidt]

On Wed, 2014-06-18 at 14:23 +0200, Gerd Hoffmann wrote:
> With this patch the qemu console core stops using PixelFormat and pixman
> format codes side-by-side, pixman format code is the primary way to
> specify the DisplaySurface format:
> 
>  * DisplaySurface stops carrying a PixelFormat field.
>  * qemu_create_displaysurface_from() expects a pixman format now.
> 
> Functions to convert PixelFormat to pixman_format_code_t (and back)
> exist for those who still use PixelFormat.   As PixelFormat allows
> easy access to masks and shifts it will probably continue to exist.

Finally got to start playing with that stuff (got busy with other things
for a while) and untangling vga.c (remove whole chunks of it actually
since we know the target is always 32bpp when we need to convert or
"draw" so it's a lot easier).

While at it, I noticed this patch is missing this, feel free to fold in
no need to add an ack or anything, it's trivial:

diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index 032eb7a..f9cdd96 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -711,15 +711,17 @@ static void xenfb_update(void *opaque)
 
     /* resize if needed */
     if (xenfb->do_resize) {
+        pixman_format_code_t format;
+
         xenfb->do_resize = 0;
         switch (xenfb->depth) {
         case 16:
         case 32:
             /* console.c supported depth -> buffer can be used directly */
+            format = qemu_default_pixman_format(xenfb->depth, true);
             surface = qemu_create_displaysurface_from
-                (xenfb->width, xenfb->height, xenfb->depth,
-                 xenfb->row_stride, xenfb->pixels + xenfb->offset,
-                 false);
+                (xenfb->width, xenfb->height, format,
+                 xenfb->row_stride, xenfb->pixels + xenfb->offset);
             break;
         default:
             /* we must convert stuff */