Qemu vnc color depth

Qemu vnc color depth

[Stefano Stabellini]

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

Hi all,

the qemu vnc server changes its internal colour depth based on the 
client request. This way just one colour conversion is done: the one in 
vga_template.h, from the guest colour depth and the vnc server internal 
colour depth.

This patch is meant to remove this colour conversion to improve 
performances. It accomplishes the goal making the qemu internal colour 
depth always the same as the guest colour depth.
The basic idea is that the vnc client is the one that should do the 
colour conversion, if necessary. In general it should accept the pixel 
format suggested by the server during the initial negotiation. This 
behaviour can be set in most vnc clients (vncviewer included).

If the guest changes colour depth, the qemu vnc server changes colour 
depth too and notifies the client. The problem is that the vnc protocol 
doesn't provide a message from the server to the client to ask for a 
colour depth change. So what I am doing is either:

1) quietly starting to do the conversion on vnc server (not gaining any 
performances here);

2) closing the vnc connection with the client, so the client can 
reconnect and choose the new pixel format.

By default I am doing 1), however the second choice can be enabled 
passing the -vnc-switch-bpp command line option.
In order to do the colour conversion on the vnc server I had to improve 
the colour conversion code already in place because it only supported 
conversions from 32 bpp. The patch adds colour conversion code that 
support conversions from any resolution to any resolution.

A last note: to get most out of this patch it is best to set Windows to 
16 bit colour depth, because the 24 bit mode is 24 bit depth and 24 bpp, 
meaning no alpha channel. The vnc protocol doesn't support 24 bpp, only 
32 bpp, so this conversion is unavoidable.

Comments and critics are welcome.

Best Regards,

Stefano Stabellini

