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

[wm4]

On Sat, 24 Jan 2015 17:21:40 +0100
Clément Bœsch <u at pkh.me> 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.

Which ones? We even expect C99 support from the compiler.

> 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.

AFAIK only 1 person does.

> Regards,
>