Follow up on Average power spectrum path

Follow up on Average power spectrum path

[Pander]

Hi all,

Because of a question on the users list, I would like to ask how to move
forward the following patch and documentation?

Average power spectrum patch:
https://sourceforge.net/p/sox/mailman/message/33186778/
and
https://sourceforge.net/p/sox/mailman/message/33175940/

Extra documentation:
https://sourceforge.net/p/sox/mailman/message/33625255/

Please let me know how I can help to move this forward.

Thanks,

Pander

------------------------------------------------------------------------------

Re: Follow up on Average power spectrum path

[Eric Wong]

Pander <pander@users.sourceforge.net> wrote:
> Because of a question on the users list, I would like to ask how to move
> forward the following patch and documentation?

I guess the real sox developers are still busy...
I suppose I'll start publishing things I've reviewed/tested
and push things I've reviewed to branches my server (see below)[1]
so it'll hopefully be easier for them when they return.

I'm not really qualified to review the maths parts of this;
as I am only a simple Unix/C plumber and I already commented
some last year on this patch...

I wonder if "-a" should imply "-freq", since it's "-a"
is a no-op without "-freq".

And is this option is intended to be the average of each hunk in the
flow and not for the entire file?  In other words, it outputs a LOT of
lines with 2 floating point numbers in them when I run:
"sox foo.flac -n stat -freq -a" on an arbitrary file.

Most of the normal "stat" output is printed in the sox_stat_stop
function instead of sox_stat_flow; the lone exception being '-d'
which seems mainly for troubleshooting.

Also, did you intend fft_average to be extended for non-boolean
purposes?

I don't mind using "int" as a bool for other pre-C99 projects, but the
"== 1" checks in conditionals looked a bit odd to me and sox already
defines a sox_bool type, so I figure squashing in the following would
improve readability:

diff --git a/src/stat.c b/src/stat.c
index 63d1741..89bed55 100644
--- a/src/stat.c
+++ b/src/stat.c
@@ -34,7 +34,7 @@ typedef struct {
   float *re_out;
   unsigned long fft_size;
   unsigned long fft_offset;
-  int fft_average;
+  sox_bool fft_average;
 } priv_t;
 
 
@@ -71,7 +71,7 @@ static int sox_stat_getopts(sox_effect_t * effp, int argc, char **argv)
     else if (!(strcmp(*argv, "-d")))
       stat->volume = 2;
     else if (!(strcmp(*argv, "-a")))
