diff mbox series

[FFmpeg-devel,1/5] libavcodec/jpeg2000.c: Precinct size check removed

Message ID 20200621184208.27665-1-gautamramk@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel,1/5] libavcodec/jpeg2000.c: Precinct size check removed
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Gautam Ramakrishnan June 21, 2020, 6:42 p.m. UTC
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(-)

Comments

Gautam Ramakrishnan June 21, 2020, 6:44 p.m. UTC | #1
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.
Michael Niedermayer June 22, 2020, 9 p.m. UTC | #2
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

[...]
Michael Niedermayer June 27, 2020, 9:43 a.m. UTC | #3
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

[...]
Gautam Ramakrishnan June 27, 2020, 11:19 a.m. UTC | #4
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.
Michael Niedermayer June 27, 2020, 2:06 p.m. UTC | #5
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


[...]
Gautam Ramakrishnan June 27, 2020, 3:44 p.m. UTC | #6
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?
Michael Niedermayer June 28, 2020, 6:56 p.m. UTC | #7
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

[...]
Gautam Ramakrishnan June 29, 2020, 4:14 p.m. UTC | #8
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 mbox series

Patch

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)