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

Fix wrong visual play position when inside loop #11840

Merged
merged 8 commits into from Sep 1, 2023
Merged

Fix wrong visual play position when inside loop #11840

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
38a9391
Fix display of waveform playmarker shown outside loop, while track is…
JoergAtGithub Aug 19, 2023
b67e104
Renamed m_playPosition to m_playPos to unify all position variable names
JoergAtGithub Aug 20, 2023
acfa18f
Consider play rate direction
JoergAtGithub Aug 20, 2023
58025bf
Merge remote-tracking branch 'upstream/2.4' into playindicator
JoergAtGithub Aug 21, 2023
0c0b308
Consider loops when calculating visual play and slip position for the…
JoergAtGithub Aug 21, 2023
e50eb82
Qt6 build fix by SlipModeState type declaration into own file
JoergAtGithub Aug 21, 2023
154863c
Read loop start and loop end position in one atomic access
JoergAtGithub Aug 27, 2023
77164b8
Added a comment, which describes SlipModeState::Armed
JoergAtGithub Aug 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/engine/controls/loopingcontrol.h
View file
Open in desktop
Expand Up @@ -58,6 +58,12 @@ class LoopingControl : public EngineControl {
void setLoop(mixxx::audio::FramePos startPosition,
mixxx::audio::FramePos endPosition,
bool enabled);
mixxx::audio::FramePos getLoopStartPosition() {
Copy link
Member

Choose a reason for hiding this comment

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

Loop Start and End should be only read atomically. This avoids that the position can slip out during moving the loop.
In this case LoopInfo should be returned, or both values in a single call.

return m_loopInfo.getValue().startPosition;
}
mixxx::audio::FramePos getLoopEndPosition() {
return m_loopInfo.getValue().endPosition;
}
void setRateControl(RateControl* rateControl);
bool isLoopingEnabled();
bool isLoopRollActive();
Expand Down
83 changes: 48 additions & 35 deletions src/engine/enginebuffer.cpp
View file
Open in desktop
Expand Up @@ -78,7 +78,7 @@ EngineBuffer::EngineBuffer(const QString& group,
m_pKeyControl(nullptr),
m_pReadAheadManager(nullptr),
m_pReader(nullptr),
m_playPosition(kInitialPlayPosition),
m_playPos(kInitialPlayPosition),
m_speed_old(0),
m_tempo_ratio_old(1.),
m_scratching_old(false),
Expand All @@ -87,7 +87,7 @@ EngineBuffer::EngineBuffer(const QString& group,
m_baserate_old(0),
m_rate_old(0.),
m_trackEndPositionOld(mixxx::audio::kInvalidFramePos),
m_slipPosition(mixxx::audio::kStartFramePos),
m_slipPos(mixxx::audio::kStartFramePos),
m_dSlipRate(1.0),
m_bSlipEnabledProcessing(false),
m_pRepeat(nullptr),
Expand Down Expand Up @@ -455,7 +455,7 @@ void EngineBuffer::readToCrossfadeBuffer(const int iBufferSize) {
// (Must be called only once per callback)
m_pScale->scaleBuffer(m_pCrossfadeBuffer, iBufferSize);
// Restore the original position that was lost due to scaleBuffer() above
m_pReadAheadManager->notifySeek(m_playPosition);
m_pReadAheadManager->notifySeek(m_playPos);
m_bCrossfadeReady = true;
}
}
Expand All @@ -471,14 +471,14 @@ void EngineBuffer::setNewPlaypos(mixxx::audio::FramePos position) {
kLogger.trace() << m_group << "EngineBuffer::setNewPlaypos" << position;
}

m_playPosition = position;
m_playPos = position;

if (m_rate_old != 0.0) {
// Before seeking, read extra buffer for crossfading
// this also sets m_pReadAheadManager to newpos
readToCrossfadeBuffer(m_iLastBufferSize);
} else {
m_pReadAheadManager->notifySeek(m_playPosition);
m_pReadAheadManager->notifySeek(m_playPos);
}
m_pScale->clear();

Expand All @@ -487,7 +487,7 @@ void EngineBuffer::setNewPlaypos(mixxx::audio::FramePos position) {

// Must hold the engineLock while using m_engineControls
for (const auto& pControl: qAsConst(m_engineControls)) {
pControl->notifySeek(m_playPosition);
pControl->notifySeek(m_playPos);
}

verifyPlay(); // verify or update play button and indicator
Expand Down Expand Up @@ -544,7 +544,7 @@ void EngineBuffer::slotTrackLoaded(TrackPointer pTrack,
m_pause.lock();

m_visualPlayPos->setInvalid();
m_playPosition = kInitialPlayPosition; // for execute seeks to 0.0
m_playPos = kInitialPlayPosition; // for execute seeks to 0.0
m_pCurrentTrack = pTrack;
m_pTrackSamples->set(trackNumSamples);
m_pTrackSampleRate->set(trackSampleRate);
Expand All @@ -553,7 +553,7 @@ void EngineBuffer::slotTrackLoaded(TrackPointer pTrack,
// Reset slip mode
m_pSlipButton->set(0);
m_bSlipEnabledProcessing = false;
m_slipPosition = mixxx::audio::kStartFramePos;
m_slipPos = mixxx::audio::kStartFramePos;
m_dSlipRate = 0;

m_queuedSeek.setValue(kNoQueuedSeek);
Expand Down Expand Up @@ -585,7 +585,7 @@ void EngineBuffer::ejectTrack() {
TrackPointer pOldTrack = m_pCurrentTrack;
m_pause.lock();

m_visualPlayPos->set(0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0);
m_visualPlayPos->set(0.0, 0.0, 0.0, 0.0, 0.0, false, 0.0, 0.0, 0.0, 0.0);
doSeekPlayPos(mixxx::audio::kStartFramePos, SEEK_EXACT);

m_pCurrentTrack.reset();
Expand Down Expand Up @@ -626,7 +626,7 @@ void EngineBuffer::notifyTrackLoaded(
const auto sampleRate = mixxx::audio::SampleRate::fromDouble(m_pTrackSampleRate->get());
for (const auto& pControl : qAsConst(m_engineControls)) {
pControl->trackLoaded(pNewTrack);
pControl->setFrameInfo(m_playPosition, trackEndPosition, sampleRate);
pControl->setFrameInfo(m_playPos, trackEndPosition, sampleRate);
}

if (pNewTrack) {
Expand Down Expand Up @@ -716,7 +716,7 @@ bool EngineBuffer::updateIndicatorsAndModifyPlay(bool newPlay, bool oldPlay) {
const QueuedSeek queuedSeek = m_queuedSeek.getValue();
if ((!m_pCurrentTrack && atomicLoadRelaxed(m_iTrackLoading) == 0) ||
(m_pCurrentTrack && atomicLoadRelaxed(m_iTrackLoading) == 0 &&
m_playPosition >= getTrackEndPosition() &&
m_playPos >= getTrackEndPosition() &&
queuedSeek.seekType == SEEK_NONE) ||
m_pPassthroughEnabled->toBool()) {
// play not possible
Expand Down Expand Up @@ -844,7 +844,7 @@ void EngineBuffer::processTrackLocked(
// Update the slipped position and seek to it if slip mode was disabled.
processSlip(iBufferSize);

// Note: This may affect the m_playPosition, play, scaler and crossfade buffer
// Note: This may affect the m_playPos, play, scaler and crossfade buffer
processSeek(paused);

// speed is the ratio between track-time and real-time
Expand Down Expand Up @@ -1012,13 +1012,13 @@ void EngineBuffer::processTrackLocked(
rate = m_rate_old;
}

const mixxx::audio::FramePos playpos_old = m_playPosition;
const mixxx::audio::FramePos playpos_old = m_playPos;
bool bCurBufferPaused = false;
bool atEnd = false;
bool backwards = rate < 0;
const mixxx::audio::FramePos trackEndPosition = getTrackEndPosition();
if (trackEndPosition.isValid()) {
atEnd = m_playPosition >= trackEndPosition;
atEnd = m_playPos >= trackEndPosition;
if (atEnd && !backwards) {
// do not play past end
bCurBufferPaused = true;
Expand Down Expand Up @@ -1048,15 +1048,15 @@ void EngineBuffer::processTrackLocked(

if (m_bScalerOverride) {
// If testing, we don't have a real log so we fake the position.
m_playPosition += framesRead;
m_playPos += framesRead;
} else {
// Adjust filepos_play by the amount we processed.
m_playPosition =
m_pReadAheadManager->getFilePlaypositionFromLog(m_playPosition, framesRead);
m_playPos =
m_pReadAheadManager->getFilePlaypositionFromLog(m_playPos, framesRead);
}
// Note: The last buffer of a track is padded with silence.
// This silence is played together with the last samples in the last
// callback and the m_playPosition is advanced behind the end of the track.
// callback and the m_playPos is advanced behind the end of the track.
// If repeat is enabled, scaler->scaleBuffer() wraps around at end/start
// and fills the buffer with samples from the other end of the track.

Expand Down Expand Up @@ -1084,8 +1084,8 @@ void EngineBuffer::processTrackLocked(
}

for (const auto& pControl: qAsConst(m_engineControls)) {
pControl->setFrameInfo(m_playPosition, trackEndPosition, m_trackSampleRateOld);
pControl->process(rate, m_playPosition, iBufferSize);
pControl->setFrameInfo(m_playPos, trackEndPosition, m_trackSampleRateOld);
pControl->process(rate, m_playPos, iBufferSize);
}

m_scratching_old = is_scratching;
Expand All @@ -1099,7 +1099,7 @@ void EngineBuffer::processTrackLocked(
// to set the sync'ed playposition right away and fill the wrap-around buffer
// with correct samples from the sync'ed loop in / track start position?
if (m_pRepeat->toBool() && m_pQuantize->toBool() &&
(m_playPosition > playpos_old) == backwards) {
(m_playPos > playpos_old) == backwards) {
// TODO() The resulting seek is processed in the following callback
// That is to late
requestSyncPhase();
Expand Down Expand Up @@ -1189,12 +1189,12 @@ void EngineBuffer::processSlip(int iBufferSize) {
if (enabled != m_bSlipEnabledProcessing) {
m_bSlipEnabledProcessing = enabled;
if (enabled) {
m_slipPosition = m_playPosition;
m_slipPos = m_playPos;
m_dSlipRate = m_rate_old;
} else {
// TODO(owen) assuming that looping will get canceled properly
seekExact(m_slipPosition.toNearestFrameBoundary());
m_slipPosition = mixxx::audio::kStartFramePos;
seekExact(m_slipPos.toNearestFrameBoundary());
m_slipPos = mixxx::audio::kStartFramePos;
}
}

Expand All @@ -1214,12 +1214,12 @@ void EngineBuffer::processSlip(int iBufferSize) {
// Simulate looping if a regular loop is active
if (m_pLoopingControl->isLoopingEnabled() &&
!m_pLoopingControl->isLoopRollActive()) {
const mixxx::audio::FramePos newPos = m_slipPosition + slipDelta;
m_slipPosition = m_pLoopingControl->adjustedPositionForCurrentLoop(
const mixxx::audio::FramePos newPos = m_slipPos + slipDelta;
m_slipPos = m_pLoopingControl->adjustedPositionForCurrentLoop(
newPos,
m_dSlipRate < 0);
} else {
m_slipPosition += slipDelta;
m_slipPos += slipDelta;
}
}
}
Expand Down Expand Up @@ -1274,7 +1274,7 @@ void EngineBuffer::processSeek(bool paused) {
return;
case SEEK_PHASE:
// only adjust phase
position = m_playPosition;
position = m_playPos;
break;
case SEEK_STANDARD:
if (m_pQuantize->toBool()) {
Expand Down Expand Up @@ -1309,11 +1309,11 @@ void EngineBuffer::processSeek(bool paused) {
position = m_pLoopingControl->getSyncPositionInsideLoop(position, syncPosition);
if (kLogger.traceEnabled()) {
kLogger.trace()
<< "EngineBuffer::processSeek" << getGroup() << "seek info:" << m_playPosition
<< "EngineBuffer::processSeek" << getGroup() << "seek info:" << m_playPos
<< "->" << position;
}
}
if (position != m_playPosition) {
if (position != m_playPos) {
if (kLogger.traceEnabled()) {
kLogger.trace() << "EngineBuffer::processSeek" << getGroup() << "Seek to" << position;
}
Expand Down Expand Up @@ -1370,7 +1370,7 @@ mixxx::audio::FramePos EngineBuffer::queuedSeekPosition() const {
}

void EngineBuffer::updateIndicators(double speed, int iBufferSize) {
if (!m_playPosition.isValid() ||
if (!m_playPos.isValid() ||
!m_trackSampleRateOld.isValid() ||
m_tempo_ratio_old == 0 ||
m_pPassthroughEnabled->toBool()) {
Expand All @@ -1386,8 +1386,18 @@ void EngineBuffer::updateIndicators(double speed, int iBufferSize) {
// Increase samplesCalculated by the buffer size
m_iSamplesSinceLastIndicatorUpdate += iBufferSize;

const double fFractionalPlaypos = fractionalPlayposFromAbsolute(m_playPosition);
const double fFractionalSlipPos = fractionalPlayposFromAbsolute(m_slipPosition);
const double fFractionalPlaypos = fractionalPlayposFromAbsolute(m_playPos);
const double fFractionalSlipPos = fractionalPlayposFromAbsolute(m_slipPos);
double fFractionalLoopStartPos = 0.0;
auto loopStartPos = m_pLoopingControl->getLoopStartPosition();
if (loopStartPos.isValid()) {
fFractionalLoopStartPos = fractionalPlayposFromAbsolute(loopStartPos);
}
double fFractionalLoopEndPos = 0.0;
auto loopEndPos = m_pLoopingControl->getLoopEndPosition();
if (loopEndPos.isValid()) {
fFractionalLoopEndPos = fractionalPlayposFromAbsolute(loopEndPos);
}

const double tempoTrackSeconds = m_trackEndPositionOld.value() /
m_trackSampleRateOld / getRateRatio();
Expand Down Expand Up @@ -1420,14 +1430,17 @@ void EngineBuffer::updateIndicators(double speed, int iBufferSize) {
m_trackEndPositionOld.toEngineSamplePos(),
fFractionalSlipPos,
effectiveSlipRate,
m_pLoopingControl->isLoopingEnabled(),
fFractionalLoopStartPos,
fFractionalLoopEndPos,
tempoTrackSeconds,
iBufferSize / kSamplesPerFrame / m_sampleRate.toDouble() * 1000000.0);

// TODO: Especially with long audio buffers, jitter is visible. This can be fixed by moving the
// ClockControl::updateIndicators into the waveform update loop which is synced with the display refresh rate.
// Via the visual play position it's possible to access to the sample that is currently played,
// and not the one that have been processed as in the current solution.
m_pClockControl->updateIndicators(speed * m_baserate_old, m_playPosition, m_sampleRate);
m_pClockControl->updateIndicators(speed * m_baserate_old, m_playPos, m_sampleRate);
}

void EngineBuffer::hintReader(const double dRate) {
Expand All @@ -1437,7 +1450,7 @@ void EngineBuffer::hintReader(const double dRate) {
//if slipping, hint about virtual position so we're ready for it
if (m_bSlipEnabledProcessing) {
Hint hint;
hint.frame = static_cast<SINT>(m_slipPosition.toLowerFrameBoundary().value());
hint.frame = static_cast<SINT>(m_slipPos.toLowerFrameBoundary().value());
hint.type = Hint::Type::SlipPosition;
if (m_dSlipRate >= 0) {
hint.frameCount = Hint::kFrameCountForward;
Expand Down
4 changes: 2 additions & 2 deletions src/engine/enginebuffer.h
View file
Open in desktop
Expand Up @@ -313,7 +313,7 @@ class EngineBuffer : public EngineObject {
HintVector m_hintList;

// The current sample to play in the file.
mixxx::audio::FramePos m_playPosition;
mixxx::audio::FramePos m_playPos;

// The previous callback's speed. Used to check if the scaler parameters
// need updating.
Expand Down Expand Up @@ -352,7 +352,7 @@ class EngineBuffer : public EngineObject {
int m_iSamplesSinceLastIndicatorUpdate;

// The location where the track would have been had slip not been engaged
mixxx::audio::FramePos m_slipPosition;
mixxx::audio::FramePos m_slipPos;
// Saved value of rate for slip mode
double m_dSlipRate;
// m_bSlipEnabledProcessing is only used by the engine processing thread.
Expand Down
46 changes: 36 additions & 10 deletions src/waveform/visualplayposition.cpp
View file
Open in desktop
Expand Up @@ -25,21 +25,27 @@ VisualPlayPosition::~VisualPlayPosition() {
}

void VisualPlayPosition::set(
double playPos,
double rate,
double playPosition,
double playRate,
double positionStep,
double slipPosition,
double slipRate,
bool loopEnabled,
double loopStartPosition,
double loopEndPosition,
double tempoTrackSeconds,
double audioBufferMicroS) {
VisualPlayPositionData data;
data.m_referenceTime = m_timeInfoTime;
data.m_callbackEntrytoDac = static_cast<int>(m_dCallbackEntryToDacSecs * 1000000); // s to µs
data.m_enginePlayPos = playPos;
data.m_rate = rate;
data.m_playPos = playPosition;
data.m_playRate = playRate;
data.m_slipRate = slipRate;
data.m_positionStep = positionStep;
data.m_slipPosition = slipPosition;
data.m_slipPos = slipPosition;
data.m_loopEnabled = loopEnabled;
data.m_loopStartPos = loopStartPosition;
data.m_loopEndPos = loopEndPosition;
data.m_tempoTrackSeconds = tempoTrackSeconds;
data.m_audioBufferMicroS = audioBufferMicroS;

Expand Down Expand Up @@ -77,7 +83,27 @@ double VisualPlayPosition::getAtNextVSync(VSyncThread* pVSyncThread) {
if (m_valid) {
const VisualPlayPositionData data = m_data.getValue();
const double offset = calcOffsetAtNextVSync(pVSyncThread, data);
return data.m_enginePlayPos + offset * data.m_rate;

double interpolatedPlayPos = data.m_playPos + offset * data.m_playRate;

if (data.m_loopEnabled) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this must only applied when the data.m_playPos is on one side of the loop marker and the interpolatedPlayPos is on the other side.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've commited a fix.

double loopSize = data.m_loopEndPos - data.m_loopStartPos;
if (loopSize > 0) {
if (interpolatedPlayPos < data.m_loopStartPos) {
interpolatedPlayPos = data.m_loopEndPos -
std::remainder(
data.m_loopStartPos - interpolatedPlayPos,
loopSize);
}
if (interpolatedPlayPos > data.m_loopEndPos) {
interpolatedPlayPos = data.m_loopStartPos +
std::remainder(
interpolatedPlayPos - data.m_loopEndPos,
loopSize);
}
}
}
return interpolatedPlayPos;
}
return -1;
}
Expand All @@ -88,15 +114,15 @@ void VisualPlayPosition::getPlaySlipAtNextVSync(VSyncThread* pVSyncThread,
if (m_valid) {
const VisualPlayPositionData data = m_data.getValue();
const double offset = calcOffsetAtNextVSync(pVSyncThread, data);
*pPlayPosition = data.m_enginePlayPos + offset * data.m_rate;
*pSlipPosition = data.m_slipPosition + offset * data.m_slipRate;
*pPlayPosition = data.m_playPos + offset * data.m_playRate;
*pSlipPosition = data.m_slipPos + offset * data.m_slipRate;
}
}

double VisualPlayPosition::getEnginePlayPos() {
if (m_valid) {
VisualPlayPositionData data = m_data.getValue();
return data.m_enginePlayPos;
return data.m_playPos;
} else {
return -1;
}
Expand All @@ -105,7 +131,7 @@ double VisualPlayPosition::getEnginePlayPos() {
void VisualPlayPosition::getTrackTime(double* pPlayPosition, double* pTempoTrackSeconds) {
if (m_valid) {
VisualPlayPositionData data = m_data.getValue();
*pPlayPosition = data.m_enginePlayPos;
*pPlayPosition = data.m_playPos;
*pTempoTrackSeconds = data.m_tempoTrackSeconds;
} else {
*pPlayPosition = 0;
Expand Down