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.

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

lp1935703: Use nearest frame boundary in lookupBeatPositions() #4095

Merged
merged 12 commits into from
Jul 11, 2021
Merged
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions src/audio/frame.h
Original file line number Diff line number Diff line change
@@ -109,6 +109,12 @@ class FramePos final {
return FramePos(std::floor(value()));
}

/// Return position rounded to the next upper full frame position, without
/// the fractional part.
[[nodiscard]] FramePos toUpperFrameBoundary() const {
return FramePos(std::ceil(value()));
}

/// Return position rounded to the nearest full frame position, without
/// the fractional part.
[[nodiscard]] FramePos toNearestFrameBoundary() const {
88 changes: 88 additions & 0 deletions src/test/beatmaptest.cpp
Original file line number Diff line number Diff line change
@@ -10,6 +10,15 @@ using namespace mixxx;

namespace {

int countRemainingBeats(std::unique_ptr<BeatIterator> pIterator) {
int numBeatsFound = 0;
while (pIterator->hasNext()) {
pIterator->next();
numBeatsFound++;
}
return numBeatsFound;
}

class BeatMapTest : public testing::Test {
protected:
BeatMapTest()
@@ -326,4 +335,83 @@ TEST_F(BeatMapTest, TestBpmAround) {
.value());
}

TEST_F(BeatMapTest, FindBeatsWithFractionalPos) {
constexpr mixxx::Bpm bpm(60.0);
constexpr int numBeats = 120;
const mixxx::audio::FrameDiff_t beatLengthFrames = getBeatLengthFrames(bpm);
ASSERT_EQ(beatLengthFrames, std::round(beatLengthFrames));

mixxx::audio::FramePos beatPos = mixxx::audio::kStartFramePos;
const mixxx::audio::FramePos lastBeatPos = beatPos + beatLengthFrames * (numBeats - 1);
QVector<mixxx::audio::FramePos> beats;
for (; beatPos <= lastBeatPos; beatPos += beatLengthFrames) {
beats.append(beatPos);
}
const auto pMap = BeatMap::makeBeatMap(m_pTrack->getSampleRate(), QString(), beats);

// All beats are in range
auto it = pMap->findBeats(mixxx::audio::kStartFramePos, lastBeatPos);
int numBeatsFound = countRemainingBeats(std::move(it));
EXPECT_EQ(numBeats, numBeatsFound);

// Only half the beats are in range
const auto halfBeatsPosition = mixxx::audio::kStartFramePos +
beatLengthFrames * ((numBeats / 2) - 1);
it = pMap->findBeats(mixxx::audio::kStartFramePos, halfBeatsPosition);
numBeatsFound = countRemainingBeats(std::move(it));
EXPECT_EQ(numBeats / 2, numBeatsFound);

// First beat is not in range
it = pMap->findBeats(mixxx::audio::kStartFramePos + 0.5, lastBeatPos + 0.5);
numBeatsFound = countRemainingBeats(std::move(it));
EXPECT_EQ(numBeats - 1, numBeatsFound);

// Last beat is not in range
it = pMap->findBeats(mixxx::audio::kStartFramePos - 0.5, lastBeatPos - 0.5);
numBeatsFound = countRemainingBeats(std::move(it));
EXPECT_EQ(numBeats - 1, numBeatsFound);

// All beats are in range
it = pMap->findBeats(mixxx::audio::kStartFramePos - 0.5, lastBeatPos + 0.5);
numBeatsFound = countRemainingBeats(std::move(it));
EXPECT_EQ(numBeats, numBeatsFound);

// First and last beats in range
it = pMap->findBeats(mixxx::audio::kStartFramePos + 0.5, lastBeatPos - 0.5);
numBeatsFound = countRemainingBeats(std::move(it));
EXPECT_EQ(numBeats - 2, numBeatsFound);
}

TEST_F(BeatMapTest, HasBeatInRangeWithFractionalPos) {
constexpr mixxx::Bpm bpm(60.0);
constexpr int numBeats = 120;
const mixxx::audio::FrameDiff_t beatLengthFrames = getBeatLengthFrames(bpm);
ASSERT_EQ(beatLengthFrames, std::round(beatLengthFrames));

mixxx::audio::FramePos beatPos = mixxx::audio::kStartFramePos;
const mixxx::audio::FramePos lastBeatPos = beatPos + beatLengthFrames * (numBeats - 1);
QVector<mixxx::audio::FramePos> beats;
for (; beatPos <= lastBeatPos; beatPos += beatLengthFrames) {
beats.append(beatPos);
}
const auto pMap = BeatMap::makeBeatMap(m_pTrack->getSampleRate(), QString(), beats);

const mixxx::audio::FrameDiff_t halfBeatLengthFrames = beatLengthFrames / 2;
EXPECT_TRUE(pMap->hasBeatInRange(mixxx::audio::kStartFramePos,
mixxx::audio::kStartFramePos + halfBeatLengthFrames));
EXPECT_TRUE(pMap->hasBeatInRange(mixxx::audio::kStartFramePos - 0.2,
mixxx::audio::kStartFramePos + halfBeatLengthFrames));
// FIXME: The next comparison is broken due to fuzzy matching in BeatMap::findNthBeat()
//EXPECT_FALSE(pMap->hasBeatInRange(mixxx::audio::kStartFramePos + 0.2, mixxx::audio::kStartFramePos + halfBeatLengthFrames));
EXPECT_TRUE(pMap->hasBeatInRange(
mixxx::audio::kStartFramePos - halfBeatLengthFrames,
mixxx::audio::kStartFramePos));
EXPECT_FALSE(pMap->hasBeatInRange(
mixxx::audio::kStartFramePos - halfBeatLengthFrames,
mixxx::audio::kStartFramePos - 0.2));
EXPECT_TRUE(pMap->hasBeatInRange(
mixxx::audio::kStartFramePos - halfBeatLengthFrames,
mixxx::audio::kStartFramePos + 0.2));
}

} // namespace
28 changes: 28 additions & 0 deletions src/test/frametest.cpp
Original file line number Diff line number Diff line change
@@ -78,3 +78,31 @@ TEST_F(FrameTest, LowerFrameBoundary) {
EXPECT_EQ(mixxx::audio::FramePos(-101), mixxx::audio::FramePos(-100.1).toLowerFrameBoundary());
EXPECT_EQ(mixxx::audio::FramePos(-101), mixxx::audio::FramePos(-100.9).toLowerFrameBoundary());
}

TEST_F(FrameTest, UpperFrameBoundary) {
EXPECT_EQ(mixxx::audio::FramePos(0), mixxx::audio::FramePos(0).toUpperFrameBoundary());
EXPECT_EQ(mixxx::audio::FramePos(1), mixxx::audio::FramePos(0.5).toUpperFrameBoundary());
EXPECT_EQ(mixxx::audio::FramePos(1), mixxx::audio::FramePos(0.9).toUpperFrameBoundary());
EXPECT_EQ(mixxx::audio::FramePos(100), mixxx::audio::FramePos(100).toUpperFrameBoundary());
EXPECT_EQ(mixxx::audio::FramePos(101), mixxx::audio::FramePos(100.1).toUpperFrameBoundary());
EXPECT_EQ(mixxx::audio::FramePos(101), mixxx::audio::FramePos(100.9).toUpperFrameBoundary());
EXPECT_EQ(mixxx::audio::FramePos(-99), mixxx::audio::FramePos(-99.9).toUpperFrameBoundary());
EXPECT_EQ(mixxx::audio::FramePos(-99), mixxx::audio::FramePos(-99.1).toUpperFrameBoundary());
EXPECT_EQ(mixxx::audio::FramePos(-100), mixxx::audio::FramePos(-100.1).toUpperFrameBoundary());
EXPECT_EQ(mixxx::audio::FramePos(-100), mixxx::audio::FramePos(-100.9).toUpperFrameBoundary());
}

TEST_F(FrameTest, NearestFrameBoundary) {
EXPECT_EQ(mixxx::audio::FramePos(0), mixxx::audio::FramePos(0).toNearestFrameBoundary());
EXPECT_EQ(mixxx::audio::FramePos(1), mixxx::audio::FramePos(0.5).toNearestFrameBoundary());
EXPECT_EQ(mixxx::audio::FramePos(1), mixxx::audio::FramePos(0.9).toNearestFrameBoundary());
EXPECT_EQ(mixxx::audio::FramePos(100), mixxx::audio::FramePos(100).toNearestFrameBoundary());
EXPECT_EQ(mixxx::audio::FramePos(100), mixxx::audio::FramePos(100.1).toNearestFrameBoundary());
EXPECT_EQ(mixxx::audio::FramePos(101), mixxx::audio::FramePos(100.9).toNearestFrameBoundary());
EXPECT_EQ(mixxx::audio::FramePos(-100), mixxx::audio::FramePos(-99.9).toNearestFrameBoundary());
EXPECT_EQ(mixxx::audio::FramePos(-99), mixxx::audio::FramePos(-99.1).toNearestFrameBoundary());
EXPECT_EQ(mixxx::audio::FramePos(-100),
mixxx::audio::FramePos(-100.1).toNearestFrameBoundary());
EXPECT_EQ(mixxx::audio::FramePos(-101),
mixxx::audio::FramePos(-100.9).toNearestFrameBoundary());
}
34 changes: 22 additions & 12 deletions src/track/beatmap.cpp
Original file line number Diff line number Diff line change
@@ -32,7 +32,7 @@ inline Beat beatFromFramePos(mixxx::audio::FramePos beatPosition) {
return beat;
}

bool BeatLessThan(const Beat& beat1, const Beat& beat2) {
inline bool beatLessThan(const Beat& beat1, const Beat& beat2) {
return beat1.frame_position() < beat2.frame_position();
}

@@ -315,11 +315,11 @@ audio::FramePos BeatMap::findNthBeat(audio::FramePos position, int n) const {
return audio::kInvalidFramePos;
}

Beat beat = beatFromFramePos(position);
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);
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.
@@ -401,11 +401,11 @@ bool BeatMap::findPrevNextBeats(audio::FramePos position,
return false;
}

Beat beat = beatFromFramePos(position);
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);
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.
@@ -481,15 +481,19 @@ std::unique_ptr<BeatIterator> BeatMap::findBeats(
return std::unique_ptr<BeatIterator>();
}

