Skip to content

Commit

Permalink
Beats: Remove legacy BeatIterator class in favor of std::iterator
Browse files Browse the repository at this point in the history 
The custom, legacy iterator API was only used for rendering beats on the
waveform and for importing beats from Serato BeatGrids.

Since the new iterator API is way more powerful and can be used
with the iterator-based functions from `<algorithm>`, it makes sense to
replace the only occurrence and then remove the old API entirely.
  • Loading branch information
@Holzhaus
Holzhaus committed Oct 26, 2021
1 parent 5158e62 commit 98ed207
Show file tree
Hide file tree
Showing 5 changed files with 20 additions and 123 deletions.
56 changes: 0 additions & 56 deletions src/test/beatmaptest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,6 @@ 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 @@ -228,53 +219,6 @@ TEST_F(BeatMapTest, TestNthBeatWhenNotOnBeat) {
EXPECT_EQ(nextBeat, foundNextBeat);
}

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 = Beats::fromBeatPositions(m_pTrack->getSampleRate(), 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;
Expand Down
27 changes: 12 additions & 15 deletions src/test/seratobeatgridtest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,16 +173,15 @@ TEST_F(SeratoBeatGridTest, SerializeBeatMap) {
const auto pImportedBeats =
beatsImporter.importBeatsAndApplyTimingOffset(
timingOffsetMillis, signalInfo);
auto pBeatsIterator =
pImportedBeats->findBeats(beatPositionsFrames.first() - 1000,
beatPositionsFrames.last() + 1000);
auto it = pImportedBeats->iteratorFrom(beatPositionsFrames.first() - 1000);
for (int i = 0; i < beatPositionsFrames.size(); i++) {
const auto importedPosition = pBeatsIterator->next();
const auto importedPosition = *it;
EXPECT_NEAR(beatPositionsFrames[i].value(),
importedPosition.value(),
kEpsilon);
it++;
}
ASSERT_FALSE(pBeatsIterator->hasNext());
ASSERT_TRUE(*it >= beatPositionsFrames.last() + 1000);
}

constexpr int kNumBeats60BPM = 4;
Expand Down Expand Up @@ -226,16 +225,15 @@ TEST_F(SeratoBeatGridTest, SerializeBeatMap) {
const auto pImportedBeats =
beatsImporter.importBeatsAndApplyTimingOffset(
timingOffsetMillis, signalInfo);
auto pBeatsIterator =
pImportedBeats->findBeats(beatPositionsFrames.first() - 1000,
beatPositionsFrames.last() + 1000);
auto it = pImportedBeats->iteratorFrom(beatPositionsFrames.first() - 1000);
for (int i = 0; i < beatPositionsFrames.size(); i++) {
const auto importedPosition = pBeatsIterator->next();
const auto importedPosition = *it;
EXPECT_NEAR(beatPositionsFrames[i].value(),
importedPosition.value(),
kEpsilon);
it++;
}
ASSERT_FALSE(pBeatsIterator->hasNext());
ASSERT_TRUE(*it >= beatPositionsFrames.last() + 1000);
}

qInfo() << "Step 3: Add" << kNumBeats120BPM << "beats at 100 bpm to the beatgrid";
Expand Down Expand Up @@ -287,16 +285,15 @@ TEST_F(SeratoBeatGridTest, SerializeBeatMap) {
const auto pImportedBeats =
beatsImporter.importBeatsAndApplyTimingOffset(
timingOffsetMillis, signalInfo);
auto pBeatsIterator =
pImportedBeats->findBeats(beatPositionsFrames.first() - 1000,
beatPositionsFrames.last() + 1000);
auto it = pImportedBeats->iteratorFrom(beatPositionsFrames.first() - 1000);
for (int i = 0; i < beatPositionsFrames.size(); i++) {
const auto importedPosition = pBeatsIterator->next();
const auto importedPosition = *it;
EXPECT_NEAR(beatPositionsFrames[i].value(),
importedPosition.value(),
kEpsilon);
it++;
}
ASSERT_FALSE(pBeatsIterator->hasNext());
ASSERT_TRUE(*it >= beatPositionsFrames.last() + 1000);
}
}

Expand Down
20 changes: 0 additions & 20 deletions src/track/beats.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,6 @@ constexpr double kEpsilon = 0.01;

