[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