[Qemu-devel] [PATCH 1 of 3] [UPDATE] vnc dynamic resolution

[Qemu-devel] [PATCH 1 of 3] [UPDATE] vnc dynamic resolution

[Stefano Stabellini]

This patch implements dynamic colour depth changes in vnc.c:
this way the vnc server can change its own internal colour depth at run
time to follow any guest resolution change.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---

diff --git a/console.c b/console.c
index 1c94980..cb85272 100644
--- a/console.c
+++ b/console.c
@@ -190,16 +190,9 @@ static unsigned int vga_get_color(DisplayState *ds, unsigned int rgba)
     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]);
+        color = ((r >> 5) << 5 | (g >> 5) << 2 | (b >> 6));
         break;
-#endif
     case 15:
         r = (rgba >> 16) & 0xff;
         g = (rgba >> 8) & 0xff;
diff --git a/vnc.c b/vnc.c
index c0a05a8..289213c 100644
--- a/vnc.c
+++ b/vnc.c
@@ -71,8 +71,8 @@ typedef void VncWritePixels(VncState *vs, void *data, int size);
 
 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);
 
 #define VNC_MAX_WIDTH 2048
@@ -166,9 +166,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;
@@ -210,6 +210,8 @@ static void vnc_flush(VncState *vs);
 static void vnc_update_client(void *opaque);
 static void vnc_client_read(void *opaque);
 
+static void vnc_colourdepth(DisplayState *ds, int depth);
+
 static inline void vnc_set_bit(uint32_t *d, int k)
 {
     d[k >> 5] |= 1 << (k & 0x1f);
@@ -332,54 +334,73 @@ static void vnc_write_pixels_copy(VncState *vs, void *pixels, int size)
 /* slowest but generic code. */
 static void vnc_convert_pixel(VncState *vs, uint8_t *buf, uint32_t v)
 {
-    unsigned int r, g, b;
+    uint8_t 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);
+    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");
     }
 }
 
@@ -416,6 +437,18 @@ static void hextile_enc_cord(uint8_t *ptr, int x, int y, int w, int 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
 #include "vnchextile.h"
 #undef BPP
@@ -425,18 +458,23 @@ static void send_framebuffer_update_hextile(VncState *vs, int x, int y, int w, i
 {
     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)
@@ -1135,17 +1173,6 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
     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,
@@ -1165,6 +1192,7 @@ static void set_pixel_format(VncState *vs,
         return;
     }
     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) {
@@ -1173,6 +1201,7 @@ static void set_pixel_format(VncState *vs,
         vs->send_hextile_tile = send_hextile_tile_32;
     } else
     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) {
@@ -1181,6 +1210,7 @@ static void set_pixel_format(VncState *vs,
         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;
@@ -1193,28 +1223,116 @@ static void set_pixel_format(VncState *vs,
             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_colourdepth(DisplayState *ds, int depth)
+{
+    int host_big_endian_flag;
+    struct VncState *vs = ds->opaque;
+
+    switch (depth) {
+        case 24:
+            if (ds->depth == 32) return;
+            depth = 32;
+            break;
+        case 15:
+        case 8:
+        case 0:
+            return;
+        default:
+            break;
+    }
+
+#ifdef WORDS_BIGENDIAN
+    host_big_endian_flag = 1;
+#else
+    host_big_endian_flag = 0;
+#endif   
+    
+    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 (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;
+    }
+}
+
 static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
 {
     int i;
@@ -1318,7 +1436,9 @@ static int protocol_client_init(VncState *vs, uint8_t *data, size_t len)
     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
@@ -2008,7 +2128,6 @@ void vnc_display_init(DisplayState *ds)
 
     vs->lsock = -1;
     vs->csock = -1;
-    vs->depth = 4;
     vs->last_x = -1;
     vs->last_y = -1;
 
diff --git a/vnchextile.h b/vnchextile.h
index 09c1b27..eb05feb 100644
--- a/vnchextile.h
+++ b/vnchextile.h
@@ -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;
 

Re: [Qemu-devel] [PATCH 1 of 3] [UPDATE] vnc dynamic resolution

[Anthony Liguori]

Stefano Stabellini wrote:
> This patch implements dynamic colour depth changes in vnc.c:
> this way the vnc server can change its own internal colour depth at run
> time to follow any guest resolution change.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> ---
>
> diff --git a/console.c b/console.c
> index 1c94980..cb85272 100644
> --- a/console.c
> +++ b/console.c
> @@ -190,16 +190,9 @@ static unsigned int vga_get_color(DisplayState *ds, unsigned int rgba)
>      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]);
> +        color = ((r >> 5) << 5 | (g >> 5) << 2 | (b >> 6));
>          break;
> -#endif
>   

This fix seems orthogonal to the rest of the patch.  You're adding 
support for an 8-bit DisplayState depth that's 3-3-2.  It would be good 
to document this somewhere.

>      case 15:
>          r = (rgba >> 16) & 0xff;
>          g = (rgba >> 8) & 0xff;
> diff --git a/vnc.c b/vnc.c
> index c0a05a8..289213c 100644
> --- a/vnc.c
> +++ b/vnc.c
> @@ -71,8 +71,8 @@ typedef void VncWritePixels(VncState *vs, void *data, int size);
>  
>  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);
>  
>  #define VNC_MAX_WIDTH 2048
> @@ -166,9 +166,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;
> @@ -210,6 +210,8 @@ static void vnc_flush(VncState *vs);
>  static void vnc_update_client(void *opaque);
>  static void vnc_client_read(void *opaque);
>  
> +static void vnc_colourdepth(DisplayState *ds, int depth);
>   

For the purposes of consistency, please stick to American English spellings.

>  static inline void vnc_set_bit(uint32_t *d, int k)
>  {
>      d[k >> 5] |= 1 << (k & 0x1f);
> @@ -332,54 +334,73 @@ static void vnc_write_pixels_copy(VncState *vs, void *pixels, int size)
>  /* slowest but generic code. */
>  static void vnc_convert_pixel(VncState *vs, uint8_t *buf, uint32_t v)
>  {
> -    unsigned int r, g, b;
> +    uint8_t 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);
> +    r = ((v >> vs->red_shift1) & vs->red_max1) * (vs->red_max + 1) /
> +        (vs->red_max1 + 1);
>   

I don't understand this change.  The code & red_max but then also rounds 
to red_max + 1.  Is this an attempt to handle color maxes that aren't 
power of 2 - 1?  The spec insists that the max is always in the form n^2 
- 1:

"Red-max is the maximum red value (= 2n − 1 where n is the number of 
bits used for red)."

Is this just overzealous checks or was a fix for a broken client?

>      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);
>          }
>   

