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

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

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

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

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

[Andreas Rheinhardt]

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

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

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

[Ramiro Polla]

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

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

[Ramiro Polla]

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