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

More cue hint for caching reader #4771

Merged
merged 5 commits into from
May 27, 2022
Merged
Show file tree
Hide file tree
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
22 changes: 16 additions & 6 deletions src/engine/cachingreader/cachingreader.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,32 @@
// SoundSource will be used 'soon' and so it should be brought into memory by
// the reader work thread.
typedef struct Hint {
enum class Type {
SlipPosition, // prio 1 (so far unused Mixxx 2.3 priority for reference)
CurrentPosition, // prio 1
LoopStartEnabled, // prio 2
MainCue, // prio 10
HotCue, // prio 10
LoopEndEnabled, // prio 10
LoopStart, // prio 10
FirstSound,
IntroStart,
IntroEnd,
OutroStart
};

// The frame to ensure is present in memory.
SINT frame;
// If a range of frames should be present, use frameCount to indicate that the
// range (frame, frame + frameCount) should be present in memory.
SINT frameCount;
// Currently unused -- but in the future could be used to prioritize certain
// hints over others. A priority of 1 is the highest priority and should be
// used for samples that will be read imminently. Hints for samples that
// have the potential to be read (i.e. a cue point) should be issued with
// priority >10.
int priority;
// hints over others.
Type type;

// for the default frame count in forward direction
static constexpr SINT kFrameCountForward = 0;
static constexpr SINT kFrameCountBackward = -1;

} Hint;

// Note that we use a QVarLengthArray here instead of a QVector. Since this list
Expand Down
49 changes: 32 additions & 17 deletions src/engine/controls/cuecontrol.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,25 @@ inline int hotcueNumberToHotcueIndex(int hotcueNumber) {
}
}

