| Message ID | 20220828172545.8185-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/vpx_rac: Adjust vpx_rac_is_end) threshold | expand |
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
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
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
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(-)