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.