[FFmpeg-devel] [PATCH] ffmpeg: use AVBPrint API for filter args.

[Nicolas George]

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.

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

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

Unfortunately, both the original code and your modified version fails to
check whether the string was truncated.

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

>      }
>  
> +    av_bprint_finalize(&filter_args, NULL);

No need to finalize if the buffer is automatic.

>      return 0;
>  }

Regards,

-- 
  Nicolas George