From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KcUqk-0001qG-I8 for qemu-devel@nongnu.org; Sun, 07 Sep 2008 20:43:26 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KcUqj-0001pa-Qn for qemu-devel@nongnu.org; Sun, 07 Sep 2008 20:43:26 -0400 Received: from [199.232.76.173] (port=43192 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KcUqj-0001pO-NY for qemu-devel@nongnu.org; Sun, 07 Sep 2008 20:43:25 -0400 Received: from an-out-0708.google.com ([209.85.132.240]:16642) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1KcUqj-00050K-8R for qemu-devel@nongnu.org; Sun, 07 Sep 2008 20:43:25 -0400 Received: by an-out-0708.google.com with SMTP id d18so201667and.130 for ; Sun, 07 Sep 2008 17:43:24 -0700 (PDT) Message-ID: <48C474FC.70608@codemonkey.ws> Date: Sun, 07 Sep 2008 19:42:36 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 1 of 3] [UPDATE] vnc dynamic resolution References: <48BFF02F.2000803@eu.citrix.com> In-Reply-To: <48BFF02F.2000803@eu.citrix.com> Content-Type: text/plain; charset=UTF-8; format=flowed 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: qemu-devel@nongnu.org Cc: Stefano Stabellini 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 > > --- > > 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