[FFmpeg-devel] [PATCH] #define LAVF_API_MAX_STREAMS

[Måns Rullgård]

Aurelien Jacobs <aurel at gnuage.org> writes:

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

Good points.  I guess I'm OK with that.

-- 
M?ns Rullg?rd
mans at mansr.com