tinyjpeg: Dynamic luminance quantization table for Pixart JPEG

In PJPG blocks, a marker gives the quantization tables to use for image decoding. This patch dynamically updates the luminance table when the marker changes.

Note that the values of this table have been guessed from a small number of images and that they may not work fine in some situations, but, in most cases, the images are better.

index 702a2a2..4041251 100644
@@ -103,6 +103,7 @@ struct jdec_private {
 #if SANITY_CHECK
 	unsigned int current_cid;			/* For planar JPEG */
 #endif
+	unsigned char marker;			/* for PJPG (Pixart JPEG) */
 
 	/* Temp space used after the IDCT to store each components */
 	uint8_t Y[64 * 4], Cr[64], Cb[64];

Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
t

Hi,

Thanks for your work on this! I've just spend almost 4 days wrestling which the Pixart JPEG decompression code to try to better understand these cams, and I have learned quite a bit and eventually came up with a different approach.

But your effort is appreciated! After spending so much time on this myself, I can imagine that it took you quite some time to come up with your solution.

Attach is a 4 patch patchset which I plan to push to v4l-utils tomorrow (after running some more tests in daylight). I'll also try to do some kernel patches tomorrow to match...

Thanks & Regards,

Hans

From: Hans de Goede <[email protected]>
Date: Mon, 23 Apr 2012 19:43:07 +0200
Subject: [PATCH 1/4] libv4lconvert: Fix decoding of 160x120 Pixart JPEG
images

---
 lib/libv4lconvert/tinyjpeg.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

index 2c2d4af..d2a7d3f 100644
@@ -2101,7 +2101,17 @@ static int pixart_filter(struct jdec_private *priv, unsigned char *dest,
 {
 	int chunksize, copied = 0;
 
-	/* Skip mysterious first data byte */
+	/* The first data bytes encodes the image size:
+	   0x60: 160x120
+	   0x61: 320x240
+	   0x62: 640x480
+	   160x120 images are not chunked due to their small size!
+	*/
+	if (src[0] == 0x60) {
+			memcpy(dest, src + 1, n - 1);
+			return n - 1;
+	}
+
 	src++;
 	n--;
 

@@ -2124,8 +2134,8 @@ kernel: 0xff 0xff 0x00 0xff 0x96, and we skip one unknown byte */
 
 		if (src[0] != 0xff || src[1] != 0xff || src[2] != 0xff)
 			error("Missing Pixart ff ff ff xx header, "
-					"got: %02x %02x %02x %02x\n",
-					src[0], src[1], src[2], src[3]);
+			      "got: %02x %02x %02x %02x, copied sofar: %d\n",
+			      src[0], src[1], src[2], src[3], copied);
 		if (src[3] > 6)
 			error("Unexpected Pixart chunk size: %d\n", src[3]);
 

1.7.10

From: Hans de Goede <[email protected]>
Date: Sat, 21 Apr 2012 14:39:58 +0200
Subject: [PATCH 2/4] Revert "tinyjpeg: Better luminance quantization table
for Pixart JPEG"

This reverts commit e186777daeaa717b7d919e932f7d3be10156d572.

---
 lib/libv4lconvert/tinyjpeg.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

index d2a7d3f..756ad9c 100644
@@ -206,14 +206,14 @@ static const unsigned char val_ac_chrominance[] = {
 };
 
 const unsigned char pixart_quantization[][64] = { {
-		0x08, 0x08, 0x08, 0x08, 0x08, 0x08, 0x10, 0x10,
-		0x10, 0x10, 0x10, 0x10, 0x10, 0x10, 0x10, 0x10,
-		0x10, 0x10, 0x10, 0x10, 0x10, 0x10, 0x10, 0x10,
-		0x10, 0x10, 0x10, 0x10, 0x20, 0x20, 0x20, 0x20,
-		0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
-		0x20, 0x20, 0x20, 0x40, 0x40, 0x40, 0x40, 0x40,
-		0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40,
-		0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40,
+		0x07, 0x07, 0x08, 0x0a, 0x09, 0x07, 0x0d, 0x0b,
+		0x0c, 0x0d, 0x11, 0x10, 0x0f, 0x12, 0x17, 0x27,
+		0x1a, 0x18, 0x16, 0x16, 0x18, 0x31, 0x23, 0x25,
+		0x1d, 0x28, 0x3a, 0x33, 0x3d, 0x3c, 0x39, 0x33,
+		0x38, 0x37, 0x40, 0x48, 0x5c, 0x4e, 0x40, 0x44,
+		0x57, 0x45, 0x37, 0x38, 0x50, 0x6d, 0x51, 0x57,
+		0x5f, 0x62, 0x67, 0x68, 0x67, 0x3e, 0x4d, 0x71,
+		0x79, 0x70, 0x64, 0x78, 0x5c, 0x65, 0x67, 0x63,
 	},
 	{
 		0x11, 0x12, 0x12, 0x18, 0x15, 0x18, 0x2f, 0x1a,

1.7.10

From: Hans de Goede <[email protected]>
Date: Mon, 23 Apr 2012 23:03:40 +0200
Subject: [PATCH 3/4] libv4lconvert: Dynamic quantization tables for Pixart
JPEG
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Inspired by a patch from Jean-François Moine <[email protected]>, I've spend 4 days on a row investigating (through trial and error) Pixart's JPEG compression. This patch accumulates what I've learned from this, giving 2 important improvements:
1) Support for properly decompressing pac7302 generated images where some of the MCU-s are compressed with a lower quality / higher quantization values
2) Proper chrominance quantization tables for Pixart JPEG, getting rid of the sometimes horribly over saturation our decompression code was causing

The support for dynamic quantization tables this patch adds also allows us to enable lower compression ratios in the kernel driver when running at a lower framerate, resulting in better image quality.

---
 lib/libv4lconvert/tinyjpeg-internal.h |    1 +
 lib/libv4lconvert/tinyjpeg.c          |   77 ++++++++++++++++++++++-----------
 2 files changed, 53 insertions(+), 25 deletions(-)

index 702a2a2..4041251 100644
@@ -103,6 +103,7 @@ struct jdec_private {
 #if SANITY_CHECK
 	unsigned int current_cid;			/* For planar JPEG */
 #endif
+	unsigned char marker;			/* for PJPG (Pixart JPEG) */
 
 	/* Temp space used after the IDCT to store each components */
 	uint8_t Y[64 * 4], Cr[64], Cb[64];

1.7.10

From: Hans de Goede <[email protected]>
Date: Mon, 23 Apr 2012 23:18:24 +0200
Subject: [PATCH 4/4] libv4lconvert: Drop Pixart JPEG frames with changing
chrominance setting

Sometimes the pac7302 switches chrominance setting halfway though a frame, with a quite ugly looking result, so lets drop such frames.

---
 lib/libv4lconvert/tinyjpeg-internal.h |    1 +
 lib/libv4lconvert/tinyjpeg.c          |   11 +++++++++++
 2 files changed, 12 insertions(+)

index 4041251..dcbcf27 100644
@@ -104,6 +104,7 @@ struct jdec_private {
 	unsigned int current_cid;			/* For planar JPEG */
 #endif
 	unsigned char marker;			/* for PJPG (Pixart JPEG) */
+	unsigned char first_marker;		/* for PJPG (Pixart JPEG) */
 
 	/* Temp space used after the IDCT to store each components */
 	uint8_t Y[64 * 4], Cr[64], Cb[64];

index dd77d0f..8fc484e 100644
@@ -1387,6 +1387,16 @@ static void pixart_decode_MCU_2x1_3planes(struct jdec_private *priv)
 	look_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream,
 		   8, marker);
 
+	/* Sometimes the pac7302 switches chrominance setting halfway though a
+	   frame, with a quite ugly looking result, so we drop such frames. */
+	if (priv->first_marker == 0)
+		priv->first_marker = marker;
+	else if ((marker & 0x80) != (priv->first_marker & 0x80)) {
+		snprintf(priv->error_string, sizeof(priv->error_string),
+			"Pixart JPEG error: chrominance changed halfway\n");
+		longjmp(priv->jump_state, -EIO);
+	}
+
 	/* Pixart JPEG MCU-s are preceded by a marker indicating the quality
 	   setting with which the MCU is compressed, IOW the MCU-s may have a
 	   different quantization table per MCU. So if the marker changes we

@@ -2224,6 +2234,7 @@ int tinyjpeg_decode(struct jdec_private *priv, int pixfmt)
 			return length;
 		priv->stream = priv->stream_filtered;
 		priv->stream_end = priv->stream + length;
+		priv->first_marker = 0;
 
 		decode_mcu_table = pixart_decode_mcu_3comp_table;
 	}

1.7.10

Hi Hans,

I tried your patch, but I am not happy with the images I have (pac7302).

You say that the marker cannot be in the range 0..31 (index 0..7), but I have never seen a value lower than 68 (index 17).

This last marker value (68) is the default when the images have no big contrasts. With such images / blocks, the standard JPEG quantization table does not work well. It seems that, for this value, the table should be full of either 7 or 8 (8 gives a higher contrast).

Here is the sequence which works better (around line 1420 of tinyjpeg.c):

-------------8<--------------

		/* And another special Pixart feature, the DC quantization
		   factor is fixed! */
		qt[0] = 7;			// 8 gives a higher contrast
// special case for 68
	if (marker == 68) {
		for (i = 1; i < 64; i++)
			qt[i] = 7;		// also works with 8
	} else {
		for (i = 1; i < 64; i++) {
			j = (standard_quantization[0][i] * comp + 50) / 100;
			qt[i] = (j < 255) ? j : 255;
		}
	}
		build_quantization_table(priv->Q_tables[0], qt);

-------------8<--------------

About the other marker values, it seems also that the quantization tables are not optimal: some blocks are either too much (small contrasted lines) or not enough (big pixels) decompressed. As you know, a finer adjustment would ask for a long test time, so, I think we can live with your code.

Best regards.

Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Hi,

If you change register 0x80 in bank/page 1 to > 42 on pac7311 or larger then circa 100 on pac7302, you will get markers with bit 8 set. When this happens you will initially get markers 0xa0 - 0xa4 ... 0xbc and the stream tends to stabilize on 0xbc. Likewise if you remove the artificial limiting of the pac7302 to 15 fps from the driver you will get markers 0x44 - 0x48 ... 0x7c.

The images look a lot better with bit 8 set, so I plan to run some tests wrt what framerates can safely handle that (it uses more bandwidth) and set bit 8 on lower framerates.

Using a table with all 7's or 8's looses a lot of sharpness in image which high frequency components, for example the grain of a rough wall completely disappears.

I've (once more) tried to use a more simplified / flat quant table, I ended up with the table below, so as to not loose too much sharpness:

               0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x10, 0x10,
               0x10, 0x10, 0x10, 0x10, 0x10, 0x10, 0x10, 0x20,
               0x20, 0x20, 0x20, 0x20, 0x20,
                                             0x28, 0x28, 0x28,
               0x28, 0x28, 0x28, 0x28,
                                       0x30, 0x30, 0x30, 0x30,
               0x30, 0x30, 0x30, 0x30,
                                       0x38, 0x38, 0x38, 0x38,
               0x38, 0x38, 0x38,
                                 0x40, 0x40, 0x40, 0x40, 0x40,
               0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40,
               0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40,

Using the same qfactor as before, so the 0x0b translates to 7, with this table the images only change slightly, mostly because of the lower values at the end. This means we loose some sharpness of the picture (loose of high frequency components), and when watching a moving picture with that loose of sharpness we also loose some noise. But other then that the results are almost the same.

When using 0x08 rather then 0x07 we seem to get accumlating DC errors, leading to clear block divisions at the end of an mcu row.

Given the above I've decided to stick with my solution for now, I know it is not the ideal solution, but I believe it is the best we have. Also notice that my solution in essence gives us the same table for marker 68 as we used before your "tinyjpeg: Better luminance quantization table for Pixart JPEG" patch.

Yeah short of someone disassembling and reverse-engineering the windows driver we will probably never figure out the exact correct tables.

Regards,

Hans

Hi Hans,

I carefully looked at the ms-windows pac7302 traces I have. The register 1-80 stays always in the range 0d..11, except sometimes 19 at start time. In these traces, the images with marker 44 (dec 68) look really better with all 08's as the quantization table.

[snip]

Well, I got the SPC230NC.SYS of the ms-windows pac7302 driver, but it is not easy to disassemble because it has no symbol table. But, inside, I found this tables just before the Huffman table:

- 0006C888

	10 10 10 10 10 10 20 20 20 20 20 20 20 20 20 63
	63 63 63 63 63 63 63 63 63 63 63 63 63 63 63 63
	63 63 63 63 63 63 63 63 63 63 63 63 63 63 63 63
	63 63 63 63 63 63 63 63 63 63 63 63 63 63 63 63
- 0006C908
	10 10 10 10 10 10 20 20 20 20 20 20 20 20 20 63
	63 63 63 63 63 63 63 63 63 63 63 63 63 63 63 63
	63 63 63 63 63 63 63 63 63 63 63 63 63 63 63 63
	63 63 63 63 63 63 63 63 63 63 63 63 63 63 63 63
- 0006C988
	08 08 08 08 08 08 10 10 10 10 10 10 10 10 10 10
	10 10 10 10 10 10 10 10 10 10 10 10 20 20 20 20
	20 20 20 20 20 20 20 20 20 20 20 40 40 40 40 40
	40 40 40 40 40 40 40 40 40 40 40 40 40 40 40 40
- 0006CA08
	08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08
	08 08 08 08 08 08 08 08 08 08 08 08 10 10 10 10
	10 10 10 10 10 10 10 10 10 10 10 10 10 10 10 10
	10 10 10 10 10 10 20 20 20 20 20 20 20 20 20 20
- 0006CA88
	10 10 10 10 10 10 20 20 20 20 20 20 20 20 20 63
	63 63 63 63 63 63 63 63 63 63 63 63 63 63 63 63
	63 63 63 63 63 63 63 63 63 63 63 63 63 63 63 63
	63 63 63 63 63 63 63 63 63 63 63 63 63 63 63 63
- 0006CB08
	08 0b 0b 0b 0b 0b 10 10 10 10 10 10 10 10 10 10
	10 10 10 10 10 20 20 20 20 20 20 20 40 40 40 40
	40 40 40 40 63 63 63 63 63 63 63 63 63 63 63 63
	63 63 63 63 63 63 63 63 63 63 63 63 63 63 63 63
- 0006CB88
	11 12 12 18 15 18 2f 1a 1a 2f 63 42 38 42 63 63
	63 63 63 63 63 63 63 63 63 63 63 63 63 63 63 63
	63 63 63 63 63 63 63 63 63 63 63 63 63 63 63 63
	63 63 63 63 63 63 63 63 63 63 63 63 63 63 63 63
- 0006CC08
	10 0b 0c 0e 0c 0b 10 0e 0d 0e 12 11 10 13 18 28
	1a 18 16 16 18 31 23 25 1d 28 3a 33 3d 3c 39 33
	38 37 40 48 5c 4e 40 44 57 45 37 38 50 6D 51 57
	5F 62 67 68 67 3E 4D 71 78 70 64 78 5C 65 67 63

Don't they look like quantization tables? (the table 0006CB08 is quite the same the flat table you tried!)

BTW, I don't think the exposure and gain controls use the right registers as they are coded in the actual gspca pac7302 subdriver. The ms-windows driver uses the registers (3-80 / 3-03), (3-05 / 3-04), (3-12) and (1-80) for autogain/exposure. The gspca test tarball of my web site includes a new AGC using these registers, but it does not work well. Maybe you could tell me what is wrong with it...

Regards.

Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Hi,

Right, that can mean one of 2 things:
1) The traces were made during daylight, so low exposure / high framerate, and enabling the lower compression modes (which cause bit 7 of the marker to get set) is a bad idea at high framerates

2) The windows driver never enables the low compression mode. I seriously doubt that this is the case, ie older versions of the pac7311 driver have (commented) writes to page 1 register 80 with high enough values to enable it and I'm pretty sure those writes come from windows traces.

After having played with the quantization tables you've found I agree.

Yes they do, good find! I've done yet more testing / trial and error with these tables and I've just pushed another Pixart JPEG patch to v4l-utils git switching to these new tables. Thanks! Also with these tables the quality difference between high/low compression mode becomes significantly less. So much less that I've decided to not further pursue enabling low compression mode in the gspca drivers, esp. since this will cause pain for people with an older libv4l.

Let me get back on that in a separate mail.

Regards,

Hans

Hi,

<snip>

3-03, 3-04 and 3-05 are already known and they all influence framerate / exposure in some way. I've also ran some tests with 3-80, again it influences framerate in some way (*). We already have a well tested and working, fine-grained way to control exposure so I think it is best to leave things as is exposure wise.

3-12 is interesting, it is a new gain control. The pull request I've just send (with you in the CC) contains a patch to improve gain control using both 3-10 and 3-12 together.

1-80 is compression balance, since our decompression code for higher compression settings (markers > 68) still is not perfect this is best left untouched.

*) Note I've documented all registers I've ran tests with as part of the patchset for which I've just send a pull request.

Regards,

Hans