[FFmpeg-devel] [PATCH] avfilter/af_astats: count number of NaNs/Infs/denormals for floating-point audio too

[Alexander Strasser]

Hi Paul,

just three small comments from me...

On 2019-04-22 11:51 +0200, Paul B Mahol wrote:
> Signed-off-by: Paul B Mahol <onemda at gmail.com>
> ---
>  doc/filters.texi        |  6 +++
>  libavfilter/af_astats.c | 86 ++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 86 insertions(+), 6 deletions(-)

I was a bit surprised when I first saw the number of lines it
takes to add this feature. OTOH this is not a problem of this
patch, because it is mostly caused by the structure of the
code that was in place before.

Changing the structure doesn't seem worth it yet. If
there will be an addition that is applicable to all the
individual stats, it should IMHO be reconsidered.


> diff --git a/doc/filters.texi b/doc/filters.texi
> index cfff9b1f4d..945c557e8f 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -2141,6 +2141,9 @@ Bit_depth
>  Dynamic_range
>  Zero_crossings
>  Zero_crossings_rate

> +Number_of_NaNs
> +Number_of_Infs

> +Number_of_denormals

Nit: Maybe Number_of_NaNs and Number_of_Infs should not be
capitalized like that, e.g. just use Number_of_nans and
Number_of_infs . Anyway if there's a reason for choosing
this names, it should be OK as it is. Just thinking it is
harder to type these from memory if the spelling is a bit
arbitrary.


>  and for Overall:
>  DC_offset
> @@ -2158,6 +2161,9 @@ Flat_factor
>  Peak_count
>  Bit_depth
>  Number_of_samples
> +Number_of_NaNs
> +Number_of_Infs
> +Number_of_denormals
>
>  For example full key look like this @code{lavfi.astats.1.DC_offset} or
>  this @code{lavfi.astats.Overall.Peak_count}.
> diff --git a/libavfilter/af_astats.c b/libavfilter/af_astats.c
> index 92368793c2..25c700b344 100644
> --- a/libavfilter/af_astats.c
> +++ b/libavfilter/af_astats.c
> @@ -20,6 +20,7 @@
>   */
>
>  #include <float.h>
> +#include <math.h>
>
>  #include "libavutil/opt.h"
>  #include "audio.h"
> @@ -48,6 +49,9 @@
>  #define MEASURE_ZERO_CROSSINGS          (1 << 16)
>  #define MEASURE_ZERO_CROSSINGS_RATE     (1 << 17)
>  #define MEASURE_NUMBER_OF_SAMPLES       (1 << 18)
> +#define MEASURE_NUMBER_OF_NANS          (1 << 19)
> +#define MEASURE_NUMBER_OF_INFS          (1 << 20)
> +#define MEASURE_NUMBER_OF_DENORMALS     (1 << 21)
>
>  #define MEASURE_MINMAXPEAK              (MEASURE_MIN_LEVEL | MEASURE_MAX_LEVEL | MEASURE_PEAK_LEVEL)
>
> @@ -68,6 +72,9 @@ typedef struct ChannelStats {
>      uint64_t min_count, max_count;
>      uint64_t zero_runs;
>      uint64_t nb_samples;
> +    uint64_t nb_nans;
> +    uint64_t nb_infs;
> +    uint64_t nb_denormals;
>  } ChannelStats;
>
>  typedef struct AudioStatsContext {
> @@ -83,6 +90,8 @@ typedef struct AudioStatsContext {
>      int maxbitdepth;
>      int measure_perchannel;
>      int measure_overall;
> +    int is_float;
> +    int is_double;
>  } AudioStatsContext;
>
>  #define OFFSET(x) offsetof(AudioStatsContext, x)
> @@ -114,6 +123,9 @@ static const AVOption astats_options[] = {
>        { "Zero_crossings"            , "", 0, AV_OPT_TYPE_CONST, {.i64=MEASURE_ZERO_CROSSINGS      }, 0, 0, FLAGS, "measure" },
>        { "Zero_crossings_rate"       , "", 0, AV_OPT_TYPE_CONST, {.i64=MEASURE_ZERO_CROSSINGS_RATE }, 0, 0, FLAGS, "measure" },
>        { "Number_of_samples"         , "", 0, AV_OPT_TYPE_CONST, {.i64=MEASURE_NUMBER_OF_SAMPLES   }, 0, 0, FLAGS, "measure" },
> +      { "Number_of_NaNs"            , "", 0, AV_OPT_TYPE_CONST, {.i64=MEASURE_NUMBER_OF_NANS      }, 0, 0, FLAGS, "measure" },
> +      { "Number_of_Infs"            , "", 0, AV_OPT_TYPE_CONST, {.i64=MEASURE_NUMBER_OF_INFS      }, 0, 0, FLAGS, "measure" },
> +      { "Number_of_denormals"       , "", 0, AV_OPT_TYPE_CONST, {.i64=MEASURE_NUMBER_OF_DENORMALS }, 0, 0, FLAGS, "measure" },
>      { "measure_overall", "only measure_perchannel these overall statistics", OFFSET(measure_overall), AV_OPT_TYPE_FLAGS, {.i64=MEASURE_ALL}, 0, UINT_MAX, FLAGS, "measure" },
>      { NULL }
>  };
> @@ -181,6 +193,9 @@ static void reset_stats(AudioStatsContext *s)
>          p->max_count = 0;
>          p->zero_runs = 0;
>          p->nb_samples = 0;
> +        p->nb_nans = 0;
> +        p->nb_infs = 0;
> +        p->nb_denormals = 0;
>      }
>  }
>
> @@ -196,6 +211,11 @@ static int config_output(AVFilterLink *outlink)
>      s->tc_samples = 5 * s->time_constant * outlink->sample_rate + .5;
>      s->nb_frames = 0;
>      s->maxbitdepth = av_get_bytes_per_sample(outlink->format) * 8;
> +    s->is_double = outlink->format == AV_SAMPLE_FMT_DBL  ||
> +                   outlink->format == AV_SAMPLE_FMT_DBLP;
> +
> +    s->is_float = outlink->format == AV_SAMPLE_FMT_FLT  ||
> +                  outlink->format == AV_SAMPLE_FMT_FLTP;
>
>      reset_stats(s);
>
> @@ -280,6 +300,24 @@ static inline void update_stat(AudioStatsContext *s, ChannelStats *p, double d,
>      p->nb_samples++;
>  }
>
> +static inline void update_float_stat(AudioStatsContext *s, ChannelStats *p, float d)

I guess "float d" is rather uncommon, but it's not unseen in the
FFmpeg code base.


> +{
> +    int type = fpclassify(d);
> +
> +    p->nb_nans      += type == FP_NAN;
> +    p->nb_infs      += type == FP_INFINITE;
> +    p->nb_denormals += type == FP_SUBNORMAL;
> +}
> +
> +static inline void update_double_stat(AudioStatsContext *s, ChannelStats *p, double d)
> +{
> +    int type = fpclassify(d);
> +
> +    p->nb_nans      += type == FP_NAN;
> +    p->nb_infs      += type == FP_INFINITE;
> +    p->nb_denormals += type == FP_SUBNORMAL;
> +}
> +
[...]

  Alexander