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

[Ramiro Polla <ramiro.polla@gmail.com>]

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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".