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

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

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.