libv4l: possible problem found in PAC7302 JPEG decoding

libv4l: possible problem found in PAC7302 JPEG decoding

[Németh Márton]

Hello Hans,

while I was dealing with Labtec Webcam 2200 and with gspca_pac7302 driver I recognised the
following behaviour. The stream received from the webcam is splitted by the gspca_pac7302
subdriver when the byte sequence 0xff, 0xff, 0x00, 0xff, 0x96 is found (pac_find_sof()).
Before transmitting the data to the userspace a JPEG header is added (pac_start_frame())
and the footer after the bytes 0xff, 0xd9 are removed.

The data buffer which arrives to userspace looks like as follows (maybe not every detail is exact):

 1. JPEG header

 2. Some bytes of image data (near to 1024 bytes)

 3. The byte sequence 0xff, 0xff, 0xff, 0x01 followed by 1024 bytes of data.
    This marker sequence and data repeats a couple of time. Exactly how much
    depends on the image content.

 4. The byte sequence 0xff, 0xff, 0xff, 0x02 followed by 512 bytes of data.
    This marker sequence and data also repeats a couple of time.

 5. The byte sequence 0xff, 0xff, 0xff, 0x00 followed by a variable amount of
    image data bytes.

 6. The End of Image (EOI) marker 0xff, 0xd9.

Now what can be wrong with the libv4l? In libv4lconvert/tinyjpeg.c, line 315 there is a
huge macro which tries to remove the 0xff, 0xff, 0xff, xx byte sequence from the received
image. This fails, however, if the image contains 0xff bytes just before the 0xff, 0xff,
0xff, xx sequence because one byte from the image data (the first 0xff) is removed, then
the three 0xff bytes from the marker is also removed. The xx (which really belongs to the
marker) is left in the image data instead of the original 0xff byte.

Based on my experiments this problem sometimes causes corrupted image decoding or that the
JPEG image cannot be decoded at all.

I have done my experiments with a modified gspca_pac7302 kernel space driver (the JPEG header
is not added and the footer is not removed). In userspace I added the JPEG header and
then applied the following filter function to the received data. The result is that I do
not get any corrupted frame anymore. The filter function in userspace is based on a state
machine like this:

static int memcpy_filter(unsigned char *dest, unsigned char *src, int n)
{
	int i = 0;
	int j = 0;
	int state = 0;
	int last_i = 0;

	i = 5;
	j = 0;

	/* Skip the first 5 bytes: 0xff 0xff 0x00 0xff 0x96 */
	memcpy(&(dest[j]), &(src[i]), 1024-5);
	i += 1024-5;
	j += 1024-5;

	while (i < n) {
		switch (state) {
			case 0:
				if (src[i] == 0xff)
					state = 1;
				else {
					state = 0;
					dest[j++] = src[i];
				}
				break;
			case 1:
				if (src[i] == 0xff)
					state = 2;
				else {
					state = 0;
					dest[j++] = src[i-1];
					dest[j++] = src[i];
				}
				break;
			case 2:
				switch (src[i]) {
					case 0xff:
						state = 3;
						break;
					default:
						state = 0;
						dest[j++] = src[i-2];
						dest[j++] = src[i-1];
						dest[j++] = src[i];
				}
				break;
			case 3:
				switch (src[i]) {
					case 0:
						/* found 0xff 0xff 0xff 0x00 */
						state = 0;
						break;
					case 1:
						/* found 0xff 0xff 0xff 0x01 */
						last_i = i+1;
						memcpy(&(dest[j]), &(src[i+1]), 1024);
						i += 1024;
						j += 1024;
						state = 0;
						break;
					case 2:
						/* found 0xff 0xff 0xff 0x02 */
						last_i = i+1;
						memcpy(&(dest[j]), &(src[i+1]), 512);
						i += 512;
						j += 512;
						state = 0;
						break;
					case 0xff:
						/* found the 4th 0xff in a row, lets copy the first
						   one and keep the last three for later use */
						dest[j++] = src[i-3];
						state = 3;
						break;

					default:
						state = 0;
						dest[j++] = src[i-3];
						dest[j++] = src[i-2];
						dest[j++] = src[i-1];
						dest[j++] = src[i];
				
				}
		}
		i++;
	}

	/* return the length of the dest buffer */
	return j;
}

The solution is not 100% solution because there are some cases when the decoding fails, but
the error rate is much lower.

I think it is possible to solve this kind of problem just by modifying the pixart_fill_nbits()
macro. What do you think?

Best regarsd,

	Márton Németh

PS: here is the patch I used in kernel space, just for easier reference

---
Signed-off-by: Márton Németh <nm127@freemail.hu>
---
diff -r 4f102b2f7ac1 linux/drivers/media/video/gspca/pac7302.c
--- a/linux/drivers/media/video/gspca/pac7302.c	Thu Jan 28 20:35:40 2010 +0100
+++ b/linux/drivers/media/video/gspca/pac7302.c	Mon Feb 01 22:09:15 2010 +0100
@@ -835,6 +835,7 @@
 {
 	unsigned char tmpbuf[4];

+#if 0
 	gspca_frame_add(gspca_dev, FIRST_PACKET,
 		pac_jpeg_header1, sizeof(pac_jpeg_header1));

@@ -847,6 +848,11 @@
 		tmpbuf, sizeof(tmpbuf));
 	gspca_frame_add(gspca_dev, INTER_PACKET,
 		pac_jpeg_header2, sizeof(pac_jpeg_header2));
+#else
+	gspca_frame_add(gspca_dev, FIRST_PACKET,
+		NULL, 0);
+#endif
+
 }

 /* this function is run at interrupt level */
@@ -873,10 +879,10 @@
 		   image, the 14th and 15th byte after the EOF seem to
 		   correspond to the center of the image */
 		lum_offset = 61 + sizeof pac_sof_marker;
-		footer_length = 74;
+		footer_length = 74 + sizeof(pac_sof_marker);

 		/* Finish decoding current frame */
