[FFmpeg-devel] [PATCH] avfilter: add dumpwave filter.

[Moritz Barsnick]

On Wed, Jan 10, 2018 at 08:58:05 +0100, dmitry.gumenyuk at gmail.com wrote:

> + at table @option
> + at item d
> +Dimensions @code{WxH}.
> + at code{W} - number of data values in json, values will be scaled according to @code{H}.
> +The default value is @var{640x480}
> +
> + at item s
> +Samples count per value per channel

In most other filters/filtersources, s+size is used for dimensions,
d+duration for time, and n for an amount/number of frames/samples or
so. Might be a good idea to align with this. And use aliases (i.e.
implement both "s" and "size").

> +static const AVOption dumpwave_options[] = {
> +    { "d", "set width and height", OFFSET(w), AV_OPT_TYPE_IMAGE_SIZE, {.str = "640x480"}, 0, 0, FLAGS },
> +    { "s", "set number of samples per value per channel",  OFFSET(s), AV_OPT_TYPE_INT64,  {.i64 = 0}, 0, INT64_MAX, FLAGS },
> +    { "json", "set json dump file", OFFSET(json), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, FLAGS },
> +    { NULL }
[...]
> +static av_cold int init(AVFilterContext *ctx)
> +{
> +    DumpWaveContext *dumpwave = ctx->priv;
> +    if(!dumpwave->s) {
> +        dumpwave->is_disabled = 1;
> +        av_log(ctx, AV_LOG_ERROR, "Invalid samples per value config\n");
> +    }
> +    return 0;

I don't like the idea of having to provide the "s" parameter. Is there
really no useful default?

And now, if s=0, what does the filter do? Just sit around? Couldn't it
fail instead?

Apart from that:
"if (" is the bracket style ffmpeg uses.

> +        if (dumpwave->json && !(dump_fp = av_fopen_utf8(dumpwave->json, "w")))
> +            av_log(ctx, AV_LOG_WARNING, "Flushing dump failed\n");

I would appreciate evaluation of errno and printing the appropriate
string (using av_strerror(), I believe).

> +    switch (inlink->format) {
> +        case AV_SAMPLE_FMT_DBLP:

As Kyle hinted: Can this not force a conversion (implicit insertion of
aformat filter) to e.g. double by only supporting this format, and
reducing this switch to one or two cases? (The filter's output isn't
really meant for transparent reuse anyway? af_volumedetect e.g. also
supports only one, meaning its output can be a different format than
its input.) It's also really hard to go through and later to maintain.
It the big switch/case remains, one or two code macros would be
appropriate.

> +            for (i = 0; i < buf->nb_samples; i++) {
> +                for (c = 0; c < channels; c++, src++)
> +                    calc_db_rms(dumpwave, *src);
> +            }}

I believe the curly brackets need to be placed differently.

> +    .description   = NULL_IF_CONFIG_SMALL("Dumps RMS amplitude to JSON file"),

Imperative, i.e. "Dump RMS ...".

> +FATE_AFILTER-$(call FILTERDEMDEC, DUMPWAVE, WAV, PCM_S16LE) += fate-filter-dumpwave
> +fate-filter-dumpwave: SRC = $(TARGET_PATH)/tests/data/asynth-44100-2.wav
> +fate-filter-dumpwave: CMD = ffmpeg -i $(SRC) -af dumpwave=d=1800x140:s=147:json=$(TARGET_PATH)/tests/data/fate/filter-dumpwave.out -f null - && cat $(TARGET_PATH)/tests/data/fate/filter-dumpwave.out
> +

If you need to implement all formats, you might want to test them all.
Just a thought.

Moritz