| Message ID | 20170321020323.6136-2-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Commit | 7bfbb7229971a5220fed07bb931e6ff1030a319a |
| Headers | show |
On Mon, Mar 20, 2017 at 11:03:23PM -0300, James Almer wrote: > Should fix ticket #6252 > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavformat/apngdec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c > index 7a284e32c2..75dcf74a0c 100644 > --- a/libavformat/apngdec.c > +++ b/libavformat/apngdec.c > @@ -421,7 +421,7 @@ static const AVOption options[] = { > { "ignore_loop", "ignore loop setting" , offsetof(APNGDemuxContext, ignore_loop), > AV_OPT_TYPE_BOOL, { .i64 = 1 } , 0, 1 , AV_OPT_FLAG_DECODING_PARAM }, > { "max_fps" , "maximum framerate (0 is no limit)" , offsetof(APNGDemuxContext, max_fps), > - AV_OPT_TYPE_INT, { .i64 = DEFAULT_APNG_FPS }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, > + AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, > { "default_fps", "default framerate (0 is as fast as possible)", offsetof(APNGDemuxContext, default_fps), > AV_OPT_TYPE_INT, { .i64 = DEFAULT_APNG_FPS }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, > { NULL }, why was there a max fps set ? are there files which have huge and incorrect fps ? if so this may cause a regression and just increasing the default value for max_fps could be better. If no such files exist then this LGTM wither way it needs a update to fate [...]
On 3/21/2017 9:52 AM, Michael Niedermayer wrote: > On Mon, Mar 20, 2017 at 11:03:23PM -0300, James Almer wrote: >> Should fix ticket #6252 >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavformat/apngdec.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c >> index 7a284e32c2..75dcf74a0c 100644 >> --- a/libavformat/apngdec.c >> +++ b/libavformat/apngdec.c >> @@ -421,7 +421,7 @@ static const AVOption options[] = { >> { "ignore_loop", "ignore loop setting" , offsetof(APNGDemuxContext, ignore_loop), >> AV_OPT_TYPE_BOOL, { .i64 = 1 } , 0, 1 , AV_OPT_FLAG_DECODING_PARAM }, >> { "max_fps" , "maximum framerate (0 is no limit)" , offsetof(APNGDemuxContext, max_fps), >> - AV_OPT_TYPE_INT, { .i64 = DEFAULT_APNG_FPS }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, >> + AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, >> { "default_fps", "default framerate (0 is as fast as possible)", offsetof(APNGDemuxContext, default_fps), >> AV_OPT_TYPE_INT, { .i64 = DEFAULT_APNG_FPS }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, >> { NULL }, > > why was there a max fps set ? > are there files which have huge and incorrect fps ? I have no idea. The author of the decoder may know. But apng is far from a widespread format seeing it has been supported by only one browser until like a week ago, so the chances of bad files like it could happen with jpg or png is most likely low. > if so this may cause a regression and just increasing the default value for > max_fps could be better. I guess 60 would be a saner max value than 15 as it is now, but i still wonder why would we have a max fps set as default to begin with. IMO, if the worry was about a broken/incorrect headers (fuzzing or such), then checking the CRC field for correctness may be a better idea than crippling decoding of valid files by default. > > If no such files exist then this LGTM > > wither way it needs a update to fate > > [...] > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Tue, Mar 21, 2017 at 10:03:48AM -0300, James Almer wrote: > On 3/21/2017 9:52 AM, Michael Niedermayer wrote: > > On Mon, Mar 20, 2017 at 11:03:23PM -0300, James Almer wrote: > >> Should fix ticket #6252 > >> > >> Signed-off-by: James Almer <jamrial@gmail.com> > >> --- > >> libavformat/apngdec.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c > >> index 7a284e32c2..75dcf74a0c 100644 > >> --- a/libavformat/apngdec.c > >> +++ b/libavformat/apngdec.c > >> @@ -421,7 +421,7 @@ static const AVOption options[] = { > >> { "ignore_loop", "ignore loop setting" , offsetof(APNGDemuxContext, ignore_loop), > >> AV_OPT_TYPE_BOOL, { .i64 = 1 } , 0, 1 , AV_OPT_FLAG_DECODING_PARAM }, > >> { "max_fps" , "maximum framerate (0 is no limit)" , offsetof(APNGDemuxContext, max_fps), > >> - AV_OPT_TYPE_INT, { .i64 = DEFAULT_APNG_FPS }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, > >> + AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, > >> { "default_fps", "default framerate (0 is as fast as possible)", offsetof(APNGDemuxContext, default_fps), > >> AV_OPT_TYPE_INT, { .i64 = DEFAULT_APNG_FPS }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, > >> { NULL }, > > > > why was there a max fps set ? > > are there files which have huge and incorrect fps ? > > I have no idea. The author of the decoder may know. But apng is far from > a widespread format seeing it has been supported by only one browser until > like a week ago, so the chances of bad files like it could happen with > jpg or png is most likely low. > > > if so this may cause a regression and just increasing the default value for > > max_fps could be better. > > I guess 60 would be a saner max value than 15 as it is now, but i still > wonder why would we have a max fps set as default to begin with. > > IMO, if the worry was about a broken/incorrect headers (fuzzing or such), > then checking the CRC field for correctness may be a better idea than > crippling decoding of valid files by default. why do you think this could be related to fuzzing ? i dont know why the check is there but i had suspected that it was a workaround for broken encoders. Possibly not apng encoders but a more widespread format like animated gif that is transcoded to apng we have a similar max check in gifdec.c if gif files need it, gif files converted to apng should need it too and i would suspect that animated gifs are a major source for apng files, but maybe iam wrong [...]
On 3/21/2017 11:05 AM, Michael Niedermayer wrote: > On Tue, Mar 21, 2017 at 10:03:48AM -0300, James Almer wrote: >> On 3/21/2017 9:52 AM, Michael Niedermayer wrote: >>> On Mon, Mar 20, 2017 at 11:03:23PM -0300, James Almer wrote: >>>> Should fix ticket #6252 >>>> >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>>> libavformat/apngdec.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c >>>> index 7a284e32c2..75dcf74a0c 100644 >>>> --- a/libavformat/apngdec.c >>>> +++ b/libavformat/apngdec.c >>>> @@ -421,7 +421,7 @@ static const AVOption options[] = { >>>> { "ignore_loop", "ignore loop setting" , offsetof(APNGDemuxContext, ignore_loop), >>>> AV_OPT_TYPE_BOOL, { .i64 = 1 } , 0, 1 , AV_OPT_FLAG_DECODING_PARAM }, >>>> { "max_fps" , "maximum framerate (0 is no limit)" , offsetof(APNGDemuxContext, max_fps), >>>> - AV_OPT_TYPE_INT, { .i64 = DEFAULT_APNG_FPS }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, >>>> + AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, >>>> { "default_fps", "default framerate (0 is as fast as possible)", offsetof(APNGDemuxContext, default_fps), >>>> AV_OPT_TYPE_INT, { .i64 = DEFAULT_APNG_FPS }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, >>>> { NULL }, >>> >>> why was there a max fps set ? >>> are there files which have huge and incorrect fps ? >> >> I have no idea. The author of the decoder may know. But apng is far from >> a widespread format seeing it has been supported by only one browser until >> like a week ago, so the chances of bad files like it could happen with >> jpg or png is most likely low. >> >>> if so this may cause a regression and just increasing the default value for >>> max_fps could be better. >> >> I guess 60 would be a saner max value than 15 as it is now, but i still >> wonder why would we have a max fps set as default to begin with. >> > >> IMO, if the worry was about a broken/incorrect headers (fuzzing or such), >> then checking the CRC field for correctness may be a better idea than >> crippling decoding of valid files by default. > > why do you think this could be related to fuzzing ? Only reason i can think other than broken encodings to have the delay_num and delay_den fields reporting bogus values. > > i dont know why the check is there but i had suspected that it was > a workaround for broken encoders. Possibly not apng encoders but > a more widespread format like animated gif that is transcoded to apng > > we have a similar max check in gifdec.c > > if gif files need it, gif files converted to apng should need it too > and i would suspect that animated gifs are a major source for apng > files, but maybe iam wrong So what would you say should be done? commit this patch, or make the max_fps default to 60 instead?
On Tue, Mar 21, 2017 at 12:08:03PM -0300, James Almer wrote: > On 3/21/2017 11:05 AM, Michael Niedermayer wrote: > > On Tue, Mar 21, 2017 at 10:03:48AM -0300, James Almer wrote: > >> On 3/21/2017 9:52 AM, Michael Niedermayer wrote: > >>> On Mon, Mar 20, 2017 at 11:03:23PM -0300, James Almer wrote: > >>>> Should fix ticket #6252 > >>>> > >>>> Signed-off-by: James Almer <jamrial@gmail.com> > >>>> --- > >>>> libavformat/apngdec.c | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c > >>>> index 7a284e32c2..75dcf74a0c 100644 > >>>> --- a/libavformat/apngdec.c > >>>> +++ b/libavformat/apngdec.c > >>>> @@ -421,7 +421,7 @@ static const AVOption options[] = { > >>>> { "ignore_loop", "ignore loop setting" , offsetof(APNGDemuxContext, ignore_loop), > >>>> AV_OPT_TYPE_BOOL, { .i64 = 1 } , 0, 1 , AV_OPT_FLAG_DECODING_PARAM }, > >>>> { "max_fps" , "maximum framerate (0 is no limit)" , offsetof(APNGDemuxContext, max_fps), > >>>> - AV_OPT_TYPE_INT, { .i64 = DEFAULT_APNG_FPS }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, > >>>> + AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, > >>>> { "default_fps", "default framerate (0 is as fast as possible)", offsetof(APNGDemuxContext, default_fps), > >>>> AV_OPT_TYPE_INT, { .i64 = DEFAULT_APNG_FPS }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, > >>>> { NULL }, > >>> > >>> why was there a max fps set ? > >>> are there files which have huge and incorrect fps ? > >> > >> I have no idea. The author of the decoder may know. But apng is far from > >> a widespread format seeing it has been supported by only one browser until > >> like a week ago, so the chances of bad files like it could happen with > >> jpg or png is most likely low. > >> > >>> if so this may cause a regression and just increasing the default value for > >>> max_fps could be better. > >> > >> I guess 60 would be a saner max value than 15 as it is now, but i still > >> wonder why would we have a max fps set as default to begin with. > >> > > > >> IMO, if the worry was about a broken/incorrect headers (fuzzing or such), > >> then checking the CRC field for correctness may be a better idea than > >> crippling decoding of valid files by default. > > > > why do you think this could be related to fuzzing ? > > Only reason i can think other than broken encodings to have the delay_num > and delay_den fields reporting bogus values. > > > > > i dont know why the check is there but i had suspected that it was > > a workaround for broken encoders. Possibly not apng encoders but > > a more widespread format like animated gif that is transcoded to apng > > > > we have a similar max check in gifdec.c > > > > if gif files need it, gif files converted to apng should need it too > > and i would suspect that animated gifs are a major source for apng > > files, but maybe iam wrong > > So what would you say should be done? commit this patch, or make the > max_fps default to 60 instead? iam fine with either, but if its set to 0 we should keep an eye open for regressions and be prepared to change the value [...]
On 3/21/2017 12:47 PM, Michael Niedermayer wrote: > On Tue, Mar 21, 2017 at 12:08:03PM -0300, James Almer wrote: >> On 3/21/2017 11:05 AM, Michael Niedermayer wrote: >>> On Tue, Mar 21, 2017 at 10:03:48AM -0300, James Almer wrote: >>>> On 3/21/2017 9:52 AM, Michael Niedermayer wrote: >>>>> On Mon, Mar 20, 2017 at 11:03:23PM -0300, James Almer wrote: >>>>>> Should fix ticket #6252 >>>>>> >>>>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>>>> --- >>>>>> libavformat/apngdec.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c >>>>>> index 7a284e32c2..75dcf74a0c 100644 >>>>>> --- a/libavformat/apngdec.c >>>>>> +++ b/libavformat/apngdec.c >>>>>> @@ -421,7 +421,7 @@ static const AVOption options[] = { >>>>>> { "ignore_loop", "ignore loop setting" , offsetof(APNGDemuxContext, ignore_loop), >>>>>> AV_OPT_TYPE_BOOL, { .i64 = 1 } , 0, 1 , AV_OPT_FLAG_DECODING_PARAM }, >>>>>> { "max_fps" , "maximum framerate (0 is no limit)" , offsetof(APNGDemuxContext, max_fps), >>>>>> - AV_OPT_TYPE_INT, { .i64 = DEFAULT_APNG_FPS }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, >>>>>> + AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, >>>>>> { "default_fps", "default framerate (0 is as fast as possible)", offsetof(APNGDemuxContext, default_fps), >>>>>> AV_OPT_TYPE_INT, { .i64 = DEFAULT_APNG_FPS }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, >>>>>> { NULL }, >>>>> >>>>> why was there a max fps set ? >>>>> are there files which have huge and incorrect fps ? >>>> >>>> I have no idea. The author of the decoder may know. But apng is far from >>>> a widespread format seeing it has been supported by only one browser until >>>> like a week ago, so the chances of bad files like it could happen with >>>> jpg or png is most likely low. >>>> >>>>> if so this may cause a regression and just increasing the default value for >>>>> max_fps could be better. >>>> >>>> I guess 60 would be a saner max value than 15 as it is now, but i still >>>> wonder why would we have a max fps set as default to begin with. >>>> >>> >>>> IMO, if the worry was about a broken/incorrect headers (fuzzing or such), >>>> then checking the CRC field for correctness may be a better idea than >>>> crippling decoding of valid files by default. >>> >>> why do you think this could be related to fuzzing ? >> >> Only reason i can think other than broken encodings to have the delay_num >> and delay_den fields reporting bogus values. >> >>> >>> i dont know why the check is there but i had suspected that it was >>> a workaround for broken encoders. Possibly not apng encoders but >>> a more widespread format like animated gif that is transcoded to apng >>> >>> we have a similar max check in gifdec.c >>> >>> if gif files need it, gif files converted to apng should need it too >>> and i would suspect that animated gifs are a major source for apng >>> files, but maybe iam wrong >> >> So what would you say should be done? commit this patch, or make the >> max_fps default to 60 instead? > > iam fine with either, but if its set to 0 we should keep an eye open > for regressions and be prepared to change the value Ok, pushed then. Thanks
Hi, On 21/03/2017 14:03, James Almer wrote: > On 3/21/2017 9:52 AM, Michael Niedermayer wrote: >> On Mon, Mar 20, 2017 at 11:03:23PM -0300, James Almer wrote: >>> Should fix ticket #6252 >>> >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> libavformat/apngdec.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c >>> index 7a284e32c2..75dcf74a0c 100644 >>> --- a/libavformat/apngdec.c >>> +++ b/libavformat/apngdec.c >>> @@ -421,7 +421,7 @@ static const AVOption options[] = { >>> { "ignore_loop", "ignore loop setting" , offsetof(APNGDemuxContext, ignore_loop), >>> AV_OPT_TYPE_BOOL, { .i64 = 1 } , 0, 1 , AV_OPT_FLAG_DECODING_PARAM }, >>> { "max_fps" , "maximum framerate (0 is no limit)" , offsetof(APNGDemuxContext, max_fps), >>> - AV_OPT_TYPE_INT, { .i64 = DEFAULT_APNG_FPS }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, >>> + AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, >>> { "default_fps", "default framerate (0 is as fast as possible)", offsetof(APNGDemuxContext, default_fps), >>> AV_OPT_TYPE_INT, { .i64 = DEFAULT_APNG_FPS }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, >>> { NULL }, >> why was there a max fps set ? >> are there files which have huge and incorrect fps ? > I have no idea. The author of the decoder may know. > A bit late, but honestly, I don't remember why I did it that way, though both patches look fine as they are. It's easy enough to come back to that code when/if needed. Thanks,
diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c index 7a284e32c2..75dcf74a0c 100644 --- a/libavformat/apngdec.c +++ b/libavformat/apngdec.c @@ -421,7 +421,7 @@ static const AVOption options[] = { { "ignore_loop", "ignore loop setting" , offsetof(APNGDemuxContext, ignore_loop), AV_OPT_TYPE_BOOL, { .i64 = 1 } , 0, 1 , AV_OPT_FLAG_DECODING_PARAM }, { "max_fps" , "maximum framerate (0 is no limit)" , offsetof(APNGDemuxContext, max_fps), - AV_OPT_TYPE_INT, { .i64 = DEFAULT_APNG_FPS }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, + AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, { "default_fps", "default framerate (0 is as fast as possible)", offsetof(APNGDemuxContext, default_fps), AV_OPT_TYPE_INT, { .i64 = DEFAULT_APNG_FPS }, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM }, { NULL },
Should fix ticket #6252 Signed-off-by: James Almer <jamrial@gmail.com> --- libavformat/apngdec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)