[PATCH 0/2] media: coda: Add ability to decode wider range of jpegs

[PATCH 0/2] media: coda: Add ability to decode wider range of jpegs

[Adrian Ratiu]

Hello,

At Ezequiel's suggestion I'm sending these patches as a standalone
series which applies on top of Philipp's v2 decoder series located
at Message-Id: <20200318183536.15779-1-p.zabel@pengutronix.de>

The use case for these is that we get jpegs from smartphone apps
over which we have no control: they run on a variety of OSes /
technologies / libraries, could produce jpegs with dimensions from
64x64 to full HD and could use standard or optimized huffman coding.

By relaxing these two checks in the decoder we are able to correctly
display all our images and think these would also benefit others.

Tested with CODA960 on IMX6DL.

Kind regards,
Adrian

Adrian Ratiu (2):
  media: coda: jpeg: support optimized huffman tables
  media: coda: be more flexible wrt jpeg dimensions

 drivers/media/platform/coda/coda-jpeg.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

-- 
2.25.2

[PATCH 1/2] media: coda: jpeg: support optimized huffman tables

[Adrian Ratiu]

Each jpeg can have the huffman tables optimized for its specific content
meaning that the table lenghts and values don't match the standard table
of substitutions so there's no reason to hardcode and expect the sandard
lenghts, otherwise we just end up rejecting optimized jpegs altogether.

Tested on CODA960.

Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
---
 drivers/media/platform/coda/coda-jpeg.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/coda/coda-jpeg.c b/drivers/media/platform/coda/coda-jpeg.c
index 6a11b64efb6b..162ba28a6b95 100644
--- a/drivers/media/platform/coda/coda-jpeg.c
+++ b/drivers/media/platform/coda/coda-jpeg.c
@@ -343,7 +343,8 @@ int coda_jpeg_decode_header(struct coda_ctx *ctx, struct vb2_buffer *vb)
 			v4l2_err(&dev->v4l2_dev, "missing Huffman table\n");
 			return -EINVAL;
 		}
