* [FFmpeg-devel] [PATCH] libavcodec/mjpeg: keep last_dc value unclipped @ 2024-06-07 19:26 Ramiro Polla 2024-06-07 19:34 ` Andreas Rheinhardt 0 siblings, 1 reply; 4+ messages in thread From: Ramiro Polla @ 2024-06-07 19:26 UTC (permalink / raw) To: ffmpeg-devel 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". ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [FFmpeg-devel] [PATCH] libavcodec/mjpeg: keep last_dc value unclipped 2024-06-07 19:26 [FFmpeg-devel] [PATCH] libavcodec/mjpeg: keep last_dc value unclipped Ramiro Polla @ 2024-06-07 19:34 ` Andreas Rheinhardt 2024-06-07 20:10 ` Ramiro Polla 0 siblings, 1 reply; 4+ messages in thread From: Andreas Rheinhardt @ 2024-06-07 19:34 UTC (permalink / raw) To: ffmpeg-devel 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". ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [FFmpeg-devel] [PATCH] libavcodec/mjpeg: keep last_dc value unclipped 2024-06-07 19:34 ` Andreas Rheinhardt @ 2024-06-07 20:10 ` Ramiro Polla 2024-06-27 14:22 ` Ramiro Polla 0 siblings, 1 reply; 4+ messages in thread From: Ramiro Polla @ 2024-06-07 20:10 UTC (permalink / raw) To: FFmpeg development discussions and patches 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". ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [FFmpeg-devel] [PATCH] libavcodec/mjpeg: keep last_dc value unclipped 2024-06-07 20:10 ` Ramiro Polla @ 2024-06-27 14:22 ` Ramiro Polla 0 siblings, 0 replies; 4+ messages in thread From: Ramiro Polla @ 2024-06-27 14:22 UTC (permalink / raw) To: FFmpeg development discussions and patches [-- 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". ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-06-27 14:23 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-06-07 19:26 [FFmpeg-devel] [PATCH] libavcodec/mjpeg: keep last_dc value unclipped Ramiro Polla 2024-06-07 19:34 ` Andreas Rheinhardt 2024-06-07 20:10 ` Ramiro Polla 2024-06-27 14:22 ` Ramiro Polla
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel This inbox may be cloned and mirrored by anyone: git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \ ffmpegdev@gitmailbox.com public-inbox-index ffmpegdev Example config snippet for mirrors. AGPL code for this site: git clone https://public-inbox.org/public-inbox.git