From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KchKt-00081x-PE for qemu-devel@nongnu.org; Mon, 08 Sep 2008 10:03:24 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KchKq-0007zq-NG for qemu-devel@nongnu.org; Mon, 08 Sep 2008 10:03:23 -0400 Received: from [199.232.76.173] (port=39235 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KchKq-0007zX-7D for qemu-devel@nongnu.org; Mon, 08 Sep 2008 10:03:20 -0400 Received: from smtp.ctxuk.citrix.com ([62.200.22.115]:33819 helo=SMTP.EU.CITRIX.COM) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1KchKo-0007HR-Pa for qemu-devel@nongnu.org; Mon, 08 Sep 2008 10:03:20 -0400 Message-ID: <48C5310E.5090506@eu.citrix.com> Date: Mon, 08 Sep 2008 15:05:02 +0100 From: Stefano Stabellini MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 1 of 3] [UPDATE] vnc dynamic resolution References: <48BFF02F.2000803@eu.citrix.com> <48C474FC.70608@codemonkey.ws> In-Reply-To: <48C474FC.70608@codemonkey.ws> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: qemu-devel@nongnu.org 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.