[FFmpeg-devel] [PATCH] ffmpeg: use AVBPrint API for filter args.
Clément Bœsch
ubitux at gmail.com
Fri May 25 10:29:52 CEST 2012
On Fri, May 25, 2012 at 10:11:19AM +0200, Nicolas George wrote:
> Le septidi 7 prairial, an CCXX, Clément Bœsch a écrit :
> > + av_bprint_init(&filter_args, 256, 2048);
> > +
>
> Why the strange values? 1 for automatic size seems saner.
>
No particular reason; 256 was the original args size, and 2048 an
arbitrary up limit: I guess tens of -map_channel can reach it, and OTOH an
unlimited maximum sounds unreasonable.
> > avfilter_graph_free(&fg->graph);
> > if (!(fg->graph = avfilter_graph_alloc()))
> > return AVERROR(ENOMEM);
> >
> > - snprintf(args, sizeof(args), "time_base=%d/%d:sample_rate=%d:sample_fmt=%s:"
> > - "channel_layout=0x%"PRIx64, ist->st->time_base.num,
> > - ist->st->time_base.den, icodec->sample_rate,
> > - av_get_sample_fmt_name(icodec->sample_fmt), icodec->channel_layout);
> > + av_bprintf(&filter_args, "time_base=%d/%d:sample_rate=%d:sample_fmt=%s:"
> > + "channel_layout=0x%"PRIx64, ist->st->time_base.num,
> > + ist->st->time_base.den, icodec->sample_rate,
> > + av_get_sample_fmt_name(icodec->sample_fmt), icodec->channel_layout);
> > ret = avfilter_graph_create_filter(&fg->inputs[0]->filter,
> > avfilter_get_by_name("abuffer"),
> > - "src", args, NULL, fg->graph);
> > + "src", filter_args.str, NULL, fg->graph);
> > if (ret < 0)
> > return ret;
>
> For a fixed format with a bounded length, is it worth the divergence wrt the
> fork?
>
Doing so would mean keeping both args and filter_args buffers. If Michael
think it will be a pain to keep both in sync, I don't mind much retracting
this patch (thought I like the overall simplification, and it's more
flexible to insert several other filters if needed).
> >
> > @@ -841,19 +843,13 @@ static int configure_audio_filters(FilterGraph *fg, AVFilterContext **in_filter,
> > channel_layouts = choose_channel_layouts(ost);
> > if (sample_fmts || sample_rates || channel_layouts) {
> > AVFilterContext *format;
> > - char args[256];
> > - int len = 0;
> >
> > - if (sample_fmts)
> > - len += snprintf(args + len, sizeof(args) - len, "sample_fmts=%s:",
> > - sample_fmts);
> > - if (sample_rates)
> > - len += snprintf(args + len, sizeof(args) - len, "sample_rates=%s:",
> > - sample_rates);
> > - if (channel_layouts)
> > - len += snprintf(args + len, sizeof(args) - len, "channel_layouts=%s:",
> > - channel_layouts);
> > - args[len - 1] = 0;
> > + av_bprint_clear(&filter_args);
> > + if (sample_fmts) av_bprintf(&filter_args, "sample_fmts=%s:", sample_fmts);
> > + if (sample_rates) av_bprintf(&filter_args, "sample_rates=%s:", sample_rates);
> > + if (channel_layouts) av_bprintf(&filter_args, "channel_layouts=%s:", channel_layouts);
> > + filter_args.len--;
> > + filter_args.str[filter_args.len] = 0;
>
> That is rather ugly, but that was in the original code, not yours.
>
Since that scope triggers at least one of the av_bprintf(), it looked like
the simpler way to do that operation.
> Unfortunately, both the original code and your modified version fails to
> check whether the string was truncated.
>
Truncated? What do you mean?
> >
> > av_freep(&sample_fmts);
> > av_freep(&sample_rates);
> > @@ -861,7 +857,8 @@ static int configure_audio_filters(FilterGraph *fg, AVFilterContext **in_filter,
> >
> > ret = avfilter_graph_create_filter(&format,
> > avfilter_get_by_name("aformat"),
> > - "aformat", args, NULL, fg->graph);
> > + "aformat", filter_args.str,
> > + NULL, fg->graph);
> > if (ret < 0)
> > return ret;
> >
> > @@ -872,15 +869,16 @@ static int configure_audio_filters(FilterGraph *fg, AVFilterContext **in_filter,
> > *out_filter = format;
> > }
> >
> > -#define AUTO_INSERT_FILTER(opt_name, filter_name, arg) do { \
> > +#define AUTO_INSERT_FILTER(opt_name, filter_name) do { \
> > AVFilterContext *filt_ctx; \
> > \
> > av_log(NULL, AV_LOG_INFO, opt_name " is forwarded to lavfi " \
> > - "similarly to -af " filter_name "=%s.\n", arg); \
> > + "similarly to -af " filter_name "=%s.\n", filter_args.str); \
> > \
> > ret = avfilter_graph_create_filter(&filt_ctx, \
> > avfilter_get_by_name(filter_name), \
> > - filter_name, arg, NULL, fg->graph); \
> > + filter_name, filter_args.str, \
> > + NULL, fg->graph); \
> > if (ret < 0) \
> > return ret; \
> > \
> > @@ -892,36 +890,32 @@ static int configure_audio_filters(FilterGraph *fg, AVFilterContext **in_filter,
> > } while (0)
> >
> > if (audio_sync_method > 0) {
> > - char args[256] = {0};
> > -
> > - av_strlcatf(args, sizeof(args), "min_comp=0.001:min_hard_comp=%f", audio_drift_threshold);
> > + av_bprint_clear(&filter_args);
> > + av_bprintf(&filter_args, "min_comp=0.001:min_hard_comp=%f", audio_drift_threshold);
> > if (audio_sync_method > 1)
> > - av_strlcatf(args, sizeof(args), ":max_soft_comp=%f", audio_sync_method/(double)icodec->sample_rate);
> > - AUTO_INSERT_FILTER("-async", "aresample", args);
> > + av_bprintf(&filter_args, ":max_soft_comp=%f", audio_sync_method/(double)icodec->sample_rate);
> > + AUTO_INSERT_FILTER("-async", "aresample");
> > }
> >
> > if (ost->audio_channels_mapped) {
> > int i;
> > - AVBPrint pan_buf;
> >
> > - av_bprint_init(&pan_buf, 256, 8192);
> > - av_bprintf(&pan_buf, "0x%"PRIx64,
> > + av_bprint_clear(&filter_args);
> > + av_bprintf(&filter_args, "0x%"PRIx64,
> > av_get_default_channel_layout(ost->audio_channels_mapped));
> > for (i = 0; i < ost->audio_channels_mapped; i++)
> > if (ost->audio_channels_map[i] != -1)
> > - av_bprintf(&pan_buf, ":c%d=c%d", i, ost->audio_channels_map[i]);
> > -
> > - AUTO_INSERT_FILTER("-map_channel", "pan", pan_buf.str);
> > - av_bprint_finalize(&pan_buf, NULL);
> > + av_bprintf(&filter_args, ":c%d=c%d", i, ost->audio_channels_map[i]);
> > + AUTO_INSERT_FILTER("-map_channel", "pan");
> > }
> >
> > if (audio_volume != 256) {
> > - char args[256];
> > -
> > - snprintf(args, sizeof(args), "%lf", audio_volume / 256.);
> > - AUTO_INSERT_FILTER("-vol", "volume", args);
> > + av_bprint_clear(&filter_args);
> > + av_bprintf(&filter_args, "%lf", audio_volume / 256.);
> > + AUTO_INSERT_FILTER("-vol", "volume");
>
> Same remark about code divergence vs. fixed format.
>
The fork has no af volume, and thus still has the volume hack in. The only
input filter they auto insert is asyncts, and we do things differently on
that matter anyway.
> > }
> >
> > + av_bprint_finalize(&filter_args, NULL);
>
> No need to finalize if the buffer is automatic.
>
Ah that's nice to know; but since I think it's better to allow a larger
buffer (see my top comment), it still makes sense (afaik).
--
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120525/64e4b7e3/attachment.asc>
More information about the ffmpeg-devel
mailing list