[FFmpeg-devel,1/3] avcodec/vp56: Require 1 undamaged frame for 5 damaged frames for concealment to be used

Submitted by Michael Niedermayer on March 15, 2017, 3:12 a.m.

Details

Message ID 20170315031251.22570-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer March 15, 2017, 3:12 a.m.
Fixes timeout with 847/clusterfuzz-testcase-5291877358108672
Fixes timeout with 850/clusterfuzz-testcase-5721296509861888

This likely will need to be tweaked

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/vp56.c | 10 +++++++---
 libavcodec/vp56.h |  1 +
 2 files changed, 8 insertions(+), 3 deletions(-)

Comments

Ronald S. Bultje March 15, 2017, 11:58 a.m.
Hi,

On Tue, Mar 14, 2017 at 11:12 PM, Michael Niedermayer <
michael@niedermayer.cc> wrote:

> Fixes timeout with 847/clusterfuzz-testcase-5291877358108672
> Fixes timeout with 850/clusterfuzz-testcase-5721296509861888
>
> This likely will need to be tweaked


Sorry, but this is getting insane. I can't possibly be expected to just
approve this. What's your end game?

Ronald
Kieran Kunhya March 15, 2017, 12:21 p.m.
On Wed, 15 Mar 2017 at 12:05 Ronald S. Bultje <rsbultje@gmail.com> wrote:

> Hi,
>
> On Tue, Mar 14, 2017 at 11:12 PM, Michael Niedermayer <
> michael@niedermayer.cc> wrote:
>
> > Fixes timeout with 847/clusterfuzz-testcase-5291877358108672
> > Fixes timeout with 850/clusterfuzz-testcase-5721296509861888
> >
> > This likely will need to be tweaked
>
>
> Sorry, but this is getting insane. I can't possibly be expected to just
> approve this. What's your end game?
>

I have tons of testcases for h264 that are 1KB and can make error
concealment run for ages.
Trying to fix them will just become a never ending set of heuristics to
guess the conditions like the above.

Kieran
Ronald S. Bultje March 15, 2017, 12:58 p.m.
Hi,

On Wed, Mar 15, 2017 at 8:21 AM, Kieran Kunhya <kierank@obe.tv> wrote:

> On Wed, 15 Mar 2017 at 12:05 Ronald S. Bultje <rsbultje@gmail.com> wrote:
>
> > Hi,
> >
> > On Tue, Mar 14, 2017 at 11:12 PM, Michael Niedermayer <
> > michael@niedermayer.cc> wrote:
> >
> > > Fixes timeout with 847/clusterfuzz-testcase-5291877358108672
> > > Fixes timeout with 850/clusterfuzz-testcase-5721296509861888
> > >
> > > This likely will need to be tweaked
> >
> >
> > Sorry, but this is getting insane. I can't possibly be expected to just
> > approve this. What's your end game?
> >
>
> I have tons of testcases for h264 that are 1KB and can make error
> concealment run for ages.
> Trying to fix them will just become a never ending set of heuristics to
> guess the conditions like the above.


Right. Also, this is fuzz-specific code. I've made remarks about this
before, but I'll say it again: ideally, I don't want fuzz-specific code
anywhere. Especially not "carefully crafted" crap like this.

I'm actually starting to believe that the error concealment code in this
decoder (vp56) is fuzz-specific code also. Is there a real-world input
where the user experience is improved by this code?

