[FFmpeg-devel] [PATCH 2/5] cmdutils: add insert_timeline_graph()

[Clément Bœsch]

On Tue, Jan 06, 2015 at 10:06:15PM +0100, wm4 wrote:
> On Tue, 6 Jan 2015 20:40:51 +0100
> Clément Bœsch <u at pkh.me> wrote:
> 
> > On Tue, Jan 06, 2015 at 08:28:48PM +0100, wm4 wrote:
> > > On Tue,  6 Jan 2015 18:09:58 +0100
> > > Clément Bœsch <u at pkh.me> wrote:
> > > 
> > > > From: Clément Bœsch <clement at stupeflix.com>
> > > > 
> > > > This function will be used in the following commits in ffmpeg and
> > > > ffplay.
> > > > ---
> > > >  cmdutils.c | 170 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  cmdutils.h |  12 +++++
> > > >  2 files changed, 182 insertions(+)
> > > > 
> > > > diff --git a/cmdutils.c b/cmdutils.c
> > > > index b35180e..0e22e57 100644
> > > > --- a/cmdutils.c
> > > > +++ b/cmdutils.c
> > > > @@ -31,7 +31,9 @@
> > > >  
> > > >  #include "config.h"
> > > >  #include "compat/va_copy.h"
> > > > +#include "libavcodec/bytestream.h"
> > > >  #include "libavformat/avformat.h"
> > > > +#include "libavformat/isom.h"
> > > >  #include "libavfilter/avfilter.h"
> > > >  #include "libavdevice/avdevice.h"
> > > >  #include "libavresample/avresample.h"
> > [...]
> > > > +/**
> > > > + * Get the MOV timeline from the stream side data, construct a libavfilter
> > > > + * filtergraph, and insert it after the last filter.
> > > > + *
> > > > + * @param st          the stream with the timeline
> > > > + * @param last_filter pointer to last filter to stick the filtergraph (will be updated)
> > > > + * @param start_time  initial timestamp offset in AV_TIME_BASE_Q time base
> > > > + * @param reverse     if set, prepend the timeline filtergraph instead of appending it
> > > > + */
> > > > +int insert_timeline_graph(const AVStream *st, AVFilterContext **last_filter,
> > > > +                          int64_t start_time, int reverse);
> > > > +
> > > >  #endif /* CMDUTILS_H */
> > > 
> > > So libavformat exports the raw MOV atom, and all tools ffmpeg.c uses to
> > > parse it are private libavformat/libavcodec API?? That seems very
> > > unfair to the API user.
> > 
> > libavformat/isom.h is needed only for MOVElst, which is the following:
> > 
> >     typedef struct MOVElst {
> >         int64_t duration;
> >         int64_t time;
> >         float rate;
> >     } MOVElst;
> > 
> > About the bytestream, that's kind of the same issue you will have for parsing
> > kind of any bytestream, I don't think it is really unfair since most multimedia
> > app will probably already have that.
> > 
> 
> Why not just wrap this struct?

Just because it's way less API and ABI maintenance.

Would you really prefer that I add a MOV/MP4 field to AVFormatContext,
with a public header for the elst structures, which will possibly change
anytime soon? Changes in that structure already happened with the 32 vs 64
fields, and might happen again because it's still kind of experimental -
remember someone talking about adding a "loop" feature into this the other
day? I'm not exactly willing to change these structures all the time.

I believe adding a field to AVFormatContext should be an abstract enough
and well thought API for time lines. Right now, this only support MOV/MP4.
If we happen to support that also for MKV, then we can start thinking
about such API. Right now, I took the less risky approach, that is
exporting the atom verbatim and let the end user deal with it.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150106/a4660b23/attachment.asc>