Re: sox spectrogram patches

[Eric Wong <normalperson@yhbt.net>]

Martin Guy <martinwguy@gmail.com> wrote:
> On 26/12/2015, Eric Wong <normalperson@yhbt.net> wrote:
> > --- a/src/spectrogram.c
> > +++ b/src/spectrogram.c
> > @@ -70,7 +70,11 @@ typedef struct {
> >    sox_bool   using_stdout; /* output image to stdout */
> >
> >    /* Shared work area */
> > +#if HAVE_FFTW
> > +  fftw_plan  fftw_plan;		/* Used if FFT_type == FFT_fftw */
> > +#else
> >    double     * shared, * * shared_ptr;
> > +#endif
> >
> >    /* Per-channel work area */
> >    int        WORK;  /* Start of work area is marked by this dummy variable.
> > */
> > @@ -84,9 +88,6 @@ typedef struct {
> >    double     block_norm, max;
> >    double     * magnitudes;	/* [(dft_size / 2) + 1] */
> >    float      * dBfs;
> > -#if HAVE_FFTW
> > -  fftw_plan  fftw_plan;		/* Used if FFT_type == FFT_fftw */
> > -#endif
> >  } priv_t;
> >
> >  #define secs(cols) \
> > +  /* We have one FFT plan per flow because the input/output arrays differ.
> > */
> > +  p->fftw_plan = fftw_plan_r2r_1d(p->dft_size, p->dft_buf, p->dft_buf,
> > +                      FFTW_R2HC, FFTW_MEASURE);
> > +}
> 
> This may not be right. FFTW3 plans depend on the memory addresses of
> the input and output vectors, so if you have two FFTs that are exactly
> the same except for the input and output buffer addresses, they need
> separate plans.
> In this case, the plan depends on dft_buf, which seems to be specific
> to each flow, so I think you should have a separare plan for each
> flow.

I'm not entirely sure what you mean, here.  Basically the shared_start
function doesn't change anything for FFTW users; and I've verified the
PNGs of using FFTW with/without my work-in-progress patch via cmp(1)
with identical results.

Or did you mean the location of fftw_plan field inside priv_t?
That doesn't seem to matter, does it?

I don't know the FFT stuff and math stuff well at all,
so I could be wrong.

> I don't know what "shared" is used for, so I just left it alone. AFter
> all, it's ore important that the code work, than to save four bytes of
> RAM and risk breaking it...

Heh, I did break it --without-fftw :x  I wasn't even thinking about
the RAM usage, but about documenting how the structure would be used.

Here's my work-in-progress fix, output now matches the FFTW-using PNG:

diff --git a/src/spectrogram.c b/src/spectrogram.c
index 1b82aad..d1e98c5 100644
--- a/src/spectrogram.c
+++ b/src/spectrogram.c
@@ -72,9 +72,8 @@ typedef struct {
   /* Shared work area */
 #if HAVE_FFTW
   fftw_plan  fftw_plan;		/* Used if FFT_type == FFT_fftw */
-#else
-  double     * shared, * * shared_ptr;
 #endif
+  double     * shared, * * shared_ptr; /* unused for FFTW */
 
   /* Per-channel work area */
   int        WORK;  /* Start of work area is marked by this dummy variable. */
@@ -170,6 +169,7 @@ static int getopts(sox_effect_t * effp, int argc, char **argv)
   p->spectrum_points += 2;
   if (p->alt_palette)
     p->spectrum_points = min(p->spectrum_points, (int)alt_palette_len);
+  p->shared_ptr = &p->shared;
   if (!strcmp(p->out_name, "-")) {
     if (effp->global_info->global_info->stdout_in_use_by) {
       lsx_fail("stdout already in use by `%s'", effp->global_info->global_info->stdout_in_use_by);
@@ -244,12 +244,14 @@ static void shared_start(sox_effect_t * effp)
 {
   priv_t * p = (priv_t *)effp->priv;
 
-  p->shared_ptr = &p->shared;
-  if (!is_p2(p->dft_size) && !effp->flow) {
-    if (p->y_size) {
-      p->shared = rdft_init((size_t)(p->dft_size));
+  if (!effp->flow) {
+    if (!is_p2(p->dft_size)) {
+      if (p->y_size) {
+        p->shared = rdft_init((size_t)(p->dft_size));
+      }
+    } else {
+      lsx_safe_rdft(p->dft_size, 1, p->dft_buf);
     }
-    lsx_safe_rdft(p->dft_size, 1, p->dft_buf);
   }
 }
 

> By the way, that "/* Used if FFT_type == FFT_fftw */" comment is
> stale, dating back to when I had an option to choose the FFT algorithm
> at runtime - sorry about that...

No worries.  Better we catch our mistakes now instead of
throwing a nasty heap at Chris and the gang when they
decide to return :)

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