(If you want to speed up fuzzing, just add an artificial resolution limit
of 64x64 in your fuzzing binary's get_buffer2() callback.)

Ronald
wm4 March 15, 2017, 4:37 p.m.
On Wed, 15 Mar 2017 08:58:39 -0400
"Ronald S. Bultje" <rsbultje@gmail.com> wrote:

> Hi,
> 
> On Wed, Mar 15, 2017 at 8:21 AM, Kieran Kunhya <kierank@obe.tv> wrote:
> 
> > On Wed, 15 Mar 2017 at 12:05 Ronald S. Bultje <rsbultje@gmail.com> wrote:
> >  
> > > Hi,
> > >
> > > On Tue, Mar 14, 2017 at 11:12 PM, Michael Niedermayer <  
> > > michael@niedermayer.cc> wrote:  
> > >  
> > > > Fixes timeout with 847/clusterfuzz-testcase-5291877358108672
> > > > Fixes timeout with 850/clusterfuzz-testcase-5721296509861888
> > > >
> > > > This likely will need to be tweaked  
> > >
> > >
> > > Sorry, but this is getting insane. I can't possibly be expected to just
> > > approve this. What's your end game?
> > >  
> >
> > I have tons of testcases for h264 that are 1KB and can make error
> > concealment run for ages.
> > Trying to fix them will just become a never ending set of heuristics to
> > guess the conditions like the above.  
> 
> 
> Right. Also, this is fuzz-specific code. I've made remarks about this
> before, but I'll say it again: ideally, I don't want fuzz-specific code
> anywhere. Especially not "carefully crafted" crap like this.

+1

Effort should be put into generally making the code more robust,
instead of adding new elaborate special cases for every fuzz sample we
hit...
Michael Niedermayer March 15, 2017, 6:51 p.m.
On Wed, Mar 15, 2017 at 08:58:39AM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Wed, Mar 15, 2017 at 8:21 AM, Kieran Kunhya <kierank@obe.tv> wrote:
> 
> > On Wed, 15 Mar 2017 at 12:05 Ronald S. Bultje <rsbultje@gmail.com> wrote:
> >
> > > Hi,
> > >
> > > On Tue, Mar 14, 2017 at 11:12 PM, Michael Niedermayer <
> > > michael@niedermayer.cc> wrote:
> > >
> > > > Fixes timeout with 847/clusterfuzz-testcase-5291877358108672
> > > > Fixes timeout with 850/clusterfuzz-testcase-5721296509861888
> > > >
> > > > This likely will need to be tweaked
> > >
> > >
> > > Sorry, but this is getting insane. I can't possibly be expected to just
> > > approve this. What's your end game?
> > >
> >

> > I have tons of testcases for h264 that are 1KB and can make error
> > concealment run for ages.

and how is this related to a fix for th vp* decoder ?



> > Trying to fix them will just become a never ending set of heuristics to
> > guess the conditions like the above.

Thats only true to some extend. The issue that a file can take a long
time to decode isnt fixable, a file can always have a long duration
and high resolution.
The issue that a file that has few bytes size takes a long time to
decode can be fixed for some codecs.
Id say we should fix this where it can be done cleanly.
In formats that allow one to encode gigabyte sized blue frames in 1
byte theres no fix possible OTOH.

In cases where it is fixable the fix has to ensure that theres a
limit on the computations per byte. Thats exactly what happens by
limiting the number of concealed frames in relation to error free
frames. Keep in mind a "fixable" codec implies that undamaged frames
would not require unbound computational resources per byte.


> 
> 
> Right. Also, this is fuzz-specific code. I've made remarks about this
> before, but I'll say it again: ideally, I don't want fuzz-specific code
> anywhere. Especially not "carefully crafted" crap like this.
> 

> I'm actually starting to believe that the error concealment code in this
> decoder (vp56) is fuzz-specific code also. Is there a real-world input
> where the user experience is improved by this code?

I have no real world damaged vp56 files iam aware of, in fact i dont
think i remember seeing vp56 in the wild, what i have are just our
samples or at least thats what iam concioulsy aware of

Damaged data is inevitable though from packet loss, damaged sectors
and other, so damaged data is real it occurs with anything eventually
that is stored or transmitted. The only way it can never occur is if
a codec is never used more or less

and error concealment can provide better quality than lack of EC.

when fixing fuzz issues iam trying to do something usefull when theres
a related feature like implementing the very basic EC code in vp56.
Working error concealment code would be nice for every codec IMO

but if people dont want EC code in vp56 then we can drop this code of
course, makes no difference to me.


[...]
Ronald S. Bultje March 15, 2017, 7:02 p.m.
Hi,

On Wed, Mar 15, 2017 at 2:51 PM, Michael Niedermayer <michael@niedermayer.cc
> wrote:

> On Wed, Mar 15, 2017 at 08:58:39AM -0400, Ronald S. Bultje wrote:
> > I'm actually starting to believe that the error concealment code in this
> > decoder (vp56) is fuzz-specific code also. Is there a real-world input
> > where the user experience is improved by this code?
>
> I have no real world damaged vp56 files iam aware of, in fact i dont
> think i remember seeing vp56 in the wild, what i have are just our
> samples or at least thats what iam concioulsy aware of
>
> Damaged data is inevitable though from packet loss, damaged sectors
> and other, so damaged data is real it occurs with anything eventually
> that is stored or transmitted. The only way it can never occur is if
> a codec is never used more or less
>
> and error concealment can provide better quality than lack of EC.
>
> when fixing fuzz issues iam trying to do something usefull when theres
> a related feature like implementing the very basic EC code in vp56.
> Working error concealment code would be nice for every codec IMO
>

I agree in general, but we need a way of verifying that it actually works,
which in this case means "leads to an improvement in decoded stream
quality". I don't think that has been verified here.

No benefit (..) for a very quickly escalation in code complexity related to
this feature makes me wonder whether that's the right approach.

but if people dont want EC code in vp56 then we can drop this code of
> course, makes no difference to me.


I would personally probably vote to remove EC from vp56. I'd like to hear
other people's opinions also, don't remove it just because I said so ;-).

