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

Michael Niedermayer michaelni at gmx.at
Sat Jan 24 23:59:01 CET 2015


On Sat, Jan 24, 2015 at 12:26:49PM -0800, jon morley wrote:
> 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
> >

>  timecode.c |   33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
> af5b5d09fd78bb929dc38e2cc11e562a281d5544  0001-libavutil-timecode.c-Add-support-for-frame-rates-bey.patch
> 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);

unrelated cosmetic change

also some of this code is used in the mov muxer while mov is limited
to 8bits for one of the fields, see mov_write_tmcd_tag()
i dont think an unlimited range will work there


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150124/c580cab2/attachment.asc>


More information about the ffmpeg-devel mailing list