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

[Stefano Sabatini]

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.

Still kept, but I can change it if you insist (or you can change back
later if that saves time).

> 
> > +
> >  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.

Yes and true, it is now simpler and easier.

> 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.

Changed to:
dumo_extra+a,mp4toannexb+v.

> 
> > +
> >  static const AVClass tee_muxer_class = {
> >      .class_name = "Tee muxer",
> >      .item_name  = av_default_item_name,
> >      .version    = LIBAVUTIL_VERSION_INT,
> > +    .option     = tee_options,
> >  };
> >  
> >  static int parse_slave_options(void *log, char *slave,
> > @@ -82,7 +96,93 @@ fail:
> >      return ret;
> >  }
> >  
> > -static int open_slave(AVFormatContext *avf, char *slave, AVFormatContext **ravf)
> > +/**
> > + * Parse bitstream filter, in the form:
> > + * BSF ::= BSF_NAME[:STREAM_SPECIFIER]
> > + */
> > +static int parse_bsf(void *log_ctx, char *bsf,
> > +                     AVFormatContext *fmt_ctx,
> > +                     AVBitStreamFilterContext **bsf_ctxs)
> > +{
> > +    int i, ret = 0;
> > +    char *bsf1 = av_strdup(bsf);
> > +    char *bsf_name, *spec;
> > +
> 
> > +    if (!bsf1)
> > +        return AVERROR(ENOMEM);
> 

> Maybe strdup the whole string once and for all, so you can butcher it all
> you want without bothering about it?

Not sure what you mean, I'm already duping it.

> 
> > +    bsf_name = av_strtok(bsf1, ":", &spec);
> > +
> > +    /* select the streams matched by the specifier */
> > +    for (i = 0; i < fmt_ctx->nb_streams; i++) {
> > +        AVBitStreamFilterContext *bsf_ctx;
> > +
> > +        if (spec && spec[0]) {
> > +            ret = avformat_match_stream_specifier(fmt_ctx, fmt_ctx->streams[i], spec);
> > +            if (ret < 0) {
> > +                av_log(log_ctx, AV_LOG_ERROR,
> > +                       "Invalid stream specifier for bitstream filter '%s'\n", bsf);
> > +                goto end;
> > +            }
> > +
> > +            if (ret == 0) /* no match */
> > +                continue;
> > +        }
> > +
> 
> > +        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.
> 
[...]
> > +static void log_slave(TeeSlave *slave, void *log_ctx, int log_level)
> > +{
> > +    int i;
> > +    av_log(log_ctx, log_level, "filename:'%s' format:%s\n",
> > +           slave->fmt_ctx->filename, slave->fmt_ctx->oformat->name);
> > +    for (i = 0; i < slave->fmt_ctx->nb_streams; i++) {
> > +        AVStream *st = slave->fmt_ctx->streams[i];
> > +        AVBitStreamFilterContext *bsf_ctx = slave->bsf_ctxs[i];
> > +
> > +        av_log(log_ctx, log_level, "    stream:%d codec:%s type:%s",
> > +               i, avcodec_get_name(st->codec->codec_id),
> > +               st->codec->codec ?
> > +               av_get_media_type_string(st->codec->codec->type) : "copy");
> > +        if (bsf_ctx) {
> 
> > +            int first = 1;
> > +            av_log(log_ctx, log_level, " bsfs:");
> > +            while (bsf_ctx) {
> > +                av_log(log_ctx, log_level, "%s%s", first ? "" : ",",
> > +                       bsf_ctx->filter->name);
> > +                bsf_ctx = bsf_ctx->next;
> > +                first = 0;
> 
> 
> You could do slightly simpler with either these solutions:
> 
>   av_log(..., "%s%s", name, bsf_ctx->next ? "," : "");
> 
>   const char *delim = "";
>   av_log(..., "%s%s", delim, name);
>   delim = ",";

Done.

[...] 

Patch updated with documentation.
-- 
FFmpeg = Faithful Fancy Mastodontic Proud Elitarian Glue
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lavf-tee-add-support-for-bitstream-filtering.patch
Type: text/x-diff
Size: 12077 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130731/5c2cb705/attachment.bin>