Ronald
wm4 March 15, 2017, 7:08 p.m.
On Wed, 15 Mar 2017 15:02:08 -0400
"Ronald S. Bultje" <rsbultje@gmail.com> wrote:

> Hi,
> 
> On Wed, Mar 15, 2017 at 2:51 PM, Michael Niedermayer <michael@niedermayer.cc
> > wrote:  
> 
> > On Wed, Mar 15, 2017 at 08:58:39AM -0400, Ronald S. Bultje wrote:  
> > > I'm actually starting to believe that the error concealment code in this
> > > decoder (vp56) is fuzz-specific code also. Is there a real-world input
> > > where the user experience is improved by this code?  
> >
> > I have no real world damaged vp56 files iam aware of, in fact i dont
> > think i remember seeing vp56 in the wild, what i have are just our
> > samples or at least thats what iam concioulsy aware of
> >
> > Damaged data is inevitable though from packet loss, damaged sectors
> > and other, so damaged data is real it occurs with anything eventually
> > that is stored or transmitted. The only way it can never occur is if
> > a codec is never used more or less
> >
> > and error concealment can provide better quality than lack of EC.
> >
> > when fixing fuzz issues iam trying to do something usefull when theres
> > a related feature like implementing the very basic EC code in vp56.
> > Working error concealment code would be nice for every codec IMO
> >  
> 
> I agree in general, but we need a way of verifying that it actually works,
> which in this case means "leads to an improvement in decoded stream
> quality". I don't think that has been verified here.
> 
> No benefit (..) for a very quickly escalation in code complexity related to
> this feature makes me wonder whether that's the right approach.
> 
> but if people dont want EC code in vp56 then we can drop this code of
> > course, makes no difference to me.  
> 
> 
> I would personally probably vote to remove EC from vp56. I'd like to hear
> other people's opinions also, don't remove it just because I said so ;-).

I think the right question is: are vp56 streams usually broken?

For example, for mpeg2 or h264, the answer can be "yes" because those
are used in DVB (i.e. very unreliable transport).
Michael Niedermayer March 15, 2017, 7:10 p.m.
On Wed, Mar 15, 2017 at 03:02:08PM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Wed, Mar 15, 2017 at 2:51 PM, Michael Niedermayer <michael@niedermayer.cc
> > wrote:
> 
> > On Wed, Mar 15, 2017 at 08:58:39AM -0400, Ronald S. Bultje wrote:
[...]

> but if people dont want EC code in vp56 then we can drop this code of
> > course, makes no difference to me.
> 
> 
> I would personally probably vote to remove EC from vp56. I'd like to hear
> other people's opinions also, don't remove it just because I said so ;-).

note, if we remove it we may need to be carefull with the resulting
frames or frames using them as reference as they would have areas that
havent been initialized.


[...]
Kieran Kunhya March 15, 2017, 7:18 p.m.
>
> > > I have tons of testcases for h264 that are 1KB and can make error
> > > concealment run for ages.
>
> and how is this related to a fix for th vp* decoder ?
>

My point is you can spend a lifetime fixing obscure conditions that cause
error concealment to take a long time.

Error concealment is good in itself but the above is essentially an
arbitrary condition that you've come up with to stop the bug. This is
essentially lavf-style guesswork and probing inside lavc.

