* [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