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 <address@hidden>
>
> ---
>
> 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