[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1 of 3] [UPDATE] vnc dynamic resolution
From: |
Stefano Stabellini |
Subject: |
Re: [Qemu-devel] [PATCH 1 of 3] [UPDATE] vnc dynamic resolution |
Date: |
Mon, 08 Sep 2008 15:05:02 +0100 |
User-agent: |
Thunderbird 2.0.0.14 (X11/20080505) |
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.