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@xxxxxxx> > > 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@xxxxxxx> > --- > 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@xxxxxxxxxxxxxx> regards Philipp