[FFmpeg-devel] [PATCH] lavf/tee: add support for bitstream filtering
Nicolas George
nicolas.george at normalesup.org
Thu Jul 18 12:30:23 CEST 2013
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.
> +
> 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.
> +
> 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?
> + 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.
> + if (!bsf_ctx) {
> + av_log(log_ctx, AV_LOG_ERROR,
> + "Cannot create bitstream filter with name '%s' for stream %d, "
> + "unknown filter or internal error happened\n",
> + bsf_name, i);
> + ret = AVERROR_UNKNOWN;
> + goto end;
> + }
> +
> + /* append bsf context to the list of per-stream bsf contexts */
> + if (!bsf_ctxs[i]) {
> + bsf_ctxs[i] = bsf_ctx;
> + } else {
> + AVBitStreamFilterContext *bsf_ctx1 = bsf_ctxs[i];
> + while (bsf_ctx1->next)
> + bsf_ctx1 = bsf_ctx1->next;
> + bsf_ctx1->next = bsf_ctx;
> + }
> + }
> +
> +end:
> + av_free(bsf1);
> + return ret;
> +}
> +
> +/**
> + * Parse list of bitstream filters in the form:
> + * BSFS ::= BSF[,BSFS]
> + * BSF ::= BSF_NAME[:STREAM_SPECIFIER]
> + */
> +static int parse_slave_bsfs(void *log_ctx, char *slave_bsfs,
> + AVFormatContext *fmt_ctx,
> + AVBitStreamFilterContext **bsf_ctx)
> +{
> + char *bsf, *slave_bsfs1 = av_strdup(slave_bsfs);
> + char *saveptr, *buf = slave_bsfs1;
> + int ret;
> +
> + if (!slave_bsfs1)
> + return AVERROR(ENOMEM);
> +
> + while (bsf = av_strtok(buf, ",", &saveptr)) {
> + buf = NULL;
> + ret = parse_bsf(log_ctx, bsf, fmt_ctx, bsf_ctx);
> + if (ret < 0)
> + goto end;
> + }
> +
> +end:
> + av_free(slave_bsfs1);
> + return ret;
> +}
> +
> +static int open_slave(AVFormatContext *avf, char *slave, char *slave_bsfs, TeeSlave *tee_slave)
> {
> int i, ret;
> AVDictionary *options = NULL;
> @@ -138,6 +238,7 @@ static int open_slave(AVFormatContext *avf, char *slave, AVFormatContext **ravf)
> slave, av_err2str(ret));
> goto fail;
> }
> +
> if (options) {
> entry = NULL;
> while ((entry = av_dict_get(options, "", entry, AV_DICT_IGNORE_SUFFIX)))
> @@ -146,7 +247,16 @@ static int open_slave(AVFormatContext *avf, char *slave, AVFormatContext **ravf)
> goto fail;
> }
>
> - *ravf = avf2;
> + tee_slave->fmt_ctx = avf2;
> + tee_slave->bsf_ctxs = av_mallocz(avf2->nb_streams * sizeof(*(tee_slave->bsf_ctxs)));
> + if (!tee_slave->bsf_ctxs)
> + return AVERROR(ENOMEM);
> +
> + /* generate the array of bitstream filters */
> + if (slave_bsfs) {
> + if ((ret = parse_slave_bsfs(avf, slave_bsfs, avf2, tee_slave->bsf_ctxs)) < 0)
> + return ret;
> + }
> return 0;
>
> fail:
> @@ -158,14 +268,50 @@ static void close_slaves(AVFormatContext *avf)
> {
> TeeContext *tee = avf->priv_data;
> AVFormatContext *avf2;
> - unsigned i;
> + unsigned i, j;
>
> for (i = 0; i < tee->nb_slaves; i++) {
> - avf2 = tee->slaves[i];
> + avf2 = tee->slaves[i].fmt_ctx;
> +
> + for (j = 0; j < avf2->nb_streams; j++) {
> + AVBitStreamFilterContext *bsf_ctx = tee->slaves[i].bsf_ctxs[j];
> + while (bsf_ctx) {
> + bsf_ctx = bsf_ctx->next;
> + av_bitstream_filter_close(bsf_ctx);
> + }
> + }
> +
> avio_close(avf2->pb);
> avf2->pb = NULL;
> avformat_free_context(avf2);
> - tee->slaves[i] = NULL;
> + tee->slaves[i].fmt_ctx = NULL;
> + }
> +}
> +
> +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 = ",";
> + }
> + }
> + av_log(log_ctx, log_level, "\n");
> }
> }
>
> @@ -174,7 +320,9 @@ static int tee_write_header(AVFormatContext *avf)
> TeeContext *tee = avf->priv_data;
> unsigned nb_slaves = 0, i;
> const char *filename = avf->filename;
> - char *slaves[MAX_SLAVES];
> + const char *bsfs = tee->bsfs;
> + char *slaves [MAX_SLAVES];
> + char *slave_bsfs[MAX_SLAVES] = { NULL };
> int ret;
>
> while (*filename) {
> @@ -192,10 +340,29 @@ static int tee_write_header(AVFormatContext *avf)
> filename++;
> }
>
> + i = 0;
> + while (bsfs && *bsfs) {
> + if (i >= nb_slaves) {
> + av_log(avf, AV_LOG_ERROR,
> + "%d bitstream filter sequences are specified, but only %d slaves "
> + "were specified, aborting\n", i+1, nb_slaves);
> + ret = AVERROR(EINVAL);
> + goto fail;
> + }
> + if (!(slave_bsfs[i++] = av_get_token(&bsfs, slave_delim))) {
> + ret = AVERROR(ENOMEM);
> + goto fail;
> + }
> + if (strspn(bsfs, slave_delim))
> + bsfs++;
> + }
> +
> for (i = 0; i < nb_slaves; i++) {
> - if ((ret = open_slave(avf, slaves[i], &tee->slaves[i])) < 0)
> + if ((ret = open_slave(avf, slaves[i], slave_bsfs[i], &tee->slaves[i])) < 0)
> goto fail;
> + log_slave(&tee->slaves[i], avf, AV_LOG_VERBOSE);
> av_freep(&slaves[i]);
> + av_freep(&slave_bsfs[i]);
> }
>
> tee->nb_slaves = nb_slaves;
> @@ -208,6 +375,48 @@ fail:
> return ret;
> }
>
> +static int filter_packet(void *log_ctx, AVPacket *pkt,
> + AVCodecContext *enc_ctx, AVBitStreamFilterContext *bsf_ctx)
> +{
> + while (bsf_ctx) {
> + AVPacket new_pkt = *pkt;
> + int ret = av_bitstream_filter_filter(bsf_ctx, enc_ctx, NULL,
> + &new_pkt.data, &new_pkt.size,
> + pkt->data, pkt->size,
> + pkt->flags & AV_PKT_FLAG_KEY);
> + if (ret == 0 && new_pkt.data != pkt->data && new_pkt.destruct) {
> + uint8_t *buf = av_malloc(new_pkt.size + FF_INPUT_BUFFER_PADDING_SIZE);
> + if (buf) {
> + memcpy(buf, new_pkt.data, new_pkt.size);
> + memset(buf + new_pkt.size, 0, FF_INPUT_BUFFER_PADDING_SIZE);
> + new_pkt.data = buf;
> + new_pkt.buf = NULL;
> + ret = 1;
> + } else
> + ret = AVERROR(ENOMEM);
> + }
> +
> + if (ret > 0) {
> + av_free_packet(pkt);
> + new_pkt.buf = av_buffer_create(new_pkt.data, new_pkt.size,
> + av_buffer_default_free, NULL, 0);
> + if (!new_pkt.buf)
> + return AVERROR(ENOMEM);
> + } else if (ret < 0) {
> + av_log(log_ctx, AV_LOG_ERROR,
> + "Failed to filter bitstream %s for stream %d with codec %s",
> + bsf_ctx->filter->name, pkt->stream_index,
> + enc_ctx->codec ? enc_ctx->codec->name : "copy");
> + return ret;
> + }
> + *pkt = new_pkt;
> +
> + bsf_ctx = bsf_ctx->next;
> + }
> +
> + return 0;
> +}
> +
> 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.
Regards,
--
Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130718/e15e59db/attachment.asc>
More information about the ffmpeg-devel
mailing list