Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Beats::findNthBeat fuzzy behavior #4099

Merged
merged 5 commits into from
Jul 24, 2021
Merged

Remove Beats::findNthBeat fuzzy behavior #4099

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2f92d71
BeatMapTest: Add additional checks to TestNthBeat
Holzhaus Jul 12, 2021
d77c4f7
BeatMap: Reimplement findNthBeat without fuzzy behavior
Holzhaus Jul 11, 2021
95f7d36
BeatGrid: Remove fuzzy behavior from findNthBeat
Holzhaus Jul 12, 2021
9037bb5
BeatMap: Clean up findNthBeat() code and add comment
Holzhaus Jul 22, 2021
78d51df
BeatMap: Fix typo and wording in comment
Holzhaus Jul 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 0 additions & 94 deletions src/test/beatgridtest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,100 +99,6 @@ TEST(BeatGridTest, TestNthBeatWhenOnBeat) {
EXPECT_NEAR(position.value(), pGrid->findPrevBeat(position).value(), kMaxBeatError);
}

TEST(BeatGridTest, TestNthBeatWhenOnBeat_BeforeEpsilon) {
constexpr int sampleRate = 44100;
TrackPointer pTrack = newTrack(sampleRate);

constexpr double bpm = 60.1;
pTrack->trySetBpm(bpm);
constexpr double beatLengthFrames = 60.0 * sampleRate / bpm;

auto pGrid = BeatGrid::makeBeatGrid(pTrack->getSampleRate(),
QString(),
mixxx::Bpm(bpm),
mixxx::audio::kStartFramePos);

// Pretend we're just before the 20th beat.
constexpr mixxx::audio::FramePos kClosestBeat(20 * beatLengthFrames);
const mixxx::audio::FramePos position(kClosestBeat - beatLengthFrames * 0.005);

// The spec dictates that a value of 0 is always invalid and returns an invalid position
EXPECT_FALSE(pGrid->findNthBeat(position, 0).isValid());

// findNthBeat should return exactly the current beat if we ask for 1 or
// -1. For all other values, it should return n times the beat length.
for (int i = 1; i < 20; ++i) {
EXPECT_NEAR((kClosestBeat + beatLengthFrames * (i - 1)).value(),
pGrid->findNthBeat(position, i).value(),
kMaxBeatError);
EXPECT_NEAR((kClosestBeat + beatLengthFrames * (-i + 1)).value(),
pGrid->findNthBeat(position, -i).value(),
kMaxBeatError);
}

// Also test prev/next beat calculation.
mixxx::audio::FramePos prevBeat, nextBeat;
pGrid->findPrevNextBeats(position, &prevBeat, &nextBeat, true);
EXPECT_NEAR(kClosestBeat.value(), prevBeat.value(), kMaxBeatError);
EXPECT_NEAR((kClosestBeat + beatLengthFrames).value(), nextBeat.value(), kMaxBeatError);

// Also test prev/next beat calculation without snapping tolerance
pGrid->findPrevNextBeats(position, &prevBeat, &nextBeat, false);
EXPECT_NEAR((kClosestBeat - beatLengthFrames).value(), prevBeat.value(), kMaxBeatError);
EXPECT_NEAR(kClosestBeat.value(), nextBeat.value(), kMaxBeatError);

// Both previous and next beat should return the closest beat.
EXPECT_NEAR(kClosestBeat.value(), pGrid->findNextBeat(position).value(), kMaxBeatError);
EXPECT_NEAR(kClosestBeat.value(), pGrid->findPrevBeat(position).value(), kMaxBeatError);
}

