[FFmpeg-devel] [RFC PATCH] libavcodec/jpeg2000: Make corrections jpeg2000 decoder
Gautam Ramakrishnan
gautamramk at gmail.com
Fri Jun 19 05:40:05 EEST 2020
On Fri, Jun 19, 2020 at 1:34 AM Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
>
> 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?
>
Yes, it will have to split. However, I just wanted to get a few
comments first regarding the
changes. Especially with respect to the colour format.
> > ---
> > 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?
Firstly, even if log2_prec_width or log2_prec_height are 0, we will
not face any division by 0
issues as it is an exponent. 2^0 would give us 1. This check prevents
decoding of p1_07.j2k
and I believe it is wrong. This condition is handled in the COD marker
itself and permits the
first component to have these values as 0. This condition could be
changed to check if the
values are positive.
>
> >
> > /* 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".
Yes, I shall split the patches. However, purely in terms of adding
support for image offsets, this is the only change required.
All other changes look like other errors in the code.
>
> > 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?
In this case, the image p1_07.j2k has different sample separations for the
two different layers. If it's fair to assume that sample separation
and subsampling
are the same, the rest of the code accounts for the difference in 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?
This check was for the sample p1_01.j2k. It has only 1 layer. However,
The sample separation x = 2 and sample separation y = 1. I feel this could
be the reason it is not getting recognized. I guess the pix_fmt_match()
needs some modification, but I do not fully understand pixel formats.
Any reference I could use would be really helpful.
>
> I cannot comment on the other changes.
I do not intend this patch to get merged, I wanted to review the
changes first. Hopefully I could get some help with the colour
formats.
>
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
--
-------------
Gautam |
More information about the ffmpeg-devel
mailing list