Regards,
Kieran Kunhya
James Almer March 15, 2017, 7:29 p.m.
On 3/15/2017 3:51 PM, Michael Niedermayer wrote:
> On Wed, Mar 15, 2017 at 08:58:39AM -0400, Ronald S. Bultje wrote:
>> Hi,
>>
>> On Wed, Mar 15, 2017 at 8:21 AM, Kieran Kunhya <kierank@obe.tv> wrote:
>>
>>> On Wed, 15 Mar 2017 at 12:05 Ronald S. Bultje <rsbultje@gmail.com> wrote:
>>>
>>>> Hi,
>>>>
>>>> On Tue, Mar 14, 2017 at 11:12 PM, Michael Niedermayer <
>>>> michael@niedermayer.cc> wrote:
>>>>
>>>>> Fixes timeout with 847/clusterfuzz-testcase-5291877358108672
>>>>> Fixes timeout with 850/clusterfuzz-testcase-5721296509861888
>>>>>
>>>>> This likely will need to be tweaked
>>>>
>>>>
>>>> Sorry, but this is getting insane. I can't possibly be expected to just
>>>> approve this. What's your end game?
>>>>
>>>
> 
>>> I have tons of testcases for h264 that are 1KB and can make error
>>> concealment run for ages.
> 
> and how is this related to a fix for th vp* decoder ?
> 
> 
> 
>>> Trying to fix them will just become a never ending set of heuristics to
>>> guess the conditions like the above.
> 
> Thats only true to some extend. The issue that a file can take a long
> time to decode isnt fixable, a file can always have a long duration
> and high resolution.
> The issue that a file that has few bytes size takes a long time to
> decode can be fixed for some codecs.
> Id say we should fix this where it can be done cleanly.
> In formats that allow one to encode gigabyte sized blue frames in 1
> byte theres no fix possible OTOH.
> 
> In cases where it is fixable the fix has to ensure that theres a
> limit on the computations per byte. Thats exactly what happens by
> limiting the number of concealed frames in relation to error free
> frames. Keep in mind a "fixable" codec implies that undamaged frames
> would not require unbound computational resources per byte.
> 
> 
>>
>>
>> Right. Also, this is fuzz-specific code. I've made remarks about this
>> before, but I'll say it again: ideally, I don't want fuzz-specific code
>> anywhere. Especially not "carefully crafted" crap like this.
>>
> 
>> I'm actually starting to believe that the error concealment code in this
>> decoder (vp56) is fuzz-specific code also. Is there a real-world input
>> where the user experience is improved by this code?
> 
> I have no real world damaged vp56 files iam aware of, in fact i dont
> think i remember seeing vp56 in the wild, what i have are just our
> samples or at least thats what iam concioulsy aware of

Afaik, old flv videos used VP6 (libavcodec reports them as vp6f). There
are some real world files out there, like video game trailers.
I have a Diablo 3 and a Starcraft 2 trailers encoded in VP6 here if you'd
like to check them.
Michael Niedermayer March 15, 2017, 10:12 p.m.
On Wed, Mar 15, 2017 at 07:18:30PM +0000, Kieran Kunhya wrote:
> >
> > > > I have tons of testcases for h264 that are 1KB and can make error
> > > > concealment run for ages.
> >
> > and how is this related to a fix for th vp* decoder ?
> >
> 
> My point is you can spend a lifetime fixing obscure conditions that cause
> error concealment to take a long time.

the vp56 EC code
just sets every block to the reference frames content, thats speedwise
at the level of a memcpy() or memset().

if its slow thats because the resolution is high and the frame has
few bytes resulting in lots of pixels being initilaized from just
frame headers data.


[...]
Alexander Strasser March 15, 2017, 10:33 p.m.
Hi!

This is not a review, just a comment you can address in case you push this.


