From fdcb065e99bbaac26135e7395d6ddcf65249a892 Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Thu, 8 Jul 2021 17:45:47 +0200 Subject: [PATCH] EngineBuffer: Fix broken playback due to rate integer division This is a critical bug that slipped in during the latest semantic type refactorings. Hitting play doesn't actually start playback anymore. It's unfortunate that there is apparently no test that catches this bug. Bisecting revealed that commit 9c33e11da04be1c6ba22384ff65bd9877139c724 from PR #4065 introduced this issue: $ git show --oneline --no-patch 9c33e11da04be1c6ba22384ff65bd9877139c724 9c33e11da0 (origin/enginebuffer-samplerate) EngineBuffer: Use mixxx::audio::SampleRate instead of int/double The root cause is that the `mixxx::audio::SampleRate` is an integer and when dividing a sample rate by another sample rate, it implicitly uses integer division instead of double division. When we divide the old sample rate by the new sample rate to calculate the base playback speed, the result is seems to always be 0 due to this issue. This is unexpected and not at all apparent from the diff. This commit is just a band-aid to fix the issue. In the long-term, we should rework the `mixxx::audio::SampleRate` type and remove the implicit conversion. All operators should have been implemented explicitly for the possible operand types, like we do for `mixxx::audio::FramePos`. Furthermore, this shows that our test coverage is too low. We should definitely add a test for this. PR that introduced the issue: https://github.com/mixxxdj/mixxx/pull/4065 --- src/engine/enginebuffer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/enginebuffer.cpp b/src/engine/enginebuffer.cpp index d4ecf108260..d951a1c5352 100644 --- a/src/engine/enginebuffer.cpp +++ b/src/engine/enginebuffer.cpp @@ -809,7 +809,7 @@ void EngineBuffer::processTrackLocked( double baserate = 0.0; if (sampleRate.isValid()) { - baserate = m_trackSampleRateOld / sampleRate; + baserate = static_cast(m_trackSampleRateOld) / sampleRate; } // Sync requests can affect rate, so process those first.