Re: [gtk-vnc-devel] Wrong colors

["Daniel P. Berrange" <berrange redhat com>]

On Wed, Aug 29, 2007 at 08:29:51AM -0300, Jonh Wendell wrote:
> Hi, folks.
> 
> Since latest changes, i'm getting wrong colors in vinagre. I'll attach a
> screenshot. (I've reduced the image so that it gets a small size)
> 
> Any idea?

Yep, its (at least partly) my fault :-)

So Anthony's original code had support for adjusting the colours if the
server colour depth was less than the client colour depth. When I fixed
the big-endian &  BGR support though I broke this. Also we never worked
correctly when the local colour depth was less than the remote colour
depth.

So currently we work correctly if the colour depths are matched (eg both
24 bit, both 32 bit or both 16 bit), and also deal with endianness and
BGR correctly.  In the case of mis-matched colours though we're truncating
the most significant bits, instead of shifting off the least significant
bits and keeping the MSB. This basically makes the colours very dark and
gives the green tint!

The key is the code in blt.h doing the shifts. It currently applies the
server shift going right, and the client shift going left.

        *dp = ((*sp >> f->red_shift) & gvnc->rm) << gvnc->local.red_shift
            | ((*sp >> f->green_shift) & gvnc->gm) << gvnc->local.green_shift
            | ((*sp >> f->blue_shift) & gvnc->bm) << gvnc->local.blue_shift


In the case of the server depth being greater than the local depth, we
need to increase the right shift to compensate. In the case of the server
depth being less than the local depth, we need to increase the left shift
to compensate. With matching depths, the plain code above handles it. 

So I'm attaching a patch changes the above to look like:

        *dp = (((uint32_t)*sp >> gvnc->rrs) & gvnc->rm) << gvnc->rls
            | (((uint32_t)*sp >> gvnc->grs) & gvnc->gm) << gvnc->gls
            | (((uint32_t)*sp >> gvnc->brs) & gvnc->bm) << gvnc->bls;

And initirlizes  rrs, grs, brs, rls, gls, and bls in the gvnc_set_local
method:

        /* Setup shifts assuming matched bpp (but not neccessarily match rgb order)*/
        gvnc->rrs = gvnc->fmt.red_shift;
        gvnc->grs = gvnc->fmt.green_shift;
        gvnc->brs = gvnc->fmt.blue_shift;

        gvnc->rls = gvnc->local.red_shift;
        gvnc->gls = gvnc->local.green_shift;
        gvnc->bls = gvnc->local.blue_shift;

        /* This adjusts for remote having more bpp than local */
        for (n = gvnc->fmt.red_max; n > gvnc->local.red_mask ; n>>= 1)
                gvnc->rrs++;
        for (n = gvnc->fmt.green_max; n > gvnc->local.green_mask ; n>>= 1)
                gvnc->grs++;
        for (n = gvnc->fmt.blue_max; n > gvnc->local.blue_mask ; n>>= 1)
                gvnc->brs++;

        /* This adjusts for remote having less bpp than remote */
        for (n = gvnc->local.red_mask ; n > gvnc->fmt.red_max ; n>>= 1)
                gvnc->rls++;
        for (n = gvnc->local.green_mask ; n > gvnc->fmt.green_max ; n>>= 1)
                gvnc->gls++;
        for (n = gvnc->local.blue_mask ; n > gvnc->fmt.blue_max ; n>>= 1)
                gvnc->bls++;

I'm also moving the endianness adjustment into the same place.

I was going to send this out earlier in the week, but I still need to get 
access to a PPC  OS-X box again to test the big-endian handling is correct

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

changeset:   55:ccee737658e4
tag:         tip
user:        "Daniel P. Berrange <berrange redhat com>"
date:        Fri Aug 24 17:19:37 2007 -0400
files:       src/blt.h src/gvnc.c src/gvnc.h src/vncdisplay.c
description:
Fixed adjustment for mis-matched colour depths local vs remote


