[FFmpeg-devel] [PATCH 1/2] avutil: Add av_get_time_base{_q, }() and deprecate AV_TIME_BASE{_Q, }

[wm4]

On Mon, 30 Dec 2013 05:33:19 +0000
Derek Buitenhuis <derek.buitenhuis at gmail.com> wrote:

> On 12/30/2013 1:58 AM, Michael Niedermayer wrote:
> > On Sun, Dec 29, 2013 at 11:35:37PM +0000, Derek Buitenhuis wrote:
> >> It was never a good idea to have tehse values as defines in the first place,
> >> since they are internal values, and may change.
> > 
> > they start with a AV_ prefix and are in a public header, so the
> > statement that they ARE "internal values, and may change" is
> > certainly not correct.
> 
> Their own documentation begs to differ:
> 
>     /**
>      * Internal time base represented as integer
>      */
> 
>     [...]
> 
>     /**
>      * Internal time base represented as fractional value
>      */
> 
> Internal in the sense of "we use this time base internally", and not
> "for internal use only". The user doesn't need to know the specific
> value, or what the define is, in theory.
> 
> >> This also fixes the impossibility of using AV_TIME_BASE_Q in pre-C99/C++
> >> applications, which cannot use compound literals directly in their code.
> > 
> > that explains the addition of a function returning the timebase,
> > i agree that this part (or something equivalent) is needed
> > 
> > I dont understand why the defines are deprecated though, can you
> > elaborate on why you change that ?
> 
> The obvious reason is that it's pretty stupid to have two ways to
> access/do the same thing.
> 
> The other reason is that it is poor practice to have downstream
> user code hardcoding values, since if we ever needed to change
> the internal timebase, it would cause very ugly ABI issues pretty
> easily if mishandled.

Do you plan the change the timebase any time soon? And is changing the
time base once really a reason to change this from a compile time
constant to a function call? You could just change the timebase on a
major bump, if you have to.

If you really hate hardcoded values, why not add an actual timebase
field to all places where AV_TIME_BASE is currently used?