[FFmpeg-devel] [PATCH] #define LAVF_API_MAX_STREAMS

[Aurelien Jacobs]

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.

Aurel