Add spectrogram layer, similar to the waveform graph

[GoogleCodeExporter]

I'm not sure if submitting an issue is the best way to do this, but it allows 
discussion and attachments, so I'm going to go with it.  Also, a short story, 
because I like stories.

I popped in here a long while back, on issue 132, which was the one that 
eventually led to the (awesome) waveform graph.  At that point, I had an idea 
for a different graphical representation (a spectrogram), presented pretty much 
identically to the waveform.  At the time I commented, the waveform graph 
wasn't implemented yet, and my proof-of-concept was very hacky and used 
pre-generated waveforms.

But!  Once I saw the waveform update, I modified the waveform code to instead 
generate a spectrogram, and it actually worked pretty well, but cannibalized 
the waveform graph.

Then this week I checked in here, and was delighted to find that EOF was not 
only still being maintained, but very actively!  I pulled the code I'd written 
a while back off an old hard drive, reworked it to function alongside the 
waveform graph, and it seems to be working just fine.

I've attached a patch based off the latest revision, r1130.  Since it requires 
new libraries to generate the spectrogram, I modified all the platform-specific 
makefiles, but am only set up to test it on Linux.  Judging by appearances, the 
same flags should apply to the other makefiles as well, but I can't be sure I 
didn't break anything.

On that note: since it does introduce another (possibly burdensome) library 
dependency in fftw3, if you think it worth doing I could go in and separate it 
out with some compile flags to make it an optional feature - conditionally stub 
out the functions called in the main code, and have compile flags around the 
fftw3 include.

There's also a quick fix in there to allow EOF to respond to close buttons, 
which it previously didn't.  I had to add another loop around the main loop to 
get it to work right, but it gets the job done just fine.

I've attached diffs/patches (just a patch -p1 < both.diff should do the trick) 
for each feature independently, and then for both combined.

Since I'm a git guy myself, I also mirrored the svn repo on my GitHub.  My 
changes are in the spectrogram branch:
https://github.com/cincodenada/editor-on-fire/commits/spectrogram

...whoops, sorry for the novel.
TL;DR: Look, I added a spectrogram layer and made the close button work!  I 
initially tried a while ago, but revived my code finally got it working right, 
and am quite glad to see that EOF is still being actively developed.  Yay!  
Patches!  Git mirror!  

Original issue reported on code.google.com by cincoden...@gmail.com on 19 Apr 2013 at 6:43

Attachments:

• spectrogram.patch
• close_button.patch
• both.patch

[GoogleCodeExporter]

Just realized maybe I should throw in some screenshots...here are a few that 
pretty much cover the functionality.  There's an example of it playing nicely 
with the waveform overlay as well.

Original comment by cincoden...@gmail.com on 19 Apr 2013 at 6:55

Attachments:

• song_menu.png
• settings_window.png
• in_action.png
• with_waveform.png

[GoogleCodeExporter]

The spectrogram patch works fine on Mac OS X after installing the extra 
dependency.

The close button code looks fine but it will malfunction if it is used while a 
dialog is showing inside EOF. For this to work properly, the 'eof_quit_mustask' 
variable would need to be reset to 0 after every blocking dialog call.

Original comment by xander4j...@yahoo.com on 19 Apr 2013 at 2:40

[GoogleCodeExporter]

Hmm, yes I noticed that when working on it yesterday...when I closed it it just 
waited until I closed the dialog to present the confirmation message, and then 
worked normally.  I'll take a look at it.

Original comment by cincoden...@gmail.com on 19 Apr 2013 at 6:44

[GoogleCodeExporter]

How did we want to integrate FFTW with EOF's source code?  Put the source files 
in or make it built separately (like Allegro and its dependencies)?

Original comment by raynebc on 19 Apr 2013 at 7:21

• Added labels: Type-Enhancement
• Removed labels: Type-Defect

[GoogleCodeExporter]

I would rather have it be a dependency since it is a large library with many 
source files. I think it is a well-established library so it shouldn't be a big 
deal to require people to have it installed if they want to build EOF.

Original comment by xander4j...@yahoo.com on 19 Apr 2013 at 9:12

