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

[Clément Bœsch]

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,

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150124/1e6856eb/attachment.asc>