[FFmpeg-devel] [PATCH 2/2] avformat/apng: set max_fps to no limit by default

Michael Niedermayer michael at niedermayer.cc
Tue Mar 21 17:47:56 EET 2017


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 at 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 


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
then the original author, trying to rewrite it will not make it better.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170321/fa68286d/attachment.sig>


More information about the ffmpeg-devel mailing list