| Message ID | 20201112030557.8540-8-mirela.rabulea@oss.nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add V4L2 driver for i.MX8 JPEG Encoder/Decoder | expand |
Hi Philipp, any thoughts on the jpeg helpers related patches from this patch set (7,8,9,10)? Thanks, Mirela On Thu, 2020-11-12 at 05:05 +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 v5: > This was patch 8 in previous version > Replaced a struct for app14 data with just an int, since the > transform flag is the only meaningfull information from this segment > > drivers/media/v4l2-core/v4l2-jpeg.c | 39 > +++++++++++++++++++++++++++-- > include/media/v4l2-jpeg.h | 6 +++-- > 2 files changed, 41 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c > b/drivers/media/v4l2-core/v4l2-jpeg.c > index 8947fd95c6f1..3181ce544f79 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,37 @@ 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; > + > + /* 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 +506,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 +552,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..008e0476d928 100644 > --- a/include/media/v4l2-jpeg.h > +++ b/include/media/v4l2-jpeg.h > @@ -100,10 +100,11 @@ 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 > - * or point at valid memory. > + * quantization_tables and huffman_tables pointers must be > initialized > + * to NULL or point at valid memory. > */ > struct v4l2_jpeg_header { > struct v4l2_jpeg_reference sof; > @@ -119,6 +120,7 @@ struct v4l2_jpeg_header { > struct v4l2_jpeg_reference *huffman_tables; > u16 restart_interval; > size_t ecs_offset; > + int app14_tf; > }; > > int v4l2_jpeg_parse_header(void *buf, size_t len, struct > v4l2_jpeg_header *out);
Hi Mirela, On Thu, 2020-11-12 at 05:05 +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 v5: > This was patch 8 in previous version > Replaced a struct for app14 data with just an int, since the > transform flag is the only meaningfull information from this segment Could we turn this into an enum for the transform flag, and include the above spec reference in its kerneldoc comment? I think this would be better than checking for (app14_tf == <magic_number>) in the drivers. > drivers/media/v4l2-core/v4l2-jpeg.c | 39 +++++++++++++++++++++++++++-- > include/media/v4l2-jpeg.h | 6 +++-- > 2 files changed, 41 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c b/drivers/media/v4l2-core/v4l2-jpeg.c > index 8947fd95c6f1..3181ce544f79 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,37 @@ 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; Let's keep variables lower case. > + int skip; > + int tf; > + > + Lp = jpeg_get_word_be(stream); > + if (Lp < 0) > + return Lp; Should we check that Ap1 to 6 are actually "Adobe\0"? > + /* 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 +506,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 +552,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..008e0476d928 100644 > --- a/include/media/v4l2-jpeg.h > +++ b/include/media/v4l2-jpeg.h > @@ -100,10 +100,11 @@ 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 > - * or point at valid memory. > + * quantization_tables and huffman_tables pointers must be initialized > + * to NULL or point at valid memory. Unnecessary, unrelated change? I'd drop this. > */ > struct v4l2_jpeg_header { > struct v4l2_jpeg_reference sof; > @@ -119,6 +120,7 @@ struct v4l2_jpeg_header { > struct v4l2_jpeg_reference *huffman_tables; > u16 restart_interval; > size_t ecs_offset; > + int app14_tf; > }; > > int v4l2_jpeg_parse_header(void *buf, size_t len, struct v4l2_jpeg_header *out); regards Philipp
Hi Phipipp, On Wed, 2020-12-02 at 16:18 +0100, Philipp Zabel wrote: > > Hi Mirela, > > On Thu, 2020-11-12 at 05:05 +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 v5: > > This was patch 8 in previous version > > Replaced a struct for app14 data with just an int, since the > > transform flag is the only meaningfull information from this > > segment > > Could we turn this into an enum for the transform flag, and include > the > above spec reference in its kerneldoc comment? I think this would be > better than checking for (app14_tf == <magic_number>) in the drivers. Appreciate your feedback, for all patches, I'll address it in v6. Where would be a better place for this enum, v4l2-jpeg.h, or maybe include/uapi/linux/v4l2-controls.h? Thanks, Mirela > > > drivers/media/v4l2-core/v4l2-jpeg.c | 39 > > +++++++++++++++++++++++++++-- > > include/media/v4l2-jpeg.h | 6 +++-- > > 2 files changed, 41 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c > > b/drivers/media/v4l2-core/v4l2-jpeg.c > > index 8947fd95c6f1..3181ce544f79 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,37 @@ 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; > > Let's keep variables lower case. > > > + int skip; > > + int tf; > > + > > + Lp = jpeg_get_word_be(stream); > > + if (Lp < 0) > > + return Lp; > > Should we check that Ap1 to 6 are actually "Adobe\0"? > > > + /* 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 +506,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 +552,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..008e0476d928 100644 > > --- a/include/media/v4l2-jpeg.h > > +++ b/include/media/v4l2-jpeg.h > > @@ -100,10 +100,11 @@ 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 > > - * or point at valid memory. > > + * quantization_tables and huffman_tables pointers must be > > initialized > > + * to NULL or point at valid memory. > > Unnecessary, unrelated change? I'd drop this. > > > */ > > struct v4l2_jpeg_header { > > struct v4l2_jpeg_reference sof; > > @@ -119,6 +120,7 @@ struct v4l2_jpeg_header { > > struct v4l2_jpeg_reference *huffman_tables; > > u16 restart_interval; > > size_t ecs_offset; > > + int app14_tf; > > }; > > > > int v4l2_jpeg_parse_header(void *buf, size_t len, struct > > v4l2_jpeg_header *out); > > regards > Philipp
On Fri, 2020-12-04 at 14:13 +0000, Mirela Rabulea (OSS) wrote: > Hi Phipipp, > > On Wed, 2020-12-02 at 16:18 +0100, Philipp Zabel wrote: > > Hi Mirela, > > > > On Thu, 2020-11-12 at 05:05 +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 v5: > > > This was patch 8 in previous version > > > Replaced a struct for app14 data with just an int, since the > > > transform flag is the only meaningfull information from this > > > segment > > > > Could we turn this into an enum for the transform flag, and include > > the > > above spec reference in its kerneldoc comment? I think this would be > > better than checking for (app14_tf == <magic_number>) in the drivers. > > Appreciate your feedback, for all patches, I'll address it in v6. > Where would be a better place for this enum, v4l2-jpeg.h, or maybe > include/uapi/linux/v4l2-controls.h? v4l2-jpeg.h seems like the right place to me. regards Philipp
diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c b/drivers/media/v4l2-core/v4l2-jpeg.c index 8947fd95c6f1..3181ce544f79 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,37 @@ 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; + + /* 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 +506,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 +552,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..008e0476d928 100644 --- a/include/media/v4l2-jpeg.h +++ b/include/media/v4l2-jpeg.h @@ -100,10 +100,11 @@ 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 - * or point at valid memory. + * quantization_tables and huffman_tables pointers must be initialized + * to NULL or point at valid memory. */ struct v4l2_jpeg_header { struct v4l2_jpeg_reference sof; @@ -119,6 +120,7 @@ struct v4l2_jpeg_header { struct v4l2_jpeg_reference *huffman_tables; u16 restart_interval; size_t ecs_offset; + int app14_tf; }; int v4l2_jpeg_parse_header(void *buf, size_t len, struct v4l2_jpeg_header *out);