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

[wm4]

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.

This is how it always goes. Even ffmpeg.org still links tutorials based
on ancient practices that over time became invalid, but everyone keeps
doing it because it still appears to work (but crashes in slightly
different situations).

So I think such "implicit" changes should be avoided. If the API
breaks, make sure applications using the old assumptions won't compile.
(There are various ways to achieve this goal; I'm just saying that it
should be a goal.)