Skip to content

Commit

Permalink
Fix assert and fractionnal beat skipped by iterator
Browse files Browse the repository at this point in the history 
Signed-off-by: Antoine C <[email protected]>
  • Loading branch information
@acolombier
acolombier committed Jun 9, 2024
1 parent 674fb77 commit e06279c
Show file tree
Hide file tree
Showing 3 changed files with 100 additions and 23 deletions.
50 changes: 50 additions & 0 deletions src/test/beatstest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,56 @@ TEST(BeatsTest, ConstTempoFindPrevNextBeatWhenNotOnBeat) {
EXPECT_NEAR(nextBeat.value(), foundNextBeat.value(), kMaxBeatError);
}

TEST(BeatsTest, FractionalBeats) {
auto secondMarkerPosition = kStartPosition + 4 * (60 * kSampleRate) / 120 + 4800;
auto lastMarkerPosition = kStartPosition + 4 * kSampleRate.value() / 2 +
4 * kSampleRate.value() - 4800;

// This is what the beat grid looks like (|| == marker, >/< track start/end)
// >.||....|....|....|....|.||........|........|........|.......<||
const auto tempoBeats = Beats(
std::vector<BeatMarker>{
BeatMarker{kStartPosition, (60 * kSampleRate) / 120, 4},
BeatMarker{secondMarkerPosition, (60 * kSampleRate) / 60, 4},
},
lastMarkerPosition,
kBpm,
4,
kSampleRate,
QString());

auto it = tempoBeats.cfirstmarker();
EXPECT_EQ(*it, kStartPosition);
EXPECT_EQ(*(++it), kStartPosition + 24000);
EXPECT_EQ(*(++it), kStartPosition + 48000);
EXPECT_EQ(*(++it), kStartPosition + 72000);
EXPECT_EQ(*(++it), kStartPosition + 96000);
EXPECT_EQ(*(++it), secondMarkerPosition);
EXPECT_EQ(*(++it), secondMarkerPosition + 48000);
EXPECT_EQ(*(++it), secondMarkerPosition + 96000);
EXPECT_EQ(*(++it), secondMarkerPosition + 144000);
EXPECT_EQ(*(++it), lastMarkerPosition);
EXPECT_EQ(it, tempoBeats.clastmarker());
EXPECT_EQ(*(--it), secondMarkerPosition + 144000);
EXPECT_EQ(*(--it), secondMarkerPosition + 96000);
EXPECT_EQ(*(--it), secondMarkerPosition + 48000);
EXPECT_EQ(*(--it), secondMarkerPosition);
EXPECT_EQ(*(--it), kStartPosition + 96000);
EXPECT_EQ(*(--it), kStartPosition + 72000);
EXPECT_EQ(*(--it), kStartPosition + 48000);
EXPECT_EQ(*(--it), kStartPosition + 24000);
EXPECT_EQ(*(--it), kStartPosition);
EXPECT_EQ(it, tempoBeats.cfirstmarker());

EXPECT_EQ(*tempoBeats.iteratorFrom(mixxx::audio::FramePos(-1)), kStartPosition);
EXPECT_EQ(*tempoBeats.iteratorFrom(kStartPosition + 24000), kStartPosition + 24000);
EXPECT_EQ(*tempoBeats.iteratorFrom(kStartPosition + 24001), kStartPosition + 48000);
EXPECT_EQ(*tempoBeats.iteratorFrom(kStartPosition + 96000), kStartPosition + 96000);
EXPECT_EQ(*tempoBeats.iteratorFrom(kStartPosition + 96001), secondMarkerPosition);
EXPECT_EQ(*tempoBeats.iteratorFrom(secondMarkerPosition - 1), secondMarkerPosition);
EXPECT_EQ(*tempoBeats.iteratorFrom(secondMarkerPosition + 1), secondMarkerPosition + 48000);
}

