GNOME.org
 

[gtk-vnc-devel] PATCH: Yet more endianness fixes

Despite hoping that my previous endianness fix would be the end of the
story, it wasn't. It did correctly fix the case where the client and
server were different endianess, but it did not take into account the
fact that the client & the X display could be different endianess. This
latter case of mis-matched client + X display is what Hiroyuki Kaguchi
tried to fixed in changeset 147:01b34fbc6e99. That changeset had broken
the mis-matched client + VNC server case. My change 189:767a847dbdc9
had reversed the brokenness yet again.

The core factor we've been missing is that the combinatorial problem is
a triple, not a tuple:

  (VNC server, GTK-VNC client, X display)

So the logical pixel conversion sequence we need to implement is:

  1. Endianess conversion VNC server -> GTK-VNC client 
  2. Extract/scale RGB components according VNC pixel format shifts/masks
  3. Scale/combine RGB components according to X server image visual
  4. Endianess conversion GTK-VNC client -> X server image visual

Obviously either of steps 1 & 4 may be no-ops in some cases.

This is actually fairly simple - instead of having a single SWAP() macro
we need two  SWAP_RFB (for step 1) and SWAP_IMG (for step 4), and just
use them at appropriate places in blt.h.  This is the bulk of my patch
to blt.h.

The changes to gvnc.c then deal with a couple of other issues. First of
all the RealVNC server does not follow the RFB spec for CPIXEL (or the
RFB spec is badly written). Basically when looking to see if a CPIXEL is
3-bytes, we just examine the VNC pixel format depth field & compare to
24. This is not what RealVNC does when encoding the data. It look at each
individual colour component to see if they all fit in the most or least
significant 3 bytes. So I change the cpixel decoding to follow this logic.

Second, RealVNC little endian servers are broken if you run with -depth 32
argument, giving a red-max of 2047, green-max 2047 and blue-max 1023, but
then still encoding cpixel as 3 bytes when it should in fact require 4.
So I disable use of ZRLE encoding if the advertised server pixel format
is in this configuration.

To test this all requires 2 machines of different endianness, each running
3 VNC servers (16, 24 and 32 bpp), and SSH. 

For each of the 16, 24 and 32 bpp colour depths, the following combinations
were tested:

 - GTK-VNC on i386, direct X on i386, VNC Server on i386
 - GTK-VNC on i386, direct X on i386, VNC Server on ppc
 - GTK-VNC on i386, ssh tunnel to X on ppc, VNC Server on i386
 - GTK-VNC on i386, ssh tunnel to X on ppc, VNC Server on ppc

 - GTK-VNC on ppc, ssh tunnel X on i386, VNC Server on i386
 - GTK-VNC on ppc, ssh tunnel X on i386, VNC Server on ppc
 - GTK-VNC on ppc, direct X on ppc, VNC Server on i386
 - GTK-VNC on ppc, direct X on ppc, VNC Server on ppc

Finally all these combinations work for Raw, hextile, zrle and rich cursor
encodings. I've not tested tight or the jpeg encodings

Dan.

diff -r ded3f3d05205 src/blt.h
--- a/src/blt.h	Tue Mar 25 14:59:10 2008 -0300
+++ b/src/blt.h	Thu Mar 27 11:46:22 2008 -0400
@@ -16,8 +16,9 @@
 #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_, SRC)(gvnc, pixel)
-#define COMPONENT(color, pixel) ((SWAP(gvnc, pixel) >> gvnc->fmt.SPLICE(color, _shift) & gvnc->fmt.SPLICE(color, _max)))
+#define SWAP_RFB(gvnc, pixel) SPLICE(gvnc_swap_rfb_, SRC)(gvnc, pixel)
+#define SWAP_IMG(gvnc, pixel) SPLICE(gvnc_swap_img_, DST)(gvnc, pixel)
+#define COMPONENT(color, pixel) ((SWAP_RFB(gvnc, pixel) >> gvnc->fmt.SPLICE(color, _shift) & gvnc->fmt.SPLICE(color, _max)))
 
 static void FAST_FILL(struct gvnc *gvnc, src_pixel_t *sp,
 		      int x, int y, int width, int height)
