diff mbox series

[FFmpeg-devel] libavcodec/mjpeg: keep last_dc value unclipped

Message ID 20240607192654.146995-1-ramiro.polla@gmail.com
State New
Headers show
Series [FFmpeg-devel] libavcodec/mjpeg: keep last_dc value unclipped | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Ramiro Polla June 7, 2024, 7:26 p.m. UTC
Do av_clip_int16(val) _after_ copying the value to last_dc.

Related commits: c28f648b19d and dffae122d0f
Related ticket: 4683
---
 libavcodec/mjpegdec.c    | 3 +--
 tests/ref/fate/jpg-12bpp | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

Comments

Andreas Rheinhardt June 7, 2024, 7:34 p.m. UTC | #1
Ramiro Polla:
> Do av_clip_int16(val) _after_ copying the value to last_dc.
> 
> Related commits: c28f648b19d and dffae122d0f
> Related ticket: 4683
> ---
>  libavcodec/mjpegdec.c    | 3 +--
>  tests/ref/fate/jpg-12bpp | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> index 1481a7f285..7daec649bc 100644
> --- a/libavcodec/mjpegdec.c
> +++ b/libavcodec/mjpegdec.c
> @@ -843,9 +843,8 @@ static int decode_block(MJpegDecodeContext *s, int16_t *block, int component,
>          return AVERROR_INVALIDDATA;
>      }
>      val = val * (unsigned)quant_matrix[0] + s->last_dc[component];
> -    val = av_clip_int16(val);
>      s->last_dc[component] = val;
> -    block[0] = val;
> +    block[0] = av_clip_int16(val);
>      /* AC coefs */
>      i = 0;
>      {OPEN_READER(re, &s->gb);
> diff --git a/tests/ref/fate/jpg-12bpp b/tests/ref/fate/jpg-12bpp
> index b3c662d587..9b039a92c6 100644
> --- a/tests/ref/fate/jpg-12bpp
> +++ b/tests/ref/fate/jpg-12bpp
> @@ -3,4 +3,4 @@
>  #codec_id 0: rawvideo
>  #dimensions 0: 999x749
>  #sar 0: 1/1
> -0,          0,          0,        1,  1496502, 0xd91deb4b
> +0,          0,          0,        1,  1496502, 0x44efc0af

Is the change for the fate-sample supposed to be an improvement or what
is the rationale for this? (Is this change mandated by the spec?)

- Andreas
Ramiro Polla June 7, 2024, 8:10 p.m. UTC | #2
On Fri, Jun 7, 2024 at 9:35 PM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
> Ramiro Polla:
> > Do av_clip_int16(val) _after_ copying the value to last_dc.
> >
> > Related commits: c28f648b19d and dffae122d0f
> > Related ticket: 4683
> > ---
> >  libavcodec/mjpegdec.c    | 3 +--
> >  tests/ref/fate/jpg-12bpp | 2 +-
> >  2 files changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> > index 1481a7f285..7daec649bc 100644
> > --- a/libavcodec/mjpegdec.c
> > +++ b/libavcodec/mjpegdec.c
> > @@ -843,9 +843,8 @@ static int decode_block(MJpegDecodeContext *s, int16_t *block, int component,
> >          return AVERROR_INVALIDDATA;
> >      }
> >      val = val * (unsigned)quant_matrix[0] + s->last_dc[component];
> > -    val = av_clip_int16(val);
> >      s->last_dc[component] = val;
> > -    block[0] = val;
> > +    block[0] = av_clip_int16(val);
> >      /* AC coefs */
> >      i = 0;
> >      {OPEN_READER(re, &s->gb);
> > diff --git a/tests/ref/fate/jpg-12bpp b/tests/ref/fate/jpg-12bpp
> > index b3c662d587..9b039a92c6 100644
> > --- a/tests/ref/fate/jpg-12bpp
> > +++ b/tests/ref/fate/jpg-12bpp
> > @@ -3,4 +3,4 @@
> >  #codec_id 0: rawvideo
> >  #dimensions 0: 999x749
> >  #sar 0: 1/1
> > -0,          0,          0,        1,  1496502, 0xd91deb4b
> > +0,          0,          0,        1,  1496502, 0x44efc0af
>
> Is the change for the fate-sample supposed to be an improvement or what
> is the rationale for this? (Is this change mandated by the spec?)

As far as I can tell the only sample we have that triggers this is
buggy anyways, so it's not something spec-related. It seems more
correct to me to clip the values that overflow only for the block, and
not propagate the differences from the clipping to the next dc values.

This change comes from another project where I decouple the bitstream
reading from the processing. Currently we have this comment in
MJpegDecodeContext:
int last_dc[MAX_COMPONENTS]; /* last DEQUANTIZED dc (XXX: am I right
to do that ?) */

What I do is keep the last quantized dc values as they were read from
the bitstream, but then I have to add the dc shift for every block.
Since it incurs one extra add per block, I'm not submitting the entire
patch, but only this chunk.
Ramiro Polla June 27, 2024, 2:22 p.m. UTC | #3
On Fri, Jun 7, 2024 at 10:10 PM Ramiro Polla <ramiro.polla@gmail.com> wrote:
> On Fri, Jun 7, 2024 at 9:35 PM Andreas Rheinhardt
> <andreas.rheinhardt@outlook.com> wrote:
> > Ramiro Polla:
> > > Do av_clip_int16(val) _after_ copying the value to last_dc.
> > >
> > > Related commits: c28f648b19d and dffae122d0f
> > > Related ticket: 4683
> > > ---
> > >  libavcodec/mjpegdec.c    | 3 +--
> > >  tests/ref/fate/jpg-12bpp | 2 +-
> > >  2 files changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> > > index 1481a7f285..7daec649bc 100644
> > > --- a/libavcodec/mjpegdec.c
> > > +++ b/libavcodec/mjpegdec.c
> > > @@ -843,9 +843,8 @@ static int decode_block(MJpegDecodeContext *s, int16_t *block, int component,
> > >          return AVERROR_INVALIDDATA;
> > >      }
> > >      val = val * (unsigned)quant_matrix[0] + s->last_dc[component];
> > > -    val = av_clip_int16(val);
> > >      s->last_dc[component] = val;
> > > -    block[0] = val;
> > > +    block[0] = av_clip_int16(val);
> > >      /* AC coefs */
> > >      i = 0;
> > >      {OPEN_READER(re, &s->gb);
> > > diff --git a/tests/ref/fate/jpg-12bpp b/tests/ref/fate/jpg-12bpp
> > > index b3c662d587..9b039a92c6 100644
> > > --- a/tests/ref/fate/jpg-12bpp
> > > +++ b/tests/ref/fate/jpg-12bpp
> > > @@ -3,4 +3,4 @@
> > >  #codec_id 0: rawvideo
> > >  #dimensions 0: 999x749
> > >  #sar 0: 1/1
> > > -0,          0,          0,        1,  1496502, 0xd91deb4b
> > > +0,          0,          0,        1,  1496502, 0x44efc0af
> >
> > Is the change for the fate-sample supposed to be an improvement or what
> > is the rationale for this? (Is this change mandated by the spec?)
>
> As far as I can tell the only sample we have that triggers this is
> buggy anyways, so it's not something spec-related. It seems more
> correct to me to clip the values that overflow only for the block, and
> not propagate the differences from the clipping to the next dc values.
>
> This change comes from another project where I decouple the bitstream
> reading from the processing. Currently we have this comment in
> MJpegDecodeContext:
> int last_dc[MAX_COMPONENTS]; /* last DEQUANTIZED dc (XXX: am I right
> to do that ?) */
>
> What I do is keep the last quantized dc values as they were read from
> the bitstream, but then I have to add the dc shift for every block.
> Since it incurs one extra add per block, I'm not submitting the entire
> patch, but only this chunk.

Updated patch attached with (hopefully) more descriptive commit message.

If there are no objections, I'll apply this tomorrow.
diff mbox series

Patch

diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
index 1481a7f285..7daec649bc 100644
--- a/libavcodec/mjpegdec.c
+++ b/libavcodec/mjpegdec.c
@@ -843,9 +843,8 @@  static int decode_block(MJpegDecodeContext *s, int16_t *block, int component,
         return AVERROR_INVALIDDATA;
     }
     val = val * (unsigned)quant_matrix[0] + s->last_dc[component];
-    val = av_clip_int16(val);
     s->last_dc[component] = val;
-    block[0] = val;
+    block[0] = av_clip_int16(val);
     /* AC coefs */
     i = 0;
     {OPEN_READER(re, &s->gb);
diff --git a/tests/ref/fate/jpg-12bpp b/tests/ref/fate/jpg-12bpp
index b3c662d587..9b039a92c6 100644
--- a/tests/ref/fate/jpg-12bpp
+++ b/tests/ref/fate/jpg-12bpp
@@ -3,4 +3,4 @@ 
 #codec_id 0: rawvideo
 #dimensions 0: 999x749
 #sar 0: 1/1
-0,          0,          0,        1,  1496502, 0xd91deb4b
+0,          0,          0,        1,  1496502, 0x44efc0af