From 4c350120596d48a18c56e2dde266d99f37ad2a3c Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Thu, 20 May 2021 19:05:35 +0200 Subject: [PATCH 01/10] SeratoBeatsImporter: Fix import of terminal marker beats --- src/track/serato/beatsimporter.cpp | 42 ++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 8 deletions(-) diff --git a/src/track/serato/beatsimporter.cpp b/src/track/serato/beatsimporter.cpp index dc67ab9d9da..2c77976d7fd 100644 --- a/src/track/serato/beatsimporter.cpp +++ b/src/track/serato/beatsimporter.cpp @@ -32,7 +32,7 @@ QVector SeratoBeatsImporter::importBeatsAndApplyTimingOffset( filePath, streamInfo.getSignalInfo()); QVector beats; - double beatPositionMillis; + double beatPositionMillis = 0; double beatLengthMillis; // Calculate beat positions for non-terminal markers for (int i = 0; i < m_nonTerminalMarkers.size(); ++i) { @@ -59,20 +59,46 @@ QVector SeratoBeatsImporter::importBeatsAndApplyTimingOffset( beats.reserve(beats.size() + pMarker->beatsTillNextMarker()); for (quint32 j = 0; j < pMarker->beatsTillNextMarker(); ++j) { - beats.append(streamInfo.getSignalInfo().millis2frames( - beatPositionMillis + (j * beatLengthMillis))); + beats.append(streamInfo.getSignalInfo().millis2frames(beatPositionMillis)); + beatPositionMillis += beatLengthMillis; } } - // Calculate remaining beat positions until track end - beatPositionMillis = - static_cast(m_pTerminalMarker->positionSecs()) * 1000 + - timingOffsetMillis; + // Calculate remaining beat positions between the last non-terminal marker + // beat (or the start of the track if none exist) and the terminal marker, + // using the given BPM. + // + // beatPositionMillis Terminal Marker + // v v + // | | | | | | | | |[###### Remaining range ######] beatLengthMillis = 60000.0 / static_cast(m_pTerminalMarker->bpm()); VERIFY_OR_DEBUG_ASSERT(m_pTerminalMarker->positionSecs() >= 0 && beatLengthMillis > 0) { return {}; } - while (beatPositionMillis < streamInfo.getDuration().toDoubleMillis()) { + + const double rangeEndBeatPositionMillis = + static_cast(m_pTerminalMarker->positionSecs()) * 1000 + + timingOffsetMillis; + + if (beats.isEmpty()) { + VERIFY_OR_DEBUG_ASSERT(beatPositionMillis == 0) { + return {}; + } + // If there are no beats yet (because there were no non-terminal + // markers), beatPositionMillis will be 0. In that case we have to find + // the offset backwards from the terminal marker position. + // Because we're working with doubles, we can't use modulo and need to + // implement it ourselves. + const double divisor = std::floor(rangeEndBeatPositionMillis / beatLengthMillis); + beatPositionMillis = rangeEndBeatPositionMillis - (divisor * beatLengthMillis); + } else { + // Add beatLengthMillis, otherwise we'd import the same beat position + // twice (we already imported it above). + beatPositionMillis += beatLengthMillis; + } + + // Now fill the range with beats until the end is reached + while (beatPositionMillis < rangeEndBeatPositionMillis) { beats.append(streamInfo.getSignalInfo().millis2frames(beatPositionMillis)); beatPositionMillis += beatLengthMillis; } From 267218670737176d1e635248642543b2c71d9982 Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Thu, 20 May 2021 23:22:22 +0200 Subject: [PATCH 02/10] SeratoBeatsImporter: Simplify calculations that use timingOffsetMillis --- src/track/serato/beatsimporter.cpp | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/track/serato/beatsimporter.cpp b/src/track/serato/beatsimporter.cpp index 2c77976d7fd..6e2347eef9b 100644 --- a/src/track/serato/beatsimporter.cpp +++ b/src/track/serato/beatsimporter.cpp @@ -28,7 +28,7 @@ QVector SeratoBeatsImporter::importBeatsAndApplyTimingOffset( return {}; } - double timingOffsetMillis = SeratoTags::guessTimingOffsetMillis( + const double timingOffsetMillis = SeratoTags::guessTimingOffsetMillis( filePath, streamInfo.getSignalInfo()); QVector beats; @@ -37,20 +37,17 @@ QVector SeratoBeatsImporter::importBeatsAndApplyTimingOffset( // Calculate beat positions for non-terminal markers for (int i = 0; i < m_nonTerminalMarkers.size(); ++i) { SeratoBeatGridNonTerminalMarkerPointer pMarker = m_nonTerminalMarkers.at(i); - beatPositionMillis = - static_cast(pMarker->positionSecs()) * 1000 + - timingOffsetMillis; + beatPositionMillis = static_cast(pMarker->positionSecs()) * 1000; VERIFY_OR_DEBUG_ASSERT(pMarker->positionSecs() >= 0 && pMarker->beatsTillNextMarker() > 0) { return {}; } - double nextBeatPositionMillis = + const double nextBeatPositionMillis = static_cast(i == (m_nonTerminalMarkers.size() - 1) ? m_pTerminalMarker->positionSecs() : m_nonTerminalMarkers.at(i + 1) ->positionSecs()) * - 1000 + - timingOffsetMillis; + 1000; VERIFY_OR_DEBUG_ASSERT(nextBeatPositionMillis > beatPositionMillis) { return {}; } @@ -59,7 +56,8 @@ QVector SeratoBeatsImporter::importBeatsAndApplyTimingOffset( beats.reserve(beats.size() + pMarker->beatsTillNextMarker()); for (quint32 j = 0; j < pMarker->beatsTillNextMarker(); ++j) { - beats.append(streamInfo.getSignalInfo().millis2frames(beatPositionMillis)); + beats.append(streamInfo.getSignalInfo().millis2frames( + beatPositionMillis + timingOffsetMillis)); beatPositionMillis += beatLengthMillis; } } @@ -77,8 +75,7 @@ QVector SeratoBeatsImporter::importBeatsAndApplyTimingOffset( } const double rangeEndBeatPositionMillis = - static_cast(m_pTerminalMarker->positionSecs()) * 1000 + - timingOffsetMillis; + static_cast(m_pTerminalMarker->positionSecs()) * 1000; if (beats.isEmpty()) { VERIFY_OR_DEBUG_ASSERT(beatPositionMillis == 0) { @@ -99,7 +96,8 @@ QVector SeratoBeatsImporter::importBeatsAndApplyTimingOffset( // Now fill the range with beats until the end is reached while (beatPositionMillis < rangeEndBeatPositionMillis) { - beats.append(streamInfo.getSignalInfo().millis2frames(beatPositionMillis)); + beats.append(streamInfo.getSignalInfo().millis2frames( + beatPositionMillis + timingOffsetMillis)); beatPositionMillis += beatLengthMillis; } From b4f12af6b9ec027e784d40df166d978db4bb269f Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Thu, 20 May 2021 23:23:41 +0200 Subject: [PATCH 03/10] SeratoBeatsImporter: Use separate beatLengthMillis variables --- src/track/serato/beatsimporter.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/track/serato/beatsimporter.cpp b/src/track/serato/beatsimporter.cpp index 6e2347eef9b..02c6c562200 100644 --- a/src/track/serato/beatsimporter.cpp +++ b/src/track/serato/beatsimporter.cpp @@ -33,7 +33,6 @@ QVector SeratoBeatsImporter::importBeatsAndApplyTimingOffset( QVector beats; double beatPositionMillis = 0; - double beatLengthMillis; // Calculate beat positions for non-terminal markers for (int i = 0; i < m_nonTerminalMarkers.size(); ++i) { SeratoBeatGridNonTerminalMarkerPointer pMarker = m_nonTerminalMarkers.at(i); @@ -51,7 +50,8 @@ QVector SeratoBeatsImporter::importBeatsAndApplyTimingOffset( VERIFY_OR_DEBUG_ASSERT(nextBeatPositionMillis > beatPositionMillis) { return {}; } - beatLengthMillis = (nextBeatPositionMillis - beatPositionMillis) / + const double beatLengthMillis = + (nextBeatPositionMillis - beatPositionMillis) / pMarker->beatsTillNextMarker(); beats.reserve(beats.size() + pMarker->beatsTillNextMarker()); @@ -69,7 +69,7 @@ QVector SeratoBeatsImporter::importBeatsAndApplyTimingOffset( // beatPositionMillis Terminal Marker // v v // | | | | | | | | |[###### Remaining range ######] - beatLengthMillis = 60000.0 / static_cast(m_pTerminalMarker->bpm()); + const double beatLengthMillis = 60000.0 / static_cast(m_pTerminalMarker->bpm()); VERIFY_OR_DEBUG_ASSERT(m_pTerminalMarker->positionSecs() >= 0 && beatLengthMillis > 0) { return {}; } From 26c7167397a7c18658da7af08f6e3d9580edfbf8 Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Fri, 21 May 2021 00:20:58 +0200 Subject: [PATCH 04/10] SeratoBeatsImporter: Add fix import of last beat --- src/track/serato/beatsimporter.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/track/serato/beatsimporter.cpp b/src/track/serato/beatsimporter.cpp index 02c6c562200..297f385b49f 100644 --- a/src/track/serato/beatsimporter.cpp +++ b/src/track/serato/beatsimporter.cpp @@ -92,10 +92,18 @@ QVector SeratoBeatsImporter::importBeatsAndApplyTimingOffset( // Add beatLengthMillis, otherwise we'd import the same beat position // twice (we already imported it above). beatPositionMillis += beatLengthMillis; + + // Sometimes the previous calculation can lead to a position behind the + // terminal marker position, because the BPM value is inaccurate. In + // that case, use the rangeEndBeatPositionMillis directly. + if (beatPositionMillis > rangeEndBeatPositionMillis) { + beatPositionMillis = rangeEndBeatPositionMillis; + } } - // Now fill the range with beats until the end is reached - while (beatPositionMillis < rangeEndBeatPositionMillis) { + // Now fill the range with beats until the end is reached. Add a half beat + // length, to make sure that the last beat is actually included. + while (beatPositionMillis <= (rangeEndBeatPositionMillis + beatLengthMillis / 2)) { beats.append(streamInfo.getSignalInfo().millis2frames( beatPositionMillis + timingOffsetMillis)); beatPositionMillis += beatLengthMillis; From 9ae09361c98d6dc804b6956d3b41e5b9f4e73d77 Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Fri, 21 May 2021 11:03:40 +0200 Subject: [PATCH 05/10] SeratoBeatsImporter: Add private helper method to simplify tests later --- src/track/serato/beatsimporter.cpp | 16 +++++++++++----- src/track/serato/beatsimporter.h | 3 +++ 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/track/serato/beatsimporter.cpp b/src/track/serato/beatsimporter.cpp index 297f385b49f..3d40047bc1d 100644 --- a/src/track/serato/beatsimporter.cpp +++ b/src/track/serato/beatsimporter.cpp @@ -24,13 +24,19 @@ bool SeratoBeatsImporter::isEmpty() const { QVector SeratoBeatsImporter::importBeatsAndApplyTimingOffset( const QString& filePath, const audio::StreamInfo& streamInfo) { + const audio::SignalInfo& signalInfo = streamInfo.getSignalInfo(); + const double timingOffsetMillis = SeratoTags::guessTimingOffsetMillis( + filePath, signalInfo); + + return importBeatsAndApplyTimingOffset(timingOffsetMillis, signalInfo); +} + +QVector SeratoBeatsImporter::importBeatsAndApplyTimingOffset( + double timingOffsetMillis, const audio::SignalInfo& signalInfo) { VERIFY_OR_DEBUG_ASSERT(!isEmpty()) { return {}; } - const double timingOffsetMillis = SeratoTags::guessTimingOffsetMillis( - filePath, streamInfo.getSignalInfo()); - QVector beats; double beatPositionMillis = 0; // Calculate beat positions for non-terminal markers @@ -56,7 +62,7 @@ QVector SeratoBeatsImporter::importBeatsAndApplyTimingOffset( beats.reserve(beats.size() + pMarker->beatsTillNextMarker()); for (quint32 j = 0; j < pMarker->beatsTillNextMarker(); ++j) { - beats.append(streamInfo.getSignalInfo().millis2frames( + beats.append(signalInfo.millis2frames( beatPositionMillis + timingOffsetMillis)); beatPositionMillis += beatLengthMillis; } @@ -104,7 +110,7 @@ QVector SeratoBeatsImporter::importBeatsAndApplyTimingOffset( // Now fill the range with beats until the end is reached. Add a half beat // length, to make sure that the last beat is actually included. while (beatPositionMillis <= (rangeEndBeatPositionMillis + beatLengthMillis / 2)) { - beats.append(streamInfo.getSignalInfo().millis2frames( + beats.append(signalInfo.millis2frames( beatPositionMillis + timingOffsetMillis)); beatPositionMillis += beatLengthMillis; } diff --git a/src/track/serato/beatsimporter.h b/src/track/serato/beatsimporter.h index bebe8d2758d..403f76296c2 100644 --- a/src/track/serato/beatsimporter.h +++ b/src/track/serato/beatsimporter.h @@ -21,6 +21,9 @@ class SeratoBeatsImporter : public BeatsImporter { const audio::StreamInfo& streamInfo) override; private: + QVector importBeatsAndApplyTimingOffset( + double timingOffsetMillis, const audio::SignalInfo& signalInfo); + QList m_nonTerminalMarkers; SeratoBeatGridTerminalMarkerPointer m_pTerminalMarker; }; From bb0bc07bd0e6dc3ade8405d86761dd6ab7dc5b80 Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Fri, 21 May 2021 13:18:36 +0200 Subject: [PATCH 06/10] SeratoBeatGrid: Fix export of first beat if beat frame position = 0 --- src/track/serato/beatgrid.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/track/serato/beatgrid.cpp b/src/track/serato/beatgrid.cpp index 52f529a7ad2..66982c2b2ac 100644 --- a/src/track/serato/beatgrid.cpp +++ b/src/track/serato/beatgrid.cpp @@ -468,7 +468,7 @@ void SeratoBeatGrid::setBeats(BeatsPointer pBeats, const double currentDeltaFrames = currentBeatPositionFrames - previousBeatPositionFrames; const double differenceBetweenCurrentAndPreviousDelta = abs(currentDeltaFrames - previousDeltaFrames); - if (differenceBetweenCurrentAndPreviousDelta > kEpsilon) { + if (differenceBetweenCurrentAndPreviousDelta > kEpsilon || nonTerminalMarkers.isEmpty()) { // We are adding a new beat marker, therefore we need to update the // `beatsSinceLastMarker` variable of the last marker we added. if (!nonTerminalMarkers.isEmpty()) { From f6c5a030d531e2410be6ba114be5f7c3a486eca6 Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Fri, 21 May 2021 11:13:14 +0200 Subject: [PATCH 07/10] SeratoBeatGridTest: Add roundtrip checks to SerializeBeatMap test --- src/test/seratobeatgridtest.cpp | 32 +++++++++++++++++++++++++++++++- src/track/serato/beatsimporter.h | 4 ++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/test/seratobeatgridtest.cpp b/src/test/seratobeatgridtest.cpp index 105d02e7c2d..2d3b3d21187 100644 --- a/src/test/seratobeatgridtest.cpp +++ b/src/test/seratobeatgridtest.cpp @@ -8,10 +8,18 @@ #include "track/beatgrid.h" #include "track/beatmap.h" #include "track/serato/beatgrid.h" +#include "track/serato/beatsimporter.h" #include "util/memory.h" namespace { +// Maximum allowed frame position inaccuracy after reimport +constexpr double kEpsilon = 0.1; + +} // namespace + +namespace mixxx { + class SeratoBeatGridTest : public testing::Test { protected: bool parseBeatGridData(const QByteArray& inputValue, @@ -149,6 +157,17 @@ TEST_F(SeratoBeatGridTest, SerializeBeatMap) { EXPECT_EQ(seratoBeatGrid.nonTerminalMarkers().size(), 0); EXPECT_NE(seratoBeatGrid.terminalMarker(), nullptr); EXPECT_FLOAT_EQ(seratoBeatGrid.terminalMarker()->bpm(), static_cast(bpm)); + + // Check if the beats can be re-imported losslessly + mixxx::SeratoBeatsImporter beatsImporter( + seratoBeatGrid.nonTerminalMarkers(), + seratoBeatGrid.terminalMarker()); + QVector importedBeatPositionsFrames = + beatsImporter.importBeatsAndApplyTimingOffset(0, signalInfo); + ASSERT_EQ(beatPositionsFrames.size(), importedBeatPositionsFrames.size()); + for (int i = 0; i < beatPositionsFrames.size(); i++) { + EXPECT_NEAR(beatPositionsFrames[i], importedBeatPositionsFrames[i], kEpsilon); + } } // Now add 3 minutes of beats at 50 bpm to the beatgrid @@ -171,7 +190,18 @@ TEST_F(SeratoBeatGridTest, SerializeBeatMap) { EXPECT_EQ(seratoBeatGrid.nonTerminalMarkers().size(), 2); EXPECT_NE(seratoBeatGrid.terminalMarker(), nullptr); EXPECT_FLOAT_EQ(seratoBeatGrid.terminalMarker()->bpm(), static_cast(bpm / 2)); + + // Check if the beats can be re-imported losslessly + mixxx::SeratoBeatsImporter beatsImporter( + seratoBeatGrid.nonTerminalMarkers(), + seratoBeatGrid.terminalMarker()); + QVector importedBeatPositionsFrames = + beatsImporter.importBeatsAndApplyTimingOffset(0, signalInfo); + ASSERT_EQ(beatPositionsFrames.size(), importedBeatPositionsFrames.size()); + for (int i = 0; i < beatPositionsFrames.size(); i++) { + EXPECT_NEAR(beatPositionsFrames[i], importedBeatPositionsFrames[i], kEpsilon); + } } } -} // namespace +} // namespace mixxx diff --git a/src/track/serato/beatsimporter.h b/src/track/serato/beatsimporter.h index 403f76296c2..71a0ee7e00c 100644 --- a/src/track/serato/beatsimporter.h +++ b/src/track/serato/beatsimporter.h @@ -1,5 +1,7 @@ #pragma once +#include + #include #include "track/beatsimporter.h" @@ -21,6 +23,8 @@ class SeratoBeatsImporter : public BeatsImporter { const audio::StreamInfo& streamInfo) override; private: + FRIEND_TEST(SeratoBeatGridTest, SerializeBeatMap); + QVector importBeatsAndApplyTimingOffset( double timingOffsetMillis, const audio::SignalInfo& signalInfo); From e6b2d48c7b065a78cf748a02b5f6cddf892a14c3 Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Fri, 21 May 2021 19:36:20 +0200 Subject: [PATCH 08/10] SeratoBeatGrid: Fix import/export and add roundtrip test --- src/test/seratobeatgridtest.cpp | 119 ++++++++++++++++++++++++----- src/track/serato/beatgrid.cpp | 87 ++++++++------------- src/track/serato/beatsimporter.cpp | 15 +--- 3 files changed, 134 insertions(+), 87 deletions(-) diff --git a/src/test/seratobeatgridtest.cpp b/src/test/seratobeatgridtest.cpp index 2d3b3d21187..2f507d4451f 100644 --- a/src/test/seratobeatgridtest.cpp +++ b/src/test/seratobeatgridtest.cpp @@ -130,30 +130,40 @@ TEST_F(SeratoBeatGridTest, SerializeBeatgrid) { TEST_F(SeratoBeatGridTest, SerializeBeatMap) { // Create a non-const beatmap - constexpr double bpm = 100.0; + constexpr double timingOffsetMillis = -10; + constexpr int bpm = 120; const auto sampleRate = mixxx::audio::SampleRate(44100); const auto signalInfo = mixxx::audio::SignalInfo(mixxx::audio::ChannelCount(2), sampleRate); const auto duration = mixxx::Duration::fromSeconds(300); const double framesPerMinute = signalInfo.getSampleRate() * 60; const double framesPerBeat = framesPerMinute / bpm; + const double initialFrameOffset = framesPerBeat / 2; QVector beatPositionsFrames; - double beatPositionFrames = 0; - // Add 2 minutes of beats at 100 bpm to the beatgrid - for (int i = 0; i < 2 * bpm; i++) { + double beatPositionFrames = initialFrameOffset; + + constexpr int kNumBeats120BPM = 4; + qInfo() << "Step 1: Add" << kNumBeats120BPM << "beats at 100 bpm to the beatgrid"; + for (int i = 0; i < kNumBeats120BPM; i++) { beatPositionsFrames.append(beatPositionFrames); beatPositionFrames += framesPerBeat; } + ASSERT_EQ(beatPositionsFrames.size(), kNumBeats120BPM); // Check the const beatmap { const auto pBeats = mixxx::BeatMap::makeBeatMap( sampleRate, QString("Test"), beatPositionsFrames); - // At the 1 minute mark the BPM should be 100 - EXPECT_EQ(pBeats->getBpmAroundPosition(framesPerMinute, 1), bpm); + // Check that the first section's BPM is 100 + EXPECT_EQ(pBeats->getBpmAroundPosition( + signalInfo.frames2samples( + static_cast(initialFrameOffset + + framesPerBeat * kNumBeats120BPM / 2)), + 1), + bpm); mixxx::SeratoBeatGrid seratoBeatGrid; - seratoBeatGrid.setBeats(pBeats, signalInfo, duration, 0); + seratoBeatGrid.setBeats(pBeats, signalInfo, duration, timingOffsetMillis); EXPECT_EQ(seratoBeatGrid.nonTerminalMarkers().size(), 0); EXPECT_NE(seratoBeatGrid.terminalMarker(), nullptr); EXPECT_FLOAT_EQ(seratoBeatGrid.terminalMarker()->bpm(), static_cast(bpm)); @@ -163,31 +173,46 @@ TEST_F(SeratoBeatGridTest, SerializeBeatMap) { seratoBeatGrid.nonTerminalMarkers(), seratoBeatGrid.terminalMarker()); QVector importedBeatPositionsFrames = - beatsImporter.importBeatsAndApplyTimingOffset(0, signalInfo); + beatsImporter.importBeatsAndApplyTimingOffset(timingOffsetMillis, signalInfo); ASSERT_EQ(beatPositionsFrames.size(), importedBeatPositionsFrames.size()); for (int i = 0; i < beatPositionsFrames.size(); i++) { EXPECT_NEAR(beatPositionsFrames[i], importedBeatPositionsFrames[i], kEpsilon); } } - // Now add 3 minutes of beats at 50 bpm to the beatgrid - for (int i = 0; i < 3 * bpm / 2; i++) { + constexpr int kNumBeats60BPM = 4; + qInfo() << "Step 2: Add" << kNumBeats60BPM << "beats at 50 bpm to the beatgrid"; + for (int i = 0; i < kNumBeats60BPM; i++) { beatPositionsFrames.append(beatPositionFrames); beatPositionFrames += framesPerBeat * 2; } + ASSERT_EQ(beatPositionsFrames.size(), kNumBeats120BPM + kNumBeats60BPM); - // Check the non-const beatmap { const auto pBeats = mixxx::BeatMap::makeBeatMap( sampleRate, QString("Test"), beatPositionsFrames); - // At the 1 minute mark the BPM should be 100 - EXPECT_EQ(pBeats->getBpmAroundPosition(framesPerMinute, 1), bpm); - // At the 4 minute mark the BPM should be 50 - EXPECT_EQ(pBeats->getBpmAroundPosition(framesPerMinute * 4, 1), bpm / 2); + // Check that the first section'd BPM is 100 + EXPECT_EQ(pBeats->getBpmAroundPosition( + signalInfo.frames2samples( + static_cast(initialFrameOffset + + framesPerBeat * kNumBeats120BPM / 2)), + 1), + bpm); + // Check that the second section'd BPM is 50 + EXPECT_EQ(pBeats->getBpmAroundPosition( + signalInfo.frames2samples( + static_cast(initialFrameOffset + + framesPerBeat * kNumBeats120BPM + + framesPerBeat * kNumBeats60BPM / 2)), + 1), + bpm / 2); mixxx::SeratoBeatGrid seratoBeatGrid; - seratoBeatGrid.setBeats(pBeats, signalInfo, duration, 0); - EXPECT_EQ(seratoBeatGrid.nonTerminalMarkers().size(), 2); + seratoBeatGrid.setBeats(pBeats, signalInfo, duration, timingOffsetMillis); + ASSERT_EQ(seratoBeatGrid.nonTerminalMarkers().size(), 2); + ASSERT_EQ(seratoBeatGrid.nonTerminalMarkers()[0]->beatsTillNextMarker(), kNumBeats120BPM); + ASSERT_EQ(seratoBeatGrid.nonTerminalMarkers()[1]->beatsTillNextMarker(), + kNumBeats60BPM - 1); EXPECT_NE(seratoBeatGrid.terminalMarker(), nullptr); EXPECT_FLOAT_EQ(seratoBeatGrid.terminalMarker()->bpm(), static_cast(bpm / 2)); @@ -196,7 +221,65 @@ TEST_F(SeratoBeatGridTest, SerializeBeatMap) { seratoBeatGrid.nonTerminalMarkers(), seratoBeatGrid.terminalMarker()); QVector importedBeatPositionsFrames = - beatsImporter.importBeatsAndApplyTimingOffset(0, signalInfo); + beatsImporter.importBeatsAndApplyTimingOffset(timingOffsetMillis, signalInfo); + ASSERT_EQ(beatPositionsFrames.size(), importedBeatPositionsFrames.size()); + for (int i = 0; i < beatPositionsFrames.size(); i++) { + EXPECT_NEAR(beatPositionsFrames[i], importedBeatPositionsFrames[i], kEpsilon); + } + } + + qInfo() << "Step 3: Add" << kNumBeats120BPM << "beats at 100 bpm to the beatgrid"; + for (int i = 0; i < kNumBeats120BPM; i++) { + beatPositionsFrames.append(beatPositionFrames); + beatPositionFrames += framesPerBeat; + } + ASSERT_EQ(beatPositionsFrames.size(), 2 * kNumBeats120BPM + kNumBeats60BPM); + + // Add the last beat + beatPositionsFrames.append(beatPositionFrames); + + { + const auto pBeats = mixxx::BeatMap::makeBeatMap( + sampleRate, QString("Test"), beatPositionsFrames); + // Check that the first section's BPM is 100 + EXPECT_EQ(pBeats->getBpmAroundPosition( + signalInfo.frames2samples( + static_cast(initialFrameOffset + + framesPerBeat * kNumBeats120BPM / 2)), + 1), + bpm); + // Check that the second section's BPM is 50 + EXPECT_EQ(pBeats->getBpmAroundPosition( + signalInfo.frames2samples( + static_cast(initialFrameOffset + + framesPerBeat * kNumBeats120BPM + + framesPerBeat * kNumBeats60BPM / 2)), + 1), + bpm / 2); + // Check that the third section's BPM is 100 + EXPECT_EQ(pBeats->getBpmAroundPosition( + signalInfo.frames2samples(static_cast( + initialFrameOffset + + framesPerBeat * kNumBeats120BPM * 1.5 + + framesPerBeat * kNumBeats60BPM)), + 1), + bpm / 2); + + mixxx::SeratoBeatGrid seratoBeatGrid; + seratoBeatGrid.setBeats(pBeats, signalInfo, duration, timingOffsetMillis); + ASSERT_EQ(seratoBeatGrid.nonTerminalMarkers().size(), 3); + ASSERT_EQ(seratoBeatGrid.nonTerminalMarkers()[0]->beatsTillNextMarker(), kNumBeats120BPM); + ASSERT_EQ(seratoBeatGrid.nonTerminalMarkers()[1]->beatsTillNextMarker(), kNumBeats60BPM); + ASSERT_EQ(seratoBeatGrid.nonTerminalMarkers()[2]->beatsTillNextMarker(), kNumBeats60BPM); + EXPECT_NE(seratoBeatGrid.terminalMarker(), nullptr); + EXPECT_FLOAT_EQ(seratoBeatGrid.terminalMarker()->bpm(), static_cast(bpm)); + + // Check if the beats can be re-imported losslessly + mixxx::SeratoBeatsImporter beatsImporter( + seratoBeatGrid.nonTerminalMarkers(), + seratoBeatGrid.terminalMarker()); + QVector importedBeatPositionsFrames = + beatsImporter.importBeatsAndApplyTimingOffset(timingOffsetMillis, signalInfo); ASSERT_EQ(beatPositionsFrames.size(), importedBeatPositionsFrames.size()); for (int i = 0; i < beatPositionsFrames.size(); i++) { EXPECT_NEAR(beatPositionsFrames[i], importedBeatPositionsFrames[i], kEpsilon); diff --git a/src/track/serato/beatgrid.cpp b/src/track/serato/beatgrid.cpp index 66982c2b2ac..a20a06aeaf7 100644 --- a/src/track/serato/beatgrid.cpp +++ b/src/track/serato/beatgrid.cpp @@ -443,73 +443,59 @@ void SeratoBeatGrid::setBeats(BeatsPointer pBeats, auto pBeatsIterator = pBeats->findBeats(0, trackDurationSamples); // This might be null if the track doesn't contain any beats - if (!pBeatsIterator) { + if (!pBeatsIterator || !pBeatsIterator->hasNext()) { qWarning() << "Serato Beatgrid: No beats available, exporting empty beatgrid!"; setTerminalMarker(nullptr); setNonTerminalMarkers({}); return; } + const double beatgridFrameOffset = signalInfo.samples2framesFractional(pBeatsIterator->next()); double currentBeatPositionFrames = 0; double previousBeatPositionFrames = 0; double previousDeltaFrames = -1; - int beatsSinceLastMarker = 0; QList nonTerminalMarkers; + { + const double positionSecs = + signalInfo.frames2secsFractional( + currentBeatPositionFrames + beatgridFrameOffset) - + timingOffsetSecs; + nonTerminalMarkers.append( + std::make_shared(positionSecs, 0)); + } while (pBeatsIterator->hasNext()) { previousBeatPositionFrames = currentBeatPositionFrames; currentBeatPositionFrames = - signalInfo.samples2framesFractional( - pBeatsIterator->next()); + signalInfo.samples2framesFractional(pBeatsIterator->next()) - + beatgridFrameOffset; // Calculate the delta between the current beat and the previous beat. // If the distance is the same as the distance between the previous // beat and the beat before that, we can just increment // `beatsSinceLastMarker`. If not, we need to add a new marker. const double currentDeltaFrames = currentBeatPositionFrames - previousBeatPositionFrames; + if (previousDeltaFrames < 0) { + previousDeltaFrames = currentDeltaFrames; + } + const double differenceBetweenCurrentAndPreviousDelta = abs(currentDeltaFrames - previousDeltaFrames); - if (differenceBetweenCurrentAndPreviousDelta > kEpsilon || nonTerminalMarkers.isEmpty()) { + if (differenceBetweenCurrentAndPreviousDelta >= kEpsilon) { + const double positionSecs = + signalInfo.frames2secsFractional( + previousBeatPositionFrames + beatgridFrameOffset) - + timingOffsetSecs; + nonTerminalMarkers.append( + std::make_shared(positionSecs, 1)); + } else { // We are adding a new beat marker, therefore we need to update the // `beatsSinceLastMarker` variable of the last marker we added. - if (!nonTerminalMarkers.isEmpty()) { - DEBUG_ASSERT(beatsSinceLastMarker > 0); - const auto pNonTerminalMarker = - nonTerminalMarkers.at(nonTerminalMarkers.size() - 1); - DEBUG_ASSERT(pNonTerminalMarker); - pNonTerminalMarker->setBeatsTillNextMarker(beatsSinceLastMarker); - - // After adding the first marker, the the sample delta won't - // match, because it compares the distance between beats 1 and - // two with the distance between the start of the track and the - // first beat. This special case makes sure that we don't add - // an unnecessary additional marker that has - // beatsSinceLastMarker set to 1. - if (nonTerminalMarkers.size() == 1 && beatsSinceLastMarker == 1) { - previousDeltaFrames = currentDeltaFrames; - beatsSinceLastMarker++; - continue; - } - } - // Don't create a SeratoBeatGridNonTerminalMarker entry for the - // last beat, this needs to be a terminal marker entry. - if (pBeatsIterator->hasNext()) { - const double positionSecs = - signalInfo.frames2secsFractional( - currentBeatPositionFrames) - - timingOffsetSecs; - nonTerminalMarkers.append( - std::make_shared(positionSecs, 0)); - } - beatsSinceLastMarker = 0; + const auto pNonTerminalMarker = nonTerminalMarkers.last(); + pNonTerminalMarker->setBeatsTillNextMarker( + pNonTerminalMarker->beatsTillNextMarker() + 1); } - beatsSinceLastMarker++; - previousDeltaFrames = currentDeltaFrames; - } - // The track has no beats. This isn't possible because the `pBeatsIterator` - // check above should have caught this case already. - VERIFY_OR_DEBUG_ASSERT(currentBeatPositionFrames != -1) { - return; + previousDeltaFrames = currentDeltaFrames; } // If the beatgrid is constant, i.e. if there is only a single non-terminal @@ -520,26 +506,13 @@ void SeratoBeatGrid::setBeats(BeatsPointer pBeats, nonTerminalMarkers.clear(); } - // Update the `beatsSinceLastMarker` of the last non-terminal marker we inserted. - if (!nonTerminalMarkers.isEmpty()) { - DEBUG_ASSERT(beatsSinceLastMarker > 0); - const auto pNonTerminalMarker = nonTerminalMarkers.at(nonTerminalMarkers.size() - 1); - DEBUG_ASSERT(pNonTerminalMarker); - // We need to subtract 1 from `beatsSinceLastMarker`, because at the end of - // the last iteration the counter is incremented even though we didn't move - // a beat forwards. - pNonTerminalMarker->setBeatsTillNextMarker(beatsSinceLastMarker - 1); - } - // Finally, create the terminal marker. const double positionSecs = signalInfo.frames2secsFractional( - currentBeatPositionFrames) - + currentBeatPositionFrames + beatgridFrameOffset) - timingOffsetSecs; const double bpm = pBeats->getBpmAroundPosition( - signalInfo.frames2samples( - static_cast(currentBeatPositionFrames)), - 1); + signalInfo.getChannelCount() * (currentBeatPositionFrames + beatgridFrameOffset), 1); setTerminalMarker(std::make_shared(positionSecs, bpm)); setNonTerminalMarkers(nonTerminalMarkers); diff --git a/src/track/serato/beatsimporter.cpp b/src/track/serato/beatsimporter.cpp index 3d40047bc1d..beb95d2eedc 100644 --- a/src/track/serato/beatsimporter.cpp +++ b/src/track/serato/beatsimporter.cpp @@ -81,7 +81,7 @@ QVector SeratoBeatsImporter::importBeatsAndApplyTimingOffset( } const double rangeEndBeatPositionMillis = - static_cast(m_pTerminalMarker->positionSecs()) * 1000; + static_cast(m_pTerminalMarker->positionSecs() * 1000); if (beats.isEmpty()) { VERIFY_OR_DEBUG_ASSERT(beatPositionMillis == 0) { @@ -94,17 +94,8 @@ QVector SeratoBeatsImporter::importBeatsAndApplyTimingOffset( // implement it ourselves. const double divisor = std::floor(rangeEndBeatPositionMillis / beatLengthMillis); beatPositionMillis = rangeEndBeatPositionMillis - (divisor * beatLengthMillis); - } else { - // Add beatLengthMillis, otherwise we'd import the same beat position - // twice (we already imported it above). - beatPositionMillis += beatLengthMillis; - - // Sometimes the previous calculation can lead to a position behind the - // terminal marker position, because the BPM value is inaccurate. In - // that case, use the rangeEndBeatPositionMillis directly. - if (beatPositionMillis > rangeEndBeatPositionMillis) { - beatPositionMillis = rangeEndBeatPositionMillis; - } + } else if (beatPositionMillis > rangeEndBeatPositionMillis) { + beatPositionMillis = rangeEndBeatPositionMillis; } // Now fill the range with beats until the end is reached. Add a half beat From 7de4447fae1e3f9bef9d2ee966a9b66b0fdaf448 Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Sat, 22 May 2021 00:13:15 +0200 Subject: [PATCH 09/10] SeratoBeatsImporter: Use simple DEBUG_ASSERT for non-NTM case --- src/track/serato/beatsimporter.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/track/serato/beatsimporter.cpp b/src/track/serato/beatsimporter.cpp index beb95d2eedc..96725283d5f 100644 --- a/src/track/serato/beatsimporter.cpp +++ b/src/track/serato/beatsimporter.cpp @@ -84,9 +84,7 @@ QVector SeratoBeatsImporter::importBeatsAndApplyTimingOffset( static_cast(m_pTerminalMarker->positionSecs() * 1000); if (beats.isEmpty()) { - VERIFY_OR_DEBUG_ASSERT(beatPositionMillis == 0) { - return {}; - } + DEBUG_ASSERT(beatPositionMillis == 0); // If there are no beats yet (because there were no non-terminal // markers), beatPositionMillis will be 0. In that case we have to find // the offset backwards from the terminal marker position. From 3c23fcc903a0e58c62f3537e3ba20460709f0f15 Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Sat, 22 May 2021 10:58:12 +0200 Subject: [PATCH 10/10] SeratoBeatGrid: Subtract timingOffset during export when creating marker --- src/track/serato/beatgrid.cpp | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/track/serato/beatgrid.cpp b/src/track/serato/beatgrid.cpp index a20a06aeaf7..85d8bae2e5c 100644 --- a/src/track/serato/beatgrid.cpp +++ b/src/track/serato/beatgrid.cpp @@ -456,12 +456,11 @@ void SeratoBeatGrid::setBeats(BeatsPointer pBeats, double previousDeltaFrames = -1; QList nonTerminalMarkers; { - const double positionSecs = - signalInfo.frames2secsFractional( - currentBeatPositionFrames + beatgridFrameOffset) - - timingOffsetSecs; + const double positionSecs = signalInfo.frames2secsFractional( + currentBeatPositionFrames + beatgridFrameOffset); nonTerminalMarkers.append( - std::make_shared(positionSecs, 0)); + std::make_shared( + positionSecs - timingOffsetSecs, 0)); } while (pBeatsIterator->hasNext()) { previousBeatPositionFrames = currentBeatPositionFrames; @@ -481,12 +480,11 @@ void SeratoBeatGrid::setBeats(BeatsPointer pBeats, const double differenceBetweenCurrentAndPreviousDelta = abs(currentDeltaFrames - previousDeltaFrames); if (differenceBetweenCurrentAndPreviousDelta >= kEpsilon) { - const double positionSecs = - signalInfo.frames2secsFractional( - previousBeatPositionFrames + beatgridFrameOffset) - - timingOffsetSecs; + const double positionSecs = signalInfo.frames2secsFractional( + previousBeatPositionFrames + beatgridFrameOffset); nonTerminalMarkers.append( - std::make_shared(positionSecs, 1)); + std::make_shared( + positionSecs - timingOffsetSecs, 1)); } else { // We are adding a new beat marker, therefore we need to update the // `beatsSinceLastMarker` variable of the last marker we added. @@ -507,14 +505,16 @@ void SeratoBeatGrid::setBeats(BeatsPointer pBeats, } // Finally, create the terminal marker. - const double positionSecs = - signalInfo.frames2secsFractional( - currentBeatPositionFrames + beatgridFrameOffset) - - timingOffsetSecs; + const double currentBeatPositionFramesWithOffset = + currentBeatPositionFrames + beatgridFrameOffset; + const double positionSecs = signalInfo.frames2secsFractional( + currentBeatPositionFramesWithOffset); const double bpm = pBeats->getBpmAroundPosition( - signalInfo.getChannelCount() * (currentBeatPositionFrames + beatgridFrameOffset), 1); + signalInfo.getChannelCount() * currentBeatPositionFramesWithOffset, + 1); - setTerminalMarker(std::make_shared(positionSecs, bpm)); + setTerminalMarker(std::make_shared( + positionSecs - timingOffsetSecs, bpm)); setNonTerminalMarkers(nonTerminalMarkers); }