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 <jamr...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
