> On 11 Jan 2018, at 22:41, Paul B Mahol <one...@gmail.com> wrote:
>
>> On 1/11/18, Dmitry Gumenyuk <dmitry.gumen...@gmail.com> wrote:
>> Hello
>>
>> 2018-01-10 11:33 GMT+01:00 Moritz Barsnick <barsn...@gmx.net>:
>>>> On Wed, Jan 10, 2018 at 08:58:05 +0100, dmitry.gumen...@gmail.com wrote:
>>>>
>>>> +@table @option
>>>> +@item d
>>>> +Dimensions @code{WxH}.
>>>> +@code{W} - number of data values in json, values will be scaled
>>>> according to @code{H}.
>>>> +The default value is @var{640x480}
>>>> +
>>>> +@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?
> This filter does not need support for so many formats, it could
> support just s16.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
