Merge pull request #11263 from daschuer/beat_assert_fix

Fix a rounding issue is BpmControl::getBeatContext

src/engine/controls/bpmcontrol.cpp

@@ -480,7 +480,8 @@ double BpmControl::getBeatDistance(mixxx::audio::FramePos thisPosition) const {
 }
 
 // static
-bool BpmControl::getBeatContext(const mixxx::BeatsPointer& pBeats,
+bool BpmControl::getBeatContext(
+        const mixxx::BeatsPointer& pBeats,
         mixxx::audio::FramePos position,
         mixxx::audio::FramePos* pPrevBeatPosition,
         mixxx::audio::FramePos* pNextBeatPosition,
@@ -795,19 +796,22 @@ mixxx::audio::FramePos BpmControl::getBeatMatchPosition(
         return thisPosition;
     }
 
-    const auto otherPosition = pOtherEngineBuffer->getExactPlayPos();
+    const mixxx::audio::FramePos otherPosition = pOtherEngineBuffer->getExactPlayPos();
     const mixxx::audio::SampleRate thisSampleRate = m_pBeats->getSampleRate();
 
     // Seek our next beat to the other next beat near our beat.
     // This is the only thing we can do if the track has different BPM,
     // playing the next beat together.
 
-    // First calculate the position in the other track where this next beat will be.
-    const double thisSecs2ToNextBeat = (thisNextBeatPosition - thisPosition) /
-            thisSampleRate / thisRateRatio;
-    const mixxx::audio::FramePos otherPositionOfThisNextBeat = otherPosition +
-            thisSecs2ToNextBeat * otherBeats->getSampleRate() *
-                    pOtherEngineBuffer->getRateRatio();
+    // calculate framesTransposeFactor first to avoid rounding issues, because it is often 1
+    double framesTransposeFactor = otherBeats->getSampleRate() /
+            thisSampleRate * pOtherEngineBuffer->getRateRatio() / thisRateRatio;
+    // subtract first to avoid a rounding issue, because lower double values
+    // have a smaller minimum step width
+    const mixxx::audio::FrameDiff_t otherToThisOffset =
+            otherPosition - thisPosition * framesTransposeFactor;
+    const mixxx::audio::FramePos otherPositionOfThisNextBeat =
+            thisNextBeatPosition * framesTransposeFactor + otherToThisOffset;
 
     mixxx::audio::FramePos otherPrevBeatPosition;
     mixxx::audio::FramePos otherNextBeatPosition;

src/test/enginesynctest.cpp

@@ -3021,3 +3021,24 @@ TEST_F(EngineSyncTest, ImplicitLeaderToInternalClock) {
     ASSERT_FALSE(isSoftLeader(m_sGroup2));
     ASSERT_FALSE(isSoftLeader(m_sInternalClockGroup));
 }
+
+TEST_F(EngineSyncTest, BeatContextRounding) {
+    ControlObject::getControl(ConfigKey(m_sGroup1, "quantize"))->set(1.0);
+    mixxx::BeatsPointer pBeats1 = mixxx::Beats::fromConstTempo(
+            m_pTrack1->getSampleRate(), mixxx::audio::FramePos(11166), mixxx::Bpm(162));
+    m_pTrack1->trySetBeats(pBeats1);
+
+    // This playposition is was able to fail a DEBUG_ASSERT before
+    // https://github.com/mixxxdj/mixxx/pull/11263 It was not possible to pass
+    // it as one double literal
+    ControlObject::set(ConfigKey(m_sGroup1, "playposition"),
+            (-5167.0 - 0.33333333333393966313) / 220500);
+    ProcessBuffer();
+    ControlObject::getControl(ConfigKey(m_sGroup1, "play"))->set(1.0);
+    // this must not abort due to the assertion in Beats::iteratorFrom()
+    ProcessBuffer();
+
+    EXPECT_NEAR(-0.021112622826908536,
+            ControlObject::get(ConfigKey(m_sGroup1, "playposition")),
+            kMaxFloatingPointErrorHighPrecision);
+}

src/track/beats.cpp

@@ -416,20 +416,24 @@ bool Beats::findPrevNextBeats(audio::FramePos position,
     return true;
 }
 
+// Find the next beat at or after the position
 Beats::ConstIterator Beats::iteratorFrom(audio::FramePos position) const {
     DEBUG_ASSERT(isValid());
     auto it = cfirstmarker();
-    if (position > m_lastMarkerPosition) {
+
+    audio::FrameDiff_t diff = position - m_lastMarkerPosition;
+    if (diff > 0) {
         DEBUG_ASSERT(*clastmarker() == m_lastMarkerPosition);
         // Lookup position is after the last marker position
-        const double n = std::ceil((position - m_lastMarkerPosition) / lastBeatLengthFrames());
+        const double n = std::ceil(diff / lastBeatLengthFrames());
         if (n >= static_cast<double>(std::numeric_limits<int>::max())) {
             return cend();
         }
         it = clastmarker() + static_cast<int>(n);
 
-        // In some rare cases there may be extremely tiny floating point errors
-        // that make `std::ceil` round up and makes us end up one beat too
+        // In positive direction the minimum step width of a double increases.
+        // This may lead to tiny floating point errors that make `std::ceil`
+        // round up and makes us end up one beat too
         // late. This works around that issue by going back to the previous
         // beat if necessary.
         auto previousBeatIt = it - 1;
@@ -448,7 +452,7 @@ Beats::ConstIterator Beats::iteratorFrom(audio::FramePos position) const {
     }
     DEBUG_ASSERT(it == cbegin() || it == cend() || *it >= position);
     DEBUG_ASSERT(it == cbegin() || it == cend() ||
-            *it - position < std::prev(it).beatLengthFrames());
+            (*it >= position && *std::prev(it) < position));
     return it;
 }