| Message ID | 20240607192654.146995-1-ramiro.polla@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] libavcodec/mjpeg: keep last_dc value unclipped | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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
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.
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 --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