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

["Mirela Rabulea (OSS)" <mirela.rabulea@oss.nxp.com>]

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