I think this stinks compared to the previous code.  I don't see a 
functional difference between the two.  Can you elaborate on why you 
made this change?

> +    }
>          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);
>          }
>   

And here.

>  }
>  
> @@ -416,6 +437,18 @@ static void hextile_enc_cord(uint8_t *ptr, int x, int y, int w, int 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
>  #include "vnchextile.h"
>  #undef BPP
> @@ -425,18 +458,23 @@ static void send_framebuffer_update_hextile(VncState *vs, int x, int y, int w, i
>  {
>      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);
>   

Probably should just have uint8_t last_fg[4], last_bg[4].  That avoids 
error checking on the malloc.

>      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)
> @@ -1135,17 +1173,6 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
>      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,
> @@ -1165,6 +1192,7 @@ static void set_pixel_format(VncState *vs,
>          return;
>      }
>      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) {
> @@ -1173,6 +1201,7 @@ static void set_pixel_format(VncState *vs,
>          vs->send_hextile_tile = send_hextile_tile_32;
>      } else
>      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) {
> @@ -1181,6 +1210,7 @@ static void set_pixel_format(VncState *vs,
>          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;
> @@ -1193,28 +1223,116 @@ static void set_pixel_format(VncState *vs,
>              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;
>   

I think the previous way was better.  This code seems to be trying to 
deal with red_max that's not in the form of 2^n-1, but it has to be in 
that form according to the spec.

>      vga_hw_invalidate();
>      vga_hw_update();
>  }
>  
> +static void vnc_colourdepth(DisplayState *ds, int depth)
>   

colordepth.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1 of 3] [UPDATE] vnc dynamic resolution

[Stefano Stabellini]

Anthony Liguori wrote:

> Stefano Stabellini wrote:
>> -#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]);
>> +        color = ((r >> 5) << 5 | (g >> 5) << 2 | (b >> 6));
>>          break;
>> -#endif
>>   
> 
> This fix seems orthogonal to the rest of the patch.  You're adding
> support for an 8-bit DisplayState depth that's 3-3-2.  It would be good
> to document this somewhere.


You are right: I saw the "#if 0" and I just tried to "fix" it.
Reading the code again I realize that this change doesn't make sense for
at least two different reasons, so I'll just drop it.

 
>>  
>> +static void vnc_colourdepth(DisplayState *ds, int depth);
>>   
> 
> For the purposes of consistency, please stick to American English
> spellings.


OK, no problems, I'll start practicing with this email :)

 
>>  static inline void vnc_set_bit(uint32_t *d, int k)
>>  {
>>      d[k >> 5] |= 1 << (k & 0x1f);
>> @@ -332,54 +334,73 @@ static void vnc_write_pixels_copy(VncState *vs,
>> void *pixels, int size)
>>  /* slowest but generic code. */
>>  static void vnc_convert_pixel(VncState *vs, uint8_t *buf, uint32_t v)
>>  {
>> -    unsigned int r, g, b;
>> +    uint8_t 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);
>> +    r = ((v >> vs->red_shift1) & vs->red_max1) * (vs->red_max + 1) /
>> +        (vs->red_max1 + 1);
>>   
> 
> I don't understand this change.  The code & red_max but then also rounds
> to red_max + 1.  Is this an attempt to handle color maxes that aren't
> power of 2 - 1?  The spec insists that the max is always in the form n^2
> - 1:
> 
> "Red-max is the maximum red value (= 2n − 1 where n is the number of
> bits used for red)."
> 
> Is this just overzealous checks or was a fix for a broken client?


This code is meant to convert pixels from the vnc server internal pixel
format to the vnc client pixel format.
red_max refers to the vnc client red max, while red_max1 refers to the
vnc server internal red max.
Before we were just handling the case red_max1 = 0xff, this code should
be able to handle other cases as well (necessary for handling the shared
buffer).
Does this answer your question? May be with the assumption that red_max
= 2^n - 1 is still possible to simplify the conversion code...

 
>>      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);
>>          }
>>   
> 
> I think this stinks compared to the previous code.  I don't see a
> functional difference between the two.  Can you elaborate on why you
> made this change?


