[PATCH v2] tinyjpeg: Dynamic luminance quantization table for Pixart JPEG

[PATCH v2] tinyjpeg: Dynamic luminance quantization table for Pixart JPEG

[Jean-Francois Moine]

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.

Signed-off-by: Jean-François Moine <moinejf@free.fr>

diff --git a/lib/libv4lconvert/tinyjpeg-internal.h b/lib/libv4lconvert/tinyjpeg-internal.h
index 702a2a2..4041251 100644
--- a/lib/libv4lconvert/tinyjpeg-internal.h
+++ b/lib/libv4lconvert/tinyjpeg-internal.h
@@ -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];
diff --git a/lib/libv4lconvert/tinyjpeg.c b/lib/libv4lconvert/tinyjpeg.c
index 2c2d4af..d986a45 100644
--- a/lib/libv4lconvert/tinyjpeg.c
+++ b/lib/libv4lconvert/tinyjpeg.c
@@ -1376,6 +1376,8 @@ static void decode_MCU_2x1_3planes(struct jdec_private *priv)
 	IDCT(&priv->component_infos[cCr], priv->Cr, 8);
 }
 
+static void build_quantization_table(float *qtable, const unsigned char *ref_table);
+
 static void pixart_decode_MCU_2x1_3planes(struct jdec_private *priv)
 {
 	unsigned char marker;
@@ -1384,10 +1386,8 @@ static void pixart_decode_MCU_2x1_3planes(struct jdec_private *priv)
 	/* I think the marker indicates which quantization table to use, iow
 	   a Pixart JPEG may have a different quantization table per MCU, most
 	   MCU's have 0x44 as marker for which our special Pixart quantization
-	   tables are correct. Unfortunately with a 7302 some blocks also have 0x48,
-	   and sometimes even other values. As 0x48 is quite common too, we really
-	   need to find out the correct table for that, as currently the blocks
-	   with a 0x48 marker look wrong. During normal operation the marker stays
+	   tables are correct. [jfm: snip]
+	   During normal operation the marker stays
 	   within the range below, if it gets out of this range we're most likely
 	   decoding garbage */
 	if (marker < 0x20 || marker > 0x7f) {
@@ -1396,6 +1396,53 @@ static void pixart_decode_MCU_2x1_3planes(struct jdec_private *priv)
 				(unsigned int)marker);
 		longjmp(priv->jump_state, -EIO);
 	}
+
+	/* rebuild the Y quantization table when the marker changes */
+	if (marker != priv->marker) {
+		unsigned char quant_new[64];
+		int i, j;
+		/*
+		 * table to rebuild the Y quantization table
+		 * 	index 1 = marker / 4
+		 *	index 2 = 4 end indexes in the quantization table
+		 *	values = 0x08, 0x10, 0x20, 0x40, 0x63
+		 * jfm: The values have been guessed from 4 images, so,
+		 *	better values may be found...
+		 */
+		static const unsigned char q_tb[12][4] = {
+			{ 64, 64, 64, 64 },	/* 68 */
+			{  8, 32, 64, 64 },
+			{  1, 16, 50, 64 },
+			{  1, 16, 30, 60 },	/* 80 */
+			{  1,  8, 16, 32 },
+			{  1,  4, 16, 31 },
+			{  1,  3, 16, 30 },
+			{  1,  2, 16, 21 },
+			{  1,  1, 16, 18 },	/* 100 */
+			{  1,  1, 16, 17 },
+			{  1,  1, 16, 16 },
+			{  1,  1, 15, 15 },
+		};
+
+		priv->marker = marker;
+		j = marker - 68;
+		if (j < 0)
+			j = 0;
+		j >>= 2;
+		if (j > sizeof q_tb / sizeof q_tb[0])
+			j = sizeof q_tb / sizeof q_tb[0] - 1;
+		for (i = 0; i < q_tb[j][0]; i++)
+			quant_new[i] = 0x08;
+		for (; i < q_tb[j][1]; i++)
+			quant_new[i] = 0x10;
+		for (; i < q_tb[j][2]; i++)
+			quant_new[i] = 0x20;
+		for (; i < q_tb[j][3]; i++)
+			quant_new[i] = 0x40;
+		for (; i < 64; i++)
+			quant_new[i] = 0x63;
+		build_quantization_table(priv->Q_tables[0], quant_new);
+	}
 	skip_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream, 8);
 
 	// Y
