Date: Mon, 8 May 2006 18:24:53 +1000 (EST) From: Peter Jeremy <peterjeremy@optushome.com.au> To: FreeBSD-gnats-submit@FreeBSD.org Cc: peter@vk2pj.dyndns.org Subject: ports/96971: [patch] graphics/xv incorrectly handles xwd files Message-ID: <200605080824.k488Orpv001057@turion.vk2pj.dyndns.org> Resent-Message-ID: <200605080830.k488U9OZ037119@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>Number: 96971
>Category: ports
>Synopsis: [patch] graphics/xv incorrectly handles xwd files
>Confidential: no
>Severity: non-critical
>Priority: low
>Responsible: freebsd-ports-bugs
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: sw-bug
>Submitter-Id: current-users
>Arrival-Date: Mon May 08 08:30:08 GMT 2006
>Closed-Date:
>Last-Modified:
>Originator: Peter Jeremy
>Release: FreeBSD 6.1-STABLE amd64
>Organization:
n/a
>Environment:
System: FreeBSD turion.vk2pj.dyndns.org 6.1-STABLE FreeBSD 6.1-STABLE #9: Mon May 8 12:06:14 EST 2006 root@turion.vk2pj.dyndns.org:/usr/obj/usr/src/sys/turion amd64
xv-3.10a_5
>Description:
With 24/32-bit xwd files, xv swaps the red and blue channels.
With 16-bit xwd files, the image is very dark green (almost black).
Both problems are caused by hard-coding the channel order and
offsets, rather than using the colour masks in the xwd header.
xv reads the input into a 24-bit internal image, which is then
displayed. The lack of brightness in the 16-bit display is
because the colour values are copied into the low-order bits of
the internal pixmap rather than the high order bits. The green
hue is because the green channel has 6 bits, whereas red and
blue only have 5 bits, making the green twice as (relatively)
bright.
Thanks to Callum Gibson for initial pointers to the bug as well
as 16-bit dumps.
>How-To-Repeat:
Do a window or screen dump using xwd from a 24/32-bit TrueColor
display. Display the screen dump using xv.
Do a window or screen dump using xwd from a 16-bit TrueColor
display. Display the screen dump using xv.
>Fix:
The following patch replaces the byte-swap routines as well - I
initially suspected that the problem was at least partially
caused by the non-portability of "union cheat". Whilst that was
not a problem, the resultant code is cleaner and I did not want
to expend further effort removing the change.
Rather than hard-coding the shift values (and the mask for 24-bit
colour), the shift value is calculated from the mask during
initialisation. Since the low colour bits must be shifted left
whilst all other shifts are right, each pixel colour is shifted
so the MSB of the colour is in bit 31 and then shifted right 24
bits to correctly locate the colour in an 8-bit field.
--- xvxwd.c~ Sat May 6 07:29:07 2006
+++ xvxwd.c Sat May 6 17:24:12 2006
@@ -19,7 +19,7 @@
*/
#include "xv.h"
-
+#include <sys/endian.h>
/***************************** x11wd.h *****************************/
#define X11WD_FILE_VERSION 7
@@ -66,23 +66,25 @@
typedef byte pixel;
+#define bs_short(x) bswap16(x)
+#define bs_long(x) bswap32(x)
+
/* local functions */
static int getinit PARM((FILE *, int*, int*, int*, CARD32 *,
CARD32, PICINFO *));
static CARD32 getpixnum PARM((FILE *));
static int xwdError PARM((char *));
static void xwdWarning PARM((char *));
-static int bs_short PARM((int));
-static CARD32 bs_long PARM((CARD32));
static int readbigshort PARM((FILE *, CARD16 *));
static int readbiglong PARM((FILE *, CARD32 *));
static int readlittleshort PARM((FILE *, CARD16 *));
static int readlittlelong PARM((FILE *, CARD32 *));
static int writebigshort PARM((FILE *, int));
static int writebiglong PARM((FILE *, CARD32));
-
+static int get_shift PARM((CARD32));
static byte *pic8, *pic24;
static CARD32 red_mask, green_mask, blue_mask;
+static int red_shift, green_shift, blue_shift;
static int bits_per_item, bits_used, bit_shift, bits_per_pixel;
static char buf[4];
static char *byteP;
@@ -181,24 +183,9 @@
CARD32 ul;
ul = getpixnum(ifp);
- switch (bits_per_pixel) {
- case 16:
- *xP++ = ((ul & red_mask) >> 0);
- *xP++ = ((ul & green_mask) >> 5);
- *xP++ = ((ul & blue_mask) >> 10);
- break;
-
- case 24:
- case 32:
- *xP++ = (ul ) & 0xff;
- *xP++ = (ul>> 8) & 0xff;
- *xP++ = (ul>>16) & 0xff;
- break;
-
- default:
- xwdError("True/Direct only supports 16, 24, and 32 bits");
- return 0;
- }
+ *xP++ = ((ul & red_mask) << red_shift) >> 24;
+ *xP++ = ((ul & green_mask) << green_shift) >> 24;
+ *xP++ = ((ul & blue_mask) << blue_shift) >> 24;
}
for (col=0; col<padright; col++) getpixnum(ifp);
@@ -266,6 +253,8 @@
byte_swap = 1;
h11P->header_size = bs_long(h11P->header_size);
h11P->file_version = bs_long(h11P->file_version);
+ if (h11P->file_version != X11WD_FILE_VERSION)
+ return(xwdError("unsupported X11 XWD file version"));
h11P->pixmap_format = bs_long(h11P->pixmap_format);
h11P->pixmap_depth = bs_long(h11P->pixmap_depth);
h11P->pixmap_width = bs_long(h11P->pixmap_width);
@@ -427,6 +416,10 @@
green_mask = h11P->green_mask;
blue_mask = h11P->blue_mask;
+ red_shift = get_shift(red_mask);
+ green_shift = get_shift(green_mask);
+ blue_shift = get_shift(blue_mask);
+
byteP = (char *) buf;
shortP = (CARD16 *) buf;
longP = (CARD32 *) buf;
@@ -531,44 +524,6 @@
/*
- * Byte-swapping junk.
- */
-
-union cheat {
- CARD32 l;
- CARD16 s;
- CARD8 c[sizeof(CARD32)];
-};
-
-static int bs_short(s)
- int s;
-{
- union cheat u;
- unsigned char t;
-
- u.s = (CARD16) s;
- t = u.c[0]; u.c[0] = u.c[1]; u.c[1] = t;
- return (int) u.s;
-}
-
-static CARD32 bs_long(l)
- CARD32 l;
-{
- union cheat u;
- unsigned char t;
-
- u.l = l;
- t = u.c[0]; u.c[0] = u.c[3]; u.c[3] = t;
- t = u.c[1]; u.c[1] = u.c[2]; u.c[2] = t;
- return u.l;
-}
-
-
-
-
-
-
-/*
* Endian I/O.
*/
@@ -642,4 +597,17 @@
putc((s>>8)&0xff, out);
putc(s&0xff, out);
return 0;
+}
+
+
+static int get_shift(val)
+ CARD32 val;
+{
+ int shift;
+
+ if (!val)
+ return (0);
+ for (shift = 0; !(val & 0x80000000); shift++)
+ val <<= 1;
+ return (shift);
}
>Release-Note:
>Audit-Trail:
>Unformatted:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200605080824.k488Orpv001057>