diff -r b1c48ddc01d9 -r ccee737658e4 src/blt.h
--- a/src/blt.h	Wed Aug 22 15:10:12 2007 -0400
+++ b/src/blt.h	Fri Aug 24 17:19:37 2007 -0400
@@ -32,7 +32,6 @@ static void FILL(struct gvnc *gvnc, src_
 		 int x, int y, int width, int height)
 {
 	uint8_t *dst = gvnc_get_local(gvnc, x, y);
-	struct gvnc_pixel_format *f = &gvnc->fmt;
 	int i;
 
 	for (i = 0; i < 1; i++) {
@@ -40,9 +39,9 @@ static void FILL(struct gvnc *gvnc, src_
 		int j;
 
 		for (j = 0; j < width; j++) {
-			*dp = ((*sp >> f->red_shift) & gvnc->rm) << gvnc->local.red_shift
-			    | ((*sp >> f->green_shift) & gvnc->gm) << gvnc->local.green_shift
-			    | ((*sp >> f->blue_shift) & gvnc->bm) << gvnc->local.blue_shift;
+			*dp = (((uint32_t)*sp >> gvnc->rrs) & gvnc->rm) << gvnc->rls
+			    | (((uint32_t)*sp >> gvnc->grs) & gvnc->gm) << gvnc->gls
+			    | (((uint32_t)*sp >> gvnc->brs) & gvnc->bm) << gvnc->bls;
 			dp++;
 		}
 		dst += gvnc->local.linesize;
@@ -56,7 +55,6 @@ static void BLIT(struct gvnc *gvnc, uint
 static void BLIT(struct gvnc *gvnc, uint8_t *src, int pitch, int x, int y, int w, int h)
 {
 	uint8_t *dst = gvnc_get_local(gvnc, x, y);
-	struct gvnc_pixel_format *f = &gvnc->fmt;
 	int i;
 
 	for (i = 0; i < h; i++) {
@@ -65,9 +63,9 @@ static void BLIT(struct gvnc *gvnc, uint
 		int j;
 
 		for (j = 0; j < w; j++) {
-			*dp = ((*sp >> f->red_shift) & gvnc->rm) << gvnc->local.red_shift
-			    | ((*sp >> f->green_shift) & gvnc->gm) << gvnc->local.green_shift
-			    | ((*sp >> f->blue_shift) & gvnc->bm) << gvnc->local.blue_shift;
+			*dp = (((uint32_t)*sp >> gvnc->rrs) & gvnc->rm) << gvnc->rls
+			    | (((uint32_t)*sp >> gvnc->grs) & gvnc->gm) << gvnc->gls
+			    | (((uint32_t)*sp >> gvnc->brs) & gvnc->bm) << gvnc->bls;
 			dp++;
 			sp++;
 		}
@@ -146,3 +144,12 @@ static void SUBRECT(struct gvnc *gvnc, u
 #undef BLIT
 #undef dst_pixel_t
 #undef src_pixel_t
+
+
+/*
+ * Local variables:
+ *  c-indent-level: 8
+ *  c-basic-offset: 8
+ *  tab-width: 8
+ * End:
+ */
diff -r b1c48ddc01d9 -r ccee737658e4 src/gvnc.c
--- a/src/gvnc.c	Wed Aug 22 15:10:12 2007 -0400
+++ b/src/gvnc.c	Fri Aug 24 17:19:37 2007 -0400
@@ -104,6 +104,8 @@ struct gvnc
 	struct gvnc_framebuffer local;
 
 	int rm, gm, bm;
+	int rrs, grs, brs;
+	int rls, gls, bls;
 
 	gvnc_blt_func *blt;
 	gvnc_hextile_func *hextile;
@@ -673,7 +675,7 @@ static void gvnc_read_pixel_format(struc
 
 	fmt->bits_per_pixel  = gvnc_read_u8(gvnc);
 	fmt->depth           = gvnc_read_u8(gvnc);
-	fmt->big_endian_flag = gvnc_read_u8(gvnc);
+	fmt->byte_order      = gvnc_read_u8(gvnc) ? __BIG_ENDIAN : __LITTLE_ENDIAN;
 	fmt->true_color_flag = gvnc_read_u8(gvnc);
 
 	fmt->red_max         = gvnc_read_u16(gvnc);
@@ -686,23 +688,12 @@ static void gvnc_read_pixel_format(struc
 
 	gvnc_read(gvnc, pad, 3);
 
-	GVNC_DEBUG("Pixel format BPP: %d,  Depth: %d, Endian: %d, True color: %d\n"
+	GVNC_DEBUG("Pixel format BPP: %d,  Depth: %d, Byte order: %d, True color: %d\n"
 		   "             Mask  red: %3d, green: %3d, blue: %3d\n"
 		   "             Shift red: %3d, green: %3d, blue: %3d\n",
-		   fmt->bits_per_pixel, fmt->depth, fmt->big_endian_flag, fmt->true_color_flag,
+		   fmt->bits_per_pixel, fmt->depth, fmt->byte_order, fmt->true_color_flag,
 		   fmt->red_max, fmt->green_max, fmt->blue_max,
 		   fmt->red_shift, fmt->green_shift, fmt->blue_shift);
-
-
-	if (((__BYTE_ORDER == __BIG_ENDIAN) && !fmt->big_endian_flag) ||
-	    ((__BYTE_ORDER == __LITTLE_ENDIAN) && fmt->big_endian_flag)) {
-		fmt->red_shift = fmt->bits_per_pixel - fmt->red_shift - (fmt->bits_per_pixel - fmt->depth);
-		fmt->green_shift = fmt->bits_per_pixel - fmt->green_shift - (fmt->bits_per_pixel - fmt->depth);
-		fmt->blue_shift = fmt->bits_per_pixel - fmt->blue_shift - (fmt->bits_per_pixel - fmt->depth);
-
-		GVNC_DEBUG("Flipped shifts Shift red: %3d, green: %3d, blue: %3d\n",
-			   fmt->red_shift, fmt->green_shift, fmt->blue_shift);
-	}
 }
 
 /* initialize function */
@@ -722,7 +713,7 @@ gboolean gvnc_set_pixel_format(struct gv
 
 	gvnc_write_u8(gvnc, fmt->bits_per_pixel);
 	gvnc_write_u8(gvnc, fmt->depth);
-	gvnc_write_u8(gvnc, fmt->big_endian_flag);
+	gvnc_write_u8(gvnc, fmt->byte_order == __BIG_ENDIAN ? 1 : 0);
 	gvnc_write_u8(gvnc, fmt->true_color_flag);
 
 	gvnc_write_u16(gvnc, fmt->red_max);
@@ -1985,7 +1976,7 @@ gboolean gvnc_set_credential_x509_cert(s
 
 gboolean gvnc_set_local(struct gvnc *gvnc, struct gvnc_framebuffer *fb)
 {
-	int i, j;
+	int i, j, n;
 	int depth;
 
 	memcpy(&gvnc->local, fb, sizeof(*fb));
@@ -1997,8 +1988,7 @@ gboolean gvnc_set_local(struct gvnc *gvn
 	    fb->red_shift == gvnc->fmt.red_shift &&
 	    fb->green_shift == gvnc->fmt.green_shift &&
 	    fb->blue_shift == gvnc->fmt.blue_shift &&
-	    ((gvnc->fmt.big_endian_flag && (__BYTE_ORDER == __BIG_ENDIAN)) ||
-	     (!gvnc->fmt.big_endian_flag && (__BYTE_ORDER == __LITTLE_ENDIAN))))
+	    __BYTE_ORDER == gvnc->fmt.byte_order)
 		gvnc->perfect_match = TRUE;
 	else
 		gvnc->perfect_match = FALSE;
@@ -2016,6 +2006,47 @@ gboolean gvnc_set_local(struct gvnc *gvn
 		   gvnc->local.red_mask, gvnc->local.green_mask, gvnc->local.blue_mask,
 		   gvnc->fmt.red_max, gvnc->fmt.green_max, gvnc->fmt.blue_max,
 		   gvnc->rm, gvnc->gm, gvnc->bm);
+
+	/* Setup shifts assuming matched bpp (but not neccessarily match rgb order)*/
+	gvnc->rrs = gvnc->fmt.red_shift;
+	gvnc->grs = gvnc->fmt.green_shift;
+	gvnc->brs = gvnc->fmt.blue_shift;
+
+	gvnc->rls = gvnc->local.red_shift;
+	gvnc->gls = gvnc->local.green_shift;
+	gvnc->bls = gvnc->local.blue_shift;
+
+
+	/* This adjusts for server/client endianness mismatch */
+	if (__BYTE_ORDER != gvnc->fmt.byte_order) {
+		gvnc->rrs = gvnc->fmt.bits_per_pixel - gvnc->rrs - (gvnc->fmt.bits_per_pixel - gvnc->fmt.depth);
+		gvnc->grs = gvnc->fmt.bits_per_pixel - gvnc->grs - (gvnc->fmt.bits_per_pixel - gvnc->fmt.depth);
+		gvnc->brs = gvnc->fmt.bits_per_pixel - gvnc->brs - (gvnc->fmt.bits_per_pixel - gvnc->fmt.depth);
+
+		GVNC_DEBUG("Flipped shifts red: %3d, green: %3d, blue: %3d\n",
+			   gvnc->rrs, gvnc->grs, gvnc->brs);
+	}
+
+
+	/* This adjusts for remote having more bpp than local */
+	for (n = gvnc->fmt.red_max; n > gvnc->local.red_mask ; n>>= 1)
+		gvnc->rrs++;
+	for (n = gvnc->fmt.green_max; n > gvnc->local.green_mask ; n>>= 1)
+		gvnc->grs++;
+	for (n = gvnc->fmt.blue_max; n > gvnc->local.blue_mask ; n>>= 1)
+		gvnc->brs++;
+
+	/* This adjusts for remote having less bpp than remote */
+	for (n = gvnc->local.red_mask ; n > gvnc->fmt.red_max ; n>>= 1)
+		gvnc->rls++;
+	for (n = gvnc->local.green_mask ; n > gvnc->fmt.green_max ; n>>= 1)
+		gvnc->gls++;
+	for (n = gvnc->local.blue_mask ; n > gvnc->fmt.blue_max ; n>>= 1)
+		gvnc->bls++;
+	GVNC_DEBUG("Pixel shifts\n   right: %3d %3d %3d\n    left: %3d %3d %3d\n",
+		   gvnc->rrs, gvnc->grs, gvnc->brs,
+		   gvnc->rls, gvnc->gls, gvnc->bls);
+
 	i = gvnc->fmt.bits_per_pixel / 8;
 	j = gvnc->local.bpp;
 
diff -r b1c48ddc01d9 -r ccee737658e4 src/gvnc.h
--- a/src/gvnc.h	Wed Aug 22 15:10:12 2007 -0400
+++ b/src/gvnc.h	Fri Aug 24 17:19:37 2007 -0400
@@ -24,7 +24,7 @@ struct gvnc_pixel_format
 {
 	uint8_t bits_per_pixel;
 	uint8_t depth;
-	uint8_t big_endian_flag;
+	uint16_t byte_order;
 	uint8_t true_color_flag;
 	uint16_t red_max;
 	uint16_t green_max;
diff -r b1c48ddc01d9 -r ccee737658e4 src/vncdisplay.c
--- a/src/vncdisplay.c	Wed Aug 22 15:10:12 2007 -0400
+++ b/src/vncdisplay.c	Fri Aug 24 17:19:37 2007 -0400
@@ -426,7 +426,6 @@ static gboolean on_resize(void *opaque, 
 	VncDisplay *obj = VNC_DISPLAY(opaque);
 	VncDisplayPrivate *priv = obj->priv;
 	GdkVisual *visual;
-	int depth;
 
 	if (priv->gvnc == NULL || !gvnc_is_initialized(priv->gvnc))
 		return TRUE;
@@ -446,9 +445,8 @@ static gboolean on_resize(void *opaque, 
 		priv->gc = gdk_gc_new(GTK_WIDGET(obj)->window);
 	}
 
-	depth = gdk_drawable_get_depth(GTK_WIDGET(obj)->window);
-	visual = gdk_visual_get_best_with_depth(depth);
-
+	visual = gdk_drawable_get_visual(GTK_WIDGET(obj)->window);
+	
 	priv->shm_image = vnc_shm_image_new(visual, width, height, priv->use_shm);
 	priv->fb.shm_id = priv->shm_image->shmid;