[FFmpeg-devel] [PATCH 2/2] Move AVSubtitle from lavc to lavu

[wm4]

On Mon, 15 Dec 2014 11:38:58 +0100
Clément Bœsch <u at pkh.me> wrote:

> On Mon, Dec 15, 2014 at 10:49:47AM +0100, wm4 wrote:
> > On Mon, 15 Dec 2014 10:38:01 +0100
> > Clément Bœsch <u at pkh.me> wrote:
> > 
> > > On Mon, Dec 15, 2014 at 10:23:33AM +0100, wm4 wrote:
> > > > On Sun, 14 Dec 2014 16:47:44 +0100
> > > > Clément Bœsch <u at pkh.me> wrote:
> > > > 
> > > > > ---
> > > > >  doc/APIchanges       |   5 +++
> > > > >  libavcodec/avcodec.h |  63 +++---------------------------
> > > > >  libavcodec/utils.c   |  29 +++-----------
> > > > >  libavcodec/version.h |   3 ++
> > > > >  libavutil/Makefile   |   2 +
> > > > >  libavutil/subtitle.c |  62 +++++++++++++++++++++++++++++
> > > > >  libavutil/subtitle.h | 107 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  7 files changed, 191 insertions(+), 80 deletions(-)
> > > > >  create mode 100644 libavutil/subtitle.c
> > > > >  create mode 100644 libavutil/subtitle.h
> > > > > 
> > > > > diff --git a/doc/APIchanges b/doc/APIchanges
> > > > > index ba7eae1..989794a 100644
> > > > > --- a/doc/APIchanges
> > > > > +++ b/doc/APIchanges
> > > > > @@ -15,6 +15,11 @@ libavutil:     2014-08-09
> > > > >  
> > > > >  API changes, most recent first:
> > > > >  
> > > > > +2014-12-xx - xxxxxxx - lavc 56.16.100 / lavu 54.17.100 - lavc/avcodec.h lavu/picture.h
> > > > > +  Move AVSubtitle definition from libavcodec to libavutil and add
> > > > > +  av_subtitle_*() functions which should be used instead of stack allocation
> > > > > +  and the now deprecated avsubtitle_free().
> > > > > +
> > > > 
> > > > "should"?
> > > > 
> > > > Can this requirement be lifted/delayed until there's actually a reason
> > > > to do so?
> > > 
> > > The structure is unchanged for now, so yeah it's "should" (as a prevention
> > > for the future). I don't plan to add a field that soon, but it's highly
> > > recommended. Maybe I can change the "should" into "must" and saying that
> > > for now it has no impact, but will in the future.
> > > 
> > > > At that point, the struct should probably be renamed, to
> > > > avoid turning valid code into subtly broken code merely by adding API
> > > > compatible implementation details.
> > > 
> > > There are actually 2 changes in this code, and maybe I should split. But
> > > basically the addition of the functions is kind of unrelated to the move.
> > > It's just a security for us for later, when we will be willing to add a
> > > field (if we ever do).
> > > 
> > > We should have created these functions a long time ago, but the fact that
> > > AVSubtitle actually belong into libavutil prevented us from doing it:
> > > basically, if we had added these helpers, the move to libavutil would have
> > > been way more complicated (moving a structure definition is way easier
> > > than moving a structure definition AND its functions).
> > > 
> > > [...]
> > > 
> > 
> > My point is: barely anyone (applications) will notice this API change.
> > It will go unnoticed, or there will be no perceived need to update the
> > code. And then when you actually extend this stuff, you will start
> > breaking application code that worked before.
> 
> What do you suggest to make people notice the API change? Currently
> AVSubtitle has one single function, which I deprecated.

That could be ok. You can't use the current API without using
avsubtitle_free(). (Although I'd still prefer something more "explicit".)
The deprecated function must be completely removed as soon as using the
new functions is mandatory.

> So you think we should introduce a AVSubtitle2 into libavutil instead? I
> don't like that that much TBH, especially since it will require me to name
> the new functions av_subtitle2_* to make it consistent.
>