[GoogleCodeExporter]

It would be easy to add an EOF_SPECTROGRAM compiler flag so that the related 
code only compiles if it is wanted.  What changes would need to be made to the 
makefiles so that the FFTW and spectrogram source code are only linked if they 
were wanted?  Would separate makefiles be needed?

Original comment by raynebc on 19 Apr 2013 at 9:39

[GoogleCodeExporter]

Separate library makes sense to me - it's readily available in Ubuntu's 
repositories, for instance, starting with Lucid (April 2010) and is backported 
to Hardy LTS (April 2008), so it shouldn't be difficult for people to get on 
Linux anyway.  I just tried a fresh clone/build on a different machine here and 
it worked fine with the default package.

As for conditionals, can compile flags take care of the dependencies?  There's 
just -lm and -lfftw3, plus the added o-files.  I'm not terribly familiar with 
them.  There are calls to spectrogram functions peppered throughout the main 
code, everywhere the waveform calls are - I could either conditionally stub 
those out in spectrogram.c as I mentioned, or put #ifdefs around all of the 
calls and just not compile spectrogram.c.

Original comment by cincoden...@gmail.com on 19 Apr 2013 at 10:02

[GoogleCodeExporter]

I was able to get EOF built and running with the great spectrogram code 
(updated and tested my Code::Blocks project and tested one of the makefiles), 
so r1131 adds the spectrogram logic.  We can iron out the details of 
deactivating the spectrogram logic based on compiler/linker settings in the 
next few commits.

Original comment by raynebc on 19 Apr 2013 at 11:54

• Changed state: Started

[GoogleCodeExporter]

The next EOF release candidate is long overdue, so for now, I'm going to bundle 
FFTW's license and copyright notices and readme file in a subfolder of the 
binary package.  I'm not sure if there's a more appropriate means to meet the 
requirements since the source code itself isn't going to be maintained in EOF's 
code base.

Original comment by raynebc on 20 Apr 2013 at 3:23

[GoogleCodeExporter]

For GPL you just need to include a copy of the license. Since EOF is 
open-source and free, I don't think it will be an issue. The GPL license has to 
be included with both source and binary releases.

It may be a good idea to mention in the license file which part is licensed 
under GPL so users can have a clearer picture of the copyrights on all of the 
code.

Original comment by xander4j...@yahoo.com on 20 Apr 2013 at 4:12

[GoogleCodeExporter]

To fix the close button, I would recommend just wrapping the few blocking 
dialog functions we use and grep-ing the source to replace calls to those 
functions with our own wrapped versions. The wrappers would fit nicely in 
'dialog.c'.

The main dialog function we use for custom dialogs is already there so we just 
need to deal with alert() and whatever else we are using.

Original comment by xander4j...@yahoo.com on 20 Apr 2013 at 4:16

[GoogleCodeExporter]

I'm not sure where in the source the GPL license should go, the root of the src 
folder?

Original comment by raynebc on 20 Apr 2013 at 5:25

[GoogleCodeExporter]

It should go with the binary files only so it is easier to package. The source 
code includes the binary data so it will work this way.

Original comment by xander4j...@yahoo.com on 20 Apr 2013 at 6:23

[GoogleCodeExporter]

File names like "COPYRIGHT" aren't very informative, should I keep the files in 
the FFTW subfolder so people know what they're about?  Or should we rename the 
files (ie. "FFTW COPYRIGHT")?

Original comment by raynebc on 20 Apr 2013 at 7:07

[GoogleCodeExporter]

I would just rename it. Something like 'FFTW_license.txt' should be fine (I 
prefer the filename not to have spaces). 

The main license file can mention that the FFTW library is used under the terms 
of the GPL and reference the name of the file so users can check it out if they 
care to.

Original comment by xander4j...@yahoo.com on 20 Apr 2013 at 8:13

[GoogleCodeExporter]

The license stuff should be covered as of r1134.

Original comment by raynebc on 20 Apr 2013 at 10:55

[GoogleCodeExporter]

After the changes that have been made to 5of0's original spectrogram logic, 
there are some differences in how the graph looks, especially with smaller 
window sizes (see attachments).  I reviewed all the changes I made and fixed 
one cached value issue with r1136, but I don't see where I made any other 
mistakes so I can't tell if this change was due to a bug in the original code 
or a bug in the changes I've made.  Would you be able to glance at it to see if 
I goofed it up?

Original comment by raynebc on 21 Apr 2013 at 2:06

Attachments:

• r1131.jpg
• r1136.jpg

[GoogleCodeExporter]

Yeah, I'll take a look - I've been doing some other improvements that may also 
fix things.  I'll pull down your changes and look at it in the next couple days.

Original comment by cincoden...@gmail.com on 21 Apr 2013 at 6:23

[GoogleCodeExporter]

Actually, I can't reproduce that difference anymore even though I kept the 
executable created with r1131.  I must have just been mistaken about which 
settings or audio were being used in each.

Original comment by raynebc on 22 Apr 2013 at 10:03

[GoogleCodeExporter]

Hey all, not sure if this is the best place to update things.  I've been off on 
vacation and otherwise keeping busy, but I've also ported over a few more 
features of the spectrogram display from my first attempt, as well as adding 
some more cacheing to hopefully improve (somewhat) on the speed while 
displaying it.  I've finally gotten them all wrapped up into a clean patch on 
r1148.

There are quite a few changes here, which are again more parted out over on 
GitHub, but I've tried to comment helpfully.  Changes of note:

* Added a little more docs to the html documentation - this is actually merging 
what I had written while you were adding your own docs, so edit as you see fit.
* Adding ability to view just a given y-range of the spectrogram, specified by 
note names, with my supporting helper includes.
* Caching the colorscale table, to avoid calculating colors every time
* Caching the y-axis pixel->frequency map, to avoid calculating that every time
* Removing the averaging if there were multiple bins in a pixel to improve 
speed (this doesn't seem to affect the display much at all)
* Rearranging the settings dialog for new options, removing the built-in 32 
window size (not a useful window size)

I also noticed the re-creation bug you'd fixed as well while I was debugging 
things - thanks :)  I've attached a patch with all my changes - it's a big 
patch, but I've tried to comment well and such.  Feel free to ask about things, 
here or I'll be in #fofix on and off this week.  There are probably more 
optimizations/caching/etc that could be done, but I'm not really sure what 
would be the best way to approach it - it's similar to the waveform in a lot of 
ways, but is obviously more intensive to render.

Original comment by cincoden...@gmail.com on 19 May 2013 at 11:42

Attachments:

• spectrogram_improvements.patch

[GoogleCodeExporter]

Thanks, I uploaded your changes in r1149.  Rendering performance is HUGELY 
increased!  One thing though is that no matter which options I pick, the graph 
looks a little different (ie. grainier) than it used to when using a 1024 
window size.  I'm not sure why that is.

Original comment by raynebc on 20 May 2013 at 10:42

[GoogleCodeExporter]

Heh, nice little benchmarking tool you threw together there.  I did some 
comparative runs with gprof and the numbers looked good, but the image sequence 
dual-purposing is a much more accurate measure.  Glad things are looking good 
there too!

Off the top of my head, the graininess may be a symptom of disabling averaging. 
 Did you try checking the avg bins option?  I know that option does 
*something*, but I may have inadvertently changed how the averaging works.  
I'll compare some screenshots between builds and see myself.

Original comment by cincoden...@gmail.com on 20 May 2013 at 11:11

[GoogleCodeExporter]

I tried every combination of the options (average, log plot, both, neither), 
and I never saw identical graphics compared to your original spectrogram code, 
so there may be some problem that creeped up.

Original comment by raynebc on 22 May 2013 at 7:17

[GoogleCodeExporter]

Minor graphical differences aside, I don't think there's much else to 
accomplish with the spectrogram.  Should we go ahead and close this out?

Original comment by raynebc on 25 Apr 2014 at 10:17

[GoogleCodeExporter]

I'll go ahead and close this out, it seems about as optimized as it will get.

Original comment by raynebc on 17 Dec 2014 at 9:48

• Changed state: Fixed