[-- Attachment #2: colour-conversion.patch --]
[-- Type: text/x-patch, Size: 19857 bytes --]

diff -r e6cf98edf0c5 tools/ioemu/console.c
--- a/tools/ioemu/console.c	Thu Feb 07 13:21:38 2008 +0000
+++ b/tools/ioemu/console.c	Fri Feb 08 16:19:49 2008 +0000
@@ -169,16 +169,12 @@ static unsigned int vga_get_color(Displa
     unsigned int r, g, b, color;
 
     switch(ds->depth) {
-#if 0
     case 8:
         r = (rgba >> 16) & 0xff;
         g = (rgba >> 8) & 0xff;
         b = (rgba) & 0xff;
-        color = (rgb_to_index[r] * 6 * 6) + 
-            (rgb_to_index[g] * 6) + 
-            (rgb_to_index[b]);
-        break;
-#endif
+        color = ((r >> 5) << 5 | (g >> 5) << 2 | (b >> 6));
+        break;
     case 15:
         r = (rgba >> 16) & 0xff;
         g = (rgba >> 8) & 0xff;
diff -r e6cf98edf0c5 tools/ioemu/hw/vga.c
--- a/tools/ioemu/hw/vga.c	Thu Feb 07 13:21:38 2008 +0000
+++ b/tools/ioemu/hw/vga.c	Fri Feb 08 16:33:03 2008 +0000
@@ -1071,7 +1071,7 @@ static const uint8_t cursor_glyph[32 * 4
  */
 static void vga_draw_text(VGAState *s, int full_update)
 {
-    int cx, cy, cheight, cw, ch, cattr, height, width, ch_attr;
+    int cx, cy, cheight, cw, ch, cattr, height, width, ch_attr, depth;
     int cx_min, cx_max, linesize, x_incr;
     uint32_t offset, fgcol, bgcol, v, cursor_offset;
     uint8_t *d1, *d, *src, *s1, *dest, *cursor_ptr;
@@ -1134,6 +1134,11 @@ static void vga_draw_text(VGAState *s, i
         return;
     }
 
+    depth = s->get_bpp(s);
+    if (depth == 24)
+        depth = 32;
+    if (s->ds->dpy_colourdepth != NULL && s->ds->depth != depth)
+        s->ds->dpy_colourdepth(s->ds, depth);
     if (width != s->last_width || height != s->last_height ||
         cw != s->last_cw || cheight != s->last_ch) {
         s->last_scr_width = width * cw;
@@ -1477,7 +1482,7 @@ void check_sse2(void)
  */
 static void vga_draw_graphic(VGAState *s, int full_update)
 {
-    int y1, y, update, linesize, y_start, double_scan, mask;
+    int y1, y, update, linesize, y_start, double_scan, mask, depth;
     int width, height, shift_control, line_offset, bwidth;
     ram_addr_t page0, page1;
     int disp_width, multi_scan, multi_run;
@@ -1551,6 +1556,11 @@ static void vga_draw_graphic(VGAState *s
     }
     vga_draw_line = vga_draw_line_table[v * NB_DEPTHS + get_depth_index(s->ds)];
 
+    depth = s->get_bpp(s);
+    if (depth == 24)
+        depth = 32;
+    if (s->ds->dpy_colourdepth != NULL && s->ds->depth != depth)
+        s->ds->dpy_colourdepth(s->ds, depth);
     if (disp_width != s->last_width ||
         height != s->last_height) {
         dpy_resize(s->ds, disp_width, height);
diff -r e6cf98edf0c5 tools/ioemu/sdl.c
--- a/tools/ioemu/sdl.c	Thu Feb 07 13:21:38 2008 +0000
+++ b/tools/ioemu/sdl.c	Sat Feb 09 17:25:14 2008 +0000
@@ -499,6 +499,7 @@ void sdl_display_init(DisplayState *ds, 
     ds->dpy_update = sdl_update;
     ds->dpy_resize = sdl_resize;
     ds->dpy_refresh = sdl_refresh;
+    ds->dpy_colourdepth = NULL;
 
     sdl_resize(ds, 640, 400);
     sdl_update_caption();
diff -r e6cf98edf0c5 tools/ioemu/vl.c
--- a/tools/ioemu/vl.c	Thu Feb 07 13:21:38 2008 +0000
+++ b/tools/ioemu/vl.c	Sat Feb 09 17:10:51 2008 +0000
@@ -146,6 +146,7 @@ int nographic;
 int nographic;
 int vncviewer;
 int vncunused;
+int vncswitchbpp;
 const char* keyboard_layout = NULL;
 int64_t ticks_per_sec;
 char *boot_device = NULL;
@@ -4397,6 +4398,7 @@ void dumb_display_init(DisplayState *ds)
     ds->depth = 0;
     ds->dpy_update = dumb_update;
     ds->dpy_resize = dumb_resize;
+    ds->dpy_colourdepth = NULL;
     ds->dpy_refresh = dumb_refresh;
 }
 
@@ -6510,6 +6512,7 @@ void help(void)
 	   "-vnc display    start a VNC server on display\n"
            "-vncviewer      start a vncviewer process for this domain\n"
            "-vncunused      bind the VNC server to an unused port\n"
+           "-vnc-switch-bpp VNC server closes connections when the guest OS changes colour depth\n"
 #ifndef _WIN32
 	   "-daemonize      daemonize QEMU after initializing\n"
 #endif
@@ -6609,6 +6612,7 @@ enum {
     QEMU_OPTION_acpi,
     QEMU_OPTION_vncviewer,
     QEMU_OPTION_vncunused,
+    QEMU_OPTION_vncswitchbpp,
     QEMU_OPTION_pci,
 };
 
@@ -6692,6 +6696,7 @@ const QEMUOption qemu_options[] = {
     { "vnc", HAS_ARG, QEMU_OPTION_vnc },
     { "vncviewer", 0, QEMU_OPTION_vncviewer },
     { "vncunused", 0, QEMU_OPTION_vncunused },
+    { "vnc-switch-bpp", 0, QEMU_OPTION_vncswitchbpp },
 
     /* temporary options */
     { "usb", 0, QEMU_OPTION_usb },
@@ -7103,6 +7108,7 @@ int main(int argc, char **argv)
     nographic = 0;
     vncviewer = 0;
     vncunused = 0;
+    vncswitchbpp = 0;
     kernel_filename = NULL;
     kernel_cmdline = "";
 #ifndef CONFIG_DM
@@ -7531,6 +7537,9 @@ int main(int argc, char **argv)
             case QEMU_OPTION_vncunused:
                 vncunused++;
                 break;
+            case QEMU_OPTION_vncswitchbpp:
+                vncswitchbpp++;
+                break;
             case QEMU_OPTION_pci:
                 direct_pci = optarg;
                 break;
@@ -7755,6 +7764,7 @@ int main(int argc, char **argv)
     } else if (vnc_display != NULL || vncunused != 0) {
 	int vnc_display_port;
 	char password[20];
+        ds->switchbpp = vncswitchbpp;
 	vnc_display_init(ds);
 	xenstore_read_vncpasswd(domid, password, sizeof(password));
 	vnc_display_password(ds, password);
diff -r e6cf98edf0c5 tools/ioemu/vl.h
--- a/tools/ioemu/vl.h	Thu Feb 07 13:21:38 2008 +0000
+++ b/tools/ioemu/vl.h	Fri Feb 08 16:54:30 2008 +0000
@@ -912,8 +912,11 @@ struct DisplayState {
     int height;
     void *opaque;
 
+    int switchbpp;
+    
     void (*dpy_update)(struct DisplayState *s, int x, int y, int w, int h);
     void (*dpy_resize)(struct DisplayState *s, int w, int h);
+    void (*dpy_colourdepth)(struct DisplayState *s, int depth);
     void (*dpy_refresh)(struct DisplayState *s);
     void (*dpy_copy)(struct DisplayState *s, int src_x, int src_y, int dst_x, int dst_y, int w, int h);
 };
diff -r e6cf98edf0c5 tools/ioemu/vnc.c
--- a/tools/ioemu/vnc.c	Thu Feb 07 13:21:38 2008 +0000
+++ b/tools/ioemu/vnc.c	Sat Feb 09 18:16:54 2008 +0000
@@ -27,6 +27,7 @@
 #include <sys/stat.h>
 #include <sys/socket.h>
 #include <netinet/in.h>
+#include <arpa/inet.h>
 #include "vl.h"
 #include "qemu_socket.h"
 #include <assert.h>
@@ -85,8 +86,8 @@ typedef void VncWritePixels(VncState *vs
 
 typedef void VncSendHextileTile(VncState *vs,
                                 int x, int y, int w, int h,
-                                uint32_t *last_bg, 
-                                uint32_t *last_fg,
+                                void *last_bg, 
+                                void *last_fg,
                                 int *has_bg, int *has_fg);
 
 #if 0
@@ -187,9 +188,9 @@ struct VncState
     VncWritePixels *write_pixels;
     VncSendHextileTile *send_hextile_tile;
     int pix_bpp, pix_big_endian;
-    int red_shift, red_max, red_shift1;
-    int green_shift, green_max, green_shift1;
-    int blue_shift, blue_max, blue_shift1;
+    int red_shift, red_max, red_shift1, red_max1;
+    int green_shift, green_max, green_shift1, green_max1;
+    int blue_shift, blue_max, blue_shift1, blue_max1;
 
     VncReadEvent *read_handler;
     size_t read_handler_expect;
@@ -379,54 +380,67 @@ static void vnc_write_pixels_copy(VncSta
 /* slowest but generic code. */
 static void vnc_convert_pixel(VncState *vs, uint8_t *buf, uint32_t v)
 {
-    unsigned int r, g, b;
-
-    r = (v >> vs->red_shift1) & vs->red_max;
-    g = (v >> vs->green_shift1) & vs->green_max;
-    b = (v >> vs->blue_shift1) & vs->blue_max;
-    v = (r << vs->red_shift) | 
-        (g << vs->green_shift) | 
-        (b << vs->blue_shift);
+    uint8_t r, g, b;
+
+    r = ((v >> vs->red_shift1) & vs->red_max1) * (vs->red_max + 1) / (vs->red_max1 + 1);
+    g = ((v >> vs->green_shift1) & vs->green_max1) * (vs->green_max + 1) / (vs->green_max1 + 1);
+    b = ((v >> vs->blue_shift1) & vs->blue_max1) * (vs->blue_max + 1) / (vs->blue_max1 + 1);
     switch(vs->pix_bpp) {
     case 1:
-        buf[0] = v;
+        buf[0] = (r << vs->red_shift) | (g << vs->green_shift) | (b << vs->blue_shift);
         break;
     case 2:
+    {
+        uint16_t *p = (uint16_t *) buf;
+        *p = (r << vs->red_shift) | (g << vs->green_shift) | (b << vs->blue_shift);
         if (vs->pix_big_endian) {
-            buf[0] = v >> 8;
-            buf[1] = v;
-        } else {
-            buf[1] = v >> 8;
-            buf[0] = v;
+            *p = htons(*p);
         }
+    }
         break;
     default:
     case 4:
+    {
+        uint32_t *p = (uint32_t *) buf;
+        *p = (r << vs->red_shift) | (g << vs->green_shift) | (b << vs->blue_shift);
         if (vs->pix_big_endian) {
-            buf[0] = v >> 24;
-            buf[1] = v >> 16;
-            buf[2] = v >> 8;
-            buf[3] = v;
-        } else {
-            buf[3] = v >> 24;
-            buf[2] = v >> 16;
-            buf[1] = v >> 8;
-            buf[0] = v;
+            *p = htonl(*p);
         }
         break;
     }
+    }
 }
 
 static void vnc_write_pixels_generic(VncState *vs, void *pixels1, int size)
 {
-    uint32_t *pixels = pixels1;
     uint8_t buf[4];
-    int n, i;
-
-    n = size >> 2;
-    for(i = 0; i < n; i++) {
-        vnc_convert_pixel(vs, buf, pixels[i]);
-        vnc_write(vs, buf, vs->pix_bpp);
+
+    if (vs->depth == 4) {
+        uint32_t *pixels = pixels1;
+        int n, i;
+        n = size >> 2;
+        for(i = 0; i < n; i++) {
+            vnc_convert_pixel(vs, buf, pixels[i]);
+            vnc_write(vs, buf, vs->pix_bpp);
+        }
+    } else if (vs->depth == 2) {
+        uint16_t *pixels = pixels1;
+        int n, i;
+        n = size >> 1;
+        for(i = 0; i < n; i++) {
+            vnc_convert_pixel(vs, buf, pixels[i]);
+            vnc_write(vs, buf, vs->pix_bpp);
+        }
+    } else if (vs->depth == 1) {
+        uint8_t *pixels = pixels1;
+        int n, i;
+        n = size;
+        for(i = 0; i < n; i++) {
+            vnc_convert_pixel(vs, buf, pixels[i]);
+            vnc_write(vs, buf, vs->pix_bpp);
+        }
+    } else {
+        fprintf(stderr, "vnc_write_pixels_generic: VncState color depth not supported\n");
     }
 }
 
@@ -461,6 +475,18 @@ static void hextile_enc_cord(uint8_t *pt
 #define BPP 32
 #include "vnchextile.h"
 #undef BPP
+
+#define GENERIC
+#define BPP 8
+#include "vnchextile.h"
+#undef BPP
+#undef GENERIC
+
+#define GENERIC
+#define BPP 16
+#include "vnchextile.h"
+#undef BPP
+#undef GENERIC
 
 #define GENERIC
 #define BPP 32
@@ -472,18 +498,22 @@ static void send_framebuffer_update_hext
 {
     int i, j;
     int has_fg, has_bg;
-    uint32_t last_fg32, last_bg32;
+    void *last_fg, *last_bg;
 
     vnc_framebuffer_update(vs, x, y, w, h, 5);
 
+    last_fg = (void *) malloc(vs->depth);
+    last_bg = (void *) malloc(vs->depth);
     has_fg = has_bg = 0;
     for (j = y; j < (y + h); j += 16) {
 	for (i = x; i < (x + w); i += 16) {
             vs->send_hextile_tile(vs, i, j, 
                                   MIN(16, x + w - i), MIN(16, y + h - j),
-                                  &last_bg32, &last_fg32, &has_bg, &has_fg);
-	}
-    }
+                                  last_bg, last_fg, &has_bg, &has_fg);
+	}
+    }
+    free(last_fg);
+    free(last_bg);    
 }
 
 static void send_framebuffer_update(VncState *vs, int x, int y, int w, int h)
@@ -1306,17 +1336,6 @@ static void set_encodings(VncState *vs, 
     check_pointer_type_change(vs, kbd_mouse_is_absolute());
 }
 
-static int compute_nbits(unsigned int val)
-{
-    int n;
-    n = 0;
-    while (val != 0) {
-        n++;
-        val >>= 1;
-    }
-    return n;
-}
-
 static void set_pixel_format(VncState *vs,
 			     int bits_per_pixel, int depth,
 			     int big_endian_flag, int true_color_flag,
@@ -1335,23 +1354,24 @@ static void set_pixel_format(VncState *v
 	vnc_client_error(vs);
         return;
     }
-    if (bits_per_pixel == 32 && 
+    if (bits_per_pixel == 32 &&
+        bits_per_pixel == vs->depth * 8 && 
         host_big_endian_flag == big_endian_flag &&
         red_max == 0xff && green_max == 0xff && blue_max == 0xff &&
         red_shift == 16 && green_shift == 8 && blue_shift == 0) {
-        vs->depth = 4;
         vs->write_pixels = vnc_write_pixels_copy;
         vs->send_hextile_tile = send_hextile_tile_32;
     } else 
-    if (bits_per_pixel == 16 && 
+    if (bits_per_pixel == 16 &&
+        bits_per_pixel == vs->depth * 8 && 
         host_big_endian_flag == big_endian_flag &&
         red_max == 31 && green_max == 63 && blue_max == 31 &&
         red_shift == 11 && green_shift == 5 && blue_shift == 0) {
-        vs->depth = 2;
         vs->write_pixels = vnc_write_pixels_copy;
         vs->send_hextile_tile = send_hextile_tile_16;
     } else 
     if (bits_per_pixel == 8 && 
+        bits_per_pixel == vs->depth * 8 &&
         red_max == 7 && green_max == 7 && blue_max == 3 &&
         red_shift == 5 && green_shift == 2 && blue_shift == 0) {
         vs->depth = 1;
@@ -1364,26 +1384,109 @@ static void set_pixel_format(VncState *v
             bits_per_pixel != 16 &&
             bits_per_pixel != 32)
             goto fail;
-        vs->depth = 4;
-        vs->red_shift = red_shift;
-        vs->red_max = red_max;
-        vs->red_shift1 = 24 - compute_nbits(red_max);
-        vs->green_shift = green_shift;
-        vs->green_max = green_max;
-        vs->green_shift1 = 16 - compute_nbits(green_max);
-        vs->blue_shift = blue_shift;
-        vs->blue_max = blue_max;
-        vs->blue_shift1 = 8 - compute_nbits(blue_max);
-        vs->pix_bpp = bits_per_pixel / 8;
+        if (vs->depth == 4) {
+            vs->send_hextile_tile = send_hextile_tile_generic_32;
+        } else if (vs->depth == 2) {
+            vs->send_hextile_tile = send_hextile_tile_generic_16;
+        } else {
+            vs->send_hextile_tile = send_hextile_tile_generic_8;
+        }
+            
         vs->pix_big_endian = big_endian_flag;
         vs->write_pixels = vnc_write_pixels_generic;
-        vs->send_hextile_tile = send_hextile_tile_generic;
-    }
-
-    vnc_dpy_resize(vs->ds, vs->ds->width, vs->ds->height);
+    }
+ 
+    vs->red_shift = red_shift;
+    vs->red_max = red_max;
+    vs->green_shift = green_shift;
+    vs->green_max = green_max;
+    vs->blue_shift = blue_shift;
+    vs->blue_max = blue_max;
+    vs->pix_bpp = bits_per_pixel / 8;
 
     vga_hw_invalidate();
     vga_hw_update();
+}
+
+static void vnc_dpy_colourdepth(DisplayState *ds, int depth)
+{
+    int host_big_endian_flag;
+    struct VncState *vs;
+    
+    if (!depth) return;
+    
+#ifdef WORDS_BIGENDIAN
+    host_big_endian_flag = 1;
+#else
+    host_big_endian_flag = 0;
+#endif
+    vs = ds->opaque;   
+    
+    switch (depth) {
+        case 8:
+            vs->depth = depth / 8;
+            vs->red_max1 = 7;
+            vs->green_max1 = 7;
+            vs->blue_max1 = 3;
+            vs->red_shift1 = 5;
+            vs->green_shift1 = 2;
+            vs->blue_shift1 = 0;
+            break;
+        case 16:
+            vs->depth = depth / 8;
+            vs->red_max1 = 31;
+            vs->green_max1 = 63;
+            vs->blue_max1 = 31;
+            vs->red_shift1 = 11;
+            vs->green_shift1 = 5;
+            vs->blue_shift1 = 0;
+            break;
+        case 32:
+            vs->depth = 4;
+            vs->red_max1 = 255;
+            vs->green_max1 = 255;
+            vs->blue_max1 = 255;
+            vs->red_shift1 = 16;
+            vs->green_shift1 = 8;
+            vs->blue_shift1 = 0;
+            break;
+        default:
+            return;
+    }
+    if (ds->switchbpp) {
+        vnc_client_error(vs);
+    } else {
+        if (vs->pix_bpp == 4 && vs->depth == 4 &&
+            host_big_endian_flag == vs->pix_big_endian &&
+            vs->red_max == 0xff && vs->green_max == 0xff && vs->blue_max == 0xff &&
+            vs->red_shift == 16 && vs->green_shift == 8 && vs->blue_shift == 0) {
+            vs->write_pixels = vnc_write_pixels_copy;
+            vs->send_hextile_tile = send_hextile_tile_32;
+        } else if (vs->pix_bpp == 2 && vs->depth == 2 &&
+            host_big_endian_flag == vs->pix_big_endian &&
+            vs->red_max == 31 && vs->green_max == 63 && vs->blue_max == 31 &&
+            vs->red_shift == 11 && vs->green_shift == 5 && vs->blue_shift == 0) {
+            vs->write_pixels = vnc_write_pixels_copy;
+            vs->send_hextile_tile = send_hextile_tile_16;
+        } else if (vs->pix_bpp == 1 && vs->depth == 1 &&
+            host_big_endian_flag == vs->pix_big_endian &&
+            vs->red_max == 7 && vs->green_max == 7 && vs->blue_max == 3 &&
+            vs->red_shift == 5 && vs->green_shift == 2 && vs->blue_shift == 0) {
+            vs->write_pixels = vnc_write_pixels_copy;
+            vs->send_hextile_tile = send_hextile_tile_8;
+        } else {
+            if (vs->depth == 4) {
+                vs->send_hextile_tile = send_hextile_tile_generic_32;
+            } else if (vs->depth == 2) {
+                vs->send_hextile_tile = send_hextile_tile_generic_16;
+            } else {
+                vs->send_hextile_tile = send_hextile_tile_generic_8;
+            }
+            vs->write_pixels = vnc_write_pixels_generic;
+        }
+    }
+
+    vnc_dpy_resize(ds, ds->width, ds->height);
 }
 
 static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
@@ -1483,7 +1586,9 @@ static int protocol_client_init(VncState
     vnc_write_u16(vs, vs->ds->height);
 
     vnc_write_u8(vs, vs->depth * 8); /* bits-per-pixel */
-    vnc_write_u8(vs, vs->depth * 8); /* depth */
+    if (vs->depth == 4) vnc_write_u8(vs, 24); /* depth */
+    else vnc_write_u8(vs, vs->depth * 8); /* depth */
+
 #ifdef WORDS_BIGENDIAN
     vnc_write_u8(vs, 1);             /* big-endian-flag */
 #else
@@ -2160,7 +2265,6 @@ void vnc_display_init(DisplayState *ds)
 
     vs->lsock = -1;
     vs->csock = -1;
-    vs->depth = 4;
     vs->last_x = -1;
     vs->last_y = -1;
 
@@ -2177,9 +2281,12 @@ void vnc_display_init(DisplayState *ds)
     vs->ds->data = NULL;
     vs->ds->dpy_update = vnc_dpy_update;
     vs->ds->dpy_resize = vnc_dpy_resize;
+    vs->ds->dpy_colourdepth = vnc_dpy_colourdepth;
     vs->ds->dpy_refresh = vnc_dpy_refresh;
 
-    vnc_dpy_resize(vs->ds, 640, 400);
+    vs->ds->width = 640;
+    vs->ds->height = 400;
+    vnc_dpy_colourdepth(vs->ds, 32);
 }
 
 #if CONFIG_VNC_TLS
diff -r e6cf98edf0c5 tools/ioemu/vnchextile.h
--- a/tools/ioemu/vnchextile.h	Thu Feb 07 13:21:38 2008 +0000
+++ b/tools/ioemu/vnchextile.h	Fri Feb 08 16:29:50 2008 +0000
@@ -2,29 +2,29 @@
 #define CONCAT(a, b) CONCAT_I(a, b)
 #define pixel_t CONCAT(uint, CONCAT(BPP, _t))
 #ifdef GENERIC
-#define NAME generic
+#define NAME CONCAT(generic_, BPP)
 #else
 #define NAME BPP
 #endif
 
 static void CONCAT(send_hextile_tile_, NAME)(VncState *vs,
                                              int x, int y, int w, int h,
-                                             uint32_t *last_bg32, 
-                                             uint32_t *last_fg32,
+                                             void *last_bg_, 
+                                             void *last_fg_,
                                              int *has_bg, int *has_fg)
 {
     uint8_t *row = (vs->ds->data + y * vs->ds->linesize + x * vs->depth);
     pixel_t *irow = (pixel_t *)row;
     int j, i;
-    pixel_t *last_bg = (pixel_t *)last_bg32;
-    pixel_t *last_fg = (pixel_t *)last_fg32;
+    pixel_t *last_bg = (pixel_t *)last_bg_;
+    pixel_t *last_fg = (pixel_t *)last_fg_;
     pixel_t bg = 0;
     pixel_t fg = 0;
     int n_colors = 0;
     int bg_count = 0;
     int fg_count = 0;
     int flags = 0;
-    uint8_t data[(sizeof(pixel_t) + 2) * 16 * 16];
+    uint8_t data[(vs->pix_bpp + 2) * 16 * 16];
     int n_data = 0;
     int n_subtiles = 0;
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

RE: Qemu vnc color depth

[James Harper]

> The basic idea is that the vnc client is the one that should do the
> colour conversion, if necessary.

What impact, if any, does this have on the bandwidth required for the
VNC connection? I've been using VNC to access HVM Xen clients over a
512k link lately and it's only barely acceptable. My color level is set
to 'Low (64 Colors)'. If the client is doing the conversion then doesn't
this mean that the server has to transmit 16/24/32 bits of color
information to the client, rather than 6?

Or more likely, maybe I don't understand the implications of your patch
:)

James

Re: Qemu vnc color depth

[Stefano Stabellini]

James Harper wrote:
>> The basic idea is that the vnc client is the one that should do the
>> colour conversion, if necessary.
> 
> What impact, if any, does this have on the bandwidth required for the
> VNC connection? I've been using VNC to access HVM Xen clients over a
> 512k link lately and it's only barely acceptable. My color level is set
> to 'Low (64 Colors)'. If the client is doing the conversion then doesn't
> this mean that the server has to transmit 16/24/32 bits of color
> information to the client, rather than 6?
> 
> Or more likely, maybe I don't understand the implications of your patch
> :)
> 

It is the opposite :)
Now most vnc clients run on 32 bpp display, so they automatically ask 
for 32 bpp connections to the server, even if the guest is using 16 bpp.
This is a waste of bandwidth.
Using my patch together with the right configuration of your vnc client 
you can just use the bandwidth needed to preserve the colour depth 
resolution of the guest and nothing more.

Re: Qemu vnc color depth

[Daniel P. Berrange]

On Mon, Feb 11, 2008 at 11:20:03AM +0000, Stefano Stabellini wrote:
> the qemu vnc server changes its internal colour depth based on the 
> client request. This way just one colour conversion is done: the one in 
> vga_template.h, from the guest colour depth and the vnc server internal 
> colour depth.
> 
> This patch is meant to remove this colour conversion to improve 
> performances. It accomplishes the goal making the qemu internal colour 
> depth always the same as the guest colour depth.
> The basic idea is that the vnc client is the one that should do the 
> colour conversion, if necessary. In general it should accept the pixel 
> format suggested by the server during the initial negotiation. This 
> behaviour can be set in most vnc clients (vncviewer included).
> 
> If the guest changes colour depth, the qemu vnc server changes colour 
> depth too and notifies the client. The problem is that the vnc protocol 
> doesn't provide a message from the server to the client to ask for a 
> colour depth change. So what I am doing is either:
> 
> 1) quietly starting to do the conversion on vnc server (not gaining any 
> performances here);

This should be the default behaviour
 
> 2) closing the vnc connection with the client, so the client can 
> reconnect and choose the new pixel format.

This is evil. If we need a way to notify the client of colour depth
changes, then we should define an official VNC extension for this 
that clients can implement. cf the desktop-resize extension

So, if the client supports the extension use that to notify, otherwise
fallback to doing server-side conversions. 

> By default I am doing 1), however the second choice can be enabled 
> passing the -vnc-switch-bpp command line option.

Don't add more command line options - the existing -vnc flag already
has the ability to take multiple flags in the format:

    -vnc hostname:display,flag,flag,flag,flag

eg

    -vnc localhost:1,passwd,tls

> A last note: to get most out of this patch it is best to set Windows to 
> 16 bit colour depth, because the 24 bit mode is 24 bit depth and 24 bpp, 
> meaning no alpha channel. The vnc protocol doesn't support 24 bpp, only 
> 32 bpp, so this conversion is unavoidable.
> 
> Comments and critics are welcome.

Please send this to qemu-devel - we should not be introducing yet more
xen-specific forks to the QEMU code we have - it is a maintainance
disaster already with the number of patches we have.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: Qemu vnc color depth

[Stefano Stabellini]

Daniel P. Berrange wrote:
>> 1) quietly starting to do the conversion on vnc server (not gaining any 
>> performances here);
> 
> This should be the default behaviour
>  
>> 2) closing the vnc connection with the client, so the client can 
>> reconnect and choose the new pixel format.
> 
> This is evil. If we need a way to notify the client of colour depth
> changes, then we should define an official VNC extension for this 
> that clients can implement. cf the desktop-resize extension
> 
> So, if the client supports the extension use that to notify, otherwise
> fallback to doing server-side conversions. 

This is a good idea, I just hope it won't take ages to be accepted.
I'll work on this.

>> By default I am doing 1), however the second choice can be enabled 
>> passing the -vnc-switch-bpp command line option.
> 
> Don't add more command line options - the existing -vnc flag already
> has the ability to take multiple flags in the format:
> 
>     -vnc hostname:display,flag,flag,flag,flag
> 
> eg
> 
>     -vnc localhost:1,passwd,tls

another good suggestion, I'll modify the patch to use this.

> 
> Please send this to qemu-devel - we should not be introducing yet more
> xen-specific forks to the QEMU code we have - it is a maintainance
> disaster already with the number of patches we have.
> 

Yeah I know, but sending patches to qemu-devel is like &>/dev/null

Re: Qemu vnc color depth

[Daniel P. Berrange]

On Mon, Feb 11, 2008 at 03:18:59PM +0000, Stefano Stabellini wrote:
> Daniel P. Berrange wrote:
> >>1) quietly starting to do the conversion on vnc server (not gaining any 
> >>performances here);
> >
> >This should be the default behaviour
> > 
> >>2) closing the vnc connection with the client, so the client can 
> >>reconnect and choose the new pixel format.
> >
> >This is evil. If we need a way to notify the client of colour depth
> >changes, then we should define an official VNC extension for this 
> >that clients can implement. cf the desktop-resize extension
> >
> >So, if the client supports the extension use that to notify, otherwise
> >fallback to doing server-side conversions. 
> 
> This is a good idea, I just hope it won't take ages to be accepted.
> I'll work on this.

There's not really any formal process for VNC extensions. You pretty
much just post to the vnc mailing list & propose what you want to do
and unless its absolutely insane you should get given a psuedo encoding
number reasonably quickly.

Getting it implement in clients is more fun - but feel free to propose
patches to the gtk-vnc mailing list - we aim to be broad compatability
with as many servers as possible & are particularly interested in stuff
that is useful to virtualization.  Also send ideas for the protocol
extension there & we can give feedback on how well it'll work from
the client POV.

> >>By default I am doing 1), however the second choice can be enabled 
> >>passing the -vnc-switch-bpp command line option.
> >
> >Don't add more command line options - the existing -vnc flag already
> >has the ability to take multiple flags in the format:
> >
> >    -vnc hostname:display,flag,flag,flag,flag
> >
> >eg
> >
> >    -vnc localhost:1,passwd,tls
> 
> another good suggestion, I'll modify the patch to use this.

