| Message ID | 20201112030557.8540-10-mirela.rabulea@oss.nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add V4L2 driver for i.MX8 JPEG Encoder/Decoder | expand |
On 12/11/2020 04:05, Mirela Rabulea (OSS) wrote: > From: Mirela Rabulea <mirela.rabulea@nxp.com> > > These are optional in struct v4l2_jpeg_header, so do not parse if > not requested, save some time. > > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > --- > 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 */ >
Hi Mirela, On Thu, 2020-11-12 at 05:05 +0200, Mirela Rabulea (OSS) wrote: > From: Mirela Rabulea <mirela.rabulea@nxp.com> > > These are optional in struct v4l2_jpeg_header, so do not parse if > not requested, save some time. > > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > --- > 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
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 <mirela.rabulea@nxp.com> > > > > These are optional in struct v4l2_jpeg_header, so do not parse if > > not requested, save some time. > > > > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > > --- > > 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
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 */