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 10:43:16 +0000 Message-ID: <20151226104316.GA5868@dcvr.yhbt.net> References: 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 1451126632 5103 80.91.229.3 (26 Dec 2015 10:43:52 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sat, 26 Dec 2015 10:43:52 +0000 (UTC) To: sox-devel@lists.sourceforge.net Original-X-From: sox-devel-bounces@lists.sourceforge.net Sat Dec 26 11:43:44 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: 1aCmJm-0006rh-PD 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:455 Archived-At: Received: from lists.sourceforge.net ([216.34.181.88]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1aCmK7-0002xs-5Q for gcasd-sox-devel@m.gmane.org; Sat, 26 Dec 2015 11:43:43 +0100 Received: from localhost ([127.0.0.1] helo=sfs-ml-4.v29.ch3.sourceforge.com) by sfs-ml-4.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1aCmJo-0001lK-Tu; Sat, 26 Dec 2015 10:43:24 +0000 Received: from sog-mx-1.v43.ch3.sourceforge.com ([172.29.43.191] helo=mx.sourceforge.net) by sfs-ml-4.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1aCmJo-0001lC-C7 for sox-devel@lists.sourceforge.net; Sat, 26 Dec 2015 10:43:24 +0000 Received: from dcvr.yhbt.net ([64.71.152.64]) by sog-mx-1.v43.ch3.sourceforge.com with esmtp (Exim 4.76) id 1aCmJm-0006rh-PD for sox-devel@lists.sourceforge.net; Sat, 26 Dec 2015 10:43:24 +0000 Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id 1C8E21F724; Sat, 26 Dec 2015 10:43:17 +0000 (UTC) Martin Guy wrote: > On 26/12/2015, Eric Wong wrote: > > I've started maintaining a branch of things in their absence. > > Thanks Eric. I have three commits, all regarding "sox spectrogram": > - remove arbitrary limit on spectrogram output height (was 1200) Seems alright, did you check for possible integer overflow issues from raising the limit? I was mildly alarmed at this: + if (end) memset(p->window, 0, sizeof(*(p->window)) * p->dft_size); Until I noticed p->window was allocated with calloc which does overflow checking: p->window = lsx_calloc(p->dft_size + 1, sizeof(*(p->window))); > - add "spectrogram -n" flag to normalize the image brightness > regardless of audio file loudness No issues here. > - add FFTW3 support, which is between 150 and 250 times faster than > the slow built-in FFT routine Nice! I've tested with/without FFTW3 installed and can confirm the speedup. I do have problems with eyesight and do not see well; so I can't comment on actual images. I've made some minor edits to configure.ac for portability and robustness (I realize you're taking nearby statements as a guideline, but I think we can do better when introducing new code) - prefer AS_IF macro to generate portable "if" statements in sh I also realize you're only testing auto-generated variables, so the next two are unnecessary, but can aid in avoiding red flags for other reviewers: - avoid "-a" with "test" since it is error-prone compared to using "&&" and multiple "test" statements. E.g. test -n "$x" -a "$a" = "$b" is buggy if "$x" is "=" (example taken from Documentation/CodingGuidelines in git.git) - prefix variables with "x" in "test" conditionals. E.g. test "$var" = yes is buggy if "$var" is "-n" or other "test" operators We avoid this problem with: test "x$var" = xyes I'm also trying to keep configure.ac wrapped at 80 columns (I wish 64 columns were standard, I want to use bigger fonts). I also added #endif labels for the larger flow function. 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. So I'll squash the following and merge into my "pu" branch for now: diff --git a/configure.ac b/configure.ac index 4bc3064..794f19c 100644 --- a/configure.ac +++ b/configure.ac @@ -337,19 +337,18 @@ AC_ARG_WITH(fftw, AS_HELP_STRING([--without-fftw], [Don't try to use FFTW])) using_fftw=no -if test "$with_fftw" != "no"; then - AC_CHECK_HEADERS(fftw3.h, using_fftw=yes) - if test "$using_fftw" = "yes"; then - AC_CHECK_LIB(fftw3, fftw_execute, FFTW_LIBS="$FFTW_LIBS -lfftw3", using_fftw=no) - fi - if test "$with_fftw" = "yes" -a "$using_fftw" = "no"; then - AC_MSG_FAILURE([cannot find FFTW3]) - fi -fi -if test "$using_fftw" = yes; then - AC_DEFINE(HAVE_FFTW, 1, [Define to 1 if you have FFTW3.]) -fi -AM_CONDITIONAL(HAVE_FFTW, test x$using_fftw = xyes) +AS_IF([test "x$with_fftw" != xno], [ + AC_CHECK_HEADERS([fftw3.h], [using_fftw=yes]) + AS_IF([test "x$using_fftw" = xyes], + [ AC_CHECK_LIB([fftw3], [fftw_execute], + [FFTW_LIBS="$FFTW_LIBS -lfftw3"], [using_fftw=no]) ]) + + AS_IF([test "x$with_fftw" = xyes && test "x$using_fftw" = xno], + [ AC_MSG_FAILURE([cannot find FFTW3]) ]) +]) +AS_IF([test "x$using_fftw" = xyes], + [ AC_DEFINE(HAVE_FFTW, 1, [Define to 1 if you have FFTW3.]) ]) +AM_CONDITIONAL(HAVE_FFTW, test x"$using_fftw" = xyes) AC_SUBST(FFTW_LIBS) diff --git a/src/spectrogram.c b/src/spectrogram.c index 3d8c208..fb7313e 100644 --- a/src/spectrogram.c +++ b/src/spectrogram.c @@ -392,7 +392,7 @@ static int flow(sox_effect_t * effp, ]); } p->magnitudes[p->dft_size / 2] += sqr(p->dft_buf[p->dft_size / 2]); -#else +#else /* ! HAVE_FFTW */ if (is_p2(p->dft_size)) { lsx_safe_rdft(p->dft_size, 1, p->dft_buf); p->magnitudes[0] += sqr(p->dft_buf[0]); @@ -401,7 +401,7 @@ static int flow(sox_effect_t * effp, p->magnitudes[p->dft_size >> 1] += sqr(p->dft_buf[1]); } else rdft_p(*p->shared_ptr, p->dft_buf, p->magnitudes, p->dft_size); -#endif +#endif /* ! HAVE_FFTW */ if (++p->block_num == p->block_steps && do_column(effp) == SOX_EOF) return SOX_EOF; > They are the last three commits on https://github.com/martinwguy/sox.git > which is a fork of the main sf.net repository and are also attached. Thanks, I've also setup a mirror of yours at: git://80x24.org/mirrors/martinwguy/sox.git (as well as one for Mans @ s/martinwguy/mansr/) And I forgot to mention I made a mirror of my own repo at: git://repo.or.cz/sox/ew.git ------------------------------------------------------------------------------