* [FFmpeg-devel] [PATCH] avcodec/vpx_rac: Adjust vpx_rac_is_end) threshold @ 2022-08-28 17:25 Michael Niedermayer 2022-08-29 15:29 ` Andreas Rheinhardt 0 siblings, 1 reply; 3+ messages in thread From: Michael Niedermayer @ 2022-08-28 17:25 UTC (permalink / raw) To: FFmpeg development discussions and patches A threshold of 180 is needed and sufficient for the sample, twice this is used to cover potentially worse samples fate/vp5 changes as the sample file is truncated and the damaged part is handled differently Fixes: ticket #9754 Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/vpx_rac.h | 2 +- tests/ref/fate/vp5 | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/libavcodec/vpx_rac.h b/libavcodec/vpx_rac.h index b158cc0754..2f5486f501 100644 --- a/libavcodec/vpx_rac.h +++ b/libavcodec/vpx_rac.h @@ -52,7 +52,7 @@ static av_always_inline int vpx_rac_is_end(VPXRangeCoder *c) { if (c->end <= c->buffer && c->bits >= 0) c->end_reached ++; - return c->end_reached > 10; + return c->end_reached > 360; } static av_always_inline unsigned int vpx_rac_renorm(VPXRangeCoder *c) diff --git a/tests/ref/fate/vp5 b/tests/ref/fate/vp5 index 09ebe62b25..2116fb9b81 100644 --- a/tests/ref/fate/vp5 +++ b/tests/ref/fate/vp5 @@ -249,4 +249,4 @@ 0, 243, 243, 1, 233472, 0x6f530ac6 0, 244, 244, 1, 233472, 0x94f7466c 0, 245, 245, 1, 233472, 0xa8c1d365 -0, 246, 246, 1, 233472, 0x4f3ef38c +0, 246, 246, 1, 233472, 0xedcff050 -- 2.17.1 _______________________________________________ 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] 3+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/vpx_rac: Adjust vpx_rac_is_end) threshold 2022-08-28 17:25 [FFmpeg-devel] [PATCH] avcodec/vpx_rac: Adjust vpx_rac_is_end) threshold Michael Niedermayer @ 2022-08-29 15:29 ` Andreas Rheinhardt 2022-08-29 22:48 ` Michael Niedermayer 0 siblings, 1 reply; 3+ messages in thread From: Andreas Rheinhardt @ 2022-08-29 15:29 UTC (permalink / raw) To: ffmpeg-devel Michael Niedermayer: > A threshold of 180 is needed and sufficient for the sample, twice this is used to > cover potentially worse samples > > fate/vp5 changes as the sample file is truncated and the damaged part is handled > differently > > Fixes: ticket #9754 > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/vpx_rac.h | 2 +- > tests/ref/fate/vp5 | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/vpx_rac.h b/libavcodec/vpx_rac.h > index b158cc0754..2f5486f501 100644 > --- a/libavcodec/vpx_rac.h > +++ b/libavcodec/vpx_rac.h > @@ -52,7 +52,7 @@ static av_always_inline int vpx_rac_is_end(VPXRangeCoder *c) > { > if (c->end <= c->buffer && c->bits >= 0) > c->end_reached ++; > - return c->end_reached > 10; > + return c->end_reached > 360; Is the file from #9754 defective? Or is it our decoder that is overcautious? Your commit message sounds as if it were the latter. Is it guaranteed that is now enough for all spec-compliant samples? Does the answer depend upon the codec? (vpx_rac_is_end() is shared between several VPx codecs.); I presume you can't give any guarantees. > } > > static av_always_inline unsigned int vpx_rac_renorm(VPXRangeCoder *c) > diff --git a/tests/ref/fate/vp5 b/tests/ref/fate/vp5 > index 09ebe62b25..2116fb9b81 100644 > --- a/tests/ref/fate/vp5 > +++ b/tests/ref/fate/vp5 > @@ -249,4 +249,4 @@ > 0, 243, 243, 1, 233472, 0x6f530ac6 > 0, 244, 244, 1, 233472, 0x94f7466c > 0, 245, 245, 1, 233472, 0xa8c1d365 > -0, 246, 246, 1, 233472, 0x4f3ef38c > +0, 246, 246, 1, 233472, 0xedcff050 _______________________________________________ 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] 3+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/vpx_rac: Adjust vpx_rac_is_end) threshold 2022-08-29 15:29 ` Andreas Rheinhardt @ 2022-08-29 22:48 ` Michael Niedermayer 0 siblings, 0 replies; 3+ messages in thread From: Michael Niedermayer @ 2022-08-29 22:48 UTC (permalink / raw) To: FFmpeg development discussions and patches [-- Attachment #1.1: Type: text/plain, Size: 4433 bytes --] On Mon, Aug 29, 2022 at 05:29:03PM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > A threshold of 180 is needed and sufficient for the sample, twice this is used to > > cover potentially worse samples > > > > fate/vp5 changes as the sample file is truncated and the damaged part is handled > > differently > > > > Fixes: ticket #9754 > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/vpx_rac.h | 2 +- > > tests/ref/fate/vp5 | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/libavcodec/vpx_rac.h b/libavcodec/vpx_rac.h > > index b158cc0754..2f5486f501 100644 > > --- a/libavcodec/vpx_rac.h > > +++ b/libavcodec/vpx_rac.h > > @@ -52,7 +52,7 @@ static av_always_inline int vpx_rac_is_end(VPXRangeCoder *c) > > { > > if (c->end <= c->buffer && c->bits >= 0) > > c->end_reached ++; > > - return c->end_reached > 10; > > + return c->end_reached > 360; > > Is the file from #9754 defective? Or is it our decoder that is > overcautious? > Your commit message sounds as if it were the latter. Is it > guaranteed that is now enough for all spec-compliant samples? Does the > answer depend upon the codec? (vpx_rac_is_end() is shared between > several VPx codecs.); I presume you can't give any guarantees. The file in question is VP8 looking at rfc6386, i see no obvious end of data check in the AC decoder in the RFC that would hint toward defective but the renorm code only tries reading once over the end. Our large threshold is the result of avoiding adding code in the ac reader itself It would require more investigation to be really sure though. Looking at this with the reference decoder and some instrumentation in it to see if it overreads too for example. But as the awnser to this wouldnt really affect the solution i think thats wasted time. Also heres a better looking fix: sadly i cannot replicate most of the timeouts with git master also i do not remember why i did not choose this simpler solution originally Author: Michael Niedermayer <michael@niedermayer.cc> Date: Mon Aug 29 23:43:41 2022 +0200 libavcodec/vpx_rac: Change end detection logic fate/vp5 changes as the sample file is truncated and the damaged part is handled differently Fixes: ticket #9754 This is a different and simpler solution to b6b9ac5698c8f911841b469af77199153278c55c Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> diff --git a/libavcodec/vpx_rac.c b/libavcodec/vpx_rac.c index cf02e9a19c9..bbfc829431c 100644 --- a/libavcodec/vpx_rac.c +++ b/libavcodec/vpx_rac.c @@ -45,7 +45,6 @@ int ff_vpx_init_range_decoder(VPXRangeCoder *c, const uint8_t *buf, int buf_size c->bits = -16; c->buffer = buf; c->end = buf + buf_size; - c->end_reached = 0; if (buf_size < 1) return AVERROR_INVALIDDATA; c->code_word = bytestream_get_be24(&c->buffer); diff --git a/libavcodec/vpx_rac.h b/libavcodec/vpx_rac.h index b158cc0754d..392e59904bd 100644 --- a/libavcodec/vpx_rac.h +++ b/libavcodec/vpx_rac.h @@ -39,7 +39,6 @@ typedef struct VPXRangeCoder { const uint8_t *buffer; const uint8_t *end; unsigned int code_word; - int end_reached; } VPXRangeCoder; extern const uint8_t ff_vpx_norm_shift[256]; @@ -50,9 +49,7 @@ int ff_vpx_init_range_decoder(VPXRangeCoder *c, const uint8_t *buf, int buf_size */ static av_always_inline int vpx_rac_is_end(VPXRangeCoder *c) { - if (c->end <= c->buffer && c->bits >= 0) - c->end_reached ++; - return c->end_reached > 10; + return c->end <= c->buffer && c->bits >= 1; } static av_always_inline unsigned int vpx_rac_renorm(VPXRangeCoder *c) diff --git a/tests/ref/fate/vp5 b/tests/ref/fate/vp5 index 09ebe62b25e..2469a3ec21a 100644 --- a/tests/ref/fate/vp5 +++ b/tests/ref/fate/vp5 @@ -249,4 +249,4 @@ 0, 243, 243, 1, 233472, 0x6f530ac6 0, 244, 244, 1, 233472, 0x94f7466c 0, 245, 245, 1, 233472, 0xa8c1d365 -0, 246, 246, 1, 233472, 0x4f3ef38c +0, 246, 246, 1, 233472, 0xbf73f1b7 -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Observe your enemies, for they first find out your faults. -- Antisthenes [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] [-- Attachment #2: 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] 3+ messages in thread
end of thread, other threads:[~2022-08-29 22:49 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-28 17:25 [FFmpeg-devel] [PATCH] avcodec/vpx_rac: Adjust vpx_rac_is_end) threshold Michael Niedermayer 2022-08-29 15:29 ` Andreas Rheinhardt 2022-08-29 22:48 ` Michael Niedermayer
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