Re: sox spectrogram patches

[Eric Wong <normalperson@yhbt.net>]

Martin Guy <martinwguy@gmail.com> wrote:
>
> "cmp" tells me that the PNG files are identical with/without FFTW

Thanks.  I'll note that if I get around to making a test suite.

> Yes, I've done relatively little configure.ac hacking and was hoping
> someone who knows better might improve things. Thanks for the heads-up
> on these possible issues

No problem.  I'm still not too experienced in this area,
either but I started forcing myself to learn in another project.

> On 26/12/2015, Eric Wong <normalperson@yhbt.net> wrote:
> >
> > I'd prefer if we could avoid CPP inside C functions entirely;
> > but that's a larger task and can be split into another patch.
> 
> Do you mean, where there is a configure option, have two separate
> functions, one with the HAVE_ code and one with the HAVE_NOT code?
> Yes, it's ugly, I agree.
> One alternative would be to use "if (HAVE_FFTW)" and let the compiler
> turn if(0) or if(1) into constant code removal.

I mean defining functions with common signatures so using
them is transparent to higher-level callers.

The #ifdef in the struct seems inevitable, but below I've created
shared_{start,stop} functions which do the same thing inside
their respective "start" and "stop" functions.  It helps identify
common code and even brought me to find a memory leak (see below).

diff --git a/src/spectrogram.c b/src/spectrogram.c
index fb7313e..1b82aad 100644
--- 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) \
@@ -169,7 +170,6 @@ 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);
@@ -204,8 +204,21 @@ static double make_window(priv_t * p, int end)
   return sum;
 }
 
-#if !HAVE_FFTW
+#if HAVE_FFTW
+/* Initialize the FFT routine */
+static void shared_start(sox_effect_t * effp)
+{
+  priv_t * p = (priv_t *)effp->priv;
+  /* 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);
+}
+
+static void shared_stop(priv_t * p) {
+  (void)p; /* nothing */
+}
 
+#else /* !HAVE_FFTW */
 static double * rdft_init(size_t n)
 {
   double * q = lsx_malloc(2 * (n / 2 + 1) * n * sizeof(*q)), * p = q;
@@ -227,7 +240,24 @@ static void rdft_p(double const * q, double const * in, double * out, int n)
   }
 }
 
-#endif /* HAVE_FFTW */
+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));
+    }
+    lsx_safe_rdft(p->dft_size, 1, p->dft_buf);
+  }
+}
+
+static void shared_stop(priv_t * p)
+{
+  free(p->shared);
+}
+#endif /* !HAVE_FFTW */
 
 static int start(sox_effect_t * effp)
 {
@@ -277,10 +307,6 @@ static int start(sox_effect_t * effp)
 
   if (p->y_size) {
     p->dft_size = 2 * (p->y_size - 1);
-#if !HAVE_FFTW
-    if (!is_p2(p->dft_size) && !effp->flow)
-      p->shared = rdft_init((size_t)(p->dft_size));
-#endif
   } else {
    int y = max(32, (p->Y_size? p->Y_size : 550) / effp->in_signal.channels - 2);
    for (p->dft_size = 128; p->dft_size <= y; p->dft_size <<= 1);
@@ -292,15 +318,7 @@ static int start(sox_effect_t * effp)
   p->window	= lsx_calloc(p->dft_size + 1, sizeof(*(p->window)));
   p->magnitudes = lsx_calloc((p->dft_size / 2) + 1, sizeof(*(p->magnitudes)));
 
-  /* Initialize the FFT routine */
-#if HAVE_FFTW
-  /* 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);
-#else
-  if (is_p2(p->dft_size) && !effp->flow)
-    lsx_safe_rdft(p->dft_size, 1, p->dft_buf);
-#endif
+  shared_start(effp);
 
   lsx_debug("duration=%g x_size=%i pixels_per_sec=%g dft_size=%i", duration, p->x_size, pixels_per_sec, p->dft_size);
 
@@ -596,7 +614,7 @@ static int stop(sox_effect_t * effp) /* only called, by end(), on flow 0 */
   double      limit;
   float       autogain = 0.0;	/* Is changed if the -n flag was supplied */
 
-  free(p->shared);
+  shared_stop(p);
   if (p->using_stdout) {
     SET_BINARY_MODE(stdout);
     file = stdout;

Which also brings me to the question:

  Do we need to we need to teardown fftw_plan to avoid resource leaks?

valgrind --leak-check=full says we do:

18,040 (24 direct, 18,016 indirect) bytes in 1 blocks are definitely lost in loss record 204 of 206
   at 0x4C27ED6: memalign (vg_replace_malloc.c:755)
   by 0x5D67944: fftw_malloc_plain (in /usr/lib/x86_64-linux-gnu/libfftw3.so.3.3.2)
   by 0x5E2EFBE: fftw_mkapiplan (in /usr/lib/x86_64-linux-gnu/libfftw3.so.3.3.2)
   by 0x5E338E2: fftw_plan_many_r2r (in /usr/lib/x86_64-linux-gnu/libfftw3.so.3.3.2)
   by 0x5E33A28: fftw_plan_r2r (in /usr/lib/x86_64-linux-gnu/libfftw3.so.3.3.2)
   by 0x5E33948: fftw_plan_r2r_1d (in /usr/lib/x86_64-linux-gnu/libfftw3.so.3.3.2)
   by 0x4E8560F: start (spectrogram.c:298)
   by 0x4E5CF4F: sox_add_effect (effects.c:157)
   by 0x406EB0: add_effect (sox.c:708)
   by 0x403CED: main (sox.c:1073)

24,744 (24 direct, 24,720 indirect) bytes in 1 blocks are definitely lost in loss record 205 of 206
   at 0x4C27ED6: memalign (vg_replace_malloc.c:755)
   by 0x5D67944: fftw_malloc_plain (in /usr/lib/x86_64-linux-gnu/libfftw3.so.3.3.2)
   by 0x5E2EFBE: fftw_mkapiplan (in /usr/lib/x86_64-linux-gnu/libfftw3.so.3.3.2)
   by 0x5E338E2: fftw_plan_many_r2r (in /usr/lib/x86_64-linux-gnu/libfftw3.so.3.3.2)
   by 0x5E33A28: fftw_plan_r2r (in /usr/lib/x86_64-linux-gnu/libfftw3.so.3.3.2)
   by 0x5E33948: fftw_plan_r2r_1d (in /usr/lib/x86_64-linux-gnu/libfftw3.so.3.3.2)
   by 0x4E8560F: start (spectrogram.c:298)
   by 0x4E5D0D7: sox_add_effect (effects.c:202)
   by 0x406EB0: add_effect (sox.c:708)
   by 0x403CED: main (sox.c:1073)

Btw, I haven't touched "flow", that is an exercise for the reader :)

> Thanks again for stepping in to work on this. I was about to leave sox
> to die, given that its fathers have abandoned it. If we can get a last
> twitch of life out of them, it would be best they appoint you as its
> official maintainer.

No problem.  I hope they come back.  It is certainly a scary thought
if I were to become an official... anything :x

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