[FFmpeg-devel] [PATCH] lavf/tee: add support for bitstream filtering

Stefano Sabatini stefasab at gmail.com
Thu Jul 18 13:44:27 CEST 2013


On date Thursday 2013-07-18 12:30:23 +0200, Nicolas George encoded:
> L'octidi 18 messidor, an CCXXI, Stefano Sabatini a écrit :
> > TODO: add documentation, bump minor
> > ---
> >  libavformat/tee.c | 231 +++++++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 221 insertions(+), 10 deletions(-)
> > 
> > diff --git a/libavformat/tee.c b/libavformat/tee.c
> > index 7b8b371..96ced3e 100644
> > --- a/libavformat/tee.c
> > +++ b/libavformat/tee.c
> > @@ -27,10 +27,16 @@
> >  
> >  #define MAX_SLAVES 16
> >  
> > +typedef struct {
> > +    AVFormatContext *fmt_ctx;
> > +    AVBitStreamFilterContext **bsf_ctxs; ///< bitstream filters per stream
> > +} TeeSlave;
> 

> Do you insist on the _ctx suffix? To me, they only make the lines of code
> longer.

What do you suggest instead? fctx? I personally hate the confusion
between object/object context, but not that I mind so much (especially
if the code is authored by someone else than me).

> > +
> >  typedef struct TeeContext {
> >      const AVClass *class;
> >      unsigned nb_slaves;
> > -    AVFormatContext *slaves[MAX_SLAVES];
> > +    TeeSlave slaves[MAX_SLAVES];
> > +    char *bsfs;
> >  } TeeContext;
> >  
> >  static const char *const slave_delim     = "|";
> > @@ -38,10 +44,18 @@ static const char *const slave_opt_open  = "[";
> >  static const char *const slave_opt_close = "]";
> >  static const char *const slave_opt_delim = ":]"; /* must have the close too */
> >  
> 
> > +#define OFFSET(x) offsetof(TeeContext, x)
> > +#define E AV_OPT_FLAG_ENCODING_PARAM
> > +static const AVOption tee_options[] = {
> > +    { "tee_bsfs", "set bitstream filters for each slave", OFFSET(bsfs), AV_OPT_TYPE_STRING, {.str = NULL}, .flags=E },
> > +    { NULL },
> > +};
> 
> I believe you can make this feature much simpler and also more user-friendly
> by using the existing options system.
> 
> Your suggestion currently looks like that:
> 
> -f tee -tee_bsfs 'annexbtomp4|dump_extra' 'foo.mp4|foo.ts'
> 
> it would look like that:
> 
> -f tee '[bsfs=annexbtomp4]foo.mp4|[bsfs=dump_extra]foo.ts'
> 

> I like it better because the bsfs are nearer the corresponding file: if you
> change the order of the slaves, you do not risk to forget changing the order
> of the filters. It is also nicer when only one slave has filters and it is
> not the first.
> 
> You just have to imitate the av_dict_get(..."f") part.
> 
> Also, you could put the streams specifiers on the bsfs option rather than
> the bsfs names (which is strange when there are several filters, what does
> "dump_extra:a,mp4toannexb:v" mean?), that would make the parsing easier.

Yes, but...

this leads to the namespace problem which was never properly handled,
if we had a bsf option to any muxer then it will shadow the option.

[...]
> > +        bsf_ctx = av_bitstream_filter_init(bsf_name);
> 
> Your parser does not allow to specify options if stream specifiers are in
> place ("dump_extra:v=a"). That is the reason I suggest to put streams
> specifiers on the option rather than the filter.

My idea was indeed:
dump_extra:v=args

Since we currently don't support bsf options in ffmpeg, I didn't
bother to support them here.

[...]
> >  static int tee_write_trailer(AVFormatContext *avf)
> >  {
> >      TeeContext *tee = avf->priv_data;
> > @@ -216,7 +425,7 @@ static int tee_write_trailer(AVFormatContext *avf)
> >      unsigned i;
> >  
> >      for (i = 0; i < tee->nb_slaves; i++) {
> > -        avf2 = tee->slaves[i];
> > +        avf2 = tee->slaves[i].fmt_ctx;
> >          if ((ret = av_write_trailer(avf2)) < 0)
> >              if (!ret_all)
> >                  ret_all = ret;
> > @@ -241,7 +450,7 @@ static int tee_write_packet(AVFormatContext *avf, AVPacket *pkt)
> >      AVRational tb, tb2;
> >  
> >      for (i = 0; i < tee->nb_slaves; i++) {
> > -        avf2 = tee->slaves[i];
> > +        avf2 = tee->slaves[i].fmt_ctx;
> >          s = pkt->stream_index;
> >          if (s >= avf2->nb_streams) {
> >              if (!ret_all)
> > @@ -259,6 +468,8 @@ static int tee_write_packet(AVFormatContext *avf, AVPacket *pkt)
> >          pkt2.pts      = av_rescale_q(pkt->pts,      tb, tb2);
> >          pkt2.dts      = av_rescale_q(pkt->dts,      tb, tb2);
> >          pkt2.duration = av_rescale_q(pkt->duration, tb, tb2);
> > +
> > +        filter_packet(avf2, &pkt2, avf2->streams[s]->codec, tee->slaves[i].bsf_ctxs[s]);
> >          if ((ret = av_interleaved_write_frame(avf2, &pkt2)) < 0)
> >              if (!ret_all)
> >                  ret_all = ret;
> 

> Now, from the design point of view, I am afraid this is the first step in a
> slippery slope that leads to reimplementing one third of ffmpeg inside the
> tee muxer, and I suspect we do not want that.
> 
> Since you resurrected the discussion, I have been thinking again about
> support for data streams in lavfi, and I believe it is actually quite easy.
> If implemented, the tee muxer would be redundant with the split filter and
> muxer sink, and bitstream filters are just data filters. That also solves
> the problems you raise in another mail about selecting which streams go to
> which slave.
> 

> Filtering data is less hype than filtering subtitles, but it is much easier.
> Also, filtering data is useful for subtitles (think embedded fonts).
> 
> I can post the state of my thinking if you are interested.

Sure, I'm aware of the problem. On the other hand I need a solution in
the short term, and under these conditions I prefer the proverbial egg
today rather than a chicken tomorrow, especially considering that a
proper design/discussion/implementation would probably require
weeks/months rather than days.

You're welcome to share your thougths about the lavfi data path.
-- 
FFmpeg = Fiendish & Foolish Minimal Perfectionist Earthshaking Guide


More information about the ffmpeg-devel mailing list