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

Enable beatloop activation directly after activating a hotcue #2190

Closed
wants to merge 8 commits into from
+88 −25
Closed

Enable beatloop activation directly after activating a hotcue #2190

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7a418a7
set loop position to queued seek position before activating beatloop
goddisignz Jun 24, 2019
255fd69
implemented most requested changes
goddisignz Jun 30, 2019
c8f9a5b
added parameter for synchronized position in engines "notifySeek"
goddisignz Jun 30, 2019
cc375f6
Revert "added parameter for synchronized position in engines "notifyS…
goddisignz Jul 2, 2019
d441ca8
let the loopingcontroller adjust synced seeks if necessary
goddisignz Jul 2, 2019
2fdffa2
prevent jumping out of loop if loop is smaller than one bar
goddisignz Jul 5, 2019
7ea5f07
remove obsolete adjustingPhase parameter
goddisignz Jul 5, 2019
0d1cb49
do not unset m_iSeekQueued before controls have been notified
goddisignz Jul 14, 2019
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
3 changes: 2 additions & 1 deletion src/engine/controls/enginecontrol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,9 @@ void EngineControl::seek(double sample) {
}
}

void EngineControl::notifySeek(double dNewPlaypos, bool adjustingPhase) {
void EngineControl::notifySeek(double dNewPlaypos, double dSyncedNewPlayPos, bool adjustingPhase) {
Q_UNUSED(dNewPlaypos);
Q_UNUSED(dSyncedNewPlayPos);
Q_UNUSED(adjustingPhase);
}

Expand Down
2 changes: 1 addition & 1 deletion src/engine/controls/enginecontrol.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class EngineControl : public QObject {
}

// Called whenever a seek occurs to allow the EngineControl to respond.
virtual void notifySeek(double dNewPlaypo, bool adjustingPhase);
virtual void notifySeek(double dNewPlaypos, double dSyncedNewPlayPos, bool adjustingPhase);
virtual void trackLoaded(TrackPointer pNewTrack);

protected:
Expand Down
18 changes: 13 additions & 5 deletions src/engine/controls/loopingcontrol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,12 @@
#include <QtDebug>

#include "control/controlobject.h"
#include "preferences/usersettings.h"
#include "control/controlpushbutton.h"
#include "engine/controls/loopingcontrol.h"
#include "engine/controls/bpmcontrol.h"
#include "engine/controls/enginecontrol.h"
#include "engine/controls/loopingcontrol.h"
#include "engine/enginebuffer.h"
#include "preferences/usersettings.h"
#include "util/compatibility.h"
#include "util/math.h"
#include "util/sample.h"
Expand Down Expand Up @@ -768,7 +769,7 @@ void LoopingControl::slotLoopEndPos(double pos) {
}

// This is called from the engine thread
void LoopingControl::notifySeek(double dNewPlaypos, bool adjustingPhase) {
void LoopingControl::notifySeek(double dNewPlaypos, double dSyncedNewPlayPos, bool adjustingPhase) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you clarify and finally ad a code comment why, we need to pass two positions here?

I am unsure, if we still need the adjustingPhase value. Is it only read here? Original it was to prevent jump out of a loop when adjusting phase.
Now we have the original and the adjusted position to detect this.

What was the case, you need this code for?
What are the cases, we need to considder?

Can you rename dNewPlayposition to
something more descriptive?
Like dRequestedPlaypos or such?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assume an active loop starting exactly at hotcue A. If I activate the hotcue the synced position is very likely in front of the loop. The adjustingPhase is still false, as it is was a SEEK_STANDARD_PHASE. If we set the adjustingPhase to true in that case, the loop would not be disabled for any other seek position outside the loop.
Thus, the loopcontroller should know about the position requested by the user but the remaining controllers should still be notified with the synced position.

I assume that the adjustPhase is still necessary. When there was no sync, both positions are equal. But (I guess) the same holds true if only a sync without any seeking is requested.

Copy link
Contributor Author

@goddisignz goddisignz Jul 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I just had a thought on solving this case differently in the processSeek method. Let the loopingcontroller calculate a position inside a loop (if enabled) based on the requested and the synced position. If the requested position is outside the loop return the synced position and behave as currently. If the requested position is inside but the synced position is outside the loop, correct the synced position into the loop.

double syncPosition = position;
if (!paused && (seekType & SEEK_PHASE)) {
    syncPosition = m_pBpmControl->getNearestPositionInPhase(position, true, true);
    position = m_pLoopingControl->getSyncPositionInsideLoop(position, syncPosition);
}

The setNewPos and notifySeek could be left unchanged.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, even better readability. Thank you.

But why do we need syncPosition outside the if block?
I think it would be even more clean to have a requested position as well and not reuse position for this.

LoopSamples loopSamples = m_loopSamples.getValue();
double currentSample = m_currentSample.getValue();
if (m_bLoopingEnabled && !adjustingPhase) {
Expand All @@ -782,8 +783,8 @@ void LoopingControl::notifySeek(double dNewPlaypos, bool adjustingPhase) {
setLoopingEnabled(false);
}
if (currentSample <= loopSamples.end &&
dNewPlaypos > loopSamples.end) {
// jumping out a loop or over a catching loop forward
dNewPlaypos >= loopSamples.end) {
// jumping out or to the exact end of a loop or over a catching loop forward
setLoopingEnabled(false);
}
}
Expand Down Expand Up @@ -958,6 +959,13 @@ void LoopingControl::updateBeatLoopingControls() {
}

void LoopingControl::slotBeatLoop(double beats, bool keepStartPoint, bool enable) {
// if a seek was queued in the engine buffer move the current sample to its position
double p_seekPosition = 0;
if (getEngineBuffer()->isSeekQueued(&p_seekPosition)) {
// seek position is already quantized if quantization is enabled
m_currentSample.setValue(p_seekPosition);
}

double maxBeatSize = s_dBeatSizes[sizeof(s_dBeatSizes)/sizeof(s_dBeatSizes[0]) - 1];
double minBeatSize = s_dBeatSizes[0];
if (beats < 0) {
Expand Down
2 changes: 1 addition & 1 deletion src/engine/controls/loopingcontrol.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class LoopingControl : public EngineControl {
// sample, if set.
void hintReader(HintVector* pHintList) override;

void notifySeek(double dNewPlaypos, bool adjustingPhase) override;
void notifySeek(double dNewPlaypos, double dSyncedNewPlayPos, bool adjustingPhase) override;

bool isLoopingEnabled();

Expand Down
4 changes: 2 additions & 2 deletions src/engine/controls/ratecontrol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,7 @@ void RateControl::resetRateTemp(void)
setRateTemp(0.0);
}

void RateControl::notifySeek(double playPos, bool adjustingPhase) {
void RateControl::notifySeek(double playPos, double dSyncedNewPlayPos, bool adjustingPhase) {
Q_UNUSED(adjustingPhase);
m_pScratchController->notifySeek(playPos);
m_pScratchController->notifySeek(dSyncedNewPlayPos);
}
2 changes: 1 addition & 1 deletion src/engine/controls/ratecontrol.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ class RateControl : public EngineControl {
// Set Rate Ramp Sensitivity
static void setRateRampSensitivity(int);
static int getRateRampSensitivity();
void notifySeek(double dNewPlaypos, bool adjustingPhase) override;
void notifySeek(double dNewPlaypos, double dSyncedNewPlayPos, bool adjustingPhase) override;

public slots:
void slotReverseRollActivate(double);
Expand Down
21 changes: 14 additions & 7 deletions src/engine/enginebuffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,10 +430,10 @@ void EngineBuffer::seekCloneBuffer(EngineBuffer* pOtherBuffer) {

// WARNING: This method is not thread safe and must not be called from outside
// the engine callback!
void EngineBuffer::setNewPlaypos(double newpos, bool adjustingPhase) {
void EngineBuffer::setNewPlaypos(double dNewPlayPos, double dSyncedNewPlayPos, bool adjustingPhase) {
//qDebug() << m_group << "engine new pos " << newpos;

m_filepos_play = newpos;
m_filepos_play = dSyncedNewPlayPos;

if (m_rate_old != 0.0) {
// Before seeking, read extra buffer for crossfading
Expand All @@ -449,7 +449,7 @@ void EngineBuffer::setNewPlaypos(double newpos, bool adjustingPhase) {

// Must hold the engineLock while using m_engineControls
for (const auto& pControl: qAsConst(m_engineControls)) {
pControl->notifySeek(m_filepos_play, adjustingPhase);
pControl->notifySeek(dNewPlayPos, m_filepos_play, adjustingPhase);
}

verifyPlay(); // verify or update play button and indicator
Expand Down Expand Up @@ -1125,7 +1125,7 @@ void EngineBuffer::processSeek(bool paused) {
// call anyway again.

SeekRequests seekType = static_cast<SeekRequest>(
m_iSeekQueued.fetchAndStoreRelease(SEEK_NONE));
m_iSeekQueued.fetchAndStoreRelaxed(SEEK_NONE));
Copy link
Member

Choose a reason for hiding this comment

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

here wee need to be sure that m_queuedSeekPosition.getValue() is not accessed before reading and writing m_iSeekQueued so I think fetchAndSubAcquire() would be the right call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That must be an autocomplete-mistake. I wanted to change it back to the original version.

Copy link
Member

Choose a reason for hiding this comment

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

But it was originally wrong. We need the Aquire memory fence here, to stop the pipeline executing the reading of the seek position too early.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will change that. But which value should I subtract?
Are you sure fetchAndSubAcquire shouldn't be fetchAndStoreAcquire?

Copy link
Member

Choose a reason for hiding this comment

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

Ups ... of cause.

double position = m_queuedSeekPosition.getValue();

// Don't allow the playposition to go past the end.
Expand Down Expand Up @@ -1163,11 +1163,13 @@ void EngineBuffer::processSeek(bool paused) {
return;
}

double syncPosition = position;
if (!paused && (seekType & SEEK_PHASE)) {
position = m_pBpmControl->getNearestPositionInPhase(position, true, true);
syncPosition = m_pBpmControl->getNearestPositionInPhase(position, true, true);
}
if (position != m_filepos_play) {
setNewPlaypos(position, adjustingPhase);

if (syncPosition != m_filepos_play) {
setNewPlaypos(position, syncPosition, adjustingPhase);
}
}

Expand Down Expand Up @@ -1284,6 +1286,11 @@ bool EngineBuffer::isTrackLoaded() {
return false;
}

bool EngineBuffer::isSeekQueued(double* pSeekPosition) {
*pSeekPosition = m_queuedSeekPosition.getValue();
return m_iSeekQueued.load() != SEEK_NONE;
}

void EngineBuffer::slotEjectTrack(double v) {
if (v > 0) {
// Don't allow rejections while playing a track. We don't need to lock to
Expand Down
3 changes: 2 additions & 1 deletion src/engine/enginebuffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ class EngineBuffer : public EngineObject {

QString getGroup();
bool isTrackLoaded();
bool isSeekQueued(double* pSeekPosition);
TrackPointer getLoadedTrack() const;

double getExactPlayPos();
Expand Down Expand Up @@ -230,7 +231,7 @@ class EngineBuffer : public EngineObject {
void seekCloneBuffer(EngineBuffer* pOtherBuffer);

// Reset buffer playpos and set file playpos.
void setNewPlaypos(double playpos, bool adjustingPhase);
void setNewPlaypos(double dNewPlayPos, double dSyncedNewPlayPos, bool adjustingPhase);

void processSyncRequests();
void processSeek(bool paused);
Expand Down
2 changes: 1 addition & 1 deletion src/engine/readaheadmanager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ double ReadAheadManager::getFilePlaypositionFromLog(
// (Not looping control)
if (shouldNotifySeek) {
if (m_pRateControl) {
m_pRateControl->notifySeek(entry.virtualPlaypositionStart, false);
m_pRateControl->notifySeek(entry.virtualPlaypositionStart, 0, false);
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/test/readaheadmanager_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,9 @@ class StubLoopControl : public LoopingControl {
Q_UNUSED(pHintList);
}

void notifySeek(double dNewPlaypos, bool adjustingPhase) override {
void notifySeek(double dNewPlaypos, double dSyncedNewPlayPos, bool adjustingPhase) override {
Q_UNUSED(dNewPlaypos);
Q_UNUSED(dSyncedNewPlayPos);
Q_UNUSED(adjustingPhase);
}

Expand Down