Martin Guy <martinw...@gmail.com> wrote:
> On 26/12/2015, Eric Wong <normalper...@yhbt.net> wrote:
> > --- 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) \
> > + /* 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);
> > +}
>
> This may not be right. FFTW3 plans depend on the memory addresses of
> the input and output vectors, so if you have two FFTs that are exactly
> the same except for the input and output buffer addresses, they need
> separate plans.
> In this case, the plan depends on dft_buf, which seems to be specific
> to each flow, so I think you should have a separare plan for each
> flow.
I'm not entirely sure what you mean, here. Basically the shared_start
function doesn't change anything for FFTW users; and I've verified the
PNGs of using FFTW with/without my work-in-progress patch via cmp(1)
with identical results.
Or did you mean the location of fftw_plan field inside priv_t?
That doesn't seem to matter, does it?
I don't know the FFT stuff and math stuff well at all,
so I could be wrong.
> I don't know what "shared" is used for, so I just left it alone. AFter
> all, it's ore important that the code work, than to save four bytes of
> RAM and risk breaking it...
Heh, I did break it --without-fftw :x I wasn't even thinking about
the RAM usage, but about documenting how the structure would be used.
Here's my work-in-progress fix, output now matches the FFTW-using PNG:
diff --git a/src/spectrogram.c b/src/spectrogram.c
index 1b82aad..d1e98c5 100644
--- a/src/spectrogram.c
+++ b/src/spectrogram.c
@@ -72,9 +72,8 @@ typedef struct {
/* Shared work area */
#if HAVE_FFTW
fftw_plan fftw_plan; /* Used if FFT_type == FFT_fftw */
-#else
- double * shared, * * shared_ptr;
#endif
+ double * shared, * * shared_ptr; /* unused for FFTW */
/* Per-channel work area */
int WORK; /* Start of work area is marked by this dummy variable. */
@@ -170,6 +169,7 @@ 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);
@@ -244,12 +244,14 @@ 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));
+ if (!effp->flow) {
+ if (!is_p2(p->dft_size)) {
+ if (p->y_size) {
+ p->shared = rdft_init((size_t)(p->dft_size));
+ }
+ } else {
+ lsx_safe_rdft(p->dft_size, 1, p->dft_buf);
}
- lsx_safe_rdft(p->dft_size, 1, p->dft_buf);
}
}
> By the way, that "/* Used if FFT_type == FFT_fftw */" comment is
> stale, dating back to when I had an option to choose the FFT algorithm
> at runtime - sorry about that...
No worries. Better we catch our mistakes now instead of
throwing a nasty heap at Chris and the gang when they
decide to return :)
------------------------------------------------------------------------------
_______________________________________________
SoX-devel mailing list
SoX-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sox-devel