@@ -1948,6 +1995,7 @@ static int parse_JFIF(struct jdec_private *priv, const unsigned char *stream)
 		if (!priv->default_huffman_table_initialized) {
 			build_quantization_table(priv->Q_tables[0], pixart_quantization[0]);
 			build_quantization_table(priv->Q_tables[1], pixart_quantization[1]);
+			priv->marker = 68;	/* common starting marker */
 		}
 	}
 

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

Re: [PATCH v2] tinyjpeg: Dynamic luminance quantization table for Pixart JPEG

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 5009 bytes --]

Hi,

On 04/12/2012 12:20 PM, Jean-Francois Moine wrote:
> 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.

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


>
> Signed-off-by: Jean-François Moine<moinejf@free.fr>
>
> diff --git a/lib/libv4lconvert/tinyjpeg-internal.h b/lib/libv4lconvert/tinyjpeg-internal.h
> index 702a2a2..4041251 100644
> --- a/lib/libv4lconvert/tinyjpeg-internal.h
> +++ b/lib/libv4lconvert/tinyjpeg-internal.h
> @@ -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];
> diff --git a/lib/libv4lconvert/tinyjpeg.c b/lib/libv4lconvert/tinyjpeg.c
> index 2c2d4af..d986a45 100644
> --- a/lib/libv4lconvert/tinyjpeg.c
> +++ b/lib/libv4lconvert/tinyjpeg.c
> @@ -1376,6 +1376,8 @@ static void decode_MCU_2x1_3planes(struct jdec_private *priv)
>   	IDCT(&priv->component_infos[cCr], priv->Cr, 8);
>   }
>
> +static void build_quantization_table(float *qtable, const unsigned char *ref_table);
> +
>   static void pixart_decode_MCU_2x1_3planes(struct jdec_private *priv)
>   {
>   	unsigned char marker;
> @@ -1384,10 +1386,8 @@ static void pixart_decode_MCU_2x1_3planes(struct jdec_private *priv)
>   	/* I think the marker indicates which quantization table to use, iow
>   	   a Pixart JPEG may have a different quantization table per MCU, most
>   	   MCU's have 0x44 as marker for which our special Pixart quantization
> -	   tables are correct. Unfortunately with a 7302 some blocks also have 0x48,
> -	   and sometimes even other values. As 0x48 is quite common too, we really
> -	   need to find out the correct table for that, as currently the blocks
> -	   with a 0x48 marker look wrong. During normal operation the marker stays
> +	   tables are correct. [jfm: snip]
> +	   During normal operation the marker stays
>   	   within the range below, if it gets out of this range we're most likely
>   	   decoding garbage */
>   	if (marker<  0x20 || marker>  0x7f) {
> @@ -1396,6 +1396,53 @@ static void pixart_decode_MCU_2x1_3planes(struct jdec_private *priv)
>   				(unsigned int)marker);
>   		longjmp(priv->jump_state, -EIO);
>   	}
> +
> +	/* rebuild the Y quantization table when the marker changes */
> +	if (marker != priv->marker) {
> +		unsigned char quant_new[64];
> +		int i, j;
> +		/*
> +		 * table to rebuild the Y quantization table
> +		 * 	index 1 = marker / 4
> +		 *	index 2 = 4 end indexes in the quantization table
> +		 *	values = 0x08, 0x10, 0x20, 0x40, 0x63
> +		 * jfm: The values have been guessed from 4 images, so,
> +		 *	better values may be found...
> +		 */
> +		static const unsigned char q_tb[12][4] = {
> +			{ 64, 64, 64, 64 },	/* 68 */
> +			{  8, 32, 64, 64 },
> +			{  1, 16, 50, 64 },
> +			{  1, 16, 30, 60 },	/* 80 */
> +			{  1,  8, 16, 32 },
> +			{  1,  4, 16, 31 },
> +			{  1,  3, 16, 30 },
> +			{  1,  2, 16, 21 },
> +			{  1,  1, 16, 18 },	/* 100 */
> +			{  1,  1, 16, 17 },
> +			{  1,  1, 16, 16 },
> +			{  1,  1, 15, 15 },
> +		};
> +
> +		priv->marker = marker;
> +		j = marker - 68;
> +		if (j<  0)
> +			j = 0;
> +		j>>= 2;
> +		if (j>  sizeof q_tb / sizeof q_tb[0])
> +			j = sizeof q_tb / sizeof q_tb[0] - 1;
> +		for (i = 0; i<  q_tb[j][0]; i++)
> +			quant_new[i] = 0x08;
> +		for (; i<  q_tb[j][1]; i++)
> +			quant_new[i] = 0x10;
> +		for (; i<  q_tb[j][2]; i++)
> +			quant_new[i] = 0x20;
> +		for (; i<  q_tb[j][3]; i++)
> +			quant_new[i] = 0x40;
> +		for (; i<  64; i++)
> +			quant_new[i] = 0x63;
> +		build_quantization_table(priv->Q_tables[0], quant_new);
> +	}
>   	skip_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream, 8);
>
>   	// Y
> @@ -1948,6 +1995,7 @@ static int parse_JFIF(struct jdec_private *priv, const unsigned char *stream)
>   		if (!priv->default_huffman_table_initialized) {
>   			build_quantization_table(priv->Q_tables[0], pixart_quantization[0]);
>   			build_quantization_table(priv->Q_tables[1], pixart_quantization[1]);
> +			priv->marker = 68;	/* common starting marker */
>   		}
>   	}
>
>