-		n = (sof - data) - (footer_length + sizeof pac_sof_marker);
+		n = sof - data;
 		if (n < 0) {
 			frame->data_end += n;
 			n = 0;
@@ -884,11 +890,13 @@
 		gspca_frame_add(gspca_dev, INTER_PACKET,
 					data, n);
 		if (gspca_dev->last_packet_type != DISCARD_PACKET &&
-				frame->data_end[-2] == 0xff &&
-				frame->data_end[-1] == 0xd9)
+				frame->data_end[-footer_length-2] == 0xff &&
+				frame->data_end[-footer_length-1] == 0xd9)
 			gspca_frame_add(gspca_dev, LAST_PACKET,
 						NULL, 0);

+		sof -= 5;
+
 		n = sof - data;
 		len -= n;
 		data = sof;

[PATCH ] libv4l: skip false Pixart markers

[Németh Márton]

From: Márton Németh <nm127@freemail.hu>

The byte sequence 0xff, 0xff, 0xff 0xff is not a real marker to skip, instead
it is one byte from the image and the following three 0xff bytes might belong
to a real marker. Modify pixart_fill_nbits() macro to pass the first 0xff byte
as an image data.

Signed-off-by: Márton Németh <nm127@freemail.hu>
---
diff -r f23c5a878fb1 v4l2-apps/libv4l/libv4lconvert/tinyjpeg.c
--- a/v4l2-apps/libv4l/libv4lconvert/tinyjpeg.c	Mon Feb 01 13:32:46 2010 +0100
+++ b/v4l2-apps/libv4l/libv4lconvert/tinyjpeg.c	Mon Feb 01 23:05:39 2010 +0100
@@ -339,10 +339,15 @@
 	    } \
 	    break; \
 	  case 0xff: \
