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

Beats: Remove legacy BeatIterator class in favor of std iterator #4411

Merged
merged 2 commits into from
Oct 29, 2021
Merged

Beats: Remove legacy BeatIterator class in favor of std iterator #4411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
98ed207
Beats: Remove legacy BeatIterator class in favor of std::iterator
Holzhaus Oct 13, 2021
9480561
Beats: Add comments for iterator methods (cbegin/cend/cearliest/clatest)
Holzhaus Oct 26, 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
56 changes: 0 additions & 56 deletions src/test/beatmaptest.cpp
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
39 changes: 13 additions & 26 deletions src/track/beats.h
View file
Open in desktop
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 @@ -174,18 +173,31 @@ class Beats : private std::enable_shared_from_this<Beats> {

~Beats() = default;

/// Returns an iterator pointing to the position of the first beat marker.
ConstIterator cbegin() const {
return ConstIterator(this, m_markers.cbegin(), 0);
}

/// Returns an iterator pointing to the position of the first beat after
/// the end beat marker.
ConstIterator cend() const {
return ConstIterator(this, m_markers.cend(), 1);
}

/// Returns an iterator pointing to earliest representable beat position
/// (which is INT_MIN beats before the first beat marker).
///
/// Warning: Decrementing the iterator returned by this function will
/// result in an integer underflow.
ConstIterator cearliest() const {
return ConstIterator(this, m_markers.cbegin(), std::numeric_limits<int>::lowest());
}

/// Returns an iterator pointing to latest representable beat position
/// (which is INT_MAX beats behind the end beat marker).
///
/// Warning: Incrementing the iterator returned by this function will
/// result in an integer overflow.
ConstIterator clatest() const {
return ConstIterator(this, m_markers.cend(), std::numeric_limits<int>::max());
}
Expand Down Expand Up @@ -335,15 +347,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 +428,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
Open in desktop
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) {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for jumping in here, after merging #4255, but it I have discovered now that I have not fully understand the concept of clatest() and cend()

Unfortunately there are zero comments in beats.h explaining this. Can you catch up that in this PR?

I think I have read the original commit as if clatest()++ = cend() but It turns out that this is wrong.
Maybe we can improve the naming a bit.

Here, I have hard time to understand how the new code id equivalent to the old.

Copy link
Member Author

@Holzhaus Holzhaus Oct 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I added some comments to beats.h in 9480561.

Copy link
Member Author

@Holzhaus Holzhaus Oct 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Basically the it != clatest() check prevents an integer overflow here.

double beatPosition = it->toEngineSamplePos();
double xBeatPoint =
m_waveformRenderer->transformSamplePositionInRendererWorld(beatPosition);

Expand Down