[gtk-vnc-devel] PATCH: Fix endian conversion .... yet again

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

The previous fix we applied for endian conversion, while appearing correct,
turned out to be broken for several cases. In particular a little endian
client going to big endian server was wrong. After a day's debugging I 
think I've tracked down all the bugs - there were several hitting all at
once just to make things extra special fun to debug :-)

 - SET_PIXEL() in blt.h - this was byte swapping the destination value.
   The RFB spec says byte swapping is done on the source value. So I've
   updated all callers to do swapping before passing in the source pixel
   and removed the SWAP() call from SET_PIXEL() itself

 - Rich cursor - this was not correctly calculating pixel shifts in the
   mixed endian client/server scenario. I've updated it to use the same
   logic as done in gvnc_set_local(), except with fixed 32bpp RGB format
   for the local source, since that's what GdkPixbuf requires

 - Rich cursor - not applying alpha mask on big endian machines - it was
   masing the high bits instead of low bits.

 - gvnc_set_local() - bogus code to take account of endianness when 
   calculating pixel shits. You can't take account of endianness in pixel
   shifts alone because in 16 bpp format, the green component spans two
   bytes. This bogus code is simply removed - byte swapping is done 
   correctly by caller of SET_PIXEL() now.

 - gdk_drawable_get_visual() returns wrong visual in BGR pixelformat
   desktops, so use gdk_screen_get_system_visual() instead.

I've tested

  - 24bpp local LE client -> 16 bpp BE server
  - 24bpp local LE client -> 24 bpp BE server
  - 24bpp local LE client -> 16 bpp LE server
  - 24bpp local LE client -> 24 bpp LE server
  - 24bpp local LE client -> 16 bpp LE server BGR pixelformat
  - 24bpp local LE client -> 24 bpp LE server BGR pixelformat
  - 24bpp local LE client -> 16 bpp BE server -> 16 bpp LE server
  - 24bpp local LE client -> 16 bpp BE server -> 24 bpp LE server
  - 24bpp local LE client -> 24 bpp BE server -> 16 bpp LE server
  - 24bpp local LE client -> 24 bpp BE server -> 24 bpp LE server
  - 24bpp local LE client -> 16 bpp BE server -> 16 bpp LE server -> 24 bpp BE server
  - 24bpp local LE client -> 16 bpp BE server -> 24 bpp LE server -> 24 bpp BE server
  - 24bpp local LE client -> 24 bpp BE server -> 16 bpp LE server -> 16 bpp BE server
  - 24bpp local LE client -> 24 bpp BE server -> 24 bpp LE server -> 16 bpp BE server

Which I think is a reasonable number of combinations & recursive tests :-)

Dan

diff -r fb8ab9818527 src/blt.h
--- a/src/blt.h	Fri Mar 07 11:24:19 2008 -0600
+++ b/src/blt.h	Tue Mar 18 17:57:26 2008 -0400
@@ -16,7 +16,7 @@
 #define RGB24_BLIT SPLICE(gvnc_rgb24_blt_, SUFFIX())
 #define TIGHT_COMPUTE_PREDICTED SPLICE(gvnc_tight_compute_predicted_, SUFFIX())
 #define TIGHT_SUM_PIXEL SPLICE(gvnc_tight_sum_pixel_, SUFFIX())
-#define SWAP(gvnc, pixel) SPLICE(gvnc_swap_, DST)(gvnc, pixel)
+#define SWAP(gvnc, pixel) SPLICE(gvnc_swap_, SRC)(gvnc, pixel)
 #define COMPONENT(color, pixel) ((SWAP(gvnc, pixel) >> gvnc->fmt.SPLICE(color, _shift) & gvnc->fmt.SPLICE(color, _max)))
 
 static void FAST_FILL(struct gvnc *gvnc, src_pixel_t *sp,
@@ -41,18 +41,18 @@ static void FAST_FILL(struct gvnc *gvnc,
 	}
 }
 