-	    if (stream[1] == 0xff && (stream[2] < 7 || stream[2] == 0xff)) { \
-	      stream += 3; \
-	      c = *stream++; \
-	      break; \
+	    if (stream[1] == 0xff) { \
+		if (stream[2] < 7) { \
+		    stream += 3; \
+		    c = *stream++; \
+		    break; \
+		} else if (stream[2] == 0xff) { \
+		    /* four 0xff in a row: the first belongs to the image data */ \
+		    break; \
+		}\
 	    } \
 	    /* Error fall through */ \
 	  default: \

Re: [PATCH ] libv4l: skip false Pixart markers

[Hans de Goede]

Hi,

On 02/01/2010 11:13 PM, Németh Márton wrote:
> From: Márton Németh<nm127@freemail.hu>
>
> The byte sequence 0xff, 0xff, 0xff 0xff is not a real marker to skip, instead
> it is one byte from the image and the following three 0xff bytes might belong
> to a real marker. Modify pixart_fill_nbits() macro to pass the first 0xff byte
> as an image data.
>

Oh, good catch. I'm still seeing the occasional bad frame though :(

While on the subject of the pac7302. I've been playing around a bit, and I have the
feeling that if we were to go for a lower auto gain target (set autogain off and
lower exposure, you can do this ie with v4l2ucp), combined with a gamma correction of
1500 (again use ie v4l2ucp), the images is much better (less over exposed, more
contrast).

Do you agree ?

Regards,

Hans


> Signed-off-by: Márton Németh<nm127@freemail.hu>
> ---
> diff -r f23c5a878fb1 v4l2-apps/libv4l/libv4lconvert/tinyjpeg.c
> --- a/v4l2-apps/libv4l/libv4lconvert/tinyjpeg.c	Mon Feb 01 13:32:46 2010 +0100
> +++ b/v4l2-apps/libv4l/libv4lconvert/tinyjpeg.c	Mon Feb 01 23:05:39 2010 +0100
> @@ -339,10 +339,15 @@
>   	    } \
>   	    break; \
>   	  case 0xff: \
> -	    if (stream[1] == 0xff&&  (stream[2]<  7 || stream[2] == 0xff)) { \
> -	      stream += 3; \
> -	      c = *stream++; \
> -	      break; \
> +	    if (stream[1] == 0xff) { \
> +		if (stream[2]<  7) { \
> +		    stream += 3; \
> +		    c = *stream++; \
> +		    break; \
> +		} else if (stream[2] == 0xff) { \
> +		    /* four 0xff in a row: the first belongs to the image data */ \
> +		    break; \
> +		}\
>   	    } \
>   	    /* Error fall through */ \
>   	  default: \

Re: libv4l: possible problem found in PAC7302 JPEG decoding

[Thomas Kaiser]

On 02/01/2010 10:23 PM, Németh Márton wrote:
> Hello Hans,
> 
> while I was dealing with Labtec Webcam 2200 and with gspca_pac7302 driver I recognised the
> following behaviour. The stream received from the webcam is splitted by the gspca_pac7302
> subdriver when the byte sequence 0xff, 0xff, 0x00, 0xff, 0x96 is found (pac_find_sof()).
> Before transmitting the data to the userspace a JPEG header is added (pac_start_frame())
> and the footer after the bytes 0xff, 0xd9 are removed.
> 
> The data buffer which arrives to userspace looks like as follows (maybe not every detail is exact):
> 
>  1. JPEG header
> 
>  2. Some bytes of image data (near to 1024 bytes)
> 
>  3. The byte sequence 0xff, 0xff, 0xff, 0x01 followed by 1024 bytes of data.
>     This marker sequence and data repeats a couple of time. Exactly how much
>     depends on the image content.
> 
>  4. The byte sequence 0xff, 0xff, 0xff, 0x02 followed by 512 bytes of data.
>     This marker sequence and data also repeats a couple of time.
> 
>  5. The byte sequence 0xff, 0xff, 0xff, 0x00 followed by a variable amount of
>     image data bytes.
> 
>  6. The End of Image (EOI) marker 0xff, 0xd9.
> 
> Now what can be wrong with the libv4l? In libv4lconvert/tinyjpeg.c, line 315 there is a
> huge macro which tries to remove the 0xff, 0xff, 0xff, xx byte sequence from the received
> image. This fails, however, if the image contains 0xff bytes just before the 0xff, 0xff,
> 0xff, xx sequence because one byte from the image data (the first 0xff) is removed, then
> the three 0xff bytes from the marker is also removed. The xx (which really belongs to the
> marker) is left in the image data instead of the original 0xff byte.
> 
> Based on my experiments this problem sometimes causes corrupted image decoding or that the
> JPEG image cannot be decoded at all.
> 

Hello Németh

I remember the problem as I was working on the PAC7311.
http://www.kaiser-linux.li/index.php?title=PAC7311


This is the code I used in the JPEG decoder to remove the 0xff 0xff 0xff 
  0xnn markers.

See http://www.kaiser-linux.li/files/PAC7311/gspcav1-PAC7311-20070425.tar.gz
decoder/gspcadecoder.c pac7311_decode()

Thomas

Re: [PATCH ] libv4l: skip false Pixart markers

[Németh Márton]

Hans de Goede wrote:
> Hi,
> 
> On 02/01/2010 11:13 PM, Németh Márton wrote:
>> From: Márton Németh<nm127@freemail.hu>
>>
>> The byte sequence 0xff, 0xff, 0xff 0xff is not a real marker to skip, instead
>> it is one byte from the image and the following three 0xff bytes might belong
>> to a real marker. Modify pixart_fill_nbits() macro to pass the first 0xff byte
>> as an image data.
>>
> 
> Oh, good catch. I'm still seeing the occasional bad frame though :(

The same at my side, this patch alone does not solve the whole problem complete.
I have the feeling that at least same of the corrupted frames will not come with
this patch, through I haven't verified this with measurement.

On the other hand, in my previous email used a little bit different code: I jumped
over the 1024 and 512 bytes without parsing it. That would be maybe necessary
to add.

By the way, is there any reason why pixart_fill_nbits() is a macro?

> While on the subject of the pac7302. I've been playing around a bit, and I have the
> feeling that if we were to go for a lower auto gain target (set autogain off and
> lower exposure, you can do this ie with v4l2ucp), combined with a gamma correction of
> 1500 (again use ie v4l2ucp), the images is much better (less over exposed, more
> contrast).
> 
> Do you agree ?

Well, my Labtec Webcam 2200 works only with acceptable indoors, when I try to
capture something outdoors under direct sunshine conditions I get overexposed
frames. I found, however, an interesting pointer in two cameras' user's manual,
see the Note column:

  http://linuxtv.org/wiki/index.php/PixArt_PAC7301/PAC7302#Identification

There is a setting indoor/outdoor which is currently not available in gspca_pac7302
driver. Maybe this would be an interesting point to figure out which register
is related to this setting.

Regards,

	Márton Németh

>> Signed-off-by: Márton Németh<nm127@freemail.hu>
>> ---
>> diff -r f23c5a878fb1 v4l2-apps/libv4l/libv4lconvert/tinyjpeg.c
>> --- a/v4l2-apps/libv4l/libv4lconvert/tinyjpeg.c	Mon Feb 01 13:32:46 2010 +0100
>> +++ b/v4l2-apps/libv4l/libv4lconvert/tinyjpeg.c	Mon Feb 01 23:05:39 2010 +0100
>> @@ -339,10 +339,15 @@
>>   	    } \
>>   	    break; \
>>   	  case 0xff: \
>> -	    if (stream[1] == 0xff&&  (stream[2]<  7 || stream[2] == 0xff)) { \
>> -	      stream += 3; \
>> -	      c = *stream++; \
>> -	      break; \
>> +	    if (stream[1] == 0xff) { \
>> +		if (stream[2]<  7) { \
>> +		    stream += 3; \
>> +		    c = *stream++; \
>> +		    break; \
>> +		} else if (stream[2] == 0xff) { \
>> +		    /* four 0xff in a row: the first belongs to the image data */ \
>> +		    break; \
>> +		}\
>>   	    } \
>>   	    /* Error fall through */ \
>>   	  default: \
> 
> 

Re: libv4l: possible problem found in PAC7302 JPEG decoding

[Németh Márton]

Hi Thomas,
Thomas Kaiser wrote:
> On 02/01/2010 10:23 PM, Németh Márton wrote:
>> Hello Hans,
>>
>> while I was dealing with Labtec Webcam 2200 and with gspca_pac7302 driver I recognised the
>> following behaviour. The stream received from the webcam is splitted by the gspca_pac7302
>> subdriver when the byte sequence 0xff, 0xff, 0x00, 0xff, 0x96 is found (pac_find_sof()).
>> Before transmitting the data to the userspace a JPEG header is added (pac_start_frame())
>> and the footer after the bytes 0xff, 0xd9 are removed.
>>
>> The data buffer which arrives to userspace looks like as follows (maybe not every detail is exact):
>>
>>  1. JPEG header
>>
>>  2. Some bytes of image data (near to 1024 bytes)
>>
>>  3. The byte sequence 0xff, 0xff, 0xff, 0x01 followed by 1024 bytes of data.
>>     This marker sequence and data repeats a couple of time. Exactly how much
>>     depends on the image content.
>>
>>  4. The byte sequence 0xff, 0xff, 0xff, 0x02 followed by 512 bytes of data.
>>     This marker sequence and data also repeats a couple of time.
>>
>>  5. The byte sequence 0xff, 0xff, 0xff, 0x00 followed by a variable amount of
>>     image data bytes.
>>
>>  6. The End of Image (EOI) marker 0xff, 0xd9.
>>
>> Now what can be wrong with the libv4l? In libv4lconvert/tinyjpeg.c, line 315 there is a
>> huge macro which tries to remove the 0xff, 0xff, 0xff, xx byte sequence from the received
>> image. This fails, however, if the image contains 0xff bytes just before the 0xff, 0xff,
>> 0xff, xx sequence because one byte from the image data (the first 0xff) is removed, then
>> the three 0xff bytes from the marker is also removed. The xx (which really belongs to the
>> marker) is left in the image data instead of the original 0xff byte.
>>
>> Based on my experiments this problem sometimes causes corrupted image decoding or that the
>> JPEG image cannot be decoded at all.
>>
> 
> Hello Németh
> 
> I remember the problem as I was working on the PAC7311.
> http://www.kaiser-linux.li/index.php?title=PAC7311
> 
> 
> This is the code I used in the JPEG decoder to remove the 0xff 0xff 0xff 
>   0xnn markers.
> 
> See http://www.kaiser-linux.li/files/PAC7311/gspcav1-PAC7311-20070425.tar.gz
> decoder/gspcadecoder.c pac7311_decode()

Do you remember whether this code was working properly always?

Regards,

	Márton Németh

Re: libv4l: possible problem found in PAC7302 JPEG decoding

[Thomas Kaiser]

On 02/02/2010 07:59 PM, Németh Márton wrote:
> Hi Thomas,
> Thomas Kaiser wrote:
>> On 02/01/2010 10:23 PM, Németh Márton wrote:
>>> Hello Hans,
>>>
>>> while I was dealing with Labtec Webcam 2200 and with gspca_pac7302 driver I recognised the
>>> following behaviour. The stream received from the webcam is splitted by the gspca_pac7302
>>> subdriver when the byte sequence 0xff, 0xff, 0x00, 0xff, 0x96 is found (pac_find_sof()).
>>> Before transmitting the data to the userspace a JPEG header is added (pac_start_frame())
>>> and the footer after the bytes 0xff, 0xd9 are removed.
>>>
>>> The data buffer which arrives to userspace looks like as follows (maybe not every detail is exact):
>>>
>>>  1. JPEG header
>>>
>>>  2. Some bytes of image data (near to 1024 bytes)
>>>
>>>  3. The byte sequence 0xff, 0xff, 0xff, 0x01 followed by 1024 bytes of data.
>>>     This marker sequence and data repeats a couple of time. Exactly how much
>>>     depends on the image content.
>>>
>>>  4. The byte sequence 0xff, 0xff, 0xff, 0x02 followed by 512 bytes of data.
>>>     This marker sequence and data also repeats a couple of time.
>>>
>>>  5. The byte sequence 0xff, 0xff, 0xff, 0x00 followed by a variable amount of
>>>     image data bytes.
>>>
>>>  6. The End of Image (EOI) marker 0xff, 0xd9.
>>>
>>> Now what can be wrong with the libv4l? In libv4lconvert/tinyjpeg.c, line 315 there is a
>>> huge macro which tries to remove the 0xff, 0xff, 0xff, xx byte sequence from the received
>>> image. This fails, however, if the image contains 0xff bytes just before the 0xff, 0xff,
>>> 0xff, xx sequence because one byte from the image data (the first 0xff) is removed, then
>>> the three 0xff bytes from the marker is also removed. The xx (which really belongs to the
>>> marker) is left in the image data instead of the original 0xff byte.
>>>
>>> Based on my experiments this problem sometimes causes corrupted image decoding or that the
>>> JPEG image cannot be decoded at all.
>>>
>> Hello Németh
>>
>> I remember the problem as I was working on the PAC7311.
>> http://www.kaiser-linux.li/index.php?title=PAC7311
>>
>>
>> This is the code I used in the JPEG decoder to remove the 0xff 0xff 0xff 
>>   0xnn markers.
>>
>> See http://www.kaiser-linux.li/files/PAC7311/gspcav1-PAC7311-20070425.tar.gz
>> decoder/gspcadecoder.c pac7311_decode()
> 
> Do you remember whether this code was working properly always?

Hello Németh

I am not 100% sure but it looked OK for me.

Anyway, we have to throw away these markers before the JPEG decoding 
starts. I don't think this code will fail when you parse the Byte stream 
(frame header removed before!).
We don't know these markers, so we don't need them.

Thomas

Re: [PATCH libv4l tree, RFC] libv4l: skip false Pixart markers with buffer copy

[Németh Márton]

Hi,

This is a proof-of-concept patch to try to decode the JPEG with PixArt markers.

Please check whether it is working at your side. My experience is that the
number of frames with glitch are reduced.

Regards,

	Márton Németh

---
From: Márton Németh <nm127@freemail.hu>

Before trying to decode the image data filter the PixArt markers out.

Signed-off-by: Márton Németh <nm127@freemail.hu>
---
diff -r 966f60c672e9 v4l2-apps/libv4l/libv4lconvert/tinyjpeg-internal.h
--- a/v4l2-apps/libv4l/libv4lconvert/tinyjpeg-internal.h	Tue Feb 02 11:34:06 2010 +0100
+++ b/v4l2-apps/libv4l/libv4lconvert/tinyjpeg-internal.h	Thu Feb 04 09:13:24 2010 +0100
@@ -91,8 +91,11 @@
   /* Private variables */
   const unsigned char *stream_begin, *stream_end;
   unsigned int stream_length;
+  unsigned char *stream_begin_filtered, *stream_end_filtered;
+  unsigned int stream_length_filtered;

   const unsigned char *stream;	/* Pointer to the current stream */
+  unsigned char *stream_filtered;
   unsigned int reservoir, nbits_in_reservoir;

   struct component component_infos[COMPONENTS];
diff -r 966f60c672e9 v4l2-apps/libv4l/libv4lconvert/tinyjpeg.c
--- a/v4l2-apps/libv4l/libv4lconvert/tinyjpeg.c	Tue Feb 02 11:34:06 2010 +0100
+++ b/v4l2-apps/libv4l/libv4lconvert/tinyjpeg.c	Thu Feb 04 09:13:24 2010 +0100
@@ -312,19 +312,18 @@

 /* Special Pixart versions of the *_nbits functions, these remove the special
    ff ff ff xx sequences pixart cams insert from the bitstream */
-#define pixart_fill_nbits(reservoir,nbits_in_reservoir,stream,nbits_wanted) \
+#define pixart_fill_nbits(reservoir,nbits_in_reservoir,stream,stream_end,nbits_wanted) \
 do { \
    while (nbits_in_reservoir<nbits_wanted) \
     { \
       unsigned char c; \
-      if (stream >= priv->stream_end) { \
+      if (stream >= stream_end) { \
 	snprintf(priv->error_string, sizeof(priv->error_string), \
 	  "fill_nbits error: need %u more bits\n", \
 	  nbits_wanted - nbits_in_reservoir); \
 	longjmp(priv->jump_state, -EIO); \
       } \
       c = *stream++; \
-      reservoir <<= 8; \
       if (c == 0xff) { \
 	switch (stream[0]) { \
 	  case 0x00: \
@@ -332,7 +331,7 @@
 	    break; \
 	  case 0xd9: /* EOF marker */ \
 	    stream++; \
-	    if (stream != priv->stream_end) { \
+	    if (stream != stream_end) { \
 	      snprintf(priv->error_string, sizeof(priv->error_string), \
 		"Pixart JPEG error: premature EOF\n"); \
 	      longjmp(priv->jump_state, -EIO); \
@@ -340,14 +339,22 @@
 	    break; \
 	  case 0xff: \
 	    if (stream[1] == 0xff) { \
-		if (stream[2] < 7) { \
+		if (stream[2] == 0) { \
+		    stream += 3; \
+		    c = *stream++; \
+		    break; \
+		} else if (stream[2] == 1) { \
+		    stream += 3; \
+		    c = *stream++; \
+		    break; \
+		} else if (stream[2] == 2) { \
 		    stream += 3; \
 		    c = *stream++; \
 		    break; \
 		} else if (stream[2] == 0xff) { \
-		    /* four 0xff in a row: the first belongs to the image data */ \
+		    /* four 0xff in a row: the first belongs to the image */ \
 		    break; \
-		}\
+		} \
 	    } \
 	    /* Error fall through */ \
 	  default: \
@@ -358,15 +365,16 @@
 	    longjmp(priv->jump_state, -EIO); \
 	} \
       } \
+      reservoir <<= 8; \
       reservoir |= c; \
       nbits_in_reservoir+=8; \
     } \
 }  while(0);

 /* Signed version !!!! */
-#define pixart_get_nbits(reservoir,nbits_in_reservoir,stream,nbits_wanted,result) \
+#define pixart_get_nbits(reservoir,nbits_in_reservoir,stream,stream_end,nbits_wanted,result) \
 do { \
-   pixart_fill_nbits(reservoir,nbits_in_reservoir,stream,(nbits_wanted)); \
+   pixart_fill_nbits(reservoir,nbits_in_reservoir,stream,stream_end,(nbits_wanted)); \
    result = ((reservoir)>>(nbits_in_reservoir-(nbits_wanted))); \
    nbits_in_reservoir -= (nbits_wanted);  \
    reservoir &= ((1U<<nbits_in_reservoir)-1); \
@@ -374,9 +382,9 @@
        result += (0xFFFFFFFFUL<<(nbits_wanted))+1; \
 }  while(0);

-#define pixart_look_nbits(reservoir,nbits_in_reservoir,stream,nbits_wanted,result) \
+#define pixart_look_nbits(reservoir,nbits_in_reservoir,stream,stream_end,nbits_wanted,result) \
 do { \
-   pixart_fill_nbits(reservoir,nbits_in_reservoir,stream,(nbits_wanted)); \
+   pixart_fill_nbits(reservoir,nbits_in_reservoir,stream,stream_end,(nbits_wanted)); \
    result = ((reservoir)>>(nbits_in_reservoir-(nbits_wanted))); \
 }  while(0);

@@ -443,7 +451,8 @@
   unsigned int extra_nbits, nbits;
   uint16_t *slowtable;

-  pixart_look_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream, HUFFMAN_HASH_NBITS, hcode);
+  pixart_look_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream_filtered,
+		    priv->stream_end_filtered, HUFFMAN_HASH_NBITS, hcode);
   value = huffman_table->lookup[hcode];
   if (value >= 0)
   {
@@ -457,7 +466,8 @@
    {
      nbits = HUFFMAN_HASH_NBITS + 1 + extra_nbits;

-     pixart_look_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream, nbits, hcode);
+     pixart_look_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream_filtered,
+			priv->stream_end_filtered, nbits, hcode);
      slowtable = huffman_table->slowtable[extra_nbits];
      /* Search if the code is in this array */
      while (slowtable[0]) {
@@ -557,7 +567,8 @@
   /* DC coefficient decoding */
   huff_code = pixart_get_next_huffman_code(priv, c->DC_table);
   if (huff_code) {
-     pixart_get_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream, huff_code, DCT[0]);
+     pixart_get_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream_filtered,
+		priv->stream_end_filtered, huff_code, DCT[0]);
      DCT[0] += c->previous_DC;
      c->previous_DC = DCT[0];
   } else {
@@ -585,7 +596,8 @@
       {
 	j += count_0;	/* skip count_0 zeroes */
 	if (j < 64 ) {
-	  pixart_get_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream, size_val, DCT[j]);
+	  pixart_get_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream_filtered,
+			priv->stream_end_filtered, size_val, DCT[j]);
 	  j++;
 	}
       }