namespace mixxx {

bool BeatIterator::hasNext() const {
return *m_it <= m_endPosition;
}

audio::FramePos BeatIterator::next() {
VERIFY_OR_DEBUG_ASSERT(hasNext()) {
return audio::kInvalidFramePos;
}
const audio::FramePos position = *m_it;
m_it++;
return position;
}

mixxx::audio::FrameDiff_t Beats::ConstIterator::beatLengthFrames() const {
if (m_it == m_beats->m_markers.cend()) {
return m_beats->endBeatLengthFrames();
Expand Down Expand Up @@ -512,13 +499,6 @@ audio::FramePos Beats::findNBeatsFromPosition(audio::FramePos position, double b
return basePosition + it.beatLengthFrames() * fractionBeats;
}

std::unique_ptr<BeatIterator> Beats::findBeats(
audio::FramePos startPosition,
audio::FramePos endPosition) const {
auto it = iteratorFrom(startPosition);
return std::make_unique<BeatIterator>(std::move(it), endPosition);
}

bool Beats::hasBeatInRange(audio::FramePos startPosition, audio::FramePos endPosition) const {
const audio::FramePos nextBeatPosition = findNextBeat(startPosition);
return (nextBeatPosition <= endPosition);
Expand Down
26 changes: 0 additions & 26 deletions src/track/beats.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
namespace mixxx {

class Beats;
class BeatIterator;
typedef std::shared_ptr<const Beats> BeatsPointer;

class BeatMarker {
Expand Down Expand Up @@ -335,15 +334,6 @@ class Beats : private std::enable_shared_from_this<Beats> {
audio::FramePos findNBeatsFromPosition(
audio::FramePos position, double beats) const;

/// Returns an iterator that yields frame position of every beat occurring
/// between `startPosition` and `endPosition`. `BeatIterator` must be iterated
/// while holding a strong references to the `Beats` object to ensure that
/// the `Beats` object is not deleted. Caller takes ownership of the returned
/// `BeatIterator`.
std::unique_ptr<BeatIterator> findBeats(
audio::FramePos startPosition,
audio::FramePos endPosition) const;

/// Return whether or not a beat exists between `startPosition` and `endPosition`.
bool hasBeatInRange(audio::FramePos startPosition,
audio::FramePos endPosition) const;
Expand Down Expand Up @@ -425,20 +415,4 @@ class Beats : private std::enable_shared_from_this<Beats> {
const QString m_subVersion;
};

class BeatIterator {
public:
BeatIterator(Beats::ConstIterator it, audio::FramePos endPosition)
: m_it(std::move(it)),
m_endPosition(endPosition) {
}

~BeatIterator() = default;
bool hasNext() const;
audio::FramePos next();

private:
Beats::ConstIterator m_it;
const audio::FramePos m_endPosition;
};

} // namespace mixxx
14 changes: 8 additions & 6 deletions src/waveform/renderers/waveformrenderbeat.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,14 @@ void WaveformRenderBeat::draw(QPainter* painter, QPaintEvent* /*event*/) {
// << "firstDisplayedPosition" << firstDisplayedPosition
// << "lastDisplayedPosition" << lastDisplayedPosition;

std::unique_ptr<mixxx::BeatIterator> it(trackBeats->findBeats(
mixxx::audio::FramePos::fromEngineSamplePos(firstDisplayedPosition * trackSamples),
mixxx::audio::FramePos::fromEngineSamplePos(lastDisplayedPosition * trackSamples)));
const auto startPosition = mixxx::audio::FramePos::fromEngineSamplePos(
firstDisplayedPosition * trackSamples);
const auto endPosition = mixxx::audio::FramePos::fromEngineSamplePos(
lastDisplayedPosition * trackSamples);
auto it = trackBeats->iteratorFrom(startPosition);

// if no beat do not waste time saving/restoring painter
if (!it || !it->hasNext()) {
if (it == trackBeats->clatest() || *it > endPosition) {
return;
}

Expand All @@ -79,8 +81,8 @@ void WaveformRenderBeat::draw(QPainter* painter, QPaintEvent* /*event*/) {

int beatCount = 0;

while (it->hasNext()) {
double beatPosition = it->next().toEngineSamplePos();
for (; it != trackBeats->clatest() && *it <= endPosition; ++it) {
double beatPosition = it->toEngineSamplePos();
double xBeatPoint =
m_waveformRenderer->transformSamplePositionInRendererWorld(beatPosition);

Expand Down

0 comments on commit 98ed207

Please sign in to comment.