| Message ID | 20200621184208.27665-1-gautamramk@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,1/5] libavcodec/jpeg2000.c: Precinct size check removed | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Mon, Jun 22, 2020 at 12:12 AM <gautamramk@gmail.com> wrote: > > From: Gautam Ramakrishnan <gautamramk@gmail.com> > > The log2_chroma_wh is derived from the sample separations of the > codestream if the file is a j2k codestream. Not sure if sample > separation is same is subsampling and whether using sample > separation values from the codestream to determine pixel format. > --- > libavcodec/jpeg2000dec.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c > index c8c89803ac..2b9659bf96 100644 > --- a/libavcodec/jpeg2000dec.c > +++ b/libavcodec/jpeg2000dec.c > @@ -225,8 +225,6 @@ static int pix_fmt_match(enum AVPixelFormat pix_fmt, int components, > > case 1: > match = match && desc->comp[0].depth >= bpc && > - (log2_chroma_wh >> 2 & 3) == 0 && > - (log2_chroma_wh & 3) == 0 && > (desc->flags & AV_PIX_FMT_FLAG_PAL) == pal8 * AV_PIX_FMT_FLAG_PAL; > } > return match; > -- > 2.17.1 > This patch has to be discussed. I do not see any other formats having a check like this, but that is probably because the pixel format is determined from the headers and not from the codestream.
On Mon, Jun 22, 2020 at 12:14:39AM +0530, Gautam Ramakrishnan wrote: > On Mon, Jun 22, 2020 at 12:12 AM <gautamramk@gmail.com> wrote: > > > > From: Gautam Ramakrishnan <gautamramk@gmail.com> > > > > The log2_chroma_wh is derived from the sample separations of the > > codestream if the file is a j2k codestream. Not sure if sample > > separation is same is subsampling and whether using sample > > separation values from the codestream to determine pixel format. > > --- > > libavcodec/jpeg2000dec.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c > > index c8c89803ac..2b9659bf96 100644 > > --- a/libavcodec/jpeg2000dec.c > > +++ b/libavcodec/jpeg2000dec.c > > @@ -225,8 +225,6 @@ static int pix_fmt_match(enum AVPixelFormat pix_fmt, int components, > > > > case 1: > > match = match && desc->comp[0].depth >= bpc && > > - (log2_chroma_wh >> 2 & 3) == 0 && > > - (log2_chroma_wh & 3) == 0 && > > (desc->flags & AV_PIX_FMT_FLAG_PAL) == pal8 * AV_PIX_FMT_FLAG_PAL; > > } > > return match; > > -- > > 2.17.1 > > > This patch has to be discussed. I do not see any other formats having > a check like this, see pix_fmt_id in mjpegdec.c > but that is probably because the pixel format is determined from the > headers and not > from the codestream. If the removed check was wrong then i suspect more changes are needed and just removing this alone will not be correct [...]
On Mon, Jun 22, 2020 at 12:12:04AM +0530, gautamramk@gmail.com wrote: > From: Gautam Ramakrishnan <gautamramk@gmail.com> > > This patch removes a check which throws an error if > the log2 precinct width/height is 0. The standard allows > the first component to have 0 as the log2 width/height. > --- > libavcodec/jpeg2000.c | 3 --- > 1 file changed, 3 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; > - } This checked that log2_prec_width... has been initialized. Is there some other check that ensures this is not just 0 from allocation which IIUC is not an allowed path in the spec thanks [...]
On Sat, Jun 27, 2020 at 3:13 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Mon, Jun 22, 2020 at 12:12:04AM +0530, gautamramk@gmail.com wrote: > > From: Gautam Ramakrishnan <gautamramk@gmail.com> > > > > This patch removes a check which throws an error if > > the log2 precinct width/height is 0. The standard allows > > the first component to have 0 as the log2 width/height. > > --- > > libavcodec/jpeg2000.c | 3 --- > > 1 file changed, 3 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; > > - } > > This checked that log2_prec_width... has been initialized. > Is there some other check that ensures this is not just 0 from allocation > which IIUC is not an allowed path in the spec A check happens only when the COD and COC marker gets read. The following lines in the get_cox() function in jpeg2000dec.c does it. if (i) if (c->log2_prec_widths[i] == 0 || c->log2_prec_heights[i] == 0) { av_log(s->avctx, AV_LOG_ERROR, "PPx %d PPy %d invalid\n", c->log2_prec_widths[i], c->log2_prec_heights[i]); This check ensures that for all resolution levels except reslevel 0 cannot have the widths and heights as 0. Do you feel this check is sufficient? Instead of completely removing the check as in the patch, we could change the condition to if ((!reslevel->log2_prec_width || !reslevel->log2_prec_height) && reslevel) { } This would at least ensure that the other reslevels are definitely set.
On Sat, Jun 27, 2020 at 04:49:49PM +0530, Gautam Ramakrishnan wrote: > On Sat, Jun 27, 2020 at 3:13 PM Michael Niedermayer > <michael@niedermayer.cc> wrote: > > > > On Mon, Jun 22, 2020 at 12:12:04AM +0530, gautamramk@gmail.com wrote: > > > From: Gautam Ramakrishnan <gautamramk@gmail.com> > > > > > > This patch removes a check which throws an error if > > > the log2 precinct width/height is 0. The standard allows > > > the first component to have 0 as the log2 width/height. > > > --- > > > libavcodec/jpeg2000.c | 3 --- > > > 1 file changed, 3 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; > > > - } > > > > This checked that log2_prec_width... has been initialized. > > Is there some other check that ensures this is not just 0 from allocation > > which IIUC is not an allowed path in the spec > > A check happens only when the COD and COC marker gets read. is there a check that protects against the absence of these markers ? thx [...]
On Sat, Jun 27, 2020 at 7:36 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Sat, Jun 27, 2020 at 04:49:49PM +0530, Gautam Ramakrishnan wrote: > > On Sat, Jun 27, 2020 at 3:13 PM Michael Niedermayer > > <michael@niedermayer.cc> wrote: > > > > > > On Mon, Jun 22, 2020 at 12:12:04AM +0530, gautamramk@gmail.com wrote: > > > > From: Gautam Ramakrishnan <gautamramk@gmail.com> > > > > > > > > This patch removes a check which throws an error if > > > > the log2 precinct width/height is 0. The standard allows > > > > the first component to have 0 as the log2 width/height. > > > > --- > > > > libavcodec/jpeg2000.c | 3 --- > > > > 1 file changed, 3 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; > > > > - } > > > > > > This checked that log2_prec_width... has been initialized. > > > Is there some other check that ensures this is not just 0 from allocation > > > which IIUC is not an allowed path in the spec > > > > A check happens only when the COD and COC marker gets read. > > is there a check that protects against the absence of these markers ? > No, there is no check for this. Now I realize that this is an important check which is missing. Same would apply to other compulsory markers like the SIZ marker. Shall I add a check that would ensure that these markers are present before the tile part data starts?
On Sat, Jun 27, 2020 at 09:14:26PM +0530, Gautam Ramakrishnan wrote: > On Sat, Jun 27, 2020 at 7:36 PM Michael Niedermayer > <michael@niedermayer.cc> wrote: > > > > On Sat, Jun 27, 2020 at 04:49:49PM +0530, Gautam Ramakrishnan wrote: > > > On Sat, Jun 27, 2020 at 3:13 PM Michael Niedermayer > > > <michael@niedermayer.cc> wrote: > > > > > > > > On Mon, Jun 22, 2020 at 12:12:04AM +0530, gautamramk@gmail.com wrote: > > > > > From: Gautam Ramakrishnan <gautamramk@gmail.com> > > > > > > > > > > This patch removes a check which throws an error if > > > > > the log2 precinct width/height is 0. The standard allows > > > > > the first component to have 0 as the log2 width/height. > > > > > --- > > > > > libavcodec/jpeg2000.c | 3 --- > > > > > 1 file changed, 3 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; > > > > > - } > > > > > > > > This checked that log2_prec_width... has been initialized. > > > > Is there some other check that ensures this is not just 0 from allocation > > > > which IIUC is not an allowed path in the spec > > > > > > A check happens only when the COD and COC marker gets read. > > > > is there a check that protects against the absence of these markers ? > > > No, there is no check for this. Now I realize that this is an > important check which is > missing. Same would apply to other compulsory markers like the SIZ marker. > Shall I add a check that would ensure that these markers are present before the > tile part data starts? yes thanks [...]
On Mon, Jun 29, 2020 at 12:26 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Sat, Jun 27, 2020 at 09:14:26PM +0530, Gautam Ramakrishnan wrote: > > On Sat, Jun 27, 2020 at 7:36 PM Michael Niedermayer > > <michael@niedermayer.cc> wrote: > > > > > > On Sat, Jun 27, 2020 at 04:49:49PM +0530, Gautam Ramakrishnan wrote: > > > > On Sat, Jun 27, 2020 at 3:13 PM Michael Niedermayer > > > > <michael@niedermayer.cc> wrote: > > > > > > > > > > On Mon, Jun 22, 2020 at 12:12:04AM +0530, gautamramk@gmail.com wrote: > > > > > > From: Gautam Ramakrishnan <gautamramk@gmail.com> > > > > > > > > > > > > This patch removes a check which throws an error if > > > > > > the log2 precinct width/height is 0. The standard allows > > > > > > the first component to have 0 as the log2 width/height. > > > > > > --- > > > > > > libavcodec/jpeg2000.c | 3 --- > > > > > > 1 file changed, 3 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; > > > > > > - } > > > > > > > > > > This checked that log2_prec_width... has been initialized. > > > > > Is there some other check that ensures this is not just 0 from allocation > > > > > which IIUC is not an allowed path in the spec > > > > > > > > A check happens only when the COD and COC marker gets read. > > > > > > is there a check that protects against the absence of these markers ? > > > > > No, there is no check for this. Now I realize that this is an > > important check which is > > missing. Same would apply to other compulsory markers like the SIZ marker. > > Shall I add a check that would ensure that these markers are present before the > > tile part data starts? > > yes Looks like the check for missing SIZ marker is already there. I shall submit a check to ensure the coding style values are initialized.
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; - } /* Number of bands for each resolution level */ if (reslevelno == 0)
From: Gautam Ramakrishnan <gautamramk@gmail.com> This patch removes a check which throws an error if the log2 precinct width/height is 0. The standard allows the first component to have 0 as the log2 width/height. --- libavcodec/jpeg2000.c | 3 --- 1 file changed, 3 deletions(-)