-      stat->fft_average = 1;
+      stat->fft_average = sox_true;
     else {
       lsx_fail("Summary effect: unknown option");
       return SOX_EOF;
@@ -103,7 +103,7 @@ static int sox_stat_start(sox_effect_t * effp)
     stat->bin[i] = 0;
 
   stat->fft_size = 4096;
-  stat->fft_average = 0;
+  stat->fft_average = sox_false;
   stat->re_in = stat->re_out = NULL;
 
   if (stat->fft) {
@@ -142,7 +142,8 @@ static int sox_stat_flow(sox_effect_t * effp, const sox_sample_t *ibuf, sox_samp
   unsigned samples = 0;
   float ffa = 0.0;
   unsigned i;
-  if (stat->fft_average == 1) {
+
+  if (stat->fft_average) {
       samples = (stat->fft_size / 2);
       ffa = effp->in_signal.rate / samples;
       re_average = lsx_malloc(sizeof(float) * (int)samples);
@@ -159,7 +160,7 @@ 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 == 1) {
+          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 */
                   re_average[i] += stat->re_out[i];
@@ -169,7 +170,7 @@ static int sox_stat_flow(sox_effect_t * effp, const sox_sample_t *ibuf, sox_samp
         }
 
       }
-      if (stat->fft_average == 1) {
+      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);
       }

---
[1] you can add my repo to your existing sox repository:
    git remote add 80x24 git://80x24.org/sox.git
    git fetch 80x24
    (or a web viewer if you prefer: http://bogomips.org/sox.git )
    These are work-in-progress dev branches, so I will squash
    and rebase branches as needed with forced pushes.

------------------------------------------------------------------------------

Re: Follow up on Average power spectrum path

[Måns Rullgård]

Eric Wong <normalperson@yhbt.net> writes:

> Pander <pander@users.sourceforge.net> wrote:
>> Because of a question on the users list, I would like to ask how to move
>> forward the following patch and documentation?
>
> I guess the real sox developers are still busy...

Or missing.

-- 
Måns Rullgård

------------------------------------------------------------------------------

Re: Follow up on Average power spectrum path

[Pander]

On 12/19/2015 12:12 PM, Eric Wong wrote:
> Pander <pander@users.sourceforge.net> wrote:
>> Because of a question on the users list, I would like to ask how to move
>> forward the following patch and documentation?
> 
> I guess the real sox developers are still busy...
> I suppose I'll start publishing things I've reviewed/tested
> and push things I've reviewed to branches my server (see below)[1]
> so it'll hopefully be easier for them when they return.
> 
> I'm not really qualified to review the maths parts of this;
> as I am only a simple Unix/C plumber and I already commented
> some last year on this patch...
> 
> I wonder if "-a" should imply "-freq", since it's "-a"
> is a no-op without "-freq".

It is only allowed/used after -freq in this context.

After gaining some more experience with sox this year, I have learned
that calculating the average power spectrum is not straightforward. I
have also posted about this to one of the sox mailing lists where I
illustrated the problem with some graphs.

Only when the last sample is filled completely, an average can be
calculated over all samples. Usually, this is not the case and should
the average be calculated over over all samples except the last one.

The safest way to calculate the average, is to always omit the last
sample. However, it is more correct to include the last sample, but only
when can be detected that the last sample is filled exactly as all the
other samples are. If it is only partially filled, it (highly probably)
will distort the average too much.

Since I only work with a lot of samples, for me you can always omit the
last one. But if someone could let you know how to detect that the last
samples is used to the full and can be included, please do so.

Of course, when omitting the last sample, divide by the number of
samples - 1. Either way, how this is implemented, it should be
documented clearly when the last sample is omitted.

> 
> And is this option is intended to be the average of each hunk in the
> flow and not for the entire file?  In other words, it outputs a LOT of
> lines with 2 floating point numbers in them when I run:
> "sox foo.flac -n stat -freq -a" on an arbitrary file.
> 
> Most of the normal "stat" output is printed in the sox_stat_stop
> function instead of sox_stat_flow; the lone exception being '-d'
> which seems mainly for troubleshooting.
> 
> Also, did you intend fft_average to be extended for non-boolean
> purposes?

Initially not, but see above. It could be desirable to offer different
options:
nothing: do as now
-a   average over all samples
-ao  average over all samples but omit last sample
-as  average over all samples but omit last sample only when needed
(often needed because the last sample is partially filled)

> 
> I don't mind using "int" as a bool for other pre-C99 projects, but the
> "== 1" checks in conditionals looked a bit odd to me and sox already
> defines a sox_bool type, so I figure squashing in the following would
> improve readability:
> 
> diff --git a/src/stat.c b/src/stat.c
> index 63d1741..89bed55 100644
> --- a/src/stat.c
> +++ b/src/stat.c
> @@ -34,7 +34,7 @@ typedef struct {
>    float *re_out;
>    unsigned long fft_size;
>    unsigned long fft_offset;
> -  int fft_average;
> +  sox_bool fft_average;
>  } priv_t;
>  
>  
> @@ -71,7 +71,7 @@ static int sox_stat_getopts(sox_effect_t * effp, int argc, char **argv)
>      else if (!(strcmp(*argv, "-d")))
>        stat->volume = 2;
>      else if (!(strcmp(*argv, "-a")))
> -      stat->fft_average = 1;
> +      stat->fft_average = sox_true;
>      else {
>        lsx_fail("Summary effect: unknown option");
>        return SOX_EOF;
> @@ -103,7 +103,7 @@ static int sox_stat_start(sox_effect_t * effp)
>      stat->bin[i] = 0;
>  
>    stat->fft_size = 4096;
> -  stat->fft_average = 0;
> +  stat->fft_average = sox_false;
>    stat->re_in = stat->re_out = NULL;
>  
>    if (stat->fft) {
> @@ -142,7 +142,8 @@ static int sox_stat_flow(sox_effect_t * effp, const sox_sample_t *ibuf, sox_samp
>    unsigned samples = 0;
>    float ffa = 0.0;
>    unsigned i;
> -  if (stat->fft_average == 1) {
> +
> +  if (stat->fft_average) {
>        samples = (stat->fft_size / 2);
>        ffa = effp->in_signal.rate / samples;
>        re_average = lsx_malloc(sizeof(float) * (int)samples);
> @@ -159,7 +160,7 @@ 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 == 1) {
> +          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 */
>                    re_average[i] += stat->re_out[i];
> @@ -169,7 +170,7 @@ static int sox_stat_flow(sox_effect_t * effp, const sox_sample_t *ibuf, sox_samp
>          }
>  
>        }
> -      if (stat->fft_average == 1) {
> +      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);
>        }
> 
> ---
> [1] you can add my repo to your existing sox repository:
>     git remote add 80x24 git://80x24.org/sox.git
>     git fetch 80x24
>     (or a web viewer if you prefer: http://bogomips.org/sox.git )
>     These are work-in-progress dev branches, so I will squash
>     and rebase branches as needed with forced pushes.
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> SoX-devel mailing list
> SoX-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/sox-devel
> 


------------------------------------------------------------------------------