Re: [PATCH v5 07/10] media: Add parsing for APP14 data segment in jpeg helpers

[Philipp Zabel <p.zabel@pengutronix.de>]

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