Le septidi 17 ventôse, an CCXXV, Takayuki 'January June' Suwa a écrit : > dedicated to all audiophiles :) > > examples: > -sample_rate_fallback closest -i 48ksps_source -af > "aformat=sample_rates=44100|88200|176400|352800" > => 44.1ksps (default behavior) > -sample_rate_fallback higher > => 88.2ksps > -sample_rate_fallback higher2x > => 176.4ksps > -sample_rate_fallback highest > => 352.8ksps > ---
I have not yet given much thought to the feature itself, but here are a
few technical remarks that will need to be addressed anyway it anything
like that might get in.
> doc/ffmpeg.texi | 28 ++++++++++++++++++++++++
> ffmpeg.h | 1 +
> ffmpeg_filter.c | 1 +
> ffmpeg_opt.c | 24 +++++++++++++++++++++
> libavfilter/avfilter.h | 1 +
> libavfilter/avfiltergraph.c | 52
> ++++++++++++++++++++++++++++++++++++---------
> 6 files changed, 97 insertions(+), 10 deletions(-)
I think it would be better to split the patch between libavfilter and
ffmpeg*.
Also, the libavfilter part requires a minor version bump.
>
> diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi
> index 8b08e22..6e6fb97 100644
> --- a/doc/ffmpeg.texi
> +++ b/doc/ffmpeg.texi
> @@ -1276,6 +1276,34 @@ This option is similar to @option{-filter_complex},
> the only difference is that
> its argument is the name of the file from which a complex filtergraph
> description is to be read.
>
> +@item -sample_rate_fallback @var{method} (@emph{global})
I do not think this option should be global. FFmpeg can support several
filter graphs. Possibly, the good solution may be to extend the
"sws_flags=" feature to allow to set more graph options from the graph
description.
> +Audio sample rate fallback method when no exact match found.
> +
> +Whenever the filtergraph connects its filters each other, attributes of
> output link
> +on each filter will be compared to ones of input link on the next filter. If
> an
> +attribute on the both sides have been matched exactly, it will be passed
> through. If
> +not, of course, such attribute must be reformated or resampled.
> +
> +On audio filtergraph, sample format and channel layout are reformated with
> wider
> +ones in such situation. This brings almost good result and less problem.
> However
> +for audio sample rate, there is more than one point of view about a
> @emph{good} method.
> +@table @option
> +@item 0, closest
I would rather not have the numeric values of the option exposed to the
user, only symbolic constants.
> +Audio stream will be resampled to the closest (regardless of higher or
> lower) one
> +of the next filter's available input sample rates. It is the default
> behavior and
> +preferred for saving storage capacity or network bandwidth rather than
> preserving
> +sound quality.
> +@item 1, higher
> +Resampled to the closest higher one of the next filter's available input
> +sample rates.
> +@item 2, higher2x
> +Similar to @option{higher}, but twice high because of the sampling theorem.
> +@item 3, highest
> +Resampled to the highest one of the next filter's available input sample
> rates. It
> +may be accepted when bandwidth won't be a problem, such as putting to local
> DAC
> +directly.
> +@end table
> +
> @item -accurate_seek (@emph{input})
> This option enables or disables accurate seeking in input files with the
> @option{-ss} option. It is enabled by default, so seeking is accurate when
> diff --git a/ffmpeg.h b/ffmpeg.h
> index 06a1251..13d8e16 100644
> --- a/ffmpeg.h
> +++ b/ffmpeg.h
> @@ -601,6 +601,7 @@ extern char *videotoolbox_pixfmt;
>
> extern int filter_nbthreads;
> extern int filter_complex_nbthreads;
> +extern int sample_rate_fallback;
> extern int vstats_version;
>
> extern const AVIOInterruptCB int_cb;
> diff --git a/ffmpeg_filter.c b/ffmpeg_filter.c
> index 7f249c2..182048b 100644
> --- a/ffmpeg_filter.c
> +++ b/ffmpeg_filter.c
> @@ -1045,6 +1045,7 @@ int configure_filtergraph(FilterGraph *fg)
> } else {
> fg->graph->nb_threads = filter_complex_nbthreads;
> }
> + fg->graph->sample_rate_fallback = sample_rate_fallback;
>
> if ((ret = avfilter_graph_parse2(fg->graph, graph_desc, &inputs,
> &outputs)) < 0)
> goto fail;
> diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
> index e2c0176..d8b8918 100644
> --- a/ffmpeg_opt.c
> +++ b/ffmpeg_opt.c
> @@ -121,6 +121,7 @@ int frame_bits_per_raw_sample = 0;
> float max_error_rate = 2.0/3;
> int filter_nbthreads = 0;
> int filter_complex_nbthreads = 0;
> +int sample_rate_fallback = 0;
> int vstats_version = 2;
>
>
> @@ -3088,6 +3089,27 @@ static int opt_filter_complex_script(void *optctx,
> const char *opt, const char *
> return 0;
> }
>
> +static int opt_sample_rate_fallback(void *optctx, const char *opt, const
> char *arg)
> +{
> + static const AVOption opts[] = {
> + { "sample_rate_fallback", NULL, 0, AV_OPT_TYPE_INT, { .i64 = 0 },
> 0, 3, 0, "sample_rate_fallback" },
> + { "closest", NULL, 0, AV_OPT_TYPE_CONST, { .i64 = 0 },
> 0, 0, 0, "sample_rate_fallback" },
> + { "higher", NULL, 0, AV_OPT_TYPE_CONST, { .i64 = 1 },
> 0, 0, 0, "sample_rate_fallback" },
> + { "higher2x", NULL, 0, AV_OPT_TYPE_CONST, { .i64 = 2 },
> 0, 0, 0, "sample_rate_fallback" },
> + { "highest", NULL, 0, AV_OPT_TYPE_CONST, { .i64 = 3 },
> 0, 0, 0, "sample_rate_fallback" },
> + { NULL },
> + };
> + static const AVClass class = {
> + .class_name = "",
> + .item_name = av_default_item_name,
> + .option = opts,
> + .version = LIBAVUTIL_VERSION_INT,
> + };
> + const AVClass *pclass = &class;
> +
> + return av_opt_eval_int(&pclass, &opts[0], arg, &sample_rate_fallback);
> +}
This whole block looks very spurious. If the graph parser does not pick
the option itself, then an av_opt call on the graph itself should do the
trick.
> +
> void show_help_default(const char *opt, const char *arg)
> {
> /* per-file options have at least one of those set */
> @@ -3434,6 +3456,8 @@ const OptionDef options[] = {
> "create a complex filtergraph", "graph_description" },
> { "filter_complex_script", HAS_ARG | OPT_EXPERT, {
> .func_arg = opt_filter_complex_script },
> "read complex filtergraph description from a file", "filename" },
> + { "sample_rate_fallback", HAS_ARG | OPT_EXPERT, {
> .func_arg = opt_sample_rate_fallback },
> + "filtergraph's sample rate fallback method when no exact match
> found", "method" },
> { "stats", OPT_BOOL, {
> &print_stats },
> "print progress report during encoding", },
> { "attach", HAS_ARG | OPT_PERFILE | OPT_EXPERT |
> diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
> index b56615c..f5d47de 100644
> --- a/libavfilter/avfilter.h
> +++ b/libavfilter/avfilter.h
> @@ -891,6 +891,7 @@ typedef struct AVFilterGraph {
> avfilter_execute_func *execute;
>
> char *aresample_swr_opts; ///< swr options to use for the auto-inserted
> aresample filters, Access ONLY through AVOptions
> + int sample_rate_fallback; ///< sample rate fallback method when no exact
> match found
>
> /**
> * Private fields
> diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c
> index 534c670..4e856c6 100644
> --- a/libavfilter/avfiltergraph.c
> +++ b/libavfilter/avfiltergraph.c
> @@ -44,15 +44,26 @@
> #define OFFSET(x) offsetof(AVFilterGraph, x)
> #define FLAGS AV_OPT_FLAG_VIDEO_PARAM|AV_OPT_FLAG_FILTERING_PARAM
> static const AVOption filtergraph_options[] = {
> - { "thread_type", "Allowed thread types", OFFSET(thread_type),
> AV_OPT_TYPE_FLAGS,
> - { .i64 = AVFILTER_THREAD_SLICE }, 0, INT_MAX, FLAGS, "thread_type" },
> - { "slice", NULL, 0, AV_OPT_TYPE_CONST, { .i64 =
> AVFILTER_THREAD_SLICE }, .flags = FLAGS, .unit = "thread_type" },
> - { "threads", "Maximum number of threads", OFFSET(nb_threads),
> - AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, FLAGS },
> - {"scale_sws_opts" , "default scale filter options" ,
> OFFSET(scale_sws_opts) ,
> - AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, FLAGS },
> - {"aresample_swr_opts" , "default aresample filter options" ,
> OFFSET(aresample_swr_opts) ,
> - AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, FLAGS },
Re-aligning other lines is normally done in a separate patch, if ever.
> + { "thread_type", "Allowed thread types",
> OFFSET(thread_type),
> + AV_OPT_TYPE_FLAGS, { .i64 = AVFILTER_THREAD_SLICE }, 0, INT_MAX,
> FLAGS, "thread_type" },
> + { "slice", NULL,
> 0,
> + AV_OPT_TYPE_CONST, { .i64 = AVFILTER_THREAD_SLICE }, 0, 0,
> FLAGS, "thread_type" },
> + { "threads", "Maximum number of threads",
> OFFSET(nb_threads),
> + AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX,
> FLAGS },
> + { "scale_sws_opts", "default scale filter options",
> OFFSET(scale_sws_opts),
> + AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0,
> FLAGS },
> + { "aresample_swr_opts", "default aresample filter options",
> OFFSET(aresample_swr_opts),
> + AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0,
> FLAGS },
> + { "sample_rate_fallback", "sample rate fallback method when no exact
> match found", OFFSET(sample_rate_fallback),
> + AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 3,
> FLAGS, "sample_rate_fallback" },
> + { "closest", "to the closest sample rate",
> 0,
> + AV_OPT_TYPE_CONST, { .i64 = 0 }, 0, 0,
> FLAGS, "sample_rate_fallback" },
> + { "higher", "to the closest higher sample rate",
> 0,
> + AV_OPT_TYPE_CONST, { .i64 = 1 }, 0, 0,
> FLAGS, "sample_rate_fallback" },
> + { "higher2x", "similar to 'higher', but twice high",
> 0,
> + AV_OPT_TYPE_CONST, { .i64 = 2 }, 0, 0,
> FLAGS, "sample_rate_fallback" },
> + { "highest", "to the highest sample rate",
> 0,
> + AV_OPT_TYPE_CONST, { .i64 = 3 }, 0, 0,
> FLAGS, "sample_rate_fallback" },
> { NULL },
> };
>
> @@ -874,7 +885,28 @@ static void swap_samplerates_on_filter(AVFilterContext
> *filter)
> continue;
>
> for (j = 0; j < outlink->in_samplerates->nb_formats; j++) {
> - int diff = abs(sample_rate -
> outlink->in_samplerates->formats[j]);
> + int diff;
> + switch (filter->graph->sample_rate_fallback) {
> + default:
I think this shoulw be "case 0" (except see below) and default should
trigger an assert failure.
> + diff = abs(sample_rate -
> outlink->in_samplerates->formats[j]);
> + break;
> + case 1:
Magic constants like that are not good design, they need to be expressed
by an enum.
> + if ((diff = outlink->in_samplerates->formats[j] -
> sample_rate) < 0)
> + goto reverse_tail;
Jumping from one case clause to another does not look like an acceptable
use of goto in readable code.
> + break;
> + case 2:
> + if ((diff = sample_rate -
> outlink->in_samplerates->formats[j]) == 0)
> + break;
> + if ((diff = outlink->in_samplerates->formats[j] -
> sample_rate * 2) < 0)
> + goto reverse_tail;
> + break;
> + case 3:
> + if ((diff = sample_rate -
> outlink->in_samplerates->formats[j]) == 0)
> + break;
> +reverse_tail:
> + diff = INT_MAX - outlink->in_samplerates->formats[j];
> + break;
> + }
>
> av_assert0(diff < INT_MAX); // This would lead to the use of
> uninitialized best_diff but is only possible with invalid sample rates
>
Regards,
--
Nicolas George
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list [email protected] http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