void appendCueHint(HintVector* pHintList, const mixxx::audio::FramePos& frame, Hint::Type type) {
Copy link
Member

Choose a reason for hiding this comment

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

we should rethink our "mutable references should be pointers" rule. Taking a value that could be nullptr and assuming its not is simply bad. Especially when the consequence of that assumption is a crash.

Suggested change
void appendCueHint(HintVector* pHintList, const mixxx::audio::FramePos& frame, Hint::Type type) {
void appendCueHint(HintVector& pHintList, const mixxx::audio::FramePos& frame, Hint::Type type) {

Copy link
Contributor

Choose a reason for hiding this comment

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

This ancient rule causes lots of boilerplate extra code that might contain bugs. I have argued for years until giving up and moving on.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. It seems this "style-preference" (though IMO its much more than just a matter of style) is in the minority in the C++ community. Even ignoring the Google/Qt vs CppCoreGuidelines "controversy", the simple argument of type-safety (and the thus gained safety) suffices for a decision. The less "illegal" states your type has, the less you need to check before using it. With my Rust experience, whenever I see a pointer in C++, I'm thinking that's an intentional Option<Box<T>> so there is either something special going on with the actual presence of the contained value or its lifetime. Especially because its already trivial to express mutable "borrowing" in C++ by just using a mutable reference.

Copy link
Member

@Swiftb0y Swiftb0y May 26, 2022

Choose a reason for hiding this comment

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

The Original google styleguide says:

References can be confusing, as they have value syntax but pointer semantics.

I have never even noticed this being an issue personally. I've struggled to find any arguments on this online, so I invite everyone to come up with their own example because I can neither think of nor find one.

Copy link
Member Author

Choose a reason for hiding this comment

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

The null check is now in place.

Copy link
Member

Choose a reason for hiding this comment

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

But its not needed if one just used a mutable reference. And its not free either...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, If you assume that a reference cannot be null which is not the case. But anyway, that the Mixxx style, no need to discuss it here.

Copy link
Member

Choose a reason for hiding this comment

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

Would you mind if we continued this discussion on Zulip? It seems like the majority of mixxx devs is dissatisfied by this style and we'd like to transition away from it.

VERIFY_OR_DEBUG_ASSERT(pHintList) {
return;
}

if (frame.isValid()) {
const Hint cueHint = {
/*.frame =*/static_cast<SINT>(frame.toLowerFrameBoundary().value()),
/*.frameCount =*/Hint::kFrameCountForward,
/*.type =*/type};
pHintList->append(cueHint);
}
}

void appendCueHint(HintVector* pHintList, const double playPos, Hint::Type type) {
const auto frame = mixxx::audio::FramePos::fromEngineSamplePosMaybeInvalid(playPos);
appendCueHint(pHintList, frame, type);
}

} // namespace

CueControl::CueControl(const QString& group,
Expand Down Expand Up @@ -1160,29 +1179,25 @@ void CueControl::hotcueEndPositionChanged(
}

void CueControl::hintReader(HintVector* pHintList) {
Hint cueHint;
const auto mainCuePosition =
mixxx::audio::FramePos::fromEngineSamplePosMaybeInvalid(
m_pCuePoint->get());
if (mainCuePosition.isValid()) {
cueHint.frame = static_cast<SINT>(mainCuePosition.toLowerFrameBoundary().value());
cueHint.frameCount = Hint::kFrameCountForward;
cueHint.priority = 10;
pHintList->append(cueHint);
}
appendCueHint(pHintList, m_pCuePoint->get(), Hint::Type::MainCue);

// this is called from the engine thread
// it is no locking required, because m_hotcueControl is filled during the
// constructor and getPosition()->get() is a ControlObject
for (const auto& pControl : qAsConst(m_hotcueControls)) {
const mixxx::audio::FramePos position = pControl->getPosition();
if (position.isValid()) {
cueHint.frame = static_cast<SINT>(position.toLowerFrameBoundary().value());
cueHint.frameCount = Hint::kFrameCountForward;
cueHint.priority = 10;
pHintList->append(cueHint);
}
appendCueHint(pHintList, pControl->getPosition(), Hint::Type::HotCue);
}

CuePointer pAudibleSound =
m_pLoadedTrack->findCueByType(mixxx::CueType::AudibleSound);
if (pAudibleSound) {
const mixxx::audio::FramePos frame = pAudibleSound->getPosition();
appendCueHint(pHintList, frame, Hint::Type::FirstSound);
}

appendCueHint(pHintList, m_pIntroStartPosition->get(), Hint::Type::IntroStart);
appendCueHint(pHintList, m_pIntroEndPosition->get(), Hint::Type::IntroEnd);
appendCueHint(pHintList, m_pOutroStartPosition->get(), Hint::Type::OutroStart);
}

// Moves the cue point to current position or to closest beat in case
Expand Down
6 changes: 3 additions & 3 deletions src/engine/controls/loopingcontrol.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -486,22 +486,22 @@ void LoopingControl::hintReader(HintVector* pHintList) {
// direction we're going in, but that this is much simpler, and hints
// aren't that bad to make anyway.
if (loopInfo.startPosition.isValid()) {
loop_hint.priority = 2;
loop_hint.type = Hint::Type::LoopStartEnabled;
loop_hint.frame = static_cast<SINT>(
loopInfo.startPosition.toLowerFrameBoundary().value());
loop_hint.frameCount = Hint::kFrameCountForward;
pHintList->append(loop_hint);
}
if (loopInfo.endPosition.isValid()) {
loop_hint.priority = 10;
loop_hint.type = Hint::Type::LoopEndEnabled;
loop_hint.frame = static_cast<SINT>(
loopInfo.endPosition.toUpperFrameBoundary().value());
loop_hint.frameCount = Hint::kFrameCountBackward;
pHintList->append(loop_hint);
}
} else {
if (loopInfo.startPosition.isValid()) {
loop_hint.priority = 10;
loop_hint.type = Hint::Type::LoopStart;
loop_hint.frame = static_cast<SINT>(
loopInfo.startPosition.toLowerFrameBoundary().value());
loop_hint.frameCount = Hint::kFrameCountForward;
Expand Down
2 changes: 1 addition & 1 deletion src/engine/enginebuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1395,7 +1395,7 @@ void EngineBuffer::hintReader(const double dRate) {
if (m_bSlipEnabledProcessing) {
Hint hint;
hint.frame = static_cast<SINT>(m_slipPosition.toLowerFrameBoundary().value());
hint.priority = 1;
hint.type = Hint::Type::SlipPosition;
if (m_dSlipRate >= 0) {
hint.frameCount = Hint::kFrameCountForward;
} else {
Expand Down
2 changes: 1 addition & 1 deletion src/engine/readaheadmanager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ void ReadAheadManager::hintReader(double dRate, HintVector* pHintList) {
}

// top priority, we need to read this data immediately
current_position.priority = 1;
current_position.type = Hint::Type::CurrentPosition;
pHintList->append(current_position);
}

Expand Down