@@ -43,16 +44,16 @@ static void FAST_FILL(struct gvnc *gvnc,
 
 static void SET_PIXEL(struct gvnc *gvnc, dst_pixel_t *dp, src_pixel_t sp)
 {
-	*dp = ((sp >> gvnc->rrs) & gvnc->rm) << gvnc->rls
-		| ((sp >> gvnc->grs) & gvnc->gm) << gvnc->gls
-		| ((sp >> gvnc->brs) & gvnc->bm) << gvnc->bls;
+	*dp = SWAP_IMG(gvnc, ((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, SWAP(gvnc, *sp));
+	SET_PIXEL(gvnc, dp, SWAP_RFB(gvnc, *sp));
 }
 
 static void FILL(struct gvnc *gvnc, src_pixel_t *sp,
@@ -66,7 +67,7 @@ static void FILL(struct gvnc *gvnc, src_
 		int j;
 
 		for (j = 0; j < width; j++) {
-			SET_PIXEL(gvnc, dp, SWAP(gvnc, *sp));
+			SET_PIXEL(gvnc, dp, SWAP_RFB(gvnc, *sp));
 			dp++;
 		}
 		dst += gvnc->local.linesize;
@@ -88,7 +89,7 @@ static void BLIT(struct gvnc *gvnc, uint
 		int j;
 
 		for (j = 0; j < w; j++) {
-			SET_PIXEL(gvnc, dp, SWAP(gvnc, *sp));
+			SET_PIXEL(gvnc, dp, SWAP_RFB(gvnc, *sp));
 			dp++;
 			sp++;
 		}
@@ -272,7 +273,7 @@ static void TIGHT_COMPUTE_PREDICTED(stru
 	blue = MAX(blue, 0);
 	blue = MIN(blue, gvnc->fmt.blue_max);
 
-	*ppixel = SWAP(gvnc,
+	*ppixel = SWAP_RFB(gvnc,
 		       (red << gvnc->fmt.red_shift) |
 		       (green << gvnc->fmt.green_shift) |
 		       (blue << gvnc->fmt.blue_shift));
@@ -287,7 +288,7 @@ static void TIGHT_SUM_PIXEL(struct gvnc 
 	green = COMPONENT(green, *lhs) + COMPONENT(green, *rhs);
 	blue = COMPONENT(blue, *lhs) + COMPONENT(blue, *rhs);
 
-	*lhs = SWAP(gvnc,
+	*lhs = SWAP_RFB(gvnc,
 		    ((red & gvnc->fmt.red_max) << gvnc->fmt.red_shift) |
 		    ((green & gvnc->fmt.green_max) << gvnc->fmt.green_shift) |
 		    ((blue & gvnc->fmt.blue_max) << gvnc->fmt.blue_shift));
diff -r ded3f3d05205 src/gvnc.c
--- a/src/gvnc.c	Tue Mar 25 14:59:10 2008 -0300
+++ b/src/gvnc.c	Thu Mar 27 11:46:22 2008 -0400
@@ -826,14 +826,39 @@ gboolean gvnc_set_encodings(struct gvnc 
 gboolean gvnc_set_encodings(struct gvnc *gvnc, int n_encoding, int32_t *encoding)
 {
 	uint8_t pad[1] = {0};
-	int i;
+	int i, skip_zrle=0;
+
+	/*
+	 * RealVNC server is broken for ZRLE in some pixel formats.
+	 * Specifically if you have a format with either R, G or B
+	 * components with a max value > 255, it still uses a CPIXEL
+	 * of 3 bytes, even though the colour requirs 4 bytes. It
+	 * thus messes up the colours of the server in a way we can't
+	 * recover from on the client. Most VNC clients don't see this
+	 * problem since they send a 'set pixel format' message instead
+	 * of running with the server's default format.
+	 *
+	 * So we kill off ZRLE encoding for problematic pixel formats
+	 */
+	for (i = 0; i < n_encoding; i++)
+		if (gvnc->fmt.depth == 32 &&
+		    (gvnc->fmt.red_max > 255 ||
+		     gvnc->fmt.blue_max > 255 ||
+		     gvnc->fmt.green_max > 255) &&
+		    encoding[i] == GVNC_ENCODING_ZRLE) {
+			GVNC_DEBUG("Dropping ZRLE encoding for broken pixel format\n");
+			skip_zrle++;
+		}
 
 	gvnc->has_ext_key_event = FALSE;
 	gvnc_write_u8(gvnc, 2);
 	gvnc_write(gvnc, pad, 1);
-	gvnc_write_u16(gvnc, n_encoding);
-	for (i = 0; i < n_encoding; i++)
+	gvnc_write_u16(gvnc, n_encoding - skip_zrle);
+	for (i = 0; i < n_encoding; i++) {
+		if (skip_zrle && encoding[i] == GVNC_ENCODING_ZRLE)
+			continue;
 		gvnc_write_s32(gvnc, encoding[i]);
+	}
 	gvnc_flush(gvnc);
 	return !gvnc_has_error(gvnc);
 }
@@ -951,23 +976,52 @@ static inline uint8_t *gvnc_get_local(st
 		(x * gvnc->local.bpp);
 }
 
-static uint8_t gvnc_swap_8(struct gvnc *gvnc G_GNUC_UNUSED, uint8_t pixel)
+static uint8_t gvnc_swap_img_8(struct gvnc *gvnc G_GNUC_UNUSED, uint8_t pixel)
 {
 	return pixel;
 }
 
-static uint16_t gvnc_swap_16(struct gvnc *gvnc, uint16_t pixel)
+static uint8_t gvnc_swap_rfb_8(struct gvnc *gvnc G_GNUC_UNUSED, uint8_t pixel)
 {
-	if (gvnc->fmt.byte_order != gvnc->local.byte_order)
+	return pixel;
+}
+
+/* local host native format -> X server image format */
+static uint16_t gvnc_swap_img_16(struct gvnc *gvnc, uint16_t pixel)
+{
+	if (G_BYTE_ORDER != gvnc->local.byte_order)
 		return  (((pixel >> 8) & 0xFF) << 0) |
 			(((pixel >> 0) & 0xFF) << 8);
 	else
 		return pixel;
 }
 
-static uint32_t gvnc_swap_32(struct gvnc *gvnc, uint32_t pixel)
+/* VNC server RFB  format ->  local host native format */
+static uint16_t gvnc_swap_rfb_16(struct gvnc *gvnc, uint16_t pixel)
 {
-	if (gvnc->fmt.byte_order != gvnc->local.byte_order)
+	if (gvnc->fmt.byte_order != G_BYTE_ORDER)
+		return  (((pixel >> 8) & 0xFF) << 0) |
+			(((pixel >> 0) & 0xFF) << 8);
+	else
+		return pixel;
+}
+
+/* local host native format -> X server image format */
+static uint32_t gvnc_swap_img_32(struct gvnc *gvnc, uint32_t pixel)
+{
+	if (G_BYTE_ORDER != gvnc->local.byte_order)
+		return  (((pixel >> 24) & 0xFF) <<  0) |
+			(((pixel >> 16) & 0xFF) <<  8) |
+			(((pixel >>  8) & 0xFF) << 16) |
+			(((pixel >>  0) & 0xFF) << 24);			
+	else
+		return pixel;
+}
+
+/* VNC server RFB  format ->  local host native format */
+static uint32_t gvnc_swap_rfb_32(struct gvnc *gvnc, uint32_t pixel)
+{
+	if (gvnc->fmt.byte_order != G_BYTE_ORDER)
 		return  (((pixel >> 24) & 0xFF) <<  0) |
 			(((pixel >> 16) & 0xFF) <<  8) |
 			(((pixel >>  8) & 0xFF) << 16) |
@@ -1206,10 +1260,26 @@ static void gvnc_read_cpixel(struct gvnc
 
 	memset(pixel, 0, bpp);
 
-	if (bpp == 4 && gvnc->fmt.true_color_flag && gvnc->fmt.depth == 24) {
-		bpp = 3;
-		if (gvnc->fmt.byte_order == G_BIG_ENDIAN)
-			pixel += 1;
+	if (bpp == 4 && gvnc->fmt.true_color_flag) {
+		int fitsInMSB = ((gvnc->fmt.red_shift > 7) &&
+				 (gvnc->fmt.green_shift > 7) &&
+				 (gvnc->fmt.blue_shift > 7));
+		int fitsInLSB = (((gvnc->fmt.red_max << gvnc->fmt.red_shift) < (1 << 24)) &&
+				 ((gvnc->fmt.green_max << gvnc->fmt.green_shift) < (1 << 24)) &&
+				 ((gvnc->fmt.blue_max << gvnc->fmt.blue_shift) < (1 << 24)));
+
+		/* 
+		 * We need to analyse the shifts to see if they fit in 3 bytes,
+		 * rather than looking at the declared  'depth' for the format
+		 * because despite what the RFB spec says, this is what RealVNC
+		 * server actually does in practice.
+		 */
+		if (fitsInMSB || fitsInLSB) {
+			bpp = 3;
+			if (gvnc->fmt.depth == 24 &&
+			    gvnc->fmt.byte_order == G_BIG_ENDIAN)
+				pixel++;
+		}
 	}
 
 	gvnc_read(gvnc, pixel, bpp);
@@ -3012,7 +3082,8 @@ 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 &&
-	    fb->byte_order == gvnc->fmt.byte_order)
+	    fb->byte_order == G_BYTE_ORDER &&
+	    gvnc->fmt.byte_order == G_BYTE_ORDER)
 		gvnc->perfect_match = TRUE;
 	else
 		gvnc->perfect_match = FALSE;


-- 
|: 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 :|
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]