Re: [PATCH RESEND 9/9] Added average power spectrum for stat -freq -a

["Måns Rullgård" <mans@mansr.com>]

Eric Wong <normalperson@yhbt.net> writes:

> From: Pander <pander@users.sourceforge.net>

Does "Pander" have a real name?

> references:
> https://sourceforge.net/p/sox/mailman/message/33186778/
> https://sourceforge.net/p/sox/mailman/message/33175940/
> https://sourceforge.net/p/sox/mailman/message/33625255/
>
> Edited-by: Eric Wong <normalperson@yhbt.net>
>
> [ew: removed extraneous whitespace changes,
>  squashed re_average initialization fix from Pander,
>  removed unnecessary check before free(3) call
>  (ref: <5498A2F9.2010500@free.fr>),
>  converted fft_average to bool]
> ---
>  sox.1      |  6 ++++++
>  src/stat.c | 27 ++++++++++++++++++++++++++-
>  2 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/sox.1 b/sox.1
> index 04ab09b1..d64762af 100644
> --- a/sox.1
> +++ b/sox.1
> @@ -3596,6 +3596,12 @@ audio in SoX's internal buffer.
>  This is mainly used to help track down endian problems that
>  sometimes occur in cross-platform versions of SoX.
>  .SP
> +The
> +.B \-a
> +option
> +will output the average input's power spectrum instead of the default
> +behavior in which the power spectrum is given for each 4096 point DFT.
> +.SP
>  See also the
>  .B stats
>  effect.
> diff --git a/src/stat.c b/src/stat.c
> index cdea27cc..0ca6c232 100644
> --- a/src/stat.c
> +++ b/src/stat.c
> @@ -34,6 +34,7 @@ typedef struct {
>    float *re_out;
>    unsigned long fft_size;
>    unsigned long fft_offset;
> +  sox_bool fft_average;
>  } priv_t;
>
> @@ -69,6 +70,8 @@ static int sox_stat_getopts(sox_effect_t * effp, int argc, char **argv)
>        stat->fft = 1;
>      else if (!(strcmp(*argv, "-d")))
>        stat->volume = 2;
> +    else if (!(strcmp(*argv, "-a")))
> +      stat->fft_average = sox_true;
>      else {
>        lsx_fail("Summary effect: unknown option");
>        return SOX_EOF;
> @@ -100,6 +103,7 @@ static int sox_stat_start(sox_effect_t * effp)
>      stat->bin[i] = 0;
>
>    stat->fft_size = 4096;
> +  stat->fft_average = sox_false;
>    stat->re_in = stat->re_out = NULL;
>
>    if (stat->fft) {
> @@ -134,6 +138,16 @@ static int sox_stat_flow(sox_effect_t * effp, const sox_sample_t *ibuf, sox_samp
>    priv_t * stat = (priv_t *) effp->priv;
>    int done, x, len = min(*isamp, *osamp);
>    short count = 0;
> +  float *re_average = NULL;
> +  unsigned samples = 0;
> +  float ffa = 0.0;
> +  unsigned i;
> +
> +  if (stat->fft_average) {
> +      samples = (stat->fft_size / 2);
> +      ffa = effp->in_signal.rate / samples;
> +      re_average = lsx_malloc(sizeof(float) * (int)samples);
> +  }
>
>    if (len) {
>      if (stat->read == 0)          /* 1st sample */
> @@ -146,10 +160,20 @@ static int sox_stat_flow(sox_effect_t * effp, const sox_sample_t *ibuf, sox_samp
>
>          if (stat->fft_offset >= stat->fft_size) {
>            stat->fft_offset = 0;
> +          if (stat->fft_average) {
> +              lsx_power_spectrum_f((int)samples, stat->re_in, stat->re_out);
> +              for (i = 0; i < samples / 2; i++) /* FIXME: should be <= samples / 2 */

What's this about?  If it should be <=, why isn't it?

> +                  re_average[i] += stat->re_out[i];

re_average is never initialised.  This will produce garbage.

> +          } else {
>            print_power_spectrum((unsigned) stat->fft_size, effp->in_signal.rate, stat->re_in, stat->re_out);
> +          }
>          }
>
>        }
> +      if (stat->fft_average) {
> +          for (i = 0; i < samples / 2; i++) /* FIXME: should be <= samples / 2 */
> +              fprintf(stderr, " %f  %f\n", ffa * i, re_average[i] / len);
> +      }

I don't understand the idea here.  Unless I'm mistaken, this prints the
power spectrum averaged over however many FFT blocks happen to fit in
the input chunk, which could be none.

>      }
>
>      for (done = 0; done < len; done++) {
> @@ -192,6 +216,7 @@ static int sox_stat_flow(sox_effect_t * effp, const sox_sample_t *ibuf, sox_samp
>      stat->read += len;
>    }
>
> +  free(re_average);
>    *isamp = *osamp = len;
>    /* Process all samples */
>
> @@ -320,7 +345,7 @@ static int sox_stat_stop(sox_effect_t * effp)
>
>  static sox_effect_handler_t sox_stat_effect = {
>    "stat",
> -  "[ -s N ] [ -rms ] [-freq] [ -v ] [ -d ]",
> +  "[ -s N ] [ -rms ] [-freq] [ -v ] [ -d ] [ -a ]",
>    SOX_EFF_MCHAN | SOX_EFF_MODIFY,
>    sox_stat_getopts,
>    sox_stat_start,

I'm skipping this patch unless someone can explain what the intent is.
Then I might try to fix it.

-- 
Måns Rullgård


_______________________________________________
SoX-devel mailing list
SoX-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sox-devel