Beat startBeat = beatFromFramePos(startPosition);
Beat endBeat = beatFromFramePos(endPosition);
// Beats can only appear a full frame positions. If the start position is
// fractional, it needs to be rounded up to avoid finding a beat that
// appears *before* the requested start position. For the end position, the
// opposite applies, i.e. we need to round down to avoid finding a beat
// *after* the requested end position.
Beat startBeat = beatFromFramePos(startPosition.toUpperFrameBoundary());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the rational to shrink the range here compared to the original version?
I think a comment would be helpful.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It this whole function unused? Delete!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, it is used in WaveformRenderBeat::draw() to find the visible beats in a time range. So it is still required.

Copy link
Member Author

@Holzhaus Holzhaus Jul 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the rational to shrink the range here compared to the original version?
I think a comment would be helpful.

Beats can only appear a full frame positions.
Let's say a beat is at pos 100. If I say "Give me all beats between 100.5 and 200", it should not give me the beat at 100.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beat endBeat = beatFromFramePos(endPosition.toLowerFrameBoundary());

BeatList::const_iterator curBeat =
std::lower_bound(m_beats.constBegin(), m_beats.constEnd(),
startBeat, BeatLessThan);
std::lower_bound(m_beats.constBegin(), m_beats.constEnd(), startBeat, beatLessThan);

BeatList::const_iterator lastBeat =
std::upper_bound(m_beats.constBegin(), m_beats.constEnd(), endBeat, BeatLessThan);
std::upper_bound(m_beats.constBegin(), m_beats.constEnd(), endBeat, beatLessThan);

if (curBeat >= lastBeat) {
return std::unique_ptr<BeatIterator>();
@@ -503,8 +507,14 @@ bool BeatMap::hasBeatInRange(audio::FramePos startPosition, audio::FramePos endP
startPosition > endPosition) {
return false;
}
audio::FramePos beatPosition = findNextBeat(startPosition);
if (beatPosition <= endPosition) {
audio::FramePos beatPosition = findNextBeat(startPosition.toUpperFrameBoundary());

// FIXME: The following assertion should always hold true, but it doesn't,
// because the position matching in findNthBeat() is fuzzy. This should be
// resolved, and moved to the calling code, to only use fuzzy matches when
// actually desired.
// DEBUG_ASSERT(!beatPosition.isValid() || beatPosition >= startPosition.toUpperFrameBoundary());
if (beatPosition.isValid() && beatPosition <= endPosition.toLowerFrameBoundary()) {
return true;
}
return false;