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

[jon morley]

Hi Clément,

I am sorry I was rude. That was not my intention. I was attempting to 
follow these directions from the ffmpeg.org page:

"You can use the FFmpeg libraries in your commercial program, but you 
are encouraged to publish any patch you make. In this case the best way 
to proceed is to send your patches to the ffmpeg-devel mailing list 
following the guidelines illustrated in the remainder of this document."

I will stick to mailing patches exclusively in the future.

Patches reflecting your suggestions attached.

Sincerely,
Jon

On 1/24/15 8:21 AM, Clément Bœsch wrote:
> On Sat, Jan 24, 2015 at 07:40:38AM -0800, jon morley wrote:
>> Hi Clément,
>>
>
> Hi,
>
>> 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};
>
> You can't put statements before declarations, some compilers will choke on
> it.
>
> Also, please squash it with the previous patch since it wasn't applied
> yet.
>
>>
>>      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.
>
> I'm more concerned about how the rest of the code will behave. Typically,
> av_timecode_adjust_ntsc_framenum2() could benefit from some improvements
> (checking if fps % 30, and deducing drop_frames and frames_per_10mins
> accordingly) if you allow such thing. Then you might need to adjust
> check_timecode() as well to allow the drop frame for the other % 30.
>
>>
>> Thanks,
>> Jon
>>
>
> [...]
>
> Note: please do not top post on this mailing list, it is considered rude.
>
> Regards,
>
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
-------------- next part --------------
>From 95f1fb3695f086de1baa301015985742d688a159 Mon Sep 17 00:00:00 2001
From: Jon Morley <jon at tweaksoftware.com>
Date: Sat, 24 Jan 2015 12:18:50 -0800
Subject: [PATCH] libavutil/timecode.c: Add support for frame rates beyond 60
 fps

Instead of looking for specifically supported frame rates this
collection of changes checks to see if the given rates are evenly
divisible by supported common factors.
---
 libavutil/timecode.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/libavutil/timecode.c b/libavutil/timecode.c
index 1dfd040..c2469c0 100644
--- a/libavutil/timecode.c
+++ b/libavutil/timecode.c
@@ -33,18 +33,15 @@
 
 int av_timecode_adjust_ntsc_framenum2(int framenum, int fps)
 {
-    /* only works for NTSC 29.97 and 59.94 */
+    int factor = 1;
     int drop_frames = 0;
     int d, m, frames_per_10mins;
 
-    if (fps == 30) {
-        drop_frames = 2;
-        frames_per_10mins = 17982;
-    } else if (fps == 60) {
-        drop_frames = 4;
-        frames_per_10mins = 35964;
-    } else
-        return framenum;
+    if (fps < 30 || fps % 30 != 0) return framenum;
+
+    factor = fps / 30;
+    drop_frames = factor * 2;
+    frames_per_10mins = factor * 17982;
 
     d = framenum / frames_per_10mins;
     m = framenum % frames_per_10mins;
@@ -141,10 +138,12 @@ 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_multiples[] = {24, 25, 30};
+
+    if (fps <= 0) return -1;
 
-    for (i = 0; i < FF_ARRAY_ELEMS(supported_fps); i++)
-        if (fps == supported_fps[i])
+    for (i = 0; i < FF_ARRAY_ELEMS(supported_fps_multiples); i++)
+        if (fps % supported_fps_multiples[i] == 0)
             return 0;
     return -1;
 }
@@ -155,8 +154,8 @@ static int check_timecode(void *log_ctx, AVTimecode *tc)
         av_log(log_ctx, AV_LOG_ERROR, "Timecode frame rate must be specified\n");
         return AVERROR(EINVAL);
     }
-    if ((tc->flags & AV_TIMECODE_FLAG_DROPFRAME) && tc->fps != 30 && tc->fps != 60) {
-        av_log(log_ctx, AV_LOG_ERROR, "Drop frame is only allowed with 30000/1001 or 60000/1001 FPS\n");
+    if ((tc->flags & AV_TIMECODE_FLAG_DROPFRAME) && tc->fps % 30 != 0) {
+        av_log(log_ctx, AV_LOG_ERROR, "Drop frame is only allowed in frame rates evenly divisible by 30 FPS\n");
         return AVERROR(EINVAL);
     }
     if (check_fps(tc->fps) < 0) {
@@ -201,9 +200,9 @@ int av_timecode_init_from_string(AVTimecode *tc, AVRational rate, const char *st
     }
 
     memset(tc, 0, sizeof(*tc));
-    tc->flags = c != ':' ? AV_TIMECODE_FLAG_DROPFRAME : 0; // drop if ';', '.', ...
-    tc->rate  = rate;
-    tc->fps   = fps_from_frame_rate(rate);
+    tc->flags  = c != ':' ? AV_TIMECODE_FLAG_DROPFRAME : 0; // drop if ';', '.', ...
+    tc->rate   = rate;
+    tc->fps    = fps_from_frame_rate(rate);
 
     ret = check_timecode(log_ctx, tc);
     if (ret < 0)
-- 
1.8.5.2 (Apple Git-48)