[-- Attachment #2: 0001-libv4lconvert-Fix-decoding-of-160x120-Pixart-JPEG-im.patch --]
[-- Type: text/x-patch, Size: 1480 bytes --]

>From 296a2827375732346776357ec59d2cf446128095 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Mon, 23 Apr 2012 19:43:07 +0200
Subject: [PATCH 1/4] libv4lconvert: Fix decoding of 160x120 Pixart JPEG
 images

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 lib/libv4lconvert/tinyjpeg.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/lib/libv4lconvert/tinyjpeg.c b/lib/libv4lconvert/tinyjpeg.c
index 2c2d4af..d2a7d3f 100644
--- a/lib/libv4lconvert/tinyjpeg.c
+++ b/lib/libv4lconvert/tinyjpeg.c
@@ -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


[-- Attachment #3: 0002-Revert-tinyjpeg-Better-luminance-quantization-table-.patch --]
[-- Type: text/x-patch, Size: 1610 bytes --]

>From 3cc0517f88d07a44638b38115f76b33c31c17fd1 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
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(-)

diff --git a/lib/libv4lconvert/tinyjpeg.c b/lib/libv4lconvert/tinyjpeg.c
index d2a7d3f..756ad9c 100644
--- a/lib/libv4lconvert/tinyjpeg.c
+++ b/lib/libv4lconvert/tinyjpeg.c
@@ -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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-libv4lconvert-Dynamic-quantization-tables-for-Pixart.patch --]
[-- Type: text/x-patch; name="0003-libv4lconvert-Dynamic-quantization-tables-for-Pixart.patch", Size: 6233 bytes --]

>From 8e3907391bd4e413742fe448d27b053c82146904 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
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 <moinejf@free.fr>, 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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 lib/libv4lconvert/tinyjpeg-internal.h |    1 +
 lib/libv4lconvert/tinyjpeg.c          |   77 ++++++++++++++++++++++-----------
 2 files changed, 53 insertions(+), 25 deletions(-)

diff --git a/lib/libv4lconvert/tinyjpeg-internal.h b/lib/libv4lconvert/tinyjpeg-internal.h
index 702a2a2..4041251 100644
--- a/lib/libv4lconvert/tinyjpeg-internal.h
+++ b/lib/libv4lconvert/tinyjpeg-internal.h
@@ -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];
diff --git a/lib/libv4lconvert/tinyjpeg.c b/lib/libv4lconvert/tinyjpeg.c
index 756ad9c..dd77d0f 100644
--- a/lib/libv4lconvert/tinyjpeg.c
+++ b/lib/libv4lconvert/tinyjpeg.c
@@ -205,9 +205,11 @@ static const unsigned char val_ac_chrominance[] = {
 	0xf9, 0xfa
 };
 