-static void SET_PIXEL(struct gvnc *gvnc, dst_pixel_t *dp, src_pixel_t *sp)
+static void SET_PIXEL(struct gvnc *gvnc, dst_pixel_t *dp, src_pixel_t sp)
 {
-	*dp = SWAP(gvnc, ((*sp >> gvnc->rrs) & gvnc->rm) << gvnc->rls
-		| ((*sp >> gvnc->grs) & gvnc->gm) << gvnc->gls
-		| ((*sp >> gvnc->brs) & gvnc->bm) << gvnc->bls);
+	*dp = ((sp >> gvnc->rrs) & gvnc->rm) << gvnc->rls
+		| ((sp >> gvnc->grs) & gvnc->gm) << gvnc->gls
+		| ((sp >> gvnc->brs) & gvnc->bm) << gvnc->bls;
 }
 
 static void SET_PIXEL_AT(struct gvnc *gvnc, int x, int y, src_pixel_t *sp)
 {
 	dst_pixel_t *dp = (dst_pixel_t *)gvnc_get_local(gvnc, x, y);
 
-	SET_PIXEL(gvnc, dp, sp);
+	SET_PIXEL(gvnc, dp, SWAP(gvnc, *sp));
 }
 
 static void FILL(struct gvnc *gvnc, src_pixel_t *sp,
@@ -66,7 +66,7 @@ static void FILL(struct gvnc *gvnc, src_
 		int j;
 
 		for (j = 0; j < width; j++) {
-			SET_PIXEL(gvnc, dp, sp);
+			SET_PIXEL(gvnc, dp, SWAP(gvnc, *sp));
 			dp++;
 		}
 		dst += gvnc->local.linesize;
@@ -88,7 +88,7 @@ static void BLIT(struct gvnc *gvnc, uint
 		int j;
 
 		for (j = 0; j < w; j++) {
-			SET_PIXEL(gvnc, dp, sp);
+			SET_PIXEL(gvnc, dp, SWAP(gvnc, *sp));
 			dp++;
 			sp++;
 		}
@@ -171,11 +171,36 @@ static void RICH_CURSOR_BLIT(struct gvnc
 	uint32_t *dst = (uint32_t *)pixbuf;
 	uint8_t *src = image;
 	uint8_t *alpha = mask;
-	int rs, gs, bs;
+	int as, rs, gs, bs, n;
 
-	rs = 24 - ((sizeof(src_pixel_t) * 8) - gvnc->fmt.red_shift);
-	gs = 16 - (gvnc->fmt.red_shift - gvnc->fmt.green_shift);
-	bs = 8 - (gvnc->fmt.green_shift - gvnc->fmt.blue_shift);
+	/*
+	 * GdkPixbuf is always 32-bit RGB, so we can't use the precomputed
+	 * left / right shift data from gvnc->{r,g,b}{r,l}s. The latter
+	 * is set for the local display depth, which may be different
+	 * to GdkPixbuf's fixed 32-bit RGBA
+	 *
+	 * This function isn't called often, so just re-compute them now
+	 */
+
+#if G_BYTE_ORDER == G_BIG_ENDIAN
+	as = 0;
+	rs = 8;
+	gs = 16;
+	bs = 24;
+#else
+	as = 24;
+	rs = 16;
+	gs = 8;
+	bs = 0;
+#endif
+
+	/* Then this adjusts for remote having less bpp than 32 */
+	for (n = 255 ; n > gvnc->fmt.red_max ; n>>= 1)
+		rs++;
+	for (n = 255 ; n > gvnc->fmt.green_max ; n>>= 1)
+		gs++;
+	for (n = 255 ; n > gvnc->fmt.blue_max ; n>>= 1)
+		bs++;
 
 	for (y1 = 0; y1 < height; y1++) {
 		src_pixel_t *sp = (src_pixel_t *)src;
@@ -186,7 +211,7 @@ static void RICH_CURSOR_BLIT(struct gvnc
 				| (COMPONENT(blue, *sp) << bs);
 
 			if ((mp[x1 / 8] >> (7 - (x1 % 8))) & 1)
-				*dst |= 0xFF000000;
+				*dst |= (0xFF << as);
 
 			dst++;
 			sp++;
@@ -209,6 +234,11 @@ static void RGB24_BLIT(struct gvnc *gvnc
 		uint8_t *sp = data;
 
 		for (i = 0; i < width; i++) {
+			/*
+			 * We use gvnc->fmt.XXX_shift instead of usual gvnc->Xls
+			 * because the source pixel component is a full 8 bits in
+			 * size, and so doesn't need the adjusted shift
+			 */
 			*dp = (((sp[0] * gvnc->fmt.red_max) / 255) << gvnc->fmt.red_shift) |
 				(((sp[1] * gvnc->fmt.green_max) / 255) << gvnc->fmt.green_shift) |
 				(((sp[2] * gvnc->fmt.blue_max) / 255) << gvnc->fmt.blue_shift);
diff -r fb8ab9818527 src/gvnc.c
--- a/src/gvnc.c	Fri Mar 07 11:24:19 2008 -0600
+++ b/src/gvnc.c	Tue Mar 18 17:57:26 2008 -0400
@@ -3022,18 +3022,6 @@ gboolean gvnc_set_local(struct gvnc *gvn
 	gvnc->gls = gvnc->local.green_shift;
 	gvnc->bls = gvnc->local.blue_shift;
 
-
-	/* This adjusts for server/client endianness mismatch */
-	if (G_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++;
diff -r fb8ab9818527 src/vncdisplay.c
--- a/src/vncdisplay.c	Fri Mar 07 11:24:19 2008 -0600
+++ b/src/vncdisplay.c	Tue Mar 18 17:57:26 2008 -0400
@@ -750,9 +750,17 @@ static void setup_gdk_image(VncDisplay *
 	VncDisplayPrivate *priv = obj->priv;
 	GdkVisual *visual;
 
-	visual = gdk_drawable_get_visual(GTK_WIDGET(obj)->window);
+	visual = gdk_screen_get_system_visual(gdk_screen_get_default());
 	
 	priv->image = gdk_image_new(GDK_IMAGE_FASTEST, visual, width, height);
+	GVNC_DEBUG("Visual mask: %3d %3d %3d\n      shift: %3d %3d %3d\n",
+		   visual->red_mask,
+		   visual->green_mask,
+		   visual->blue_mask,
+		   visual->red_shift,
+		   visual->green_shift,
+		   visual->blue_shift);
+
 	priv->fb.red_mask = visual->red_mask >> visual->red_shift;
 	priv->fb.green_mask = visual->green_mask >> visual->green_shift;
 	priv->fb.blue_mask = visual->blue_mask >> visual->blue_shift;

-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|