TEST(BeatGridTest, TestNthBeatWhenOnBeat_AfterEpsilon) {
constexpr int sampleRate = 44100;
TrackPointer pTrack = newTrack(sampleRate);

constexpr double bpm = 60.1;
pTrack->trySetBpm(bpm);
constexpr double beatLengthFrames = 60.0 * sampleRate / bpm;

auto pGrid = BeatGrid::makeBeatGrid(pTrack->getSampleRate(),
QString(),
mixxx::Bpm(bpm),
mixxx::audio::kStartFramePos);

// Pretend we're just before the 20th beat.
constexpr mixxx::audio::FramePos kClosestBeat(20 * beatLengthFrames);
const mixxx::audio::FramePos position(kClosestBeat + beatLengthFrames * 0.005);

// The spec dictates that a value of 0 is always invalid and returns an invalid position
EXPECT_FALSE(pGrid->findNthBeat(position, 0).isValid());

// findNthBeat should return exactly the current beat if we ask for 1 or
// -1. For all other values, it should return n times the beat length.
for (int i = 1; i < 20; ++i) {
EXPECT_NEAR((kClosestBeat + beatLengthFrames * (i - 1)).value(),
pGrid->findNthBeat(position, i).value(),
kMaxBeatError);
EXPECT_NEAR((kClosestBeat + beatLengthFrames * (-i + 1)).value(),
pGrid->findNthBeat(position, -i).value(),
kMaxBeatError);
}

// Also test prev/next beat calculation.
mixxx::audio::FramePos prevBeat, nextBeat;
pGrid->findPrevNextBeats(position, &prevBeat, &nextBeat, true);
EXPECT_NEAR(kClosestBeat.value(), prevBeat.value(), kMaxBeatError);
EXPECT_NEAR((kClosestBeat + beatLengthFrames).value(), nextBeat.value(), kMaxBeatError);

// Also test prev/next beat calculation without snapping tolerance
pGrid->findPrevNextBeats(position, &prevBeat, &nextBeat, false);
EXPECT_NEAR(kClosestBeat.value(), prevBeat.value(), kMaxBeatError);
EXPECT_NEAR((kClosestBeat + beatLengthFrames).value(), nextBeat.value(), kMaxBeatError);

// Both previous and next beat should return the closest beat.
EXPECT_NEAR(kClosestBeat.value(), pGrid->findNextBeat(position).value(), kMaxBeatError);
EXPECT_NEAR(kClosestBeat.value(), pGrid->findPrevBeat(position).value(), kMaxBeatError);
}

