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

Dmitry Gumenyuk dmitry.gumenyuk at gmail.com
Thu Jan 11 23:24:05 EET 2018


Hello

2018-01-10 11:33 GMT+01:00 Moritz Barsnick <barsnick at gmx.net>:
> 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.

I checked solution used in volumedetect and couldn't find a way to
read across formats.
How would you implement such macros? Since version 3.2 astats filter
uses same approach for reading different formats and as far as I know
macros harder to debug

>> +            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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


More information about the ffmpeg-devel mailing list