Great, thanks.

> >Please send this to qemu-devel - we should not be introducing yet more
> >xen-specific forks to the QEMU code we have - it is a maintainance
> >disaster already with the number of patches we have.
> >
> 
> Yeah I know, but sending patches to qemu-devel is like &>/dev/null

Yes it can be - you may not get it mereged quickly, but there are quite a
few people who are knowledgable about the VNC server (who don't read 
xen-devel) & might give useful feedback.

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: Qemu vnc color depth

[Anthony Liguori]

Daniel P. Berrange wrote:
> On Mon, Feb 11, 2008 at 03:18:59PM +0000, Stefano Stabellini wrote:
>>> So, if the client supports the extension use that to notify, otherwise
>>> fallback to doing server-side conversions. 
>> This is a good idea, I just hope it won't take ages to be accepted.
>> I'll work on this.
> 
> There's not really any formal process for VNC extensions. You pretty
> much just post to the vnc mailing list & propose what you want to do
> and unless its absolutely insane you should get given a psuedo encoding
> number reasonably quickly.

We can use the existing space for gtk-vnc although VMware actually 
already has an extension to support server initiated pixel format 
changes.  See http://wiki.multimedia.cx/index.php?title=VMNC

The extension is VMVi (display mode change).  I would much rather just 
implement that in QEMU and in gtk-vnc.

