[FFmpeg-devel] [RFC PATCH] libavcodec/jpeg2000: Make corrections jpeg2000 decoder

[Carl Eugen Hoyos]

Am Do., 18. Juni 2020 um 21:50 Uhr schrieb <gautamramk at gmail.com>:
>
> From: Gautam Ramakrishnan <gautamramk at gmail.com>
>
> This is with reference to my previous email on the mailing list
> with subject: "query on pixel formats".
> I wish to cleanup some errors in the decoder code. These changes
> would allow the samples p1_01.j2k and p1_07.j2k to be decoded.
> However, I am facing issues with pixel format selection and have
> currently forced the pixel formats to demonstrate the changes made.
> Would be grateful if anyone could suggest modifications to the pix
> format selection.

The patch looks to me as if it should be split but maybe I
misunderstand and the changes are strongly related?

> ---
>  libavcodec/jpeg2000.c    |  3 ---
>  libavcodec/jpeg2000dec.c | 53 ++++++++++++++++++++++++++--------------
>  2 files changed, 34 insertions(+), 22 deletions(-)
>
> diff --git a/libavcodec/jpeg2000.c b/libavcodec/jpeg2000.c
> index 73206d17f3..1aca31ffa4 100644
> --- a/libavcodec/jpeg2000.c
> +++ b/libavcodec/jpeg2000.c
> @@ -509,9 +509,6 @@ int ff_jpeg2000_init_component(Jpeg2000Component *comp,
>          // update precincts size: 2^n value
>          reslevel->log2_prec_width  = codsty->log2_prec_widths[reslevelno];
>          reslevel->log2_prec_height = codsty->log2_prec_heights[reslevelno];

> -        if (!reslevel->log2_prec_width || !reslevel->log2_prec_height) {
> -            return AVERROR_INVALIDDATA;
> -        }

Why exactly is this a good idea?

>
>          /* Number of bands for each resolution level */
>          if (reslevelno == 0)
> diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
> index ab36009a2d..ae63c68ca8 100644
> --- a/libavcodec/jpeg2000dec.c
> +++ b/libavcodec/jpeg2000dec.c
> @@ -269,6 +269,7 @@ static int get_siz(Jpeg2000DecoderContext *s)
>      const enum AVPixelFormat *possible_fmts = NULL;
>      int possible_fmts_nb = 0;
>      int ret;
> +    int dimx, dimy;
>
>      if (bytestream2_get_bytes_left(&s->g) < 36) {
>          av_log(s->avctx, AV_LOG_ERROR, "Insufficient space for SIZ\n");
> @@ -286,10 +287,6 @@ static int get_siz(Jpeg2000DecoderContext *s)
>      s->tile_offset_y  = bytestream2_get_be32u(&s->g); // YT0Siz
>      ncomponents       = bytestream2_get_be16u(&s->g); // CSiz

> -    if (s->image_offset_x || s->image_offset_y) {
> -        avpriv_request_sample(s->avctx, "Support for image offsets");
> -        return AVERROR_PATCHWELCOME;
> -    }

I would have expected this change to be part of a patch "support
image offsets".

>      if (av_image_check_size2(s->width, s->height, s->avctx->max_pixels, AV_PIX_FMT_NONE, 0, s->avctx)) {
>          avpriv_request_sample(s->avctx, "Large Dimensions");
>          return AVERROR_PATCHWELCOME;
> @@ -371,11 +368,18 @@ static int get_siz(Jpeg2000DecoderContext *s)
>      }
>
>      /* compute image size with reduction factor */
> -    ret = ff_set_dimensions(s->avctx,
> -            ff_jpeg2000_ceildivpow2(s->width  - s->image_offset_x,
> -                                               s->reduction_factor),
> -            ff_jpeg2000_ceildivpow2(s->height - s->image_offset_y,
> -                                               s->reduction_factor));
> +    dimx = ff_jpeg2000_ceildivpow2(s->width  - s->image_offset_x,
> +                                               s->reduction_factor);
> +    dimy = ff_jpeg2000_ceildivpow2(s->height - s->image_offset_y,
> +                                               s->reduction_factor);
> +    dimx = ff_jpeg2000_ceildiv(dimx, s->cdx[0]);
> +    dimy = ff_jpeg2000_ceildiv(dimy, s->cdy[0]);
> +    for (i = 1; i < s->ncomponents; i++) {
> +        dimx = FFMAX(dimx, ff_jpeg2000_ceildiv(dimx, s->cdx[i]));
> +        dimy = FFMAX(dimy, ff_jpeg2000_ceildiv(dimy, s->cdy[i]));
> +    }
> +
> +    ret = ff_set_dimensions(s->avctx, dimx, dimy);
>      if (ret < 0)
>          return ret;
>
> @@ -427,6 +431,13 @@ static int get_siz(Jpeg2000DecoderContext *s)
>                  s->cdef[3] = 3;
>                  i = 0;
>              }

> +        } else if (ncomponents == 2) {
> +            s->avctx->pix_fmt = AV_PIX_FMT_YA8;
> +            i = 0;

I am not convinced that this is a good idea:
Why is this not detected, what is the sample and most important:
What happens if the two components have different subsampling?

> +        } else if (ncomponents == 1) {
> +            s->avctx->pix_fmt = AV_PIX_FMT_GRAY8;
> +            i = 0;
> +            s->cdef[0] = 0;

This may be ok but why is it not detected in the existing code?

I cannot comment on the other changes.

Carl Eugen