-const unsigned char pixart_quantization[][64] = { {
-		0x07, 0x07, 0x08, 0x0a, 0x09, 0x07, 0x0d, 0x0b,
-		0x0c, 0x0d, 0x11, 0x10, 0x0f, 0x12, 0x17, 0x27,
+/* Standard JPEG quantization tables from Annex K of the JPEG standard.
+   Note unlike in Annex K the entries here are in zigzag order! */
+const unsigned char standard_quantization[][64] = { {
+		0x10, 0x0b, 0x0c, 0x0e, 0x0c, 0x0a, 0x10, 0x0e,
+		0x0d, 0x0e, 0x12, 0x11, 0x10, 0x13, 0x18, 0x28,
 		0x1a, 0x18, 0x16, 0x16, 0x18, 0x31, 0x23, 0x25,
 		0x1d, 0x28, 0x3a, 0x33, 0x3d, 0x3c, 0x39, 0x33,
 		0x38, 0x37, 0x40, 0x48, 0x5c, 0x4e, 0x40, 0x44,
@@ -1376,25 +1378,57 @@ static void decode_MCU_2x1_3planes(struct jdec_private *priv)
 	IDCT(&priv->component_infos[cCr], priv->Cr, 8);
 }
 
+static void build_quantization_table(float *qtable, const unsigned char *ref_table);
+
 static void pixart_decode_MCU_2x1_3planes(struct jdec_private *priv)
 {
 	unsigned char marker;
 
-	look_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream, 8, marker);
-	/* I think the marker indicates which quantization table to use, iow
-	   a Pixart JPEG may have a different quantization table per MCU, most
-	   MCU's have 0x44 as marker for which our special Pixart quantization
-	   tables are correct. Unfortunately with a 7302 some blocks also have 0x48,
-	   and sometimes even other values. As 0x48 is quite common too, we really
-	   need to find out the correct table for that, as currently the blocks
-	   with a 0x48 marker look wrong. During normal operation the marker stays
-	   within the range below, if it gets out of this range we're most likely
-	   decoding garbage */
-	if (marker < 0x20 || marker > 0x7f) {
-		snprintf(priv->error_string, sizeof(priv->error_string),
-				"Pixart JPEG error: invalid MCU marker: 0x%02x\n",
-				(unsigned int)marker);
-		longjmp(priv->jump_state, -EIO);
+	look_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream,
+		   8, marker);
+
+	/* 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
+	   need to rebuild the quantization tables. */
+	if (marker != priv->marker) {
+		int i, j, comp;
+		unsigned char qt[64];
+		/* These values have been found by trial and error and seem to
+		   work reasonably. Markers with index 0 - 7 are never
+		   generated by the hardware, so they are likely wrong. */
+		const int qfactor[32] = {
+			 10,   12,  14,  16,  18,  20,  22,  24,
+			 25,   30,  35,  40,  45,  50,  55,  60,
+			 64,   68,  80,  90, 100, 120, 140, 160,
+			180,  200, 220, 240, 260, 280, 300, 320
+		};
+
+		i = (marker & 0x7c) >> 2; /* Bits 0 and 1 are always 0 */
+		comp = qfactor[i];
+
+		/* And another special Pixart feature, the DC quantization
+		   factor is fixed! */
+		qt[0] = 7; 
+		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);
+
+		/* And yet another Pixart JPEG special feature, Pixart JPEG
+		   uses the luminance table for chrominance too! Either
+		   as is or with all values multiplied by 2, this is encoded
+		   in bit 7 of the marker. */
+		if (!(marker & 0x80)) {
+			for (i = 0; i < 64; i++) {
+				j = qt[i] * 2;
+				qt[i] = (j < 255) ? j : 255;
+			}
+		}
+		build_quantization_table(priv->Q_tables[1], qt);
+
+		priv->marker = marker;
 	}
 	skip_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream, 8);
 
@@ -1944,13 +1978,6 @@ static int parse_JFIF(struct jdec_private *priv, const unsigned char *stream)
 			(!dqt_marker_found && !(priv->flags & TINYJPEG_FLAGS_PIXART_JPEG)))
 		goto bogus_jpeg_format;
 