@@ -1611,8 +1623,8 @@
 {
   unsigned char marker;

-  pixart_look_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream,
-		    8, marker);
+  pixart_look_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream_filtered,
+		    priv->stream_end_filtered, 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
@@ -2342,6 +2354,97 @@

 int tinyjpeg_decode_planar(struct jdec_private *priv, int pixfmt);

+static int memcpy_filter(unsigned char *dest, const unsigned char *src, int n)
+{
+	int i = 0;
+	int j = 0;
+	int state = 0;
+	int last_i = 0;
+
+	i = 0;
+	j = 0;
+
+	/* 5 bytes are already dropped in kernel: 0xff 0xff 0x00 0xff 0x96 */
+	/* Copy the rest of 1024 bytes */
+	memcpy(&(dest[j]), &(src[i]), 1024-5);
+	i += 1024-5;
+	j += 1024-5;
+
+	while (i < n) {
+		switch (state) {
+			case 0:
+				if (src[i] == 0xff)
+					state = 1;
+				else {
+					state = 0;
+					dest[j++] = src[i];
+				}
+				break;
+			case 1:
+				if (src[i] == 0xff)
+					state = 2;
+				else {
+					state = 0;
+					dest[j++] = src[i-1];
+					dest[j++] = src[i];
+				}
+				break;
+			case 2:
+				switch (src[i]) {
+					case 0xff:
+						state = 3;
+						break;
+					default:
+						state = 0;
+						dest[j++] = src[i-2];
+						dest[j++] = src[i-1];
+						dest[j++] = src[i];
+				}
+				break;
+			case 3:
+				switch (src[i]) {
+					case 0:
+						/* found 0xff 0xff 0xff 0x00 */
+						state = 0;
+						break;
+					case 1:
+						/* found 0xff 0xff 0xff 0x01 */
+						last_i = i+1;
+						memcpy(&(dest[j]), &(src[i+1]), 1024);
+						i += 1024;
+						j += 1024;
+						state = 0;
+						break;
+					case 2:
+						/* found 0xff 0xff 0xff 0x02 */
+						last_i = i+1;
+						memcpy(&(dest[j]), &(src[i+1]), 512);
+						i += 512;
+						j += 512;
+						state = 0;
+						break;
+					case 0xff:
+						printf(" ! ");
+						dest[j++] = src[i-3];
+						state = 3;
+						break;
+
+					default:
+						state = 0;
+						dest[j++] = src[i-3];
+						dest[j++] = src[i-2];
+						dest[j++] = src[i-1];
+						dest[j++] = src[i];
+				
+				}
+		}
+		i++;
+	}
+
+	return j;
+}
+
+
 /**
  * Decode and convert the jpeg image into @pixfmt@ image
  *
@@ -2356,8 +2459,10 @@
   const convert_colorspace_fct *colorspace_array_conv;
   convert_colorspace_fct convert_to_pixfmt;

-  if (setjmp(priv->jump_state))
+  if (setjmp(priv->jump_state)) {
+    printf("ERROR: %s\n", priv->error_string);
     return -1;
+  }

   if (priv->flags & TINYJPEG_FLAGS_PLANAR_JPEG)
     return tinyjpeg_decode_planar(priv, pixfmt);
@@ -2369,8 +2474,20 @@
   bytes_per_blocklines[2] = 0;

   decode_mcu_table = decode_mcu_3comp_table;
-  if (priv->flags & TINYJPEG_FLAGS_PIXART_JPEG)
+  if (priv->flags & TINYJPEG_FLAGS_PIXART_JPEG) {
+    int len_filtered = 0;
+
+    priv->stream_begin_filtered = malloc(priv->stream_end - priv->stream);
+    if (priv->stream_begin_filtered) {
+	memset(priv->stream_begin_filtered, 0, priv->stream_end - priv->stream);
+	priv->stream_filtered = priv->stream_begin_filtered;
+	len_filtered = memcpy_filter(priv->stream_filtered,
+			priv->stream, priv->stream_end - priv->stream);
+    }
+    priv->stream_end_filtered = priv->stream_filtered + len_filtered;
+
     decode_mcu_table = pixart_decode_mcu_3comp_table;
+  }

   switch (pixfmt) {
      case TINYJPEG_FMT_YUV420P:
@@ -2487,8 +2604,12 @@

   if (priv->flags & TINYJPEG_FLAGS_PIXART_JPEG) {
     /* Additional sanity check for funky Pixart format */
