2020-11-12 05:39:40

by Mirela Rabulea OSS

[permalink] [raw]
Subject: [PATCH v5 09/10] media: Avoid parsing quantization and huffman tables

From: Mirela Rabulea <[email protected]>

These are optional in struct v4l2_jpeg_header, so do not parse if
not requested, save some time.

Signed-off-by: Mirela Rabulea <[email protected]>
---
drivers/media/v4l2-core/v4l2-jpeg.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c b/drivers/media/v4l2-core/v4l2-jpeg.c
index d77e04083d57..7576cd0ce6b9 100644
--- a/drivers/media/v4l2-core/v4l2-jpeg.c
+++ b/drivers/media/v4l2-core/v4l2-jpeg.c
@@ -307,6 +307,9 @@ static int jpeg_parse_quantization_tables(struct jpeg_stream *stream,
{
int len = jpeg_get_word_be(stream);

+ if (!tables)
+ return 0;
+
if (len < 0)
return len;
/* Lq = 2 + n * 65 (for baseline DCT), n >= 1 */
@@ -361,6 +364,9 @@ static int jpeg_parse_huffman_tables(struct jpeg_stream *stream,
int mt;
int len = jpeg_get_word_be(stream);

+ if (!tables)
+ return 0;
+
if (len < 0)
return len;
/* Table B.5 - Huffman table specification parameter sizes and values */
--
2.17.1


2020-12-02 12:15:29

by Hans Verkuil

[permalink] [raw]
Subject: Re: [PATCH v5 09/10] media: Avoid parsing quantization and huffman tables

On 12/11/2020 04:05, Mirela Rabulea (OSS) wrote:
> From: Mirela Rabulea <[email protected]>
>
> These are optional in struct v4l2_jpeg_header, so do not parse if
> not requested, save some time.
>
> Signed-off-by: Mirela Rabulea <[email protected]>
> ---
> drivers/media/v4l2-core/v4l2-jpeg.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c b/drivers/media/v4l2-core/v4l2-jpeg.c
> index d77e04083d57..7576cd0ce6b9 100644
> --- a/drivers/media/v4l2-core/v4l2-jpeg.c
> +++ b/drivers/media/v4l2-core/v4l2-jpeg.c
> @@ -307,6 +307,9 @@ static int jpeg_parse_quantization_tables(struct jpeg_stream *stream,
> {
> int len = jpeg_get_word_be(stream);
>
> + if (!tables)
> + return 0;
> +

It feels more natural to check for a non-NULL out->quantization_tables
or non-NULL out->huffman_tables pointer in v4l2_jpeg_parse_header()
rather than in these low-level functions. It's weird to have this check here.

Regards,

Hans

> if (len < 0)
> return len;
> /* Lq = 2 + n * 65 (for baseline DCT), n >= 1 */
> @@ -361,6 +364,9 @@ static int jpeg_parse_huffman_tables(struct jpeg_stream *stream,
> int mt;
> int len = jpeg_get_word_be(stream);
>
> + if (!tables)
> + return 0;
> +
> if (len < 0)
> return len;
> /* Table B.5 - Huffman table specification parameter sizes and values */
>

2020-12-02 15:25:59

by Philipp Zabel

[permalink] [raw]
Subject: Re: [PATCH v5 09/10] media: Avoid parsing quantization and huffman tables

Hi Mirela,

On Thu, 2020-11-12 at 05:05 +0200, Mirela Rabulea (OSS) wrote:
> From: Mirela Rabulea <[email protected]>
>
> These are optional in struct v4l2_jpeg_header, so do not parse if
> not requested, save some time.
>
> Signed-off-by: Mirela Rabulea <[email protected]>
> ---
> drivers/media/v4l2-core/v4l2-jpeg.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c b/drivers/media/v4l2-core/v4l2-jpeg.c
> index d77e04083d57..7576cd0ce6b9 100644
> --- a/drivers/media/v4l2-core/v4l2-jpeg.c
> +++ b/drivers/media/v4l2-core/v4l2-jpeg.c
> @@ -307,6 +307,9 @@ static int jpeg_parse_quantization_tables(struct jpeg_stream *stream,
> {
> int len = jpeg_get_word_be(stream);
>
> + if (!tables)
> + return 0;
> +
> if (len < 0)
> return len;
> /* Lq = 2 + n * 65 (for baseline DCT), n >= 1 */
> @@ -361,6 +364,9 @@ static int jpeg_parse_huffman_tables(struct jpeg_stream *stream,
> int mt;
> int len = jpeg_get_word_be(stream);
>
> + if (!tables)
> + return 0;
> +
> if (len < 0)
> return len;
> /* Table B.5 - Huffman table specification parameter sizes and values */

I don't understand this. jpeg_parse_quantization_tables() is only called
if v4l2_jpeg_parse_header() finds a DQT marker. The entire quantization
table-specification parameter block is optional, but when a DQT marker
is present, IIUC the block must contain at least one table (see B.2.4.1,
specifically figure B.6).

regards
Philipp

2020-12-02 15:36:05

by Philipp Zabel

[permalink] [raw]
Subject: Re: [PATCH v5 09/10] media: Avoid parsing quantization and huffman tables

On Wed, 2020-12-02 at 13:12 +0100, Hans Verkuil wrote:
> On 12/11/2020 04:05, Mirela Rabulea (OSS) wrote:
> > From: Mirela Rabulea <[email protected]>
> >
> > These are optional in struct v4l2_jpeg_header, so do not parse if
> > not requested, save some time.
> >
> > Signed-off-by: Mirela Rabulea <[email protected]>
> > ---
> > drivers/media/v4l2-core/v4l2-jpeg.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c b/drivers/media/v4l2-core/v4l2-jpeg.c
> > index d77e04083d57..7576cd0ce6b9 100644
> > --- a/drivers/media/v4l2-core/v4l2-jpeg.c
> > +++ b/drivers/media/v4l2-core/v4l2-jpeg.c
> > @@ -307,6 +307,9 @@ static int jpeg_parse_quantization_tables(struct jpeg_stream *stream,
> > {
> > int len = jpeg_get_word_be(stream);
> >
> > + if (!tables)
> > + return 0;
> > +
>
> It feels more natural to check for a non-NULL out->quantization_tables
> or non-NULL out->huffman_tables pointer in v4l2_jpeg_parse_header()
> rather than in these low-level functions. It's weird to have this check here.

Ah, now I get it.

Yes, if you want to skip the entire DQT for performance reasons, it is
probably better to just call jpeg_skip_segment() instead of
jpeg_parse_quantization_table(). Otherwise the next jpeg_next_marker has
to scan the whole quantization table for segment markers.

regards
Philipp