EngineBuffer: Fix broken playback due to rate integer division #4085
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 9c33e11 from PR mixxxdj#4065 introduced this issue: $ git show --oneline --no-patch 9c33e11 9c33e11 (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: mixxxdj#4065
uklotzde
approved these changes
Jul 8, 2021
Holzhaus
added a commit
to Holzhaus/mixxx
that referenced
this pull request
Jul 8, 2021
This is a follow up to PR mixxxdj#4085.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 9c33e11
from PR #4065 introduced this issue:
The root cause is that the
mixxx::audio::SampleRateis an integer andwhen 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::SampleRatetype and remove theimplicit 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: #4065