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

Jump to bottom

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

Merged
merged 12 commits into from
Jul 11, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
2253c78
FramePos: Add method to rount to upper frame boundary
Holzhaus Jul 10, 2021
015ad88
BeatMap: Allow searching using fractional positions
Holzhaus Jul 10, 2021
a69db4b
BeatMapTest: Add test for finding beats with fractional position
Holzhaus Jul 10, 2021
decfdbe
BeatMap: Ensure proper frame rounding in hasBeatInRange()
Holzhaus Jul 10, 2021
9284952
FrameTest: Add tests for upper/nearest frame boundary
Holzhaus Jul 10, 2021
730c168
BeatMap: Use camelCase instead of PascalCase for beatLessThan function
Holzhaus Jul 10, 2021
51d077f
BeatMap: Add comment regarding fuzzy matches in hasBeatInRange()
Holzhaus Jul 10, 2021
21b310b
BeatMap: Add tests for hasBeatInRange() with fractional positions
Holzhaus Jul 10, 2021
e5a8699
BeatMap: Make beatLessThan helper method inline
Holzhaus Jul 10, 2021
3113dd9
BeatMap: Fix commented out debug assertion
Holzhaus Jul 10, 2021
2d47336
BeatMapTest: Rename countBeatsInIterator to countRemainingBeats
Holzhaus Jul 10, 2021
b6e1088
BeatMap: Add comment explaining rounding in findBeats()
Holzhaus Jul 10, 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
6 changes: 6 additions & 0 deletions src/audio/frame.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
88 changes: 88 additions & 0 deletions src/test/beatmaptest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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 •
edited
Loading

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.

2d47336

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>();
Expand All @@ -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;
Expand Down