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

Paul B Mahol onemda at gmail.com
Thu Jan 11 23:41:11 EET 2018


On 1/11/18, Dmitry Gumenyuk <dmitry.gumenyuk at gmail.com> wrote:
> 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.

I do not understand what you are trying to do.

> 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

astats is using all formats because of numerous reasons. astats uses raw values,
your filter just convert each raw value to float representation.

This filter does not need support for so many formats, it could
support just s16.


More information about the ffmpeg-devel mailing list