This would also count in my mind as finally fixing SetPixelFormat since 
a client could rely on getting the DisplayModeChange message before the 
server starts using the new pixel format.

> Getting it implement in clients is more fun - but feel free to propose
> patches to the gtk-vnc mailing list - we aim to be broad compatability
> with as many servers as possible & are particularly interested in stuff
> that is useful to virtualization.  Also send ideas for the protocol
> extension there & we can give feedback on how well it'll work from
> the client POV.

Regards,

Anthony Liguori

Re: Qemu vnc color depth

[Stefano Stabellini]

Anthony Liguori wrote:
> We can use the existing space for gtk-vnc although VMware actually 
> already has an extension to support server initiated pixel format 
> changes.  See http://wiki.multimedia.cx/index.php?title=VMNC
> 
> The extension is VMVi (display mode change).  I would much rather just 
> implement that in QEMU and in gtk-vnc.
> 
> This would also count in my mind as finally fixing SetPixelFormat since 
> a client could rely on getting the DisplayModeChange message before the 
> server starts using the new pixel format.

I have given a look at WMVi: the message format is fine as it is 
identical to the PIXEL_FORMAT part of the ServerInit message.
However I couldn't see the pseudo encoding number or any client side 
implementation.
I think they used WMVi as part of their format to record AVI videos and 
not as a VNC protocol extension.

