[FFmpeg-devel] [PATCH] #define LAVF_API_MAX_STREAMS

[Ronald S. Bultje]

Hi,

On Aug 17, 2010, at 4:37 AM, Aurelien Jacobs <aurel at gnuage.org> wrote:

> On Tue, Aug 17, 2010 at 01:23:17AM +0100, M?ns Rullg?rd wrote:
>> Aurelien Jacobs <aurel at gnuage.org> writes:
>> 
>>> Hi,
>>> 
>>> We currently have quite a few new API (or API deprecation) in lavf/lavc
>>> which are waiting for a major version bump to be fully enabled. Some of
>>> those new API are fully functionnal and ready to be used, some of them
>>> are work in progress that don't even compile for now (eg. AVPalette
>>> removal).
>>> All of them are currently protected by some check like:
>>>  #if LIBAVFORMAT_VERSION_MAJOR < 53
>>> So we now have quite of mess of MAJOR version check all around and it's
>>> not always easy to say which one match which API change. Moreover, it's
>>> not easy to enable just one of those API change to conduct some tests.
>>> Just bumping the major lead to a non-compilable source tree.
>>> 
>>> So I propose to add one new define for each API transition we are
>>> working on. This would allow to trivially test each transition
>>> individually, and also to trivially re-schedule an (incomplete)
>>> transition to next major bump when bumping major.
>>> 
>>> For example this allows testing the API transition with something like:
>>>  ./configure --extra-cflags=-DLAVF_API_MAX_STREAMS=0
>>> 
>>> Attached patch does this for the currently worked on MAX_STREAMS
>>> transition, and I plan to propose patches for other transitions if this
>>> one is accepted.
>>> 
>>> Aurel
>>> 
>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>>> index 1f08d1a..afcb344 100644
>>> --- a/libavformat/avformat.h
>>> +++ b/libavformat/avformat.h
>>> @@ -35,6 +35,10 @@
>>> 
>>> #define LIBAVFORMAT_IDENT       "Lavf" AV_STRINGIFY(LIBAVFORMAT_VERSION)
>>> 
>>> +#ifndef LAVF_API_MAX_STREAMS
>>> +#define LAVF_API_MAX_STREAMS  (LIBAVFORMAT_VERSION_MAJOR < 53)
>>> +#endif
>>> +
>>> /**
>>>  * I return the LIBAVFORMAT_VERSION_INT constant.  You got
>>>  * a fucking problem with that, douchebag?
>>> @@ -622,7 +624,7 @@ typedef struct AVChapter {
>>>     AVMetadata *metadata;
>>> } AVChapter;
>>> 
>>> -#if LIBAVFORMAT_VERSION_MAJOR < 53
>>> +#if LAVF_API_MAX_STREAMS
>>> #define MAX_STREAMS 20
>>> #endif
>> 
>> I dislike the idea of putting transitional hacks in the public
>> headers.
> 
> What do you call transitional hacks ?
> The #if LIBAVFORMAT_VERSION_MAJOR < 53 all over in public header is as
> much of a transitional hack IMO.
> 
>> I also doubt the usefulness of this in the first place.
> 
> There are many potential use of this.
> First let me show an example of what we could have in the end:
> 
> #define LAVF_API_MAX_STREAMS   (LIBAVFORMAT_VERSION_MAJOR < 53)
> #define LAVF_API_OLD_METADATA  (LIBAVFORMAT_VERSION_MAJOR < 53)
> #define LAVF_API_OLD_SEEK      (LIBAVFORMAT_VERSION_MAJOR < 54)
> #define LAVF_API_PALETTE       (LIBAVFORMAT_VERSION_MAJOR < 54)
> 
> The transitions which are ready and fully functional would depend on
> next major bump, and the one which are work in progress would depend on
> next+1.
> We could promote a transition from next+1 to next when it is finally
> working, by just changing this one line, instead of having to grep all
> over the tree and decide which LIBAVFORMAT_VERSION_MAJOR check belong to
> this peculiar transition. We could also demote a transition from next to
> next+1 at any time if we decide it not ready.
> It would then become easy to always be in a state where we can decide to
> bump major at any time without the fear to break anything and without
> having to review every single LIBAVFORMAT_VERSION_MAJOR check to decide
> which one is ready and which one must be postponed.
> This may also allow to have a fate machine dedicated to test those API
> transitions to ensure they always stay functional, instead of having
> some dead code that is never test-compiled.
> Right now, we can't even test transitions because bumping major just
> don't work (some transitions are incomplete and don't compile).
> 
> Overall, this is meant to ease API transition an major bump, and to
> increase tests code coverage.

Your patch doesn't do any of the above yet. Are you volunteering? :-). They are good ideas.

Ronald