[FFmpeg-devel] [PATCH] add dumpwave filter

[Kyle Swanson]

Hi,

For this to be a part of libavfilter the output needs to be more generic
than the just the Soundcloud format. If we want this to be generally useful
it should probably just output an array of floats between 0.0 and 1.0. The
consumer of this data (JS library, or whatever) can use this in whatever
way it wants. If you send another patch, just reply to this thread because
that makes it easier to follow (sending a patch as an attachment is OK).
Here are some critiques:

+typedef struct DumpWaveContext {
> +    const AVClass *class;   /**< class for AVOptions */
> +    int w;                  /**< number of data values in json */
> +    int h;                  /**< values will be scaled according to
> provided */
> +    int is_disabled;        /**< disable filter in case it's
> misconfigured */
> +    int i;                  /**< index of value */
> +    char *json;             /**< path to json */
> +    char *str;              /**< comma separated values */, wha
> +    double *values;         /**< scaling factors */
> +    int64_t s;              /**< samples per value per channel */
> +    int64_t n;              /**< current number of samples counted */
> +    int64_t max_samples;    /**< samples per value */
> +    double sum;             /**< sum of the squared samples per value */
> +} DumpWaveContext;

Use more descriptive variable names for your struct members. They don't
have to be just one letter.


> +    { "d", "set width and height", OFFSET(w), AV_OPT_TYPE_IMAGE_SIZE,
> {.str = "640x480"}, 0, 0, FLAGS },

Get rid of this. We shouldn't care how this data is used/rendered. Our only
job should be to output an array of floats.


> +    { "s", "set number of samples per value per channel",  OFFSET(s),
> AV_OPT_TYPE_INT64,  {.i64 = 0}, 0, INT64_MAX, FLAGS },

Maybe you can call this frame_size? 0 is not a useful value here, it
shouldn't be the lower bound or the default value.



