| Message ID | 20201215111843.30269-7-mirela.rabulea@oss.nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add V4L2 driver for i.MX8 JPEG Encoder/Decoder | expand |
Hi Mirela, thank you for the update. Just two issues below: On Tue, 2020-12-15 at 13:18 +0200, Mirela Rabulea (OSS) wrote: > From: Mirela Rabulea <mirela.rabulea@nxp.com> > > According to Rec. ITU-T T.872 (06/2012) 6.5.3 > APP14 segment is for color encoding, it contains a transform flag, which > may have values of 0, 1 and 2 and are interpreted as follows: > 0 - CMYK for images that are encoded with four components > - RGB for images that are encoded with three components > 1 - An image encoded with three components using YCbCr colour encoding. > 2 - An image encoded with four components using YCCK colour encoding. > > This is used in imx-jpeg decoder, to distinguish between > YUV444 and RGB24. > > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > --- > Changes in v6: > Switch variable to lowercase Lp->lp > Check for "Adobe\0" in Ap1..6 > Make the transform flag an enum > Removed a change in comment section, a leftover from a previous version > Thanks Philipp for feedback. > > drivers/media/v4l2-core/v4l2-jpeg.c | 43 +++++++++++++++++++++++++++-- > include/media/v4l2-jpeg.h | 18 ++++++++++++ > 2 files changed, 59 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c b/drivers/media/v4l2-core/v4l2-jpeg.c > index 8947fd95c6f1..d1483e7a775c 100644 > --- a/drivers/media/v4l2-core/v4l2-jpeg.c > +++ b/drivers/media/v4l2-core/v4l2-jpeg.c > @@ -45,6 +45,7 @@ MODULE_LICENSE("GPL"); > #define DHP 0xffde /* hierarchical progression */ > #define EXP 0xffdf /* expand reference */ > #define APP0 0xffe0 /* application data */ > +#define APP14 0xffee /* application data for colour encoding */ > #define APP15 0xffef > #define JPG0 0xfff0 /* extensions */ > #define JPG13 0xfffd > @@ -444,8 +445,41 @@ static int jpeg_skip_segment(struct jpeg_stream *stream) > return jpeg_skip(stream, len - 2); > } > > +/* Rec. ITU-T T.872 (06/2012) 6.5.3 */ > +static int jpeg_parse_app14_data(struct jpeg_stream *stream) > +{ > + int ret; > + int lp; > + int skip; > + int tf; > + > + lp = jpeg_get_word_be(stream); > + if (lp < 0) > + return lp; Here we should check that there are still 6 bytes available to compare: if (stream->curr + 6 > stream->end) return -EINVAL; > + /* Check for "Adobe\0" in Ap1..6 */ > + if (strncmp(stream->curr, "Adobe\0", 6)) > + return -EINVAL; > + > + /* get to Ap12 */ > + ret = jpeg_skip(stream, 11); > + if (ret < 0) > + return -EINVAL; > + > + tf = jpeg_get_byte(stream); > + if (tf < 0) > + return tf; > + > + skip = lp - 2 - 11; > + ret = jpeg_skip(stream, skip); > + if (ret < 0) > + return -EINVAL; > + else > + return tf; > +} > + > /** > - * jpeg_parse_header - locate marker segments and optionally parse headers > + * v4l2_jpeg_parse_header - locate marker segments and optionally parse headers > * @buf: address of the JPEG buffer, should start with a SOI marker > * @len: length of the JPEG buffer > * @out: returns marker segment positions and optionally parsed headers > @@ -476,6 +510,9 @@ int v4l2_jpeg_parse_header(void *buf, size_t len, struct v4l2_jpeg_header *out) > if (marker != SOI) > return -EINVAL; > > + /* init value to signal if this marker is not present */ > + out->app14_tf = -EINVAL; > + Here we set app14_tf to a value that is not part of the enum. You could define a value V4L2_JPEG_APP14_TF_UNKNOWN for the uninitialized / error state. > /* loop through marker segments */ > while ((marker = jpeg_next_marker(&stream)) >= 0) { > switch (marker) { > @@ -519,7 +556,9 @@ int v4l2_jpeg_parse_header(void *buf, size_t len, struct v4l2_jpeg_header *out) > ret = jpeg_parse_restart_interval(&stream, > &out->restart_interval); > break; > - > + case APP14: > + out->app14_tf = jpeg_parse_app14_data(&stream); Same as above in case of -EINVAL return. Apart from this, Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> regards Philipp
diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c b/drivers/media/v4l2-core/v4l2-jpeg.c index 8947fd95c6f1..d1483e7a775c 100644 --- a/drivers/media/v4l2-core/v4l2-jpeg.c +++ b/drivers/media/v4l2-core/v4l2-jpeg.c @@ -45,6 +45,7 @@ MODULE_LICENSE("GPL"); #define DHP 0xffde /* hierarchical progression */ #define EXP 0xffdf /* expand reference */ #define APP0 0xffe0 /* application data */ +#define APP14 0xffee /* application data for colour encoding */ #define APP15 0xffef #define JPG0 0xfff0 /* extensions */ #define JPG13 0xfffd @@ -444,8 +445,41 @@ static int jpeg_skip_segment(struct jpeg_stream *stream) return jpeg_skip(stream, len - 2); } +/* Rec. ITU-T T.872 (06/2012) 6.5.3 */ +static int jpeg_parse_app14_data(struct jpeg_stream *stream) +{ + int ret; + int lp; + int skip; + int tf; + + lp = jpeg_get_word_be(stream); + if (lp < 0) + return lp; + + /* Check for "Adobe\0" in Ap1..6 */ + if (strncmp(stream->curr, "Adobe\0", 6)) + return -EINVAL; + + /* get to Ap12 */ + ret = jpeg_skip(stream, 11); + if (ret < 0) + return -EINVAL; + + tf = jpeg_get_byte(stream); + if (tf < 0) + return tf; + + skip = lp - 2 - 11; + ret = jpeg_skip(stream, skip); + if (ret < 0) + return -EINVAL; + else + return tf; +} + /** - * jpeg_parse_header - locate marker segments and optionally parse headers + * v4l2_jpeg_parse_header - locate marker segments and optionally parse headers * @buf: address of the JPEG buffer, should start with a SOI marker * @len: length of the JPEG buffer * @out: returns marker segment positions and optionally parsed headers @@ -476,6 +510,9 @@ int v4l2_jpeg_parse_header(void *buf, size_t len, struct v4l2_jpeg_header *out) if (marker != SOI) return -EINVAL; + /* init value to signal if this marker is not present */ + out->app14_tf = -EINVAL; + /* loop through marker segments */ while ((marker = jpeg_next_marker(&stream)) >= 0) { switch (marker) { @@ -519,7 +556,9 @@ int v4l2_jpeg_parse_header(void *buf, size_t len, struct v4l2_jpeg_header *out) ret = jpeg_parse_restart_interval(&stream, &out->restart_interval); break; - + case APP14: + out->app14_tf = jpeg_parse_app14_data(&stream); + break; case SOS: ret = jpeg_reference_segment(&stream, &out->sos); if (ret < 0) diff --git a/include/media/v4l2-jpeg.h b/include/media/v4l2-jpeg.h index ddba2a56c321..c1904aa3f7cc 100644 --- a/include/media/v4l2-jpeg.h +++ b/include/media/v4l2-jpeg.h @@ -87,6 +87,22 @@ struct v4l2_jpeg_scan_header { /* Ss, Se, Ah, and Al are not used by any driver */ }; +/** + * enum v4l2_jpeg_app14_tf - APP14 transform flag + * According to Rec. ITU-T T.872 (06/2012) 6.5.3 + * APP14 segment is for color encoding, it contains a transform flag, + * which may have values of 0, 1 and 2 and are interpreted as follows: + * @V4L2_JPEG_APP14_TF_CMYK_RGB: CMYK for images encoded with four components + * RGB for images encoded with three components + * @V4L2_JPEG_APP14_TF_YCBCR: an image encoded with three components using YCbCr + * @V4L2_JPEG_APP14_TF_YCCK: an image encoded with four components using YCCK + */ +enum v4l2_jpeg_app14_tf { + V4L2_JPEG_APP14_TF_CMYK_RGB = 0, + V4L2_JPEG_APP14_TF_YCBCR = 1, + V4L2_JPEG_APP14_TF_YCCK = 2, +}; + /** * struct v4l2_jpeg_header - parsed JPEG header * @sof: pointer to frame header and size @@ -100,6 +116,7 @@ struct v4l2_jpeg_scan_header { * order, optional * @restart_interval: number of MCU per restart interval, Ri * @ecs_offset: buffer offset in bytes to the entropy coded segment + * @app14_tf: transform flag from app14 data * * When this structure is passed to v4l2_jpeg_parse_header, the optional scan, * quantization_tables, and huffman_tables pointers must be initialized to NULL @@ -119,6 +136,7 @@ struct v4l2_jpeg_header { struct v4l2_jpeg_reference *huffman_tables; u16 restart_interval; size_t ecs_offset; + enum v4l2_jpeg_app14_tf app14_tf; }; int v4l2_jpeg_parse_header(void *buf, size_t len, struct v4l2_jpeg_header *out);