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

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

[-- Attachment #1: Type: text/plain, Size: 2669 bytes --]

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.

[-- Attachment #2: 0001-libavcodec-mjpeg-preserve-unclipped-last_dc-value.patch --]
[-- Type: text/x-patch, Size: 1556 bytes --]

From 30bbcaf859bb90f0213d0b14284b08afe6933a75 Mon Sep 17 00:00:00 2001
From: Ramiro Polla <ramiro.polla@gmail.com>
Date: Fri, 7 Jun 2024 21:26:54 +0200
Subject: [PATCH] libavcodec/mjpeg: preserve unclipped last_dc value

Perform av_clip_int16(val) _after_ copying the value to last_dc.

This change ensures that clipping is applied only within the context of
the current block, preventing the propagation of clipped values to
subsequent DC components.

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
-- 
2.30.2


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".