Hi guys, I'm going to split my comments into two pieces -- the first is a user experience review of the branch. The second will be a code review. A user of Mixxx 1.11 may be in a variety of states: 1) New user with no BPM analysis at all. 2) New user, some files already analyzed by external programs like RapidEvolution. 3) User of Mixxx with many files annotated with SoundTouch or manually tapped/entered BPMs and/or manually-chosen offsets in the Mixxx Library. Some of their files may even have BPM analyzed by external programs like RapidEvolution. Here is how I think we should handle users in these states: * Any SoundTouch BPM should just be thrown out and re-calculated as if it weren't there. * A BPM that originated from a music-metadata tag (i.e. from Rapid Evolution or maybe manually entered) should be respected by default -- but the first-beat offset should be automatically detected using QM. * Any manually entered BPMs and offsets in Mixxx should be respected by default and not overwritten to prevent us from destroying a user's hard-work in hand-detecting the BPMs of their tracks. This means keeping the BPM implementation as a BeatGrid and not a BeatMap on all tracks with pre-existing beatgrids until the user manually requests the track be analyzed via the Analyze view. The current implementation of the VAMP branch over-writes the current beat data for a track when it calculates a BeatMap/QM-analysis. Also, we can't tell the difference between a SoundTouch generated BPM and a manually entered BPM from Mixxx 1.10 and before. This means that to satisfy the requirements I set out above, we can't over-write any pre-existing BPM by default. If no offset exists or the offset is 0 then I would be fine with auto-detecting the offset. This means that some users will not get the benefit of the new beat analyzer without manual action. This is too bad but necessary. We can't afford to anger a user by overwriting all of their data. To facilitate this: * Disable AnalyserBeats beat detection if a previous BeatGrid is present. * A preference option that says something like "Overwrite existing track beat data with VAMP-plugin beat data." defaulting to unchecked/off. * On upgrade to a 1.11 database we present a welcome screen either similar the new promo-tracks welcome message or maybe a popup box that asks the user if they want to over-write all BPMs with new, much more accurate beat data. This will make sure that new 1.11 users will be unlikely to miss the opportunity to get great new beats detected for their tracks. ============================ The following is a laundry list of various usability issues I ran into by just playing with the VAMP branch for a while: * We need a preference option that says "re-detect beats in tracks if my settings change". Right now if I change my preferences, there is no way to force a track's beats to be re-calculated. * BPM Lock -- doesn't affect anything other than AnalyserBeats. Needs to prevent Mixxx from ever editing that BPM -- such as track information dialog, editing in library view, and tapping via BPM tap widgets both near the deck and in the track information dialog. * BPM lock being its own library column isn't ideal -- in the best case it would be a clickable icon next to each BPM value in the library and display inline. Similar to how the played checkbox is in the "play count" column. The lock checkbox should be in the BPM column. * Why are there two BPM detection preferences sections? These should be merged into one. * "Reset" button in the Beat Detection dialog should say "Reset to Defaults" instead of "Reset". Hard to tell what this does. (I realize "Reset" is used in other preference dialogs and I think those should change too but don't worry about that for this branch). * Please provide a help description for the options in "Beat Detection". It's not very self-explanatory. - What does "enable beat analysis" do? What happens if it isn't on? Do I still get BPMs? - What is fixed tempo optimization? - What is offset correction? - What are the downsides of fast analysis. If there were no downsides why is this even an option? * Plugin drop-down menu is confusing. Maybe add a help link which goes to the Mixxx wiki page about this? * BPM Detection preferences window is confusing - Whats the difference between "Beat Analysis Enabled" and "Enable BPM Detection" on these two pages? - Detect BPM on import is greyed out and unchecked. Maybe remove this? - Now that we have VAMP, are BPM Schemes needed anymore? * If the user loads a song (whose overview waveform is already calculated) with an old-style BPM, there's no indicator that we are recalculating the BPM. The BPM/beats magically jump about 10 seconds later. * As of lp:2779 tracks that are already analyzed with the QM plugin are re-analyzed every time. (I think this is caused by a typo in BeatMap::getVersion()) * If I'm playing two tracks at their normal rates, one at 59BPM and one at 120BPM, Mixxx 1.10.0 would only sync the 59BPM to play a little bit faster at 60bpm. This is because scaling a song at 59BPM to 120BPM is going to result in very poor audio quality. The VAMP branch disables this feature, but it is useful even if the BPMs detected are perfect. * In the track-info dialog, the TAP and BPM spin-box are disabled for tracks. I have no idea why. (I'm speaking from a user's perspective -- I can see how this is done in code). Why disable these based on how the beats were detected for a given track? If the user wants to tap out a new BPM or change it, why not let them? It's especially confusing since you can edit the BPM via the library browser and not via the track info dialog. This is getting quite long now so I'll write anything else I come up with in another comment.