-	if (priv->flags & TINYJPEG_FLAGS_PIXART_JPEG) {
-		if (!priv->default_huffman_table_initialized) {
-			build_quantization_table(priv->Q_tables[0], pixart_quantization[0]);
-			build_quantization_table(priv->Q_tables[1], pixart_quantization[1]);
-		}
-	}
-
 	if (!dht_marker_found) {
 		trace("No Huffman table loaded, using the default one\n");
 		if (build_default_huffman_tables(priv))
-- 
1.7.10


[-- Attachment #5: 0004-libv4lconvert-Drop-Pixart-JPEG-frames-with-changing-.patch --]
[-- Type: text/x-patch, Size: 2392 bytes --]

>From eb436c4dbcb0c0985beb78ae33e8c767e6695309 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 lib/libv4lconvert/tinyjpeg-internal.h |    1 +
 lib/libv4lconvert/tinyjpeg.c          |   11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/lib/libv4lconvert/tinyjpeg-internal.h b/lib/libv4lconvert/tinyjpeg-internal.h
index 4041251..dcbcf27 100644
--- a/lib/libv4lconvert/tinyjpeg-internal.h
+++ b/lib/libv4lconvert/tinyjpeg-internal.h
@@ -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];
diff --git a/lib/libv4lconvert/tinyjpeg.c b/lib/libv4lconvert/tinyjpeg.c
index dd77d0f..8fc484e 100644
--- a/lib/libv4lconvert/tinyjpeg.c
+++ b/lib/libv4lconvert/tinyjpeg.c
@@ -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

Re: [PATCH v2] tinyjpeg: Dynamic luminance quantization table for Pixart JPEG

[Jean-Francois Moine]

On Mon, 23 Apr 2012 23:34:05 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> 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...

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/

Re: [PATCH v2] tinyjpeg: Dynamic luminance quantization table for Pixart JPEG

[Hans de Goede]

Hi,

On 04/24/2012 12:34 PM, Jean-Francois Moine wrote:
> On Mon, 23 Apr 2012 23:34:05 +0200
> Hans de Goede<hdegoede@redhat.com>  wrote:
>
>> 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...
>
> 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).

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.

>
> 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).

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.





>
> 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.

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

Regards,

Hans

Re: [PATCH v2] tinyjpeg: Dynamic luminance quantization table for Pixart JPEG

[Jean-Francois Moine]

Hi Hans,

On Wed, 25 Apr 2012 16:19:57 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> > 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).  
> 
> 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.

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]
> Yeah short of someone disassembling and reverse-engineering the windows driver
> we will probably never figure out the exact correct tables.

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/

Re: [PATCH v2] tinyjpeg: Dynamic luminance quantization table for Pixart JPEG

[Hans de Goede]

Hi,

On 04/25/2012 06:09 PM, Jean-Francois Moine wrote:
> Hi Hans,
>
> On Wed, 25 Apr 2012 16:19:57 +0200
> Hans de Goede<hdegoede@redhat.com>  wrote:
>
>>> 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).
>>
>> 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.
>
> 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.

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.

> In these traces, the images with marker 44 (dec 68) look
> really better with all 08's as the quantization table.

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

> [snip]
>> Yeah short of someone disassembling and reverse-engineering the windows driver
>> we will probably never figure out the exact correct tables.
>
> 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?

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.

> 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...

Let me get back on that in a separate mail.

Regards,

Hans

Re: [PATCH v2] tinyjpeg: Dynamic luminance quantization table for Pixart JPEG

[Hans de Goede]

Hi,

On 04/25/2012 06:09 PM, Jean-Francois Moine wrote:
> Hi Hans,

<snip>

> 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-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)

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.

> and (1-80)

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