> +static av_cold int init(AVFilterContext *ctx)
> +{
> +    DumpWaveContext *dumpwave = ctx->priv;
> +    if(!dumpwave->s) {

The filter should just fail if it's not configured correctly. You'll get
this behavior for free with better default values.


> +static int config_output(AVFilterLink *outlink)
> +{
> +    AVFilterContext *ctx = outlink->src;
> +    DumpWaveContext *dumpwave = ctx->priv;
> +    const int width = dumpwave->w;
> +    dumpwave->values = av_realloc(NULL, width * sizeof(double));
> +    dumpwave->str = av_realloc(NULL, width * sizeof(int));

You don't need a giant buffer to hold the entire string. Just keep a file
open a write to it every frame. Maybe we could just write if to stdout
instead?


> +
> +/**
> + * Converts sample to dB and calculates root mean squared value
> + */
> +static inline void dbRms(DumpWaveContext *dumpwave, double smpl)
>
Just call this RMS and spit something out between 0.0 and 1.0.  Avoid
camelcase for function names.


>
> +    switch (inlink->format) {
> +        case AV_SAMPLE_FMT_DBLP:
> +            for (c = 0; c < channels; c++) {
> +                const double *src = (const double *)buf->extended_data[c];
> +
> +                for (i = 0; i < buf->nb_samples; i++, src++)
> +                    dbRms(dumpwave, *src);
> +            }
> +            break;
> +        case AV_SAMPLE_FMT_DBL: {
> +            const double *src = (const double *)buf->extended_data[0];
> +
> +            for (i = 0; i < buf->nb_samples; i++) {
> +                for (c = 0; c < channels; c++, src++)
> +                    dbRms(dumpwave, *src);
> +            }}
> +            break;
> +        case AV_SAMPLE_FMT_FLTP:
> +            for (c = 0; c < channels; c++) {
> +                const float *src = (const float *)buf->extended_data[c];
> +
> +                for (i = 0; i < buf->nb_samples; i++, src++)
> +                    dbRms(dumpwave, *src);
> +            }
> +            break;
> +        case AV_SAMPLE_FMT_FLT: {
> +            const float *src = (const float *)buf->extended_data[0];
> +
> +            for (i = 0; i < buf->nb_samples; i++) {
> +                for (c = 0; c < channels; c++, src++)
> +                    dbRms(dumpwave, *src);
> +            }}
> +            break;
> +        case AV_SAMPLE_FMT_S64P:
> +            for (c = 0; c < channels; c++) {
> +                const int64_t *src = (const int64_t
> *)buf->extended_data[c];
> +
> +                for (i = 0; i < buf->nb_samples; i++, src++)
> +                    dbRms(dumpwave, *src / (double)INT64_MAX);
> +            }
> +            break;
> +        case AV_SAMPLE_FMT_S64: {
> +            const int64_t *src = (const int64_t *)buf->extended_data[0];
> +
> +            for (i = 0; i < buf->nb_samples; i++) {
> +                for (c = 0; c < channels; c++, src++)
> +                    dbRms(dumpwave, *src / (double)INT64_MAX);
> +            }}
> +            break;
> +        case AV_SAMPLE_FMT_S32P:
> +            for (c = 0; c < channels; c++) {
> +                const int32_t *src = (const int32_t
> *)buf->extended_data[c];
> +
> +                for (i = 0; i < buf->nb_samples; i++, src++)
> +                    dbRms(dumpwave, *src / (double)INT32_MAX);
> +            }
> +            break;
> +        case AV_SAMPLE_FMT_S32: {
> +            const int32_t *src = (const int32_t *)buf->extended_data[0];
> +
> +            for (i = 0; i < buf->nb_samples; i++) {
> +                for (c = 0; c < channels; c++, src++)
> +                    dbRms(dumpwave, *src / (double)INT32_MAX);
> +            }}
> +            break;
> +        case AV_SAMPLE_FMT_S16P:
> +            for (c = 0; c < channels; c++) {
> +                const int16_t *src = (const int16_t
> *)buf->extended_data[c];
> +
> +                for (i = 0; i < buf->nb_samples; i++, src++)
> +                    dbRms(dumpwave, *src / (double)INT16_MAX);
> +            }
> +            break;
> +        case AV_SAMPLE_FMT_S16: {
> +            const int16_t *src = (const int16_t *)buf->extended_data[0];
> +
> +            for (i = 0; i < buf->nb_samples; i++) {
> +                for (c = 0; c < channels; c++, src++)
> +                    dbRms(dumpwave, *src / (double)INT16_MAX);
> +            }}
> +            break;
> +        case AV_SAMPLE_FMT_U8P:
> +            for (c = 0; c < channels; c++) {
> +                const int8_t *src = (const int8_t *)buf->extended_data[c];
> +
> +                for (i = 0; i < buf->nb_samples; i++, src++)
> +                    dbRms(dumpwave, *src / (double)INT8_MAX);
> +            }
> +            break;
> +        case AV_SAMPLE_FMT_U8: {
> +            const int8_t *src = (const int8_t *)buf->extended_data[0];
> +
> +            for (i = 0; i < buf->nb_samples; i++) {
> +                for (c = 0; c < channels; c++, src++)
> +                    dbRms(dumpwave, *src / (double)INT8_MAX);
> +            }}
> +            break;
> +        default:
> +            break;
> +    }
> +end:
> +    return ff_filter_frame(ctx->outputs[0], buf);
> +}
>
In some filters this might make sense, but not this one. Just force
something reasonable in query_formats. See one of many audio filters for an
example.


>
> +
> +AVFilter ff_af_dumpwave = {
> +    .name          = "dumpwave",
> +    .description   = NULL_IF_CONFIG_SMALL("Dumps RMS amplitude to JSON
> file"),
> +    .init          = init,
> +    .uninit        = uninit,
> +    .priv_size     = sizeof(DumpWaveContext),
> +    .inputs        = dumpwave_inputs,
> +    .outputs       = dumpwave_outputs,
> +    .priv_class    = &dumpwave_class,
> +};
>
You can get rid of the `dumpwave_` prefixes here.

Thanks,
Kyle