[FFmpeg-devel] Question about supported_fps in libavutil/timecode.c::check_fps

[jon morley]

Hi Clément,

That is a good point! I am attaching an additional patch to remove those 
cases even before entering the mod test loop.

Now the logic should look like this:

static int check_fps(int fps)
{
     if (fps <= 0) return -1;

     int i;
     static const int supported_fps_bases[] = {24, 25, 30};

     for (i = 0; i < FF_ARRAY_ELEMS(supported_fps_bases); i++)
         if (fps % supported_fps_bases[i] == 0)
             return 0;
     return -1;
}

I am still really curious to know if switching to this division (modulo) 
test breaks the "spirit" of check_fps. I could not find anywhere that 
benefited from the explicit list the method currently used, but that 
doesn't mean it isn't out there.

Thanks,
Jon

On 1/24/15 2:27 AM, Clément Bœsch wrote:
> On Fri, Jan 23, 2015 at 08:48:37AM -0800, jon morley wrote:
>> Patch attached for consideration.
>>
>> On 1/23/15 8:03 AM, jon morley wrote:
>>> Currently check_fps has the following logic:
>>>
>>> static int check_fps(int fps)
>>> {
>>>      int i;
>>>      static const int supported_fps[] = {24, 25, 30, 48, 50, 60};
>>>
>>>      for (i = 0; i < FF_ARRAY_ELEMS(supported_fps); i++)
>>>          if (fps == supported_fps[i])
>>>              return 0;
>>>      return -1;
>>> }
>>>
>>> I am starting to see more and more movies with fps rates in excess of
>>> this list from modified GoPro files and other raw camera sources.
>>>
>>> I was originally adding more entries as the sources came rolling in
>>> because I could not see any issue in how this was getting called later
>>> with that approach.
>>>
>>> I still don't see the drawback of adding more, but I am tired of adding
>>> a new rate every time I encounter one in the wild. I was curious if it
>>> wouldn't make more sense to change the logic to the following:
>>>
>>> static int check_fps(int fps)
>>> {
>>>      int i;
>>>      static const int supported_fps_bases[] = {24, 25, 30};
>>>
>>>      for (i = 0; i < FF_ARRAY_ELEMS(supported_fps_bases); i++)
>>>          if (fps % supported_fps_bases[i] == 0)
>>>              return 0;
>>>      return -1;
>>> }
>>>
>>> If that makes sense to you, then I will submit a patch. Please let me
>>> know if I have overlooked some other usage/meaning of check_fps that I
>>> am overlooking.
>>>
>>> Thanks,
>>> Jon
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel at ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>>  From 73e5339ec76305d34214b5e84dc5a38673f784b7 Mon Sep 17 00:00:00 2001
>> From: Jon Morley <jon at tweaksoftware.com>
>> Date: Fri, 23 Jan 2015 08:43:33 -0800
>> Subject: [PATCH] libavutil/timecode.c: Extend check_fps logic to handle high
>>   frame rates
>>
>> QuickTime sources continue to push higher and higher frame rates. This
>> change moves away from explictly testing incoming fps values towards
>> ensuring the incoming value is evenly divisable by 24, 25, or 30.
>> ---
>>   libavutil/timecode.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/libavutil/timecode.c b/libavutil/timecode.c
>> index 1dfd040..c10895c 100644
>> --- a/libavutil/timecode.c
>> +++ b/libavutil/timecode.c
>> @@ -141,10 +141,10 @@ char *av_timecode_make_mpeg_tc_string(char *buf, uint32_t tc25bit)
>>   static int check_fps(int fps)
>>   {
>>       int i;
>> -    static const int supported_fps[] = {24, 25, 30, 48, 50, 60};
>> +    static const int supported_fps_bases[] = {24, 25, 30};
>>
>> -    for (i = 0; i < FF_ARRAY_ELEMS(supported_fps); i++)
>> -        if (fps == supported_fps[i])
>> +    for (i = 0; i < FF_ARRAY_ELEMS(supported_fps_bases); i++)
>> +        if (fps % supported_fps_bases[i] == 0)
>>               return 0;
>>       return -1;
>
> I don't think you want to accept fps ≤ 0
>
> [...]
>
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
-------------- next part --------------
From 0a72d78992bbeb6c2536285397149cceb64b05d8 Mon Sep 17 00:00:00 2001
From: Jon Morley <jon at tweaksoftware.com>
Date: Sat, 24 Jan 2015 07:28:40 -0800
Subject: [PATCH 2/2] libavutil/timecode.c: check_fps must reject rates at or
 below zero

An earlier change to check_fps's logic which now confirms that the
incoming evaluation fps is evenly divisable by a list of supported
rates leaves open the possibility of accepting zero and negative frame
rates. This change removes that posibility.
---
 libavutil/timecode.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libavutil/timecode.c b/libavutil/timecode.c
index c10895c..446e2d5 100644
--- a/libavutil/timecode.c
+++ b/libavutil/timecode.c
@@ -140,6 +140,8 @@ char *av_timecode_make_mpeg_tc_string(char *buf, uint32_t tc25bit)
 
 static int check_fps(int fps)
 {
+    if (fps <= 0) return -1;
+
     int i;
     static const int supported_fps_bases[] = {24, 25, 30};
 
-- 
1.8.5.2 (Apple Git-48)