Re: [gtk-vnc-devel] Fix pixel shift for BGR format servers

[Anthony Liguori <anthony codemonkey ws>]

Daniel P. Berrange wrote:
> The code which currently calculates the pixel shift for converting data
> from remote to local encoding is assuming the server is in RGB format
> pixels. If I run a server in BGR format, then the colours are rather
> messed up. I've attached a patch which tries to correct it - although its
> not perfect because now, the code assume either RGB, or BGR - is it possible
> for a server to use GRB, or GBR ?!?!  If not then this may be sufficient.
>
> I'm also not entirely convinced this is correct wrt to local server
> format, eg if your local desktop ran in BGR instead of RGB. I don't have
> any system I can make run in BGR locally though, so can't test.
>
> BTW, for testing I was running a server with
>
> vncserver -geometry 1024x800 -depth 16 -pixelformat bgr565
>
> And another with
>
> vncserver -geometry 1024x800 -depth 16 -pixelformat rgb565
>
> With this patch, both now work.
>   

VNC servers and clients are notoriously bad at respecting 
SetPixelFormat().  While I don't think we properly support the 
big_endian_flag, I do think we're doing right thing with the 
mask/shifts.  Are you sure that RealVNC isn't screwing up here?  Your 
patch looks a big magical to me.

Is the current code really assuming RGB?  It just does whatever the 
current shift/masks support.

Regards,

Anthony Liguori

> Dan.
>   
> ------------------------------------------------------------------------
>
> diff -r 8d2ec85be8c3 src/gvnc.c
> --- a/src/gvnc.c	Tue Jul 10 13:34:34 2007 -0400
> +++ b/src/gvnc.c	Tue Jul 10 17:10:57 2007 -0400
> @@ -127,7 +127,7 @@ enum {
>  };
>  
>  
> -#define DEBUG 0
> +#define DEBUG 1
>  #if DEBUG
>  #define GVNC_DEBUG(fmt, ...) do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0)
>  
> @@ -543,7 +543,7 @@ static void gvnc_read_pixel_format(struc
>  	fmt->big_endian_flag = gvnc_read_u8(gvnc);
>  	fmt->true_color_flag = gvnc_read_u8(gvnc);
>  
> -	GVNC_DEBUG("Pixel format BPP: %d,  Depth: %d, Endian: %d, True color: %d\n",
> +	GVNC_DEBUG("Read Pixel format BPP: %d,  Depth: %d, Endian: %d, True color: %d\n",
>  		   fmt->bits_per_pixel, fmt->depth, fmt->big_endian_flag, fmt->true_color_flag);
>  
>  	fmt->red_max         = gvnc_read_u16(gvnc);
> @@ -554,6 +554,11 @@ static void gvnc_read_pixel_format(struc
>  	fmt->green_shift     = gvnc_read_u8(gvnc);
>  	fmt->blue_shift      = gvnc_read_u8(gvnc);
>  
> +	GVNC_DEBUG("Pixel max: R: %d, G: %d, B:%d\n",
> +		   fmt->red_max, fmt->green_max, fmt->blue_max);
> +	GVNC_DEBUG("Pixel shift: R: %d, G: %d, B:%d\n",
> +		   fmt->red_shift, fmt->green_shift, fmt->blue_shift);
> +
>  	gvnc_read(gvnc, pad, 3);
>  }
>  
> @@ -571,6 +576,9 @@ gboolean gvnc_set_pixel_format(struct gv
>  
>  	gvnc_write_u8(gvnc, 0);
>  	gvnc_write(gvnc, pad, 3);
> +
> +	GVNC_DEBUG("Write Pixel format BPP: %d,  Depth: %d, Endian: %d, True color: %d\n",
> +		   fmt->bits_per_pixel, fmt->depth, fmt->big_endian_flag, fmt->true_color_flag);
>  
>  	gvnc_write_u8(gvnc, fmt->bits_per_pixel);
>  	gvnc_write_u8(gvnc, fmt->depth);
> @@ -584,6 +592,11 @@ gboolean gvnc_set_pixel_format(struct gv
>  	gvnc_write_u8(gvnc, fmt->red_shift);
>  	gvnc_write_u8(gvnc, fmt->green_shift);
>  	gvnc_write_u8(gvnc, fmt->blue_shift);
> +
> +	GVNC_DEBUG("Pixel max: R: %d, G: %d, B:%d\n",
> +		   fmt->red_max, fmt->green_max, fmt->blue_max);
> +	GVNC_DEBUG("Pixel shift: R: %d, G: %d, B:%d\n",
> +		   fmt->red_shift, fmt->green_shift, fmt->blue_shift);
>  
>  	gvnc_write(gvnc, pad, 3);
>  	gvnc_flush(gvnc);
> @@ -1534,16 +1547,26 @@ gboolean gvnc_set_local(struct gvnc *gvn
>  	if (depth == 32)
>  		depth = 24;
>  
> -	gvnc->rp =  (gvnc->local.depth - gvnc->local.red_shift);
> -	gvnc->rp -= (depth - gvnc->fmt.red_shift);
> -	gvnc->gp =  (gvnc->local.red_shift - gvnc->local.green_shift);
> -	gvnc->gp -= (gvnc->fmt.red_shift - gvnc->fmt.green_shift);
> -	gvnc->bp =  (gvnc->local.green_shift - gvnc->local.blue_shift);
> -	gvnc->bp -= (gvnc->fmt.green_shift - gvnc->fmt.blue_shift);
> -
> -	gvnc->rp = gvnc->local.red_shift + gvnc->rp;
> -	gvnc->gp = gvnc->local.green_shift + gvnc->gp;
> -	gvnc->bp = gvnc->local.blue_shift + gvnc->bp;
> +	GVNC_DEBUG("Depth local %d remote %d\n", gvnc->local.depth, depth);
> +	if (gvnc->fmt.red_shift > gvnc->fmt.blue_shift) {
> +		gvnc->rp =  gvnc->local.depth;
> +		gvnc->rp -= (depth - gvnc->fmt.red_shift);
> +
> +		gvnc->gp =  gvnc->local.red_shift;
> +		gvnc->gp -= (gvnc->fmt.red_shift - gvnc->fmt.green_shift);
> +
> +		gvnc->bp =  gvnc->local.green_shift;
> +		gvnc->bp -= (gvnc->fmt.green_shift - gvnc->fmt.blue_shift);
> +	} else {
> +		gvnc->rp =  gvnc->local.depth;
> +		gvnc->rp -= (gvnc->fmt.green_shift - gvnc->fmt.red_shift);
> +
> +		gvnc->gp =  gvnc->local.red_shift;
> +		gvnc->gp -= (gvnc->fmt.blue_shift - gvnc->fmt.green_shift);
> +
> +		gvnc->bp =  gvnc->local.green_shift;
> +		gvnc->bp -= (depth - gvnc->fmt.blue_shift);
> +	}
>  
>  	gvnc->rm = gvnc->local.red_mask & gvnc->fmt.red_max;
>  	gvnc->gm = gvnc->local.green_mask & gvnc->fmt.green_max;
>   
> ------------------------------------------------------------------------
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by DB2 Express
> Download DB2 Express C - the FREE version of DB2 express and take
> control of your XML. No limits. Just data. Click to get it now.
> http://sourceforge.net/powerbar/db2/
> ------------------------------------------------------------------------
>
> _______________________________________________
> Gtk-vnc-devel mailing list
> Gtk-vnc-devel lists sourceforge net
> https://lists.sourceforge.net/lists/listinfo/gtk-vnc-devel
>