[FFmpeg-devel] [PATCH] avcodec/vpx_rac: Adjust vpx_rac_is_end) threshold

[FFmpeg-devel] [PATCH] avcodec/vpx_rac: Adjust vpx_rac_is_end) threshold

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

Re: [FFmpeg-devel] [PATCH] avcodec/vpx_rac: Adjust vpx_rac_is_end) threshold

[Andreas Rheinhardt]

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

Re: [FFmpeg-devel] [PATCH] avcodec/vpx_rac: Adjust vpx_rac_is_end) threshold

[Michael Niedermayer]

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