We can use the message format they proposed, but we need also a proper 
pseudo encoding number.
Then we'll send "DisplayModeChange" messages as 1 rectangle framebuffer 
updates.

Re: Qemu vnc color depth

[Anthony Liguori]

Stefano Stabellini wrote:
> Anthony Liguori wrote:
>> We can use the existing space for gtk-vnc although VMware actually 
>> already has an extension to support server initiated pixel format 
>> changes.  See http://wiki.multimedia.cx/index.php?title=VMNC
>>
>> The extension is VMVi (display mode change).  I would much rather 
>> just implement that in QEMU and in gtk-vnc.
>>
>> This would also count in my mind as finally fixing SetPixelFormat 
>> since a client could rely on getting the DisplayModeChange message 
>> before the server starts using the new pixel format.
>
> I have given a look at WMVi: the message format is fine as it is 
> identical to the PIXEL_FORMAT part of the ServerInit message.
> However I couldn't see the pseudo encoding number or any client side 
> implementation.
> I think they used WMVi as part of their format to record AVI videos 
> and not as a VNC protocol extension.
>
> We can use the message format they proposed, but we need also a proper 
> pseudo encoding number.
> Then we'll send "DisplayModeChange" messages as 1 rectangle 
> framebuffer updates. 0x574D5669

The encoding number they use is 0x574D5669 and it is registered in the 
latest RFB spec.

Regards,

Anthony Liguori

Re: Re: Qemu vnc color depth

[Stefano Stabellini]

Anthony Liguori wrote:
> The encoding number they use is 0x574D5669 and it is registered in the 
> latest RFB spec.
> 

Thanks for looking it up.
I'll post the updated patch soon.