TEST(BeatGridTest, TestNthBeatWhenNotOnBeat) {
constexpr int sampleRate = 44100;
TrackPointer pTrack = newTrack(sampleRate);
Expand Down
106 changes: 15 additions & 91 deletions src/test/beatmaptest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,27 @@ TEST_F(BeatMapTest, TestNthBeat) {
// Check edge cases
const mixxx::audio::FramePos firstBeat = startOffsetFrames + beatLengthFrames * 0;
const mixxx::audio::FramePos lastBeat = startOffsetFrames + beatLengthFrames * (numBeats - 1);
EXPECT_EQ(lastBeat, pMap->findNthBeat(lastBeat, -1));
EXPECT_EQ(lastBeat, pMap->findNthBeat(lastBeat + beatLengthFrames, -1));
EXPECT_EQ(lastBeat - beatLengthFrames, pMap->findNthBeat(lastBeat, -2));
EXPECT_EQ(lastBeat - beatLengthFrames, pMap->findNthBeat(lastBeat + beatLengthFrames, -2));
EXPECT_EQ(lastBeat - 2 * beatLengthFrames, pMap->findNthBeat(lastBeat, -3));
EXPECT_EQ(lastBeat, pMap->findPrevBeat(lastBeat));
EXPECT_EQ(lastBeat, pMap->findNthBeat(lastBeat, 1));
EXPECT_EQ(lastBeat, pMap->findNextBeat(lastBeat));
EXPECT_FALSE(pMap->findNthBeat(lastBeat, 2).isValid());
EXPECT_FALSE(pMap->findNthBeat(lastBeat + beatLengthFrames, 2).isValid());

EXPECT_EQ(firstBeat, pMap->findNthBeat(firstBeat, 1));
EXPECT_EQ(firstBeat, pMap->findNthBeat(firstBeat - beatLengthFrames, 1));
EXPECT_EQ(firstBeat + beatLengthFrames, pMap->findNthBeat(firstBeat, 2));
EXPECT_EQ(firstBeat + beatLengthFrames, pMap->findNthBeat(firstBeat - beatLengthFrames, 2));
EXPECT_EQ(firstBeat + 2 * beatLengthFrames, pMap->findNthBeat(firstBeat, 3));
EXPECT_EQ(firstBeat, pMap->findNextBeat(firstBeat));
EXPECT_EQ(firstBeat, pMap->findNthBeat(firstBeat, -1));
EXPECT_EQ(firstBeat, pMap->findPrevBeat(firstBeat));
EXPECT_FALSE(pMap->findNthBeat(firstBeat, -2).isValid());
EXPECT_FALSE(pMap->findNthBeat(firstBeat - beatLengthFrames, -1).isValid());

mixxx::audio::FramePos prevBeat, nextBeat;
pMap->findPrevNextBeats(lastBeat, &prevBeat, &nextBeat, true);
Expand Down Expand Up @@ -156,97 +171,6 @@ TEST_F(BeatMapTest, TestNthBeatWhenOnBeat) {
EXPECT_EQ(position, pMap->findPrevBeat(position));
}

TEST_F(BeatMapTest, TestNthBeatWhenOnBeat_BeforeEpsilon) {
constexpr mixxx::Bpm bpm(60.0);
m_pTrack->trySetBpm(bpm.value());
mixxx::audio::FrameDiff_t beatLengthFrames = getBeatLengthFrames(bpm);
const auto startOffsetFrames = mixxx::audio::FramePos(7);
const int numBeats = 100;
// Note beats must be in frames, not samples.
QVector<mixxx::audio::FramePos> beats =
createBeatVector(startOffsetFrames, numBeats, beatLengthFrames);
auto pMap = BeatMap::makeBeatMap(m_pTrack->getSampleRate(), QString(), beats);

// Pretend we're just before the 20th beat;
const int curBeat = 20;
const mixxx::audio::FramePos kClosestBeat = startOffsetFrames + curBeat * beatLengthFrames;
const mixxx::audio::FramePos position = kClosestBeat - beatLengthFrames * 0.005;

// The spec dictates that a value of 0 is always invalid and returns -1
EXPECT_FALSE(pMap->findNthBeat(position, 0).isValid());

// findNthBeat should return exactly the current beat if we ask for 1 or
// -1. For all other values, it should return n times the beat length.
for (int i = 1; i < curBeat; ++i) {
EXPECT_DOUBLE_EQ((kClosestBeat + beatLengthFrames * (i - 1)).value(),
pMap->findNthBeat(position, i).value());
EXPECT_DOUBLE_EQ((kClosestBeat + beatLengthFrames * (-i + 1)).value(),
pMap->findNthBeat(position, -i).value());
}

// Also test prev/next beat calculation
mixxx::audio::FramePos prevBeat, nextBeat;
pMap->findPrevNextBeats(position, &prevBeat, &nextBeat, true);
EXPECT_EQ(kClosestBeat, prevBeat);
EXPECT_EQ(kClosestBeat + beatLengthFrames, nextBeat);

// Also test prev/next beat calculation without snapping tolerance
pMap->findPrevNextBeats(position, &prevBeat, &nextBeat, false);
EXPECT_EQ(kClosestBeat - beatLengthFrames, prevBeat);
EXPECT_EQ(kClosestBeat, nextBeat);

// Both previous and next beat should return the closest beat.
EXPECT_EQ(kClosestBeat, pMap->findNextBeat(position));
EXPECT_EQ(kClosestBeat, pMap->findPrevBeat(position));

}

TEST_F(BeatMapTest, TestNthBeatWhenOnBeat_AfterEpsilon) {
constexpr mixxx::Bpm bpm(60.0);
m_pTrack->trySetBpm(bpm.value());
mixxx::audio::FrameDiff_t beatLengthFrames = getBeatLengthFrames(bpm);
const auto startOffsetFrames = mixxx::audio::FramePos(7);
const int numBeats = 100;
// Note beats must be in frames, not samples.
QVector<mixxx::audio::FramePos> beats =
createBeatVector(startOffsetFrames, numBeats, beatLengthFrames);
auto pMap = BeatMap::makeBeatMap(m_pTrack->getSampleRate(), QString(), beats);

// Pretend we're just after the 20th beat;
const int curBeat = 20;
const mixxx::audio::FramePos kClosestBeat = startOffsetFrames + curBeat * beatLengthFrames;
const mixxx::audio::FramePos position = kClosestBeat + beatLengthFrames * 0.005;

// The spec dictates that a value of 0 is always invalid and returns -1
EXPECT_FALSE(pMap->findNthBeat(position, 0).isValid());

EXPECT_EQ(kClosestBeat, pMap->findClosestBeat(position));

// findNthBeat should return exactly the current beat if we ask for 1 or
// -1. For all other values, it should return n times the beat length.
for (int i = 1; i < curBeat; ++i) {
EXPECT_DOUBLE_EQ((kClosestBeat + beatLengthFrames * (i - 1)).value(),
pMap->findNthBeat(position, i).value());
EXPECT_DOUBLE_EQ((kClosestBeat + beatLengthFrames * (-i + 1)).value(),
pMap->findNthBeat(position, -i).value());
}

// Also test prev/next beat calculation.
mixxx::audio::FramePos prevBeat, nextBeat;
pMap->findPrevNextBeats(position, &prevBeat, &nextBeat, true);
EXPECT_EQ(kClosestBeat, prevBeat);
EXPECT_EQ(kClosestBeat + beatLengthFrames, nextBeat);

// Also test prev/next beat calculation without snapping tolerance
pMap->findPrevNextBeats(position, &prevBeat, &nextBeat, false);
EXPECT_EQ(kClosestBeat, prevBeat);
EXPECT_EQ(kClosestBeat + beatLengthFrames, nextBeat);

// Both previous and next beat should return the closest beat.
EXPECT_EQ(kClosestBeat, pMap->findNextBeat(position));
EXPECT_EQ(kClosestBeat, pMap->findPrevBeat(position));
}

TEST_F(BeatMapTest, TestNthBeatWhenNotOnBeat) {
constexpr mixxx::Bpm bpm(60.0);
m_pTrack->trySetBpm(bpm.value());
Expand Down
24 changes: 3 additions & 21 deletions src/track/beatgrid.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,27 +184,9 @@ audio::FramePos BeatGrid::findNthBeat(audio::FramePos position, int n) const {
return audio::kInvalidFramePos;
}

double beatFraction = (position - firstBeatPosition()) / m_beatLengthFrames;
double prevBeat = floor(beatFraction);
double nextBeat = ceil(beatFraction);

// If the position is within 1/100th of the next or previous beat, treat it
// as if it is that beat.
const double kEpsilon = .01;

if (fabs(nextBeat - beatFraction) < kEpsilon) {
// If we are going to pretend we were actually on nextBeat then prevBeat
// needs to be re-calculated. Since it is floor(beatFraction), that's
// the same as nextBeat. We only use prevBeat so no need to increment
// nextBeat.
prevBeat = nextBeat;
} else if (fabs(prevBeat - beatFraction) < kEpsilon) {
// If we are going to pretend we were actually on prevBeat then nextBeat
// needs to be re-calculated. Since it is ceil(beatFraction), that's
// the same as prevBeat. We will only use nextBeat so no need to
// decrement prevBeat.
nextBeat = prevBeat;
}
const double beatFraction = (position - firstBeatPosition()) / m_beatLengthFrames;
const double prevBeat = floor(beatFraction);
const double nextBeat = ceil(beatFraction);

audio::FramePos closestBeatPosition;
if (n > 0) {
Expand Down
112 changes: 46 additions & 66 deletions src/track/beatmap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,80 +315,60 @@ audio::FramePos BeatMap::findNthBeat(audio::FramePos position, int n) const {
return audio::kInvalidFramePos;
}

Beat beat = beatFromFramePos(position.toNearestFrameBoundary());

// it points at the first occurrence of beat or the next largest beat
BeatList::const_iterator it =
std::lower_bound(m_beats.constBegin(), m_beats.constEnd(), beat, beatLessThan);

// If the position is within 1/10th of a second of the next or previous
// beat, pretend we are on that beat.
const double kFrameEpsilon = 0.1 * m_sampleRate;

// Back-up by one.
if (it != m_beats.begin()) {
--it;
}

// Scan forward to find whether we are on a beat.
BeatList::const_iterator on_beat = m_beats.constEnd();
BeatList::const_iterator previous_beat = m_beats.constEnd();
BeatList::const_iterator next_beat = m_beats.constEnd();
for (; it != m_beats.end(); ++it) {
qint32 delta = it->frame_position() - beat.frame_position();

// We are "on" this beat.
if (abs(delta) < kFrameEpsilon) {
on_beat = it;
break;
const bool searchForward = n > 0;
int numBeatsLeft = std::abs(n);

// Beats are stored as full frame positions, so when searching forwards the
// smallest possible beat position we can find is the upper frame boundary
// of the search position.
//
// For searching backwards, the same applies for the lower frame boundary.
const auto searchFromPosition = searchForward
? position.toUpperFrameBoundary()
: position.toLowerFrameBoundary();
const Beat searchFromBeat = beatFromFramePos(searchFromPosition);
auto it = std::lower_bound(m_beats.cbegin(), m_beats.cend(), searchFromBeat, beatLessThan);

if (searchForward) {
// Search in forward direction
numBeatsLeft--;

while (it != m_beats.cend() && numBeatsLeft > 0) {
if (it->enabled()) {
numBeatsLeft--;
}
it++;
}
} else {
// Search in backward direction
numBeatsLeft--;

if (it == m_beats.cend() || it->frame_position() > searchFromBeat.frame_position()) {
// We may be one beat behind the searchFromBeat. In this case, we advance
// the reverse iterator by one beat.
if (it == m_beats.cbegin()) {
return audio::kInvalidFramePos;
}
it--;
}

if (delta < 0) {
// If we are not on the beat and delta < 0 then this beat comes
// before our current position.
previous_beat = it;
} else {
// If we are past the beat and we aren't on it then this beat comes
// after our current position.
next_beat = it;
// Stop because we have everything we need now.
break;
while (it != m_beats.cbegin() && numBeatsLeft > 0) {
if (it->enabled()) {
numBeatsLeft--;
}
it--;
}
}

// If we are within epsilon samples of a beat then the immediately next and
// previous beats are the beat we are on.
if (on_beat != m_beats.end()) {
next_beat = on_beat;
previous_beat = on_beat;
if (numBeatsLeft > 0 || it == m_beats.cend()) {
return audio::kInvalidFramePos;
}

if (n > 0) {
for (; next_beat != m_beats.end(); ++next_beat) {
if (!next_beat->enabled()) {
continue;
}
if (n == 1) {
return mixxx::audio::FramePos(next_beat->frame_position());
}
--n;
}
} else if (n < 0 && previous_beat != m_beats.end()) {
for (; true; --previous_beat) {
if (previous_beat->enabled()) {
if (n == -1) {
return mixxx::audio::FramePos(previous_beat->frame_position());
}
++n;
}
const auto foundBeatPosition = mixxx::audio::FramePos(it->frame_position());

// Don't step before the start of the list.
if (previous_beat == m_beats.begin()) {
break;
}
}
}
return audio::kInvalidFramePos;
DEBUG_ASSERT(foundBeatPosition >= searchFromPosition || !searchForward);
DEBUG_ASSERT(foundBeatPosition <= searchFromPosition || searchForward);
return foundBeatPosition;
}

bool BeatMap::findPrevNextBeats(audio::FramePos position,
Expand Down