Martin Guy <martinw...@gmail.com> wrote:
> On 26/12/2015, Eric Wong <normalper...@yhbt.net> 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
------------------------------------------------------------------------------
_______________________________________________
SoX-devel mailing list
SoX-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sox-devel
