Martin Guy <martinw...@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 <normalper...@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
------------------------------------------------------------------------------
_______________________________________________
SoX-devel mailing list
SoX-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sox-devel