It seems that these last changes can be dropped all together.
The color conversion changes were fixed in multiple steps on
xen-unstable, so now the latter changes seem pointless.
I'll drop them and do all the tests again...

 
>> send_framebuffer_update_hextile(VncState *vs, int x, int y, int w, i
>>  {
>>      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);
>>   
> 
> Probably should just have uint8_t last_fg[4], last_bg[4].  That avoids
> error checking on the malloc.


OK.

 
>>      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)
>> @@ -1135,17 +1173,6 @@ static void set_encodings(VncState *vs, int32_t
>> *encodings, size_t n_encodings)
>>      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,
>> @@ -1165,6 +1192,7 @@ static void set_pixel_format(VncState *vs,
>>          return;
>>      }
>>      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) {
>> @@ -1173,6 +1201,7 @@ static void set_pixel_format(VncState *vs,
>>          vs->send_hextile_tile = send_hextile_tile_32;
>>      } else
>>      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) {
>> @@ -1181,6 +1210,7 @@ static void set_pixel_format(VncState *vs,
>>          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;
>> @@ -1193,28 +1223,116 @@ static void set_pixel_format(VncState *vs,
>>              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;
>>   
> 
> I think the previous way was better.  This code seems to be trying to
> deal with red_max that's not in the form of 2^n-1, but it has to be in
> that form according to the spec.
> 

same as before: we are trying to handle a changing vnc server internal
resolution in order to be able to support a shared buffer with the guest.

Re: [Qemu-devel] [PATCH 1 of 3] [UPDATE] vnc dynamic resolution

[Anthony Liguori]

Stefano Stabellini wrote:
> Anthony Liguori wrote:
>   
>> "Red-max is the maximum red value (= 2n − 1 where n is the number of
>> bits used for red)."
>>
>> Is this just overzealous checks or was a fix for a broken client?
>>     
>
>
> This code is meant to convert pixels from the vnc server internal pixel
> format to the vnc client pixel format.
> red_max refers to the vnc client red max, while red_max1 refers to the
> vnc server internal red max.
> Before we were just handling the case red_max1 = 0xff, this code should
> be able to handle other cases as well (necessary for handling the shared
> buffer).
> Does this answer your question? May be with the assumption that red_max
> = 2^n - 1 is still possible to simplify the conversion code...
>   

Perhaps the source of my confusion is the use of red_max and red_max1.  
Could you try some more descriptive names?  I understand what the code 
is doing now but it's very easy to confuse the two masks.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1 of 3] [UPDATE] vnc dynamic resolution

[Stefano Stabellini]

Anthony Liguori wrote:

> Stefano Stabellini wrote:
>> Anthony Liguori wrote:
>>  
>>> "Red-max is the maximum red value (= 2n − 1 where n is the
>>> number of
>>> bits used for red)."
>>>
>>> Is this just overzealous checks or was a fix for a broken client?
>>>     
>>
>>
>> This code is meant to convert pixels from the vnc server internal pixel
>> format to the vnc client pixel format.
>> red_max refers to the vnc client red max, while red_max1 refers to the
>> vnc server internal red max.
>> Before we were just handling the case red_max1 = 0xff, this code should
>> be able to handle other cases as well (necessary for handling the shared
>> buffer).
>> Does this answer your question? May be with the assumption that red_max
>> = 2^n - 1 is still possible to simplify the conversion code...
>>   
> 
> Perhaps the source of my confusion is the use of red_max and red_max1. 
> Could you try some more descriptive names?  I understand what the code
> is doing now but it's very easy to confuse the two masks.
> 

I understand.
I went with red_max1 because we were already using this "notation" with
red_shift and red_shift1.
How do you suggest I should call them?

Re: [Qemu-devel] [PATCH 1 of 3] [UPDATE] vnc dynamic resolution

[Anthony Liguori]

Stefano Stabellini wrote:
> Anthony Liguori wrote:
>
>   
>> Stefano Stabellini wrote:
>>     
>>>
>> Perhaps the source of my confusion is the use of red_max and red_max1. 
>> Could you try some more descriptive names?  I understand what the code
>> is doing now but it's very easy to confuse the two masks.
>>
>>     
>
> I understand.
> I went with red_max1 because we were already using this "notation" with
> red_shift and red_shift1.
>   

Yeah, I assumed that's why.

> How do you suggest I should call them?
>   

server_red_max and client_red_max seem like the obvious candidates to 
me.  Changing the shift1's would also be a nice fix.

Regards,

Anthony Liguori