-    if ((priv->stream_end - priv->stream) > 5)
+    if ((priv->stream_end_filtered - priv->stream_filtered) > 5)
       error("Pixart JPEG error, stream does not end with EOF marker\n");
+
+    free(priv->stream_begin_filtered);
+    priv->stream_filtered = NULL;
+    priv->stream_end_filtered = NULL;
   }

   return 0;

Re: [PATCH libv4l tree, RFC] libv4l: skip false Pixart markers with buffer copy

[Hans de Goede]

Hi,

On 02/04/2010 09:22 AM, Németh Márton wrote:
> Hi,
>
> This is a proof-of-concept patch to try to decode the JPEG with PixArt markers.
>
> Please check whether it is working at your side. My experience is that the
> number of frames with glitch are reduced.
>

Hi,

Good job! I never noticed the ff ff ff xx markers where spaced a certain numbers
of bytes apart. Based on that I've written a different filtering function, which
at least for me completely removes all glitches!!

See:
http://linuxtv.org/hg/~hgoede/libv4l/rev/1fa67e17b77c

Thanks !!!

Regards,

Hans


> Regards,
>
> 	Márton Németh
>
> ---
> From: Márton Németh<nm127@freemail.hu>
>
> Before trying to decode the image data filter the PixArt markers out.
>
> Signed-off-by: Márton Németh<nm127@freemail.hu>
> ---
> diff -r 966f60c672e9 v4l2-apps/libv4l/libv4lconvert/tinyjpeg-internal.h
> --- a/v4l2-apps/libv4l/libv4lconvert/tinyjpeg-internal.h	Tue Feb 02 11:34:06 2010 +0100
> +++ b/v4l2-apps/libv4l/libv4lconvert/tinyjpeg-internal.h	Thu Feb 04 09:13:24 2010 +0100
> @@ -91,8 +91,11 @@
>     /* Private variables */
>     const unsigned char *stream_begin, *stream_end;
>     unsigned int stream_length;
> +  unsigned char *stream_begin_filtered, *stream_end_filtered;
> +  unsigned int stream_length_filtered;
>
>     const unsigned char *stream;	/* Pointer to the current stream */
> +  unsigned char *stream_filtered;
>     unsigned int reservoir, nbits_in_reservoir;
>
>     struct component component_infos[COMPONENTS];
> diff -r 966f60c672e9 v4l2-apps/libv4l/libv4lconvert/tinyjpeg.c
> --- a/v4l2-apps/libv4l/libv4lconvert/tinyjpeg.c	Tue Feb 02 11:34:06 2010 +0100
> +++ b/v4l2-apps/libv4l/libv4lconvert/tinyjpeg.c	Thu Feb 04 09:13:24 2010 +0100
> @@ -312,19 +312,18 @@
>
>   /* Special Pixart versions of the *_nbits functions, these remove the special
>      ff ff ff xx sequences pixart cams insert from the bitstream */
> -#define pixart_fill_nbits(reservoir,nbits_in_reservoir,stream,nbits_wanted) \
> +#define pixart_fill_nbits(reservoir,nbits_in_reservoir,stream,stream_end,nbits_wanted) \
>   do { \
>      while (nbits_in_reservoir<nbits_wanted) \
>       { \
>         unsigned char c; \
> -      if (stream>= priv->stream_end) { \
> +      if (stream>= stream_end) { \
>   	snprintf(priv->error_string, sizeof(priv->error_string), \
>   	  "fill_nbits error: need %u more bits\n", \
>   	  nbits_wanted - nbits_in_reservoir); \
>   	longjmp(priv->jump_state, -EIO); \
>         } \
>         c = *stream++; \
> -      reservoir<<= 8; \
>         if (c == 0xff) { \
>   	switch (stream[0]) { \
>   	  case 0x00: \
> @@ -332,7 +331,7 @@
>   	    break; \
>   	  case 0xd9: /* EOF marker */ \
>   	    stream++; \
> -	    if (stream != priv->stream_end) { \
> +	    if (stream != stream_end) { \
>   	      snprintf(priv->error_string, sizeof(priv->error_string), \
>   		"Pixart JPEG error: premature EOF\n"); \
>   	      longjmp(priv->jump_state, -EIO); \
> @@ -340,14 +339,22 @@
>   	    break; \
>   	  case 0xff: \
>   	    if (stream[1] == 0xff) { \
> -		if (stream[2]<  7) { \
> +		if (stream[2] == 0) { \
> +		    stream += 3; \
> +		    c = *stream++; \
> +		    break; \
> +		} else if (stream[2] == 1) { \
> +		    stream += 3; \
> +		    c = *stream++; \
> +		    break; \
> +		} else if (stream[2] == 2) { \
>   		    stream += 3; \
>   		    c = *stream++; \
>   		    break; \
>   		} else if (stream[2] == 0xff) { \
> -		    /* four 0xff in a row: the first belongs to the image data */ \
> +		    /* four 0xff in a row: the first belongs to the image */ \
>   		    break; \
> -		}\
> +		} \
>   	    } \
>   	    /* Error fall through */ \
>   	  default: \
> @@ -358,15 +365,16 @@
>   	    longjmp(priv->jump_state, -EIO); \
>   	} \
>         } \
> +      reservoir<<= 8; \
>         reservoir |= c; \
>         nbits_in_reservoir+=8; \
>       } \
>   }  while(0);
>
>   /* Signed version !!!! */
> -#define pixart_get_nbits(reservoir,nbits_in_reservoir,stream,nbits_wanted,result) \
> +#define pixart_get_nbits(reservoir,nbits_in_reservoir,stream,stream_end,nbits_wanted,result) \
>   do { \
> -   pixart_fill_nbits(reservoir,nbits_in_reservoir,stream,(nbits_wanted)); \
> +   pixart_fill_nbits(reservoir,nbits_in_reservoir,stream,stream_end,(nbits_wanted)); \
>      result = ((reservoir)>>(nbits_in_reservoir-(nbits_wanted))); \
>      nbits_in_reservoir -= (nbits_wanted);  \
>      reservoir&= ((1U<<nbits_in_reservoir)-1); \
> @@ -374,9 +382,9 @@
>          result += (0xFFFFFFFFUL<<(nbits_wanted))+1; \
>   }  while(0);
>
> -#define pixart_look_nbits(reservoir,nbits_in_reservoir,stream,nbits_wanted,result) \
> +#define pixart_look_nbits(reservoir,nbits_in_reservoir,stream,stream_end,nbits_wanted,result) \
>   do { \
> -   pixart_fill_nbits(reservoir,nbits_in_reservoir,stream,(nbits_wanted)); \
> +   pixart_fill_nbits(reservoir,nbits_in_reservoir,stream,stream_end,(nbits_wanted)); \
>      result = ((reservoir)>>(nbits_in_reservoir-(nbits_wanted))); \
>   }  while(0);
>
> @@ -443,7 +451,8 @@
>     unsigned int extra_nbits, nbits;
>     uint16_t *slowtable;
>
> -  pixart_look_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream, HUFFMAN_HASH_NBITS, hcode);
> +  pixart_look_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream_filtered,
> +		    priv->stream_end_filtered, HUFFMAN_HASH_NBITS, hcode);
>     value = huffman_table->lookup[hcode];
>     if (value>= 0)
>     {
> @@ -457,7 +466,8 @@
>      {
>        nbits = HUFFMAN_HASH_NBITS + 1 + extra_nbits;
>
> -     pixart_look_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream, nbits, hcode);
> +     pixart_look_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream_filtered,
> +			priv->stream_end_filtered, nbits, hcode);
>        slowtable = huffman_table->slowtable[extra_nbits];
>        /* Search if the code is in this array */
>        while (slowtable[0]) {
> @@ -557,7 +567,8 @@
>     /* DC coefficient decoding */
>     huff_code = pixart_get_next_huffman_code(priv, c->DC_table);
>     if (huff_code) {
> -     pixart_get_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream, huff_code, DCT[0]);
> +     pixart_get_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream_filtered,
> +		priv->stream_end_filtered, huff_code, DCT[0]);
>        DCT[0] += c->previous_DC;
>        c->previous_DC = DCT[0];
>     } else {
> @@ -585,7 +596,8 @@
>         {
>   	j += count_0;	/* skip count_0 zeroes */
>   	if (j<  64 ) {
> -	  pixart_get_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream, size_val, DCT[j]);
> +	  pixart_get_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream_filtered,
> +			priv->stream_end_filtered, size_val, DCT[j]);
>   	  j++;
>   	}
>         }
> @@ -1611,8 +1623,8 @@
>   {
>     unsigned char marker;
>
> -  pixart_look_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream,
> -		    8, marker);
> +  pixart_look_nbits(priv->reservoir, priv->nbits_in_reservoir, priv->stream_filtered,
> +		    priv->stream_end_filtered, 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
> @@ -2342,6 +2354,97 @@
>
>   int tinyjpeg_decode_planar(struct jdec_private *priv, int pixfmt);
>
> +static int memcpy_filter(unsigned char *dest, const unsigned char *src, int n)
> +{
> +	int i = 0;
> +	int j = 0;
> +	int state = 0;
> +	int last_i = 0;
> +
> +	i = 0;
> +	j = 0;
> +
> +	/* 5 bytes are already dropped in kernel: 0xff 0xff 0x00 0xff 0x96 */
> +	/* Copy the rest of 1024 bytes */
> +	memcpy(&(dest[j]),&(src[i]), 1024-5);
> +	i += 1024-5;
> +	j += 1024-5;
> +
> +	while (i<  n) {
> +		switch (state) {
> +			case 0:
> +				if (src[i] == 0xff)
> +					state = 1;
> +				else {
> +					state = 0;
> +					dest[j++] = src[i];
> +				}
> +				break;
> +			case 1:
> +				if (src[i] == 0xff)
> +					state = 2;
> +				else {
> +					state = 0;
> +					dest[j++] = src[i-1];
> +					dest[j++] = src[i];
> +				}
> +				break;
> +			case 2:
> +				switch (src[i]) {
> +					case 0xff:
> +						state = 3;
> +						break;
> +					default:
> +						state = 0;
> +						dest[j++] = src[i-2];
> +						dest[j++] = src[i-1];
> +						dest[j++] = src[i];
> +				}
> +				break;
> +			case 3:
> +				switch (src[i]) {
> +					case 0:
> +						/* found 0xff 0xff 0xff 0x00 */
> +						state = 0;
> +						break;
> +					case 1:
> +						/* found 0xff 0xff 0xff 0x01 */
> +						last_i = i+1;
> +						memcpy(&(dest[j]),&(src[i+1]), 1024);
> +						i += 1024;
> +						j += 1024;
> +						state = 0;
> +						break;
> +					case 2:
> +						/* found 0xff 0xff 0xff 0x02 */
> +						last_i = i+1;
> +						memcpy(&(dest[j]),&(src[i+1]), 512);
> +						i += 512;
> +						j += 512;
> +						state = 0;
> +						break;
> +					case 0xff:
> +						printf(" ! ");
> +						dest[j++] = src[i-3];
> +						state = 3;
> +						break;
> +
> +					default:
> +						state = 0;
> +						dest[j++] = src[i-3];
> +						dest[j++] = src[i-2];
> +						dest[j++] = src[i-1];
> +						dest[j++] = src[i];
> +				
> +				}
> +		}
> +		i++;
> +	}
> +
> +	return j;
> +}
> +
> +
>   /**
>    * Decode and convert the jpeg image into @pixfmt@ image
>    *
> @@ -2356,8 +2459,10 @@
>     const convert_colorspace_fct *colorspace_array_conv;
>     convert_colorspace_fct convert_to_pixfmt;
>
> -  if (setjmp(priv->jump_state))
> +  if (setjmp(priv->jump_state)) {
> +    printf("ERROR: %s\n", priv->error_string);
>       return -1;
> +  }
>
>     if (priv->flags&  TINYJPEG_FLAGS_PLANAR_JPEG)
>       return tinyjpeg_decode_planar(priv, pixfmt);
> @@ -2369,8 +2474,20 @@
>     bytes_per_blocklines[2] = 0;
>
>     decode_mcu_table = decode_mcu_3comp_table;
> -  if (priv->flags&  TINYJPEG_FLAGS_PIXART_JPEG)
> +  if (priv->flags&  TINYJPEG_FLAGS_PIXART_JPEG) {
> +    int len_filtered = 0;
> +
> +    priv->stream_begin_filtered = malloc(priv->stream_end - priv->stream);
> +    if (priv->stream_begin_filtered) {
> +	memset(priv->stream_begin_filtered, 0, priv->stream_end - priv->stream);
> +	priv->stream_filtered = priv->stream_begin_filtered;
> +	len_filtered = memcpy_filter(priv->stream_filtered,
> +			priv->stream, priv->stream_end - priv->stream);
> +    }
> +    priv->stream_end_filtered = priv->stream_filtered + len_filtered;
> +
>       decode_mcu_table = pixart_decode_mcu_3comp_table;
> +  }
>
>     switch (pixfmt) {
>        case TINYJPEG_FMT_YUV420P:
> @@ -2487,8 +2604,12 @@
>
>     if (priv->flags&  TINYJPEG_FLAGS_PIXART_JPEG) {
>       /* Additional sanity check for funky Pixart format */
> -    if ((priv->stream_end - priv->stream)>  5)
> +    if ((priv->stream_end_filtered - priv->stream_filtered)>  5)
>         error("Pixart JPEG error, stream does not end with EOF marker\n");
> +
> +    free(priv->stream_begin_filtered);
> +    priv->stream_filtered = NULL;
> +    priv->stream_end_filtered = NULL;
>     }
>
>     return 0;

Re: [PATCH libv4l tree, RFC] libv4l: skip false Pixart markers with buffer copy

[Thomas Kaiser]

On 02/05/2010 02:42 PM, Hans de Goede wrote:
> Good job! I never noticed the ff ff ff xx markers where spaced a certain 
> numbers

You can find this info here 
http://www.kaiser-linux.li/index.php?title=PAC7311
go down the page to 2006-11-12

Thomas