[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