TEST(BeatsTest, MultipleBeatsPerBar) {
const auto tempoBeats = Beats(
std::vector<BeatMarker>{
Expand Down
49 changes: 29 additions & 20 deletions src/track/beats.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ Beats::ConstIterator Beats::ConstIterator::operator+=(Beats::ConstIterator::diff
(nextMarker != m_beats->m_markers.cend())
? nextMarker->position()
: m_beats->m_lastMarkerPosition;
const auto beatsTillNextMarker = m_it->beatsTillNextMarker(nextMarkerPosition);
const auto beatsTillNextMarker = m_it->upperBoundBeatsTillNextMarker(nextMarkerPosition);
if (m_beatOffset < beatsTillNextMarker) {
break;
}
Expand Down Expand Up @@ -165,7 +165,7 @@ Beats::ConstIterator Beats::ConstIterator::operator-=(Beats::ConstIterator::diff
? m_it->position()
: m_beats->m_lastMarkerPosition;
m_it--;
m_beatOffset += m_it->beatsTillNextMarker(currentMarkerPosition);
m_beatOffset += m_it->upperBoundBeatsTillNextMarker(currentMarkerPosition);
}
updateValue();
DEBUG_ASSERT(m_value < origValue);
Expand All @@ -191,7 +191,7 @@ Beats::ConstIterator::difference_type Beats::ConstIterator::operator-(
(nextMarker != m_beats->m_markers.cend())
? nextMarker->position()
: m_beats->m_lastMarkerPosition;
result += it->beatsTillNextMarker(nextMarkerPosition);
result += it->upperBoundBeatsTillNextMarker(nextMarkerPosition);
it++;
}
DEBUG_ASSERT(it == other.m_it);
Expand Down Expand Up @@ -229,6 +229,10 @@ bool isDoubleBeat(const QVector<audio::FramePos>& beatPositions, int beatIdx) {
return false;
}

double effectiveHundredthBpm(double beatLength, mixxx::audio::SampleRate sampleRate) {
return std::round(60 * sampleRate / beatLength * 100);
}

// static
mixxx::BeatsPointer Beats::fromBeatPositions(
mixxx::audio::SampleRate sampleRate,
Expand Down Expand Up @@ -283,8 +287,11 @@ mixxx::BeatsPointer Beats::fromBeatPositions(
previousBeatLengthFrames = beatLengthFrames;
}
if (
// If the beath length difference is less than a frame...
std::fabs(previousBeatLengthFrames - beatLengthFrames) < 1.0 &&
// If the BPM difference is less than 0.01 BPM...
std::fabs(effectiveHundredthBpm(
previousBeatLengthFrames, sampleRate) -
effectiveHundredthBpm(beatLengthFrames, sampleRate)) <
1.0 &&
// ...and we are not at an "opening" double beat
(isBeatCounting || !isDoubleBeat(beatPositions, i))) {
//.. we assume the current beat is part of the same marker
Expand Down Expand Up @@ -319,15 +326,15 @@ mixxx::BeatsPointer Beats::fromBeatPositions(
// marker and thus we don't need to store this position as this will
// automatically be generated at serialisation, and the opening double beat,
// where the relevant marker is has already been store in the main loop
// above Here is an illustartion of the that usecase
// above Here is an illustration of the that usecase
//
// track end
// ...--------------------------|
// | | | | | || | | ||
// beat Marker Double beat
// from marker n-1 with dbeat end marker
// /\
// last relevant marker --|
// |
// last relevant marker --|
if (!markers.empty() && !isDoubleBeat(beatPositions, beatPositions.size() - 1)) {
markers.push_back(BeatMarker(markerPosition.toLowerFrameBoundary(),
previousBeatLengthFrames,
Expand Down Expand Up @@ -490,7 +497,7 @@ QByteArray Beats::toBeatMapByteArray() const {
closingDoubleBeat =
(position + (it.beatLengthFrames() * currentBeatsPerBar))
.toLowerFrameBoundary();
} else if (position >= closingDoubleBeat) {
} else if (closingDoubleBeat.isValid() && position >= closingDoubleBeat) {
// Closing double beat should be strictly equal to the current beat
// position, otherwise something is going wrong.
qDebug() << "Closing double beat for bar of size"
Expand Down Expand Up @@ -597,7 +604,11 @@ Beats::ConstIterator Beats::iteratorFrom(audio::FramePos position) const {
} else {
it = std::lower_bound(cfirstmarker(), clastmarker() + 1, position);
}
DEBUG_ASSERT(it == cbegin() || it == cend() || *it >= position);
// Due to precision error, it happens that `it` and `position` are equal,
// but `it` is slightly behind `position`. To mitigate that, we aim for a
// 10e-3 precision
DEBUG_ASSERT(it == cbegin() || it == cend() || *it > position ||
std::fabs(*it - position) < 0.001);
DEBUG_ASSERT(it == cbegin() || it == cend() || *it > *std::prev(it));
return it;
}
Expand Down Expand Up @@ -798,15 +809,14 @@ std::optional<BeatsPointer> Beats::tryTranslate(
// previous marker as markerIt points to the marker directly after
// our position
markerIt--;
} else {
const auto marker = *markerIt;
const auto translatedPosition = marker.position() + offsetFrames;
markerIt = markers.erase(markerIt);
markerIt = markers.emplace(markerIt,
translatedPosition.toLowerFrameBoundary(),
marker.beatsLength(),
marker.beatsPerBar());
}
const auto marker = *markerIt;
const auto translatedPosition = marker.position() + offsetFrames;
markerIt = markers.erase(markerIt);
markerIt = markers.emplace(markerIt,
translatedPosition.toLowerFrameBoundary(),
marker.beatsLength(),
marker.beatsPerBar());
}

return BeatsPointer(new Beats(markers,
Expand Down Expand Up @@ -1070,9 +1080,8 @@ std::optional<BeatsPointer> Beats::tryRemoveMarker(audio::FramePos position) con
} else {
auto markerIt = std::lower_bound(markers.begin(),
markers.end(),
position,
findClosestBeat(position),
isBeatMarkerLessThanFramePos);

// At this point we already checked that the search position is on a
// marker and that it is not at the last marker. This means that
// `markerIt` should point to a marker in any case.
Expand Down
24 changes: 21 additions & 3 deletions src/track/beats.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,29 @@ class BeatMarker {

/// Returns the number of beats between this marker and the next one.
///
/// Note that the beat count is rounded up, which means that there may be a
/// Note that the beat count is truncated, which means that there may be a
/// fraction of a beat and that the different between the current marker
/// position in the next may not match with the beat count and beat length
int beatsTillNextMarker(mixxx::audio::FramePos nextMarkerPosition) const {
return static_cast<int>(std::ceil(nextMarkerPosition - m_position) / m_beatLength);
double beatsTillNextMarker(mixxx::audio::FramePos nextMarkerPosition) const {
return (nextMarkerPosition - m_position) / m_beatLength;
}

/// Returns the number of beats between this marker and the next one.
///
/// Note that the beat count is truncated, which means that there may be a
/// fraction of a beat and that the different between the current marker
/// position in the next may not match with the beat count and beat length
int upperBoundBeatsTillNextMarker(mixxx::audio::FramePos nextMarkerPosition) const {
return static_cast<int>(std::ceil((nextMarkerPosition - m_position) / m_beatLength));
}

/// Returns the number of beats between this marker and the next one.
///
/// Note that the beat count is truncated, which means that there may be a
/// fraction of a beat and that the different between the current marker
/// position in the next may not match with the beat count and beat length
int lowerBoundBeatsTillNextMarker(mixxx::audio::FramePos nextMarkerPosition) const {
return static_cast<int>(std::floor((nextMarkerPosition - m_position) / m_beatLength));
}

/// Returns the number of beats per bar..
Expand Down

0 comments on commit e06279c

Please sign in to comment.