From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Eric Wong Newsgroups: gmane.comp.audio.sox.devel Subject: Re: sox spectrogram patches Date: Sat, 26 Dec 2015 21:18:13 +0000 Message-ID: <20151226211813.GA13684@dcvr.yhbt.net> References: <20151226104316.GA5868@dcvr.yhbt.net> Reply-To: sox-devel@lists.sourceforge.net NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit X-Trace: ger.gmane.org 1451164723 9323 80.91.229.3 (26 Dec 2015 21:18:43 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sat, 26 Dec 2015 21:18:43 +0000 (UTC) To: sox-devel@lists.sourceforge.net Original-X-From: sox-devel-bounces@lists.sourceforge.net Sat Dec 26 22:18:36 2015 Return-path: Envelope-to: gcasd-sox-devel@m.gmane.org X-ACL-Warn: Content-Disposition: inline In-Reply-To: X-Spam-Score: -0.0 (/) X-Spam-Report: Spam Filtering performed by mx.sourceforge.net. See http://spamassassin.org/tag/ for more details. -0.0 RP_MATCHES_RCVD Envelope sender domain matches handover relay domain X-Headers-End: 1aCwEF-0007qx-3N X-BeenThere: sox-devel@lists.sourceforge.net X-Mailman-Version: 2.1.9 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: sox-devel-bounces@lists.sourceforge.net Xref: news.gmane.org gmane.comp.audio.sox.devel:457 Archived-At: Received: from lists.sourceforge.net ([216.34.181.88]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1aCwEV-0007cg-Eq for gcasd-sox-devel@m.gmane.org; Sat, 26 Dec 2015 22:18:35 +0100 Received: from localhost ([127.0.0.1] helo=sfs-ml-3.v29.ch3.sourceforge.com) by sfs-ml-3.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1aCwEH-00082s-HL; Sat, 26 Dec 2015 21:18:21 +0000 Received: from sog-mx-4.v43.ch3.sourceforge.com ([172.29.43.194] helo=mx.sourceforge.net) by sfs-ml-3.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1aCwEG-000825-Qh for sox-devel@lists.sourceforge.net; Sat, 26 Dec 2015 21:18:20 +0000 Received: from dcvr.yhbt.net ([64.71.152.64]) by sog-mx-4.v43.ch3.sourceforge.com with esmtp (Exim 4.76) id 1aCwEF-0007qx-3N for sox-devel@lists.sourceforge.net; Sat, 26 Dec 2015 21:18:20 +0000 Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id 736461F724; Sat, 26 Dec 2015 21:18:13 +0000 (UTC) Martin Guy 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 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 ------------------------------------------------------------------------------