-		if (huffman_tables[i].length != ((i & 2) ? 178 : 28)) {
+		if (huffman_tables[i].length < 17 ||
+		    huffman_tables[i].length > 178) {
 			v4l2_err(&dev->v4l2_dev,
 				 "invalid Huffman table %d length: %zu\n",
 				 i, huffman_tables[i].length);
@@ -357,10 +358,12 @@ int coda_jpeg_decode_header(struct coda_ctx *ctx, struct vb2_buffer *vb)
 			return -ENOMEM;
 		ctx->params.jpeg_huff_tab = huff_tab;
 	}
-	memcpy(huff_tab->luma_dc, huffman_tables[0].start, 16 + 12);
-	memcpy(huff_tab->chroma_dc, huffman_tables[1].start, 16 + 12);
-	memcpy(huff_tab->luma_ac, huffman_tables[2].start, 16 + 162);
-	memcpy(huff_tab->chroma_ac, huffman_tables[3].start, 16 + 162);
+
+	memset(huff_tab, 0, sizeof(*huff_tab));
+	memcpy(huff_tab->luma_dc, huffman_tables[0].start, huffman_tables[0].length);
+	memcpy(huff_tab->chroma_dc, huffman_tables[1].start, huffman_tables[1].length);
+	memcpy(huff_tab->luma_ac, huffman_tables[2].start, huffman_tables[2].length);
+	memcpy(huff_tab->chroma_ac, huffman_tables[3].start, huffman_tables[3].length);
 
 	/* check scan header */
 	for (i = 0; i < scan_header.num_components; i++) {
-- 
2.25.2

Re: [PATCH 1/2] media: coda: jpeg: support optimized huffman tables

[Philipp Zabel]

Hi Adrian,

On Mon, Mar 23, 2020 at 03:09:36PM +0200, Adrian Ratiu wrote:
> Each jpeg can have the huffman tables optimized for its specific content
> meaning that the table lenghts and values don't match the standard table
> of substitutions so there's no reason to hardcode and expect the sandard
> lenghts, otherwise we just end up rejecting optimized jpegs altogether.

Thank you, that is a great improvement. There's one issue remaining
below:

> Tested on CODA960.
> 
> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
> ---
>  drivers/media/platform/coda/coda-jpeg.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/coda/coda-jpeg.c b/drivers/media/platform/coda/coda-jpeg.c
> index 6a11b64efb6b..162ba28a6b95 100644
> --- a/drivers/media/platform/coda/coda-jpeg.c
> +++ b/drivers/media/platform/coda/coda-jpeg.c
> @@ -343,7 +343,8 @@ int coda_jpeg_decode_header(struct coda_ctx *ctx, struct vb2_buffer *vb)
>  			v4l2_err(&dev->v4l2_dev, "missing Huffman table\n");
>  			return -EINVAL;
>  		}
> -		if (huffman_tables[i].length != ((i & 2) ? 178 : 28)) {
> +		if (huffman_tables[i].length < 17 ||
> +		    huffman_tables[i].length > 178) {

The maximum length of the DC tables is 16 + 12, so this should
still be checked for a maximum length of 28 if (i & 2) == 0.

>  			v4l2_err(&dev->v4l2_dev,
>  				 "invalid Huffman table %d length: %zu\n",
>  				 i, huffman_tables[i].length);
> @@ -357,10 +358,12 @@ int coda_jpeg_decode_header(struct coda_ctx *ctx, struct vb2_buffer *vb)
>  			return -ENOMEM;
>  		ctx->params.jpeg_huff_tab = huff_tab;
>  	}
> -	memcpy(huff_tab->luma_dc, huffman_tables[0].start, 16 + 12);
> -	memcpy(huff_tab->chroma_dc, huffman_tables[1].start, 16 + 12);
> -	memcpy(huff_tab->luma_ac, huffman_tables[2].start, 16 + 162);
> -	memcpy(huff_tab->chroma_ac, huffman_tables[3].start, 16 + 162);
> +
> +	memset(huff_tab, 0, sizeof(*huff_tab));
> +	memcpy(huff_tab->luma_dc, huffman_tables[0].start, huffman_tables[0].length);
> +	memcpy(huff_tab->chroma_dc, huffman_tables[1].start, huffman_tables[1].length);

Otherwise these two might overflow the luma_dc and chroma_dc arrays.

regards
Philipp

[PATCH 2/2] media: coda: be more flexible wrt jpeg dimensions

[Adrian Ratiu]

Don't require jpeg dimensions to exactly match format dimensions,
so we are able to decode and display a wider range jpegs instead
of outright rejecting the ones which don't match.

This is useful in applications which pass jpegs with arbitrary
dimensions, where buffers can be reused to decode smaller jpegs
without having to do expensive renegotiations.

Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
---
 drivers/media/platform/coda/coda-jpeg.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/media/platform/coda/coda-jpeg.c b/drivers/media/platform/coda/coda-jpeg.c
index 162ba28a6b95..782a78dcaf4d 100644
--- a/drivers/media/platform/coda/coda-jpeg.c
+++ b/drivers/media/platform/coda/coda-jpeg.c
@@ -302,13 +302,6 @@ int coda_jpeg_decode_header(struct coda_ctx *ctx, struct vb2_buffer *vb)
 	}
 
 	q_data_src = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
-	if (header.frame.height != q_data_src->height ||
-	    header.frame.width != q_data_src->width) {
-		v4l2_err(&dev->v4l2_dev,
-			 "dimensions don't match format: %dx%d\n",
-			 header.frame.width, header.frame.height);
-		return -EINVAL;
-	}
 
 	if (header.frame.num_components != 3) {
 		v4l2_err(&dev->v4l2_dev,
-- 
2.25.2

Re: [PATCH 2/2] media: coda: be more flexible wrt jpeg dimensions

[Ezequiel Garcia]

On Mon, 2020-03-23 at 15:09 +0200, Adrian Ratiu wrote:
> Don't require jpeg dimensions to exactly match format dimensions,
> so we are able to decode and display a wider range jpegs instead
> of outright rejecting the ones which don't match.
> 
> This is useful in applications which pass jpegs with arbitrary
> dimensions, where buffers can be reused to decode smaller jpegs
> without having to do expensive renegotiations.
> 
> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
> ---
>  drivers/media/platform/coda/coda-jpeg.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/media/platform/coda/coda-jpeg.c b/drivers/media/platform/coda/coda-jpeg.c
> index 162ba28a6b95..782a78dcaf4d 100644
> --- a/drivers/media/platform/coda/coda-jpeg.c
> +++ b/drivers/media/platform/coda/coda-jpeg.c
> @@ -302,13 +302,6 @@ int coda_jpeg_decode_header(struct coda_ctx *ctx, struct vb2_buffer *vb)
>  	}
>  
>  	q_data_src = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
> -	if (header.frame.height != q_data_src->height ||
> -	    header.frame.width != q_data_src->width) {
> -		v4l2_err(&dev->v4l2_dev,
> -			 "dimensions don't match format: %dx%d\n",
> -			 header.frame.width, header.frame.height);
> -		return -EINVAL;
> -	}

Since you are dropping this check, and allowing other sizes to be
decoded using, do you have any check to make sure you don't overrun
(in bytes, not in width x height) the CAPTURE (decoded) buffer?

Also, IIUC your application is negotiating W x H, but then
passing a different size: I wonder if this is not an abuse
of the spec?

Thanks,
Ezequiel

>  
>  	if (header.frame.num_components != 3) {
>  		v4l2_err(&dev->v4l2_dev,

Re: [PATCH 2/2] media: coda: be more flexible wrt jpeg dimensions

[Philipp Zabel]

Hi Adrian,

On Mon, Mar 23, 2020 at 03:09:37PM +0200, Adrian Ratiu wrote:
> Don't require jpeg dimensions to exactly match format dimensions,
> so we are able to decode and display a wider range jpegs instead
> of outright rejecting the ones which don't match.

I don't think this is right. If userspace feeds us an incomatible
JPEG we should probably stop decoding and send a source change
event instead [1].

[1] https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/dev-decoder.html#dynamic-resolution-change

regards
Philipp

Re: [PATCH 2/2] media: coda: be more flexible wrt jpeg dimensions

[Adrian Ratiu]

On Tue, 24 Mar 2020, Philipp Zabel <pza@pengutronix.de> wrote:
> Hi Adrian, 
> 
> On Mon, Mar 23, 2020 at 03:09:37PM +0200, Adrian Ratiu wrote: 
>> Don't require jpeg dimensions to exactly match format 
>> dimensions, so we are able to decode and display a wider range 
>> jpegs instead of outright rejecting the ones which don't match. 
> 
> I don't think this is right. If userspace feeds us an 
> incomatible JPEG we should probably stop decoding and send a 
> source change event instead [1]. 

Please ignore this second patch as it abuses the spec as Ezequiel
mentioned in the other reply. The correct approach would be to
do a format renegotiation in userspace so the test can be passed.

I will send an updated v2 of the huffman table patch based on
your feedback.

Thank you,
Adrian

>
> [1] https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/dev-decoder.html#dynamic-resolution-change
>
> regards
> Philipp