| Message ID | 20170315031251.22570-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
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
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
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
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...
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. [...]
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
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).
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. [...]
> > > > 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
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.
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. [...]
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
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
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
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; };
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(-)