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

[Paul B Mahol]

On 1/11/18, Dmitry Gumenyuk <dmitry.gumenyuk at gmail.com> wrote:
>
>> On 11 Jan 2018, at 22:41, Paul B Mahol <onemda at gmail.com> wrote:
>>
>>> 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.
> Sorry, I'm trying to add support for 8, 16, 24, 32, 64 bit sample 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
>>
>> astats is using all formats because of numerous reasons. astats uses raw
>> values,
>> your filter just convert each raw value to float representation.
> Is this wrong, as I'd like to have high precision?

For rendering to small size image?