On 2017-03-15 04:12 +0100, Michael Niedermayer wrote:
> Fixes timeout with 847/clusterfuzz-testcase-5291877358108672
> Fixes timeout with 850/clusterfuzz-testcase-5721296509861888
> 
> This likely will need to be tweaked
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/vp56.c | 10 +++++++---
>  libavcodec/vp56.h |  1 +
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/vp56.c b/libavcodec/vp56.c
> index 9d4162bb96..b4ee760080 100644
> --- a/libavcodec/vp56.c
> +++ b/libavcodec/vp56.c
> @@ -508,6 +508,7 @@ static int vp56_size_changed(VP56Context *s)
>      s->plane_height[1] = s->plane_height[2] = avctx->coded_height/2;
>  
>      s->have_undamaged_frame = 0;
> +    s->damaged_frame_count = 0;
>  
>      for (i=0; i<4; i++)
>          s->stride[i] = s->flip * s->frames[VP56_FRAME_CURRENT]->linesize[i];
> @@ -712,8 +713,9 @@ static int ff_vp56_decode_mbs(AVCodecContext *avctx, void *data,
>                  int ret = vp56_decode_mb(s, mb_row, mb_col, is_alpha);
>                  if (ret < 0) {
>                      damaged = 1;
> -                    if (!s->have_undamaged_frame) {
> +                    if (5*s->have_undamaged_frame <= s->damaged_frame_count) {
>                          s->discard_frame = 1;
> +                        s->damaged_frame_count ++;

If I am not mistaken, the space before ++ is not needed and not usual for FFmpeg's code base.

>                          return AVERROR_INVALIDDATA;
>                      }
>                  }
> @@ -733,8 +735,10 @@ static int ff_vp56_decode_mbs(AVCodecContext *avctx, void *data,
>          }
>      }
>  
> -    if (!damaged)
> -        s->have_undamaged_frame = 1;
> +    if (!damaged) {
> +        s->have_undamaged_frame ++;
> +    } else
> +        s->damaged_frame_count ++;

Same.

>  
>  next:
>      if (p->key_frame || s->golden_frame) {
> diff --git a/libavcodec/vp56.h b/libavcodec/vp56.h
> index c049399df8..37837d0c7b 100644
> --- a/libavcodec/vp56.h
> +++ b/libavcodec/vp56.h
> @@ -206,6 +206,7 @@ struct vp56_context {
>  
>      int have_undamaged_frame;
>      int discard_frame;
> +    int damaged_frame_count;
>  };
>  
>  
> -- 
> 2.11.0


  Alexander
Ronald S. Bultje March 16, 2017, 12:44 a.m.
Hi,

On Wed, Mar 15, 2017 at 6:12 PM, Michael Niedermayer <michael@niedermayer.cc
> wrote:

> On Wed, Mar 15, 2017 at 07:18:30PM +0000, Kieran Kunhya wrote:
> > >
> > > > > I have tons of testcases for h264 that are 1KB and can make error
> > > > > concealment run for ages.
> > >
> > > and how is this related to a fix for th vp* decoder ?
> > >
> >
> > My point is you can spend a lifetime fixing obscure conditions that cause
> > error concealment to take a long time.
>
> the vp56 EC code
> just sets every block to the reference frames content


I don't see how this helps the user experience.

I once again think you should consider removing the vp56 EC code until we
have a better idea of requirements of and need for such a feature.

Ronald
Moritz Barsnick March 16, 2017, 4:48 p.m.
On Wed, Mar 15, 2017 at 16:29:22 -0300, James Almer wrote:
> Afaik, old flv videos used VP6 (libavcodec reports them as vp6f). There
> are some real world files out there, like video game trailers.
> I have a Diablo 3 and a Starcraft 2 trailers encoded in VP6 here if you'd
> like to check them.

VP6 is the (or a) reference video codec for Flash 8, apparently. VP6
encoded Flash 8 video samples can be found with web search, such as:
http://www.mediacollege.com/adobe/flash/video/tutorial/example-flv.html

I also have some proper (non-sample) videos in VP6 from somewhere on
the web (I just found 25 such FLVs), it may have been somewhat
widespread at a certain time.

Moritz

Patch hide | download patch | download mbox

diff --git a/libavcodec/vp56.c b/libavcodec/vp56.c
index 9d4162bb96..b4ee760080 100644
--- a/libavcodec/vp56.c
+++ b/libavcodec/vp56.c
@@ -508,6 +508,7 @@  static int vp56_size_changed(VP56Context *s)
     s->plane_height[1] = s->plane_height[2] = avctx->coded_height/2;
 
     s->have_undamaged_frame = 0;
+    s->damaged_frame_count = 0;
 
     for (i=0; i<4; i++)
         s->stride[i] = s->flip * s->frames[VP56_FRAME_CURRENT]->linesize[i];
@@ -712,8 +713,9 @@  static int ff_vp56_decode_mbs(AVCodecContext *avctx, void *data,
                 int ret = vp56_decode_mb(s, mb_row, mb_col, is_alpha);
                 if (ret < 0) {
                     damaged = 1;
-                    if (!s->have_undamaged_frame) {
+                    if (5*s->have_undamaged_frame <= s->damaged_frame_count) {
                         s->discard_frame = 1;
+                        s->damaged_frame_count ++;
                         return AVERROR_INVALIDDATA;
                     }
                 }
@@ -733,8 +735,10 @@  static int ff_vp56_decode_mbs(AVCodecContext *avctx, void *data,
         }
     }
 
-    if (!damaged)
-        s->have_undamaged_frame = 1;
+    if (!damaged) {
+        s->have_undamaged_frame ++;
+    } else
+        s->damaged_frame_count ++;
 
 next:
     if (p->key_frame || s->golden_frame) {
diff --git a/libavcodec/vp56.h b/libavcodec/vp56.h
index c049399df8..37837d0c7b 100644
--- a/libavcodec/vp56.h
+++ b/libavcodec/vp56.h
@@ -206,6 +206,7 @@  struct vp56_context {
 
     int have_undamaged_frame;
     int discard_frame;
+    int damaged_frame_count;
 };