Code review comment for lp:~mixxxcontributors/mixxx/features_vamp

Vittorio / Tobias

Great job on this branch! I'm very excited to get this in the hands of our users. I've been working on the branch for the past few weeks and here's a brief summary of the changes I made:

* I changed serialization/deserialization to use protocol buffers so that we will have fewer headaches in the future when extending the beat data format.

* I added vamp back into lib (sorry Vittorio for asking you to remove it!) I agree it simplifies things for people if it's just included.

* I moved all beat related tools into src/track/beatutils.cpp and factored everything I could out of AnalyserBeats.

* I changed fixed-tempo-assumption to create a BeatGrid instead of BeatMap. There's no sense making a list of beats when the grid is defined by the BPM and first-beat.

* I scaled back the offset-correction code (BeatUtils::calculateOffset) due to performance and quality concerns. I'm open to looking at turning it back on but it would often choose bad offsets on about 50% of my tracks. Maybe there is a bug?

* I disabled the logic for setting the BPM on a BeatMap that tried to insert or disable beats. For 1.11 non-fixed-tempo-assumption beatgrids (BeatMap) will be experimental and so we can improve this support later. For now changing the BPM merely scales the beats relative to the first beat by the ratio of the old to new BPM.

Those are the main things I changed but feel free to browse my commits to see everything.

I'm merging this into trunk now. Congratulations and thanks again! This will be a killer feature in 1.11.

There is still more work to be done but I think it can be improved over the course of the beta period for 1.11. Here is my list of TODOs:

* SoundTouch legacy support (for App-Store users if QM licensing issues are not resolved)
* BPM-above-range preference is not available in the new preferences dialog
* Allow rounding-when-within-0.05 to be optionally turned off.
* Sync doesn't currently work for BeatMaps. Phase is synced fine but the tempo-sync uses the global-bpm when it should only use a local-window BPM (because the global BPM may not be the current tempo).
* Once SoundTouch support is present, we can delete the old BPM preferences dialog and make VAMP required (remove ifdefs and all).