From 58310eeceeb4ed5a22f0496d9c44102d95380821 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Fri, 20 Nov 2020 14:29:18 +0100 Subject: [PATCH 01/20] Only allow one previewing hotcue at the time. This is required for a consitent transition between loop cues and hot cues. --- src/engine/controls/cuecontrol.cpp | 319 ++++++++++++++--------------- src/engine/controls/cuecontrol.h | 8 +- 2 files changed, 156 insertions(+), 171 deletions(-) diff --git a/src/engine/controls/cuecontrol.cpp b/src/engine/controls/cuecontrol.cpp index 94d9da776a0..634b73cdff0 100644 --- a/src/engine/controls/cuecontrol.cpp +++ b/src/engine/controls/cuecontrol.cpp @@ -30,6 +30,8 @@ constexpr double CUE_MODE_CUP = 5.0; /// This is the position of a fresh loaded tack without any seek constexpr double kDefaultLoadPosition = 0.0; constexpr int kNoHotCueNumber = 0; +/// Used for a common tarcking of the previewing Hotcue in m_currentlyPreviewingIndex +constexpr int kMainCueIndex = NUM_HOT_CUES; // Helper function to convert control values (i.e. doubles) into RgbColor // instances (or nullopt if value < 0). This happens by using the integer @@ -80,10 +82,9 @@ CueControl::CueControl(const QString& group, : EngineControl(group, pConfig), m_pConfig(pConfig), m_colorPaletteSettings(ColorPaletteSettings(pConfig)), - m_bPreviewing(false), + m_currentlyPreviewingIndex(Cue::kNoHotCue), m_pPlay(ControlObject::getControl(ConfigKey(group, "play"))), m_pStopButton(ControlObject::getControl(ConfigKey(group, "stop"))), - m_iCurrentlyPreviewingHotcues(0), m_bypassCueSetByPlay(false), m_iNumHotCues(NUM_HOT_CUES), m_pLoadedTrack(), @@ -886,7 +887,7 @@ void CueControl::hotcueGotoAndStop(HotcueControl* pControl, double value) { if (pCue) { double position = pCue->getPosition(); if (position != Cue::kNoPosition) { - if (!m_iCurrentlyPreviewingHotcues && !m_bPreviewing) { + if (m_currentlyPreviewingIndex == Cue::kNoHotCue) { m_pPlay->set(0.0); seekExact(position); } else { @@ -912,21 +913,23 @@ void CueControl::hotcueGotoAndPlay(HotcueControl* pControl, double value) { // Need to unlock before emitting any signals to prevent deadlock. lock.unlock(); - if (pCue) { - double position = pCue->getPosition(); - if (position != Cue::kNoPosition) { - seekAbs(position); - if (!isPlayingByPlayButton()) { - // cueGoto is processed asynchronously. - // avoid a wrong cue set if seek by cueGoto is still pending - m_bPreviewing = false; - m_iCurrentlyPreviewingHotcues = 0; - // don't move the cue point to the hot cue point in DENON mode - m_bypassCueSetByPlay = true; - m_pPlay->set(1.0); - } + if (!pCue) { + return; + } + + double position = pCue->getPosition(); + if (position != Cue::kNoPosition) { + seekAbs(position); + // End previewing to not jump back if a sticking finger on a cue + // button is released (just in case) + updateCurrentlyPreviewingIndex(Cue::kNoHotCue); + if (!m_pPlay->toBool()) { + // don't move the cue point to the hot cue point in DENON mode + m_bypassCueSetByPlay = true; + m_pPlay->set(1.0); } } + setHotcueFocusIndex(pControl->getHotcueIndex()); } @@ -964,11 +967,10 @@ void CueControl::hotcueGotoAndLoop(HotcueControl* pControl, double value) { return; } - if (!isPlayingByPlayButton()) { - // cueGoto is processed asynchronously. - // avoid a wrong cue set if seek by cueGoto is still pending - m_bPreviewing = false; - m_iCurrentlyPreviewingHotcues = 0; + // End previewing to not jump back if a sticking finger on a cue + // button is released (just in case) + updateCurrentlyPreviewingIndex(Cue::kNoHotCue); + if (!m_pPlay->toBool()) { // don't move the cue point to the hot cue point in DENON mode m_bypassCueSetByPlay = true; m_pPlay->set(1.0); @@ -1039,51 +1041,40 @@ void CueControl::hotcueActivate(HotcueControl* pControl, double value, HotcueSet lock.unlock(); - if (pCue) { - if (value > 0) { - if (pCue->getPosition() == Cue::kNoPosition) { - hotcueSet(pControl, value, mode); - } else { - if (isPlayingByPlayButton()) { - switch (pCue->getType()) { - case mixxx::CueType::HotCue: - hotcueGoto(pControl, value); - break; - case mixxx::CueType::Loop: - if (m_pCurrentSavedLoopControl != pControl) { - setCurrentSavedLoopControlAndActivate(pControl); - } else { - bool loopActive = pControl->getStatus() == - HotcueControl::Status::Active; - setLoop(pCue->getPosition(), pCue->getEndPosition(), !loopActive); - } - break; - default: - DEBUG_ASSERT(!"Invalid CueType!"); + if (value > 0) { + // pressed + if (pCue && pCue->getPosition() != Cue::kNoPosition && + pCue->getType() != mixxx::CueType::Invalid) { + if (m_pPlay->toBool() && m_currentlyPreviewingIndex == Cue::kNoHotCue) { + // playing by Play button + switch (pCue->getType()) { + case mixxx::CueType::HotCue: + hotcueGoto(pControl, value); + break; + case mixxx::CueType::Loop: + if (m_pCurrentSavedLoopControl != pControl) { + setCurrentSavedLoopControlAndActivate(pControl); + } else { + bool loopActive = pControl->getStatus() == + HotcueControl::Status::Active; + setLoop(pCue->getPosition(), pCue->getEndPosition(), !loopActive); } - } else { - hotcueActivatePreview(pControl, value); + break; + default: + DEBUG_ASSERT(!"Invalid CueType!"); } - } - } else { - if (pCue->getPosition() != Cue::kNoPosition) { + } else { + // pressed during pause or preview hotcueActivatePreview(pControl, value); } - } - } else { - // The cue is non-existent ... - if (value > 0) { - // set it to the current position + } else { + // pressed a not existing cue hotcueSet(pControl, value, mode); - } else if (m_iCurrentlyPreviewingHotcues) { - // yet we got a release for it and are - // currently previewing a hotcue. This is indicative of a corner - // case where the cue was detached while we were pressing it. Let - // hotcueActivatePreview handle it. - hotcueActivatePreview(pControl, value); } + } else { + // released + hotcueActivatePreview(pControl, value); } - setHotcueFocusIndex(pControl->getHotcueIndex()); } @@ -1095,53 +1086,61 @@ void CueControl::hotcueActivatePreview(HotcueControl* pControl, double value) { CuePointer pCue(pControl->getCue()); if (value > 0) { - if (pCue && pCue->getPosition() != Cue::kNoPosition && - pCue->getType() != mixxx::CueType::Invalid && - pControl->getPreviewingType() == mixxx::CueType::Invalid) { - m_iCurrentlyPreviewingHotcues++; + if (pCue) { double position = pCue->getPosition(); - m_bypassCueSetByPlay = true; - pControl->setPreviewingType(pCue->getType()); - pControl->setPreviewingPosition(position); - if (pCue->getType() == mixxx::CueType::Loop) { - setCurrentSavedLoopControlAndActivate(pControl); - } else if (pControl->getStatus() == HotcueControl::Status::Set) { - pControl->setStatus(HotcueControl::Status::Active); - } - - // Need to unlock before emitting any signals to prevent deadlock. - lock.unlock(); - - seekAbs(position); - m_pPlay->set(1.0); - } - } else if (m_iCurrentlyPreviewingHotcues) { - // This is a activate release and we are previewing at least one - // hotcue. If this hotcue is previewing: - mixxx::CueType cueType = pControl->getPreviewingType(); - if (cueType != mixxx::CueType::Invalid) { - // If this is the last hotcue to leave preview. - if (--m_iCurrentlyPreviewingHotcues == 0 && !m_bPreviewing) { - // Mark this hotcue as not previewing. - double position = pControl->getPreviewingPosition(); - pControl->setPreviewingType(mixxx::CueType::Invalid); - pControl->setPreviewingPosition(Cue::kNoPosition); + mixxx::CueType type = pCue->getType(); + int index = pControl->getHotcueIndex(); + if (type != mixxx::CueType::Invalid && + position != Cue::kNoPosition && + m_currentlyPreviewingIndex != index) { + updateCurrentlyPreviewingIndex(index); + m_bypassCueSetByPlay = true; + pControl->setPreviewingType(type); + pControl->setPreviewingPosition(position); + if (type == mixxx::CueType::Loop) { + setCurrentSavedLoopControlAndActivate(pControl); + } else if (pControl->getStatus() == HotcueControl::Status::Set) { + pControl->setStatus(HotcueControl::Status::Active); + } - m_pPlay->set(0.0); // Need to unlock before emitting any signals to prevent deadlock. lock.unlock(); - if (cueType == mixxx::CueType::Loop) { - m_pLoopEnabled->set(0); - } else if (pControl->getStatus() == HotcueControl::Status::Active) { - pControl->setStatus(HotcueControl::Status::Set); - } - seekExact(position); + + seekAbs(position); + m_pPlay->set(1.0); } } + } else if (m_currentlyPreviewingIndex == pControl->getHotcueIndex()) { + // This is a release of a previewing hotcue + double position = pControl->getPreviewingPosition(); + + updateCurrentlyPreviewingIndex(Cue::kNoHotCue); + + // Need to unlock before emitting any signals to prevent deadlock. + lock.unlock(); + + m_pPlay->set(0.0); + seekExact(position); } setHotcueFocusIndex(pControl->getHotcueIndex()); } +void CueControl::updateCurrentlyPreviewingIndex(int hotcueIndex) { + int oldPreviewingIndex = m_currentlyPreviewingIndex.fetchAndStoreRelease(hotcueIndex); + if (oldPreviewingIndex >= 0 && oldPreviewingIndex < m_iNumHotCues) { + // We where already in previewing state, clean up .. + HotcueControl* pLastControl = m_hotcueControls.at(oldPreviewingIndex); + mixxx::CueType lastType = pLastControl->getPreviewingType(); + if (lastType == mixxx::CueType::Loop) { + m_pLoopEnabled->set(0); + } + CuePointer pLastCue(pLastControl->getCue()); + pLastControl->setStatus((pLastCue->getType() == mixxx::CueType::Invalid) + ? HotcueControl::Status::Empty + : HotcueControl::Status::Set); + } +} + void CueControl::hotcueClear(HotcueControl* pControl, double value) { if (value <= 0) { return; @@ -1273,6 +1272,8 @@ void CueControl::cueGoto(double value) { // Seek to cue point double cuePoint = m_pCuePoint->get(); + // Note: We do not mess with ply here, we continue playing or previewing. + // Need to unlock before emitting any signals to prevent deadlock. lock.unlock(); @@ -1287,11 +1288,12 @@ void CueControl::cueGotoAndPlay(double value) { cueGoto(value); QMutexLocker lock(&m_mutex); // Start playing if not already - if (!isPlayingByPlayButton()) { - // cueGoto is processed asynchronously. - // avoid a wrong cue set if seek by cueGoto is still pending - m_bPreviewing = false; - m_iCurrentlyPreviewingHotcues = 0; + + // End previewing to not jump back if a sticking finger on a cue + // button is released (just in case) + updateCurrentlyPreviewingIndex(Cue::kNoHotCue); + if (!m_pPlay->toBool()) { + // don't move the cue point to the hot cue point in DENON mode m_bypassCueSetByPlay = true; m_pPlay->set(1.0); } @@ -1302,7 +1304,7 @@ void CueControl::cueGotoAndStop(double value) { return; } - if (!m_iCurrentlyPreviewingHotcues && !m_bPreviewing) { + if (m_currentlyPreviewingIndex == Cue::kNoHotCue) { m_pPlay->set(0.0); double position = m_pCuePoint->get(); seekExact(position); @@ -1313,29 +1315,17 @@ void CueControl::cueGotoAndStop(double value) { } void CueControl::cuePreview(double value) { - //qDebug() << "CueControl::cuePreview" << value; - QMutexLocker lock(&m_mutex); - if (value > 0) { - if (!m_bPreviewing) { - m_bPreviewing = true; - m_bypassCueSetByPlay = true; + if (m_currentlyPreviewingIndex != kMainCueIndex) { + updateCurrentlyPreviewingIndex(kMainCueIndex); + seekAbs(m_pCuePoint->get()); m_pPlay->set(1.0); } - } else if (m_bPreviewing) { - m_bPreviewing = false; - if (m_iCurrentlyPreviewingHotcues) { - return; - } + } else if (m_currentlyPreviewingIndex == kMainCueIndex) { + updateCurrentlyPreviewingIndex(Cue::kNoHotCue); m_pPlay->set(0.0); - } else { - return; + seekExact(m_pCuePoint->get()); } - - // Need to unlock before emitting any signals to prevent deadlock. - lock.unlock(); - - seekAbs(m_pCuePoint->get()); } void CueControl::cueCDJ(double value) { @@ -1351,29 +1341,24 @@ void CueControl::cueCDJ(double value) { TrackAt trackAt = getTrackAt(); if (value > 0) { - if (m_bPreviewing) { + if (m_currentlyPreviewingIndex == kMainCueIndex) { // already previewing, do nothing return; - } else if (m_iCurrentlyPreviewingHotcues) { + } else if (m_currentlyPreviewingIndex != Cue::kNoHotCue) { // we are already previewing by hotcues // just jump to cue point and continue previewing - m_bPreviewing = true; + updateCurrentlyPreviewingIndex(kMainCueIndex); lock.unlock(); seekAbs(m_pCuePoint->get()); } else if (freely_playing || trackAt == TrackAt::End) { // Jump to cue when playing or when at end position - - // Just in case. - m_bPreviewing = false; m_pPlay->set(0.0); - - // Need to unlock before emitting any signals to prevent deadlock. + // Need to unlock before emitting any signals to prvent deadlock. lock.unlock(); - seekAbs(m_pCuePoint->get()); } else if (trackAt == TrackAt::Cue) { // pause at cue point - m_bPreviewing = true; + updateCurrentlyPreviewingIndex(kMainCueIndex); m_pPlay->set(1.0); } else { // Pause not at cue point and not at end position @@ -1387,17 +1372,15 @@ void CueControl::cueCDJ(double value) { seekAbs(m_pCuePoint->get()); } } - } else if (m_bPreviewing) { - m_bPreviewing = false; - if (!m_iCurrentlyPreviewingHotcues) { - m_pPlay->set(0.0); - - // Need to unlock before emitting any signals to prevent deadlock. - lock.unlock(); + } else if (m_currentlyPreviewingIndex == kMainCueIndex) { + updateCurrentlyPreviewingIndex(Cue::kNoHotCue); + m_pPlay->set(0.0); + // Need to unlock before emitting any signals to prevent deadlock. + lock.unlock(); - seekAbs(m_pCuePoint->get()); - } + seekExact(m_pCuePoint->get()); } + // indicator may flash because the delayed adoption of seekAbs // Correct the Indicator set via play if (m_pLoadedTrack && !freely_playing) { @@ -1418,34 +1401,31 @@ void CueControl::cueDenon(double value) { TrackAt trackAt = getTrackAt(); if (value > 0) { - if (m_bPreviewing) { + if (m_currentlyPreviewingIndex == kMainCueIndex) { // already previewing, do nothing return; - } else if (m_iCurrentlyPreviewingHotcues) { + } else if (m_currentlyPreviewingIndex != Cue::kNoHotCue) { // we are already previewing by hotcues // just jump to cue point and continue previewing - m_bPreviewing = true; + updateCurrentlyPreviewingIndex(kMainCueIndex); lock.unlock(); seekAbs(m_pCuePoint->get()); } else if (!playing && trackAt == TrackAt::Cue) { - // pause at cue point - m_bPreviewing = true; + // paused at cue point + updateCurrentlyPreviewingIndex(kMainCueIndex); m_pPlay->set(1.0); } else { // Need to unlock before emitting any signals to prevent deadlock. lock.unlock(); - seekAbs(m_pCuePoint->get()); + seekExact(m_pCuePoint->get()); } - } else if (m_bPreviewing) { - m_bPreviewing = false; - if (!m_iCurrentlyPreviewingHotcues) { - m_pPlay->set(0.0); - - // Need to unlock before emitting any signals to prevent deadlock. - lock.unlock(); + } else if (m_currentlyPreviewingIndex == kMainCueIndex) { + updateCurrentlyPreviewingIndex(Cue::kNoHotCue); + m_pPlay->set(0.0); + // Need to unlock before emitting any signals to prevent deadlock. + lock.unlock(); - seekAbs(m_pCuePoint->get()); - } + seekExact(m_pCuePoint->get()); } } @@ -1463,7 +1443,7 @@ void CueControl::cuePlay(double value) { // pressed if (value > 0) { if (freely_playing) { - m_bPreviewing = false; + updateCurrentlyPreviewingIndex(Cue::kNoHotCue); m_pPlay->set(0.0); // Need to unlock before emitting any signals to prevent deadlock. @@ -1474,7 +1454,7 @@ void CueControl::cuePlay(double value) { // Pause not at cue point and not at end position cueSet(value); // Just in case. - m_bPreviewing = false; + updateCurrentlyPreviewingIndex(Cue::kNoHotCue); m_pPlay->set(0.0); // If quantize is enabled, jump to the cue point since it's not // necessarily where we currently are @@ -1485,7 +1465,7 @@ void CueControl::cuePlay(double value) { } } } else if (trackAt == TrackAt::Cue) { - m_bPreviewing = false; + updateCurrentlyPreviewingIndex(Cue::kNoHotCue); m_pPlay->set(1.0); lock.unlock(); } @@ -1518,8 +1498,14 @@ void CueControl::playStutter(double v) { QMutexLocker lock(&m_mutex); //qDebug() << "playStutter" << v; if (v > 0.0) { - if (isPlayingByPlayButton()) { - cueGoto(1.0); + if (m_pPlay->toBool()) { + if (m_currentlyPreviewingIndex != Cue::kNoHotCue) { + // latch playing + updateCurrentlyPreviewingIndex(Cue::kNoHotCue); + } else { + // Stutter + cueGoto(1.0); + } } else { m_pPlay->set(1.0); } @@ -1868,11 +1854,10 @@ bool CueControl::updateIndicatorsAndModifyPlay( // when previewing, "play" was set by cue button, a following toggle request // (play = 0.0) is used for latching play. bool previewing = false; - if (m_bPreviewing || m_iCurrentlyPreviewingHotcues) { + if (m_currentlyPreviewingIndex != Cue::kNoHotCue) { if (!newPlay && oldPlay) { // play latch request: stop previewing and go into normal play mode. - m_bPreviewing = false; - m_iCurrentlyPreviewingHotcues = 0; + updateCurrentlyPreviewingIndex(Cue::kNoHotCue); newPlay = true; m_pPlayLatched->forceSet(1.0); } else { @@ -1973,7 +1958,7 @@ void CueControl::updateIndicators() { } else { // Here we have CUE_MODE_PIONEER or CUE_MODE_MIXXX // default to Pioneer mode - if (!m_bPreviewing) { + if (m_currentlyPreviewingIndex != kMainCueIndex) { const auto freely_playing = m_pPlay->toBool() && !getEngineBuffer()->getScratching(); if (!freely_playing) { @@ -2004,6 +1989,9 @@ void CueControl::updateIndicators() { // Cue indicator should be off when freely playing m_pCueIndicator->setBlinkValue(ControlIndicator::OFF); } + } else { + // Preview + m_pCueIndicator->setBlinkValue(ControlIndicator::ON); } } } @@ -2087,11 +2075,6 @@ bool CueControl::isTrackAtIntroCue() { 1.0f); } -bool CueControl::isPlayingByPlayButton() { - return m_pPlay->toBool() && !m_iCurrentlyPreviewingHotcues && - !m_bPreviewing; -} - SeekOnLoadMode CueControl::getSeekOnLoadPreference() { int configValue = getConfig()->getValue(ConfigKey("[Controls]", "CueRecall"), diff --git a/src/engine/controls/cuecontrol.h b/src/engine/controls/cuecontrol.h index e21629ab8e4..bd318fd6578 100644 --- a/src/engine/controls/cuecontrol.h +++ b/src/engine/controls/cuecontrol.h @@ -4,6 +4,7 @@ #ifndef CUECONTROL_H #define CUECONTROL_H +#include #include #include @@ -98,7 +99,8 @@ class HotcueControl : public QObject { void setColor(mixxx::RgbColor::optional_t newColor); mixxx::RgbColor::optional_t getColor() const; - // Used for caching the preview state of this hotcue control. + // Used for caching the preview state of this hotcue control + // for the case the cue is deleted during preview. mixxx::CueType getPreviewingType() const { return m_previewingType; } @@ -215,6 +217,7 @@ class CueControl : public EngineControl { void hotcueCueLoop(HotcueControl* pControl, double v); void hotcueActivate(HotcueControl* pControl, double v, HotcueSetMode mode); void hotcueActivatePreview(HotcueControl* pControl, double v); + void updateCurrentlyPreviewingIndex(int hotcueIndex); void hotcueClear(HotcueControl* pControl, double v); void hotcuePositionChanged(HotcueControl* pControl, double newPosition); void hotcueEndPositionChanged(HotcueControl* pControl, double newEndPosition); @@ -270,10 +273,9 @@ class CueControl : public EngineControl { UserSettingsPointer m_pConfig; ColorPaletteSettings m_colorPaletteSettings; - bool m_bPreviewing; + QAtomicInt m_currentlyPreviewingIndex; ControlObject* m_pPlay; ControlObject* m_pStopButton; - int m_iCurrentlyPreviewingHotcues; ControlObject* m_pQuantizeEnabled; ControlObject* m_pClosestBeat; parented_ptr m_pLoopStartPosition; From 5fd6d4174e2b39b25d78fa3fbf401f889a3a0d9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sat, 21 Nov 2020 01:20:51 +0100 Subject: [PATCH 02/20] discard cued seek action after a new track has been loaded --- src/engine/enginebuffer.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/engine/enginebuffer.cpp b/src/engine/enginebuffer.cpp index 30716c9fa7a..8dbe41b70f9 100644 --- a/src/engine/enginebuffer.cpp +++ b/src/engine/enginebuffer.cpp @@ -547,6 +547,8 @@ void EngineBuffer::slotTrackLoaded(TrackPointer pTrack, m_dSlipPosition = 0.; m_dSlipRate = 0; + m_iSeekQueued.storeRelease(SEEK_NONE); + // Reset the pitch value for the new track. m_pause.unlock(); @@ -586,6 +588,8 @@ void EngineBuffer::ejectTrack() { m_playposSlider->set(0); m_pCueControl->resetIndicators(); + m_iSeekQueued.storeRelease(SEEK_NONE); + m_pause.unlock(); // Close open file handles by unloading the current track From 10f9e9d2687fd4fca3c759a0228c8876d65317e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sat, 21 Nov 2020 01:25:03 +0100 Subject: [PATCH 03/20] Remove redundant handling of the Main cue --- src/engine/controls/cuecontrol.cpp | 80 ++++++++++++------------------ src/track/track.cpp | 3 +- 2 files changed, 34 insertions(+), 49 deletions(-) diff --git a/src/engine/controls/cuecontrol.cpp b/src/engine/controls/cuecontrol.cpp index 634b73cdff0..93aa8a733ae 100644 --- a/src/engine/controls/cuecontrol.cpp +++ b/src/engine/controls/cuecontrol.cpp @@ -87,7 +87,6 @@ CueControl::CueControl(const QString& group, m_pStopButton(ControlObject::getControl(ConfigKey(group, "stop"))), m_bypassCueSetByPlay(false), m_iNumHotCues(NUM_HOT_CUES), - m_pLoadedTrack(), m_pCurrentSavedLoopControl(nullptr), m_mutex(QMutex::Recursive) { // To silence a compiler warning about CUE_MODE_PIONEER. @@ -457,40 +456,11 @@ void CueControl::trackLoaded(TrackPointer pNewTrack) { &CueControl::trackCuesUpdated, Qt::DirectConnection); - CuePointer pMainCue; - const QList cuePoints = m_pLoadedTrack->getCuePoints(); - for (const CuePointer& pCue : cuePoints) { - if (pCue->getType() == mixxx::CueType::MainCue) { - DEBUG_ASSERT(!pMainCue); - pMainCue = pCue; - } - } - // Need to unlock before emitting any signals to prevent deadlock. lock.unlock(); // Use pNewTrack from now, because m_pLoadedTrack might have been reset // immediately after leaving the locking scope! - // Because of legacy, we store the (load) cue point twice and need to - // sync both values. - // The mixxx::CueType::MainCue from getCuePoints() has the priority - CuePosition mainCuePoint; - if (pMainCue) { - mainCuePoint.setPosition(pMainCue->getPosition()); - // adjust the track cue accordingly - pNewTrack->setCuePoint(mainCuePoint); - } else { - // If no load cue point is stored, read from track - // Note: This is 0:00 for new tracks - mainCuePoint = pNewTrack->getCuePoint(); - // Than add the load cue to the list of cue - CuePointer pCue(pNewTrack->createAndAddCue()); - pCue->setStartPosition(mainCuePoint.getPosition()); - pCue->setHotCue(Cue::kNoHotCue); - pCue->setType(mixxx::CueType::MainCue); - } - m_pCuePoint->set(mainCuePoint.getPosition()); - // Update COs with cues from track. loadCuesFromTrack(); @@ -560,19 +530,21 @@ void CueControl::cueUpdated() { void CueControl::loadCuesFromTrack() { QMutexLocker lock(&m_mutex); - QSet active_hotcues; - CuePointer pLoadCue, pIntroCue, pOutroCue; - if (!m_pLoadedTrack) { return; } + QSet active_hotcues; + CuePointer pMainCue; + CuePointer pIntroCue; + CuePointer pOutroCue; + const QList cues = m_pLoadedTrack->getCuePoints(); for (const auto& pCue : cues) { switch (pCue->getType()) { case mixxx::CueType::MainCue: - DEBUG_ASSERT(!pLoadCue); // There should be only one MainCue cue - pLoadCue = pCue; + DEBUG_ASSERT(!pMainCue); // There should be only one MainCue cue + pMainCue = pCue; break; case mixxx::CueType::Intro: DEBUG_ASSERT(!pIntroCue); // There should be only one Intro cue @@ -618,6 +590,14 @@ void CueControl::loadCuesFromTrack() { } } + // Detach all hotcues that are no longer present + for (int hotCue = 0; hotCue < m_iNumHotCues; ++hotCue) { + if (!active_hotcues.contains(hotCue)) { + HotcueControl* pControl = m_hotcueControls.at(hotCue); + detachCue(pControl); + } + } + if (pIntroCue) { double startPosition = pIntroCue->getPosition(); double endPosition = pIntroCue->getEndPosition(); @@ -652,20 +632,26 @@ void CueControl::loadCuesFromTrack() { m_pOutroEndEnabled->forceSet(0.0); } - if (pLoadCue) { - double position = pLoadCue->getPosition(); - m_pCuePoint->set(quantizeCuePoint(position)); + // Because of legacy, we store the main cue point twice and need to + // sync both values. + // The mixxx::CueType::MainCue from getCuePoints() has the priority + CuePosition mainCuePoint; + if (pMainCue) { + double position = pMainCue->getPosition(); + mainCuePoint.setPosition(position); + // adjust the track cue accordingly + m_pLoadedTrack->setCuePoint(mainCuePoint); } else { - m_pCuePoint->set(Cue::kNoPosition); - } - - // Detach all hotcues that are no longer present - for (int hotCue = 0; hotCue < m_iNumHotCues; ++hotCue) { - if (!active_hotcues.contains(hotCue)) { - HotcueControl* pControl = m_hotcueControls.at(hotCue); - detachCue(pControl); - } + // If no load cue point is stored, read from track + // Note: This is 0:00 for new tracks + mainCuePoint = m_pLoadedTrack->getCuePoint(); + // Than add the load cue to the list of cue + CuePointer pCue(m_pLoadedTrack->createAndAddCue()); + pCue->setType(mixxx::CueType::MainCue); + pCue->setStartPosition(mainCuePoint.getPosition()); + pCue->setHotCue(Cue::kNoHotCue); } + m_pCuePoint->set(quantizeCuePoint(mainCuePoint.getPosition())); } void CueControl::trackAnalyzed() { diff --git a/src/track/track.cpp b/src/track/track.cpp index e269d90dae9..c9d9ae7b2cb 100644 --- a/src/track/track.cpp +++ b/src/track/track.cpp @@ -825,8 +825,7 @@ CuePointer Track::createAndAddCue() { this, &Track::slotCueUpdated); m_cuePoints.push_back(pCue); - markDirtyAndUnlock(&lock); - emit cuesUpdated(); + // don't emit cuesUpdated() here, because the cue is still invalid. return pCue; } From 9f1b93c629a7e2b6e20802d0f1213997405a3cf1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sat, 21 Nov 2020 14:27:28 +0100 Subject: [PATCH 04/20] Improve locking --- src/engine/controls/cuecontrol.cpp | 266 ++++++++++------------------- src/engine/controls/cuecontrol.h | 16 +- src/track/track.cpp | 2 +- 3 files changed, 98 insertions(+), 186 deletions(-) diff --git a/src/engine/controls/cuecontrol.cpp b/src/engine/controls/cuecontrol.cpp index 93aa8a733ae..4360de6b313 100644 --- a/src/engine/controls/cuecontrol.cpp +++ b/src/engine/controls/cuecontrol.cpp @@ -88,7 +88,7 @@ CueControl::CueControl(const QString& group, m_bypassCueSetByPlay(false), m_iNumHotCues(NUM_HOT_CUES), m_pCurrentSavedLoopControl(nullptr), - m_mutex(QMutex::Recursive) { + m_trackMutex(QMutex::Recursive) { // To silence a compiler warning about CUE_MODE_PIONEER. Q_UNUSED(CUE_MODE_PIONEER); createControls(); @@ -403,7 +403,7 @@ void CueControl::detachCue(HotcueControl* pControl) { return; } - CuePointer pCue(pControl->getCue()); + CuePointer pCue = pControl->getCue(); if (!pCue) { return; } @@ -416,10 +416,16 @@ void CueControl::detachCue(HotcueControl* pControl) { pControl->resetCue(); } +// This is called from the EngineWokerThread and ends with the initial seek v +// via seekOnLoad(). There is the theoretical and pending issue of a delayed control +// command intended for the old track that might be performed instead. void CueControl::trackLoaded(TrackPointer pNewTrack) { - QMutexLocker lock(&m_mutex); + QMutexLocker lock(&m_trackMutex); if (m_pLoadedTrack) { disconnect(m_pLoadedTrack.get(), 0, this, 0); + + updateCurrentlyPreviewingIndex(Cue::kNoHotCue); + for (const auto& pControl : qAsConst(m_hotcueControls)) { detachCue(pControl); } @@ -455,9 +461,8 @@ void CueControl::trackLoaded(TrackPointer pNewTrack) { this, &CueControl::trackCuesUpdated, Qt::DirectConnection); - - // Need to unlock before emitting any signals to prevent deadlock. lock.unlock(); + // Use pNewTrack from now, because m_pLoadedTrack might have been reset // immediately after leaving the locking scope! @@ -529,7 +534,7 @@ void CueControl::cueUpdated() { } void CueControl::loadCuesFromTrack() { - QMutexLocker lock(&m_mutex); + QMutexLocker lock(&m_trackMutex); if (!m_pLoadedTrack) { return; } @@ -646,7 +651,7 @@ void CueControl::loadCuesFromTrack() { // Note: This is 0:00 for new tracks mainCuePoint = m_pLoadedTrack->getCuePoint(); // Than add the load cue to the list of cue - CuePointer pCue(m_pLoadedTrack->createAndAddCue()); + CuePointer pCue = m_pLoadedTrack->createAndAddCue(); pCue->setType(mixxx::CueType::MainCue); pCue->setStartPosition(mainCuePoint.getPosition()); pCue->setHotCue(Cue::kNoHotCue); @@ -655,10 +660,6 @@ void CueControl::loadCuesFromTrack() { } void CueControl::trackAnalyzed() { - if (!m_pLoadedTrack) { - return; - } - SampleOfTrack sampleOfTrack = getSampleOfTrack(); if (sampleOfTrack.current != m_usedSeekOnLoadPosition.getValue()) { // the track is already manual cued, don't re-cue @@ -723,7 +724,7 @@ void CueControl::hotcueSet(HotcueControl* pControl, double value, HotcueSetMode return; } - QMutexLocker lock(&m_mutex); + QMutexLocker lock(&m_trackMutex); if (!m_pLoadedTrack) { return; } @@ -734,7 +735,7 @@ void CueControl::hotcueSet(HotcueControl* pControl, double value, HotcueSetMode // https://bugs.launchpad.net/mixxx/+bug/1653276 hotcueClear(pControl, value); - CuePointer pCue(m_pLoadedTrack->createAndAddCue()); + CuePointer pCue = m_pLoadedTrack->createAndAddCue(); double cueStartPosition = Cue::kNoPosition; double cueEndPosition = Cue::kNoPosition; @@ -836,22 +837,9 @@ void CueControl::hotcueGoto(HotcueControl* pControl, double value) { if (value <= 0) { return; } - - QMutexLocker lock(&m_mutex); - if (!m_pLoadedTrack) { - return; - } - - CuePointer pCue(pControl->getCue()); - - // Need to unlock before emitting any signals to prevent deadlock. - lock.unlock(); - - if (pCue) { - double position = pCue->getPosition(); - if (position != Cue::kNoPosition) { - seekAbs(position); - } + double position = pControl->getPosition(); + if (position != Cue::kNoPosition) { + seekAbs(position); } } @@ -860,26 +848,14 @@ void CueControl::hotcueGotoAndStop(HotcueControl* pControl, double value) { return; } - QMutexLocker lock(&m_mutex); - if (!m_pLoadedTrack) { - return; - } - - CuePointer pCue(pControl->getCue()); - - // Need to unlock before emitting any signals to prevent deadlock. - lock.unlock(); - - if (pCue) { - double position = pCue->getPosition(); - if (position != Cue::kNoPosition) { - if (m_currentlyPreviewingIndex == Cue::kNoHotCue) { - m_pPlay->set(0.0); - seekExact(position); - } else { - // this becomes a play latch command if we are previewing - m_pPlay->set(0.0); - } + double position = pControl->getPosition(); + if (position != Cue::kNoPosition) { + if (m_currentlyPreviewingIndex == Cue::kNoHotCue) { + m_pPlay->set(0.0); + seekExact(position); + } else { + // this becomes a play latch command if we are previewing + m_pPlay->set(0.0); } } } @@ -888,22 +864,7 @@ void CueControl::hotcueGotoAndPlay(HotcueControl* pControl, double value) { if (value <= 0) { return; } - - QMutexLocker lock(&m_mutex); - if (!m_pLoadedTrack) { - return; - } - - CuePointer pCue(pControl->getCue()); - - // Need to unlock before emitting any signals to prevent deadlock. - lock.unlock(); - - if (!pCue) { - return; - } - - double position = pCue->getPosition(); + double position = pControl->getPosition(); if (position != Cue::kNoPosition) { seekAbs(position); // End previewing to not jump back if a sticking finger on a cue @@ -923,17 +884,7 @@ void CueControl::hotcueGotoAndLoop(HotcueControl* pControl, double value) { if (value == 0) { return; } - - QMutexLocker lock(&m_mutex); - if (!m_pLoadedTrack) { - return; - } - - CuePointer pCue(pControl->getCue()); - - // Need to unlock before emitting any signals to prevent deadlock. - lock.unlock(); - + CuePointer pCue = pControl->getCue(); if (!pCue) { return; } @@ -970,10 +921,6 @@ void CueControl::hotcueCueLoop(HotcueControl* pControl, double value) { return; } - if (!m_pLoadedTrack) { - return; - } - CuePointer pCue = pControl->getCue(); if (!pCue || pCue->getPosition() == Cue::kNoPosition) { @@ -1017,16 +964,7 @@ void CueControl::hotcueCueLoop(HotcueControl* pControl, double value) { void CueControl::hotcueActivate(HotcueControl* pControl, double value, HotcueSetMode mode) { //qDebug() << "CueControl::hotcueActivate" << value; - QMutexLocker lock(&m_mutex); - - if (!m_pLoadedTrack) { - return; - } - - CuePointer pCue(pControl->getCue()); - - lock.unlock(); - + CuePointer pCue = pControl->getCue(); if (value > 0) { // pressed if (pCue && pCue->getPosition() != Cue::kNoPosition && @@ -1061,16 +999,12 @@ void CueControl::hotcueActivate(HotcueControl* pControl, double value, HotcueSet // released hotcueActivatePreview(pControl, value); } + setHotcueFocusIndex(pControl->getHotcueIndex()); } void CueControl::hotcueActivatePreview(HotcueControl* pControl, double value) { - QMutexLocker lock(&m_mutex); - if (!m_pLoadedTrack) { - return; - } - CuePointer pCue(pControl->getCue()); - + CuePointer pCue = pControl->getCue(); if (value > 0) { if (pCue) { double position = pCue->getPosition(); @@ -1088,10 +1022,6 @@ void CueControl::hotcueActivatePreview(HotcueControl* pControl, double value) { } else if (pControl->getStatus() == HotcueControl::Status::Set) { pControl->setStatus(HotcueControl::Status::Active); } - - // Need to unlock before emitting any signals to prevent deadlock. - lock.unlock(); - seekAbs(position); m_pPlay->set(1.0); } @@ -1099,15 +1029,11 @@ void CueControl::hotcueActivatePreview(HotcueControl* pControl, double value) { } else if (m_currentlyPreviewingIndex == pControl->getHotcueIndex()) { // This is a release of a previewing hotcue double position = pControl->getPreviewingPosition(); - updateCurrentlyPreviewingIndex(Cue::kNoHotCue); - - // Need to unlock before emitting any signals to prevent deadlock. - lock.unlock(); - m_pPlay->set(0.0); seekExact(position); } + setHotcueFocusIndex(pControl->getHotcueIndex()); } @@ -1132,12 +1058,12 @@ void CueControl::hotcueClear(HotcueControl* pControl, double value) { return; } - QMutexLocker lock(&m_mutex); + QMutexLocker lock(&m_trackMutex); if (!m_pLoadedTrack) { return; } - CuePointer pCue(pControl->getCue()); + CuePointer pCue = pControl->getCue(); if (!pCue) { return; } @@ -1148,12 +1074,12 @@ void CueControl::hotcueClear(HotcueControl* pControl, double value) { void CueControl::hotcuePositionChanged( HotcueControl* pControl, double newPosition) { - QMutexLocker lock(&m_mutex); + QMutexLocker lock(&m_trackMutex); if (!m_pLoadedTrack) { return; } - CuePointer pCue(pControl->getCue()); + CuePointer pCue = pControl->getCue(); if (pCue) { // Setting the position to Cue::kNoPosition is the same as calling hotcue_x_clear if (newPosition == Cue::kNoPosition) { @@ -1169,12 +1095,7 @@ void CueControl::hotcuePositionChanged( void CueControl::hotcueEndPositionChanged( HotcueControl* pControl, double newEndPosition) { - QMutexLocker lock(&m_mutex); - if (!m_pLoadedTrack) { - return; - } - - CuePointer pCue(pControl->getCue()); + CuePointer pCue = pControl->getCue(); if (pCue) { // Setting the end position of a loop cue to Cue::kNoPosition converts // it into a regular jump cue @@ -1221,14 +1142,14 @@ void CueControl::cueSet(double value) { return; } - QMutexLocker lock(&m_mutex); - + QMutexLocker lock(&m_trackMutex); double cue = getQuantizedCurrentPosition(); - m_pCuePoint->set(cue); TrackPointer pLoadedTrack = m_pLoadedTrack; lock.unlock(); // Store cue point in loaded track + // The m_pCuePoint CO is set via loadCuesFromTrack() + // this can be done outside the locking scope if (pLoadedTrack) { pLoadedTrack->setCuePoint(CuePosition(cue)); } @@ -1239,11 +1160,9 @@ void CueControl::cueClear(double value) { return; } - QMutexLocker lock(&m_mutex); - m_pCuePoint->set(Cue::kNoPosition); + // the m_pCuePoint CO is set via loadCuesFromTrack() + // no locking required TrackPointer pLoadedTrack = m_pLoadedTrack; - lock.unlock(); - if (pLoadedTrack) { pLoadedTrack->setCuePoint(CuePosition()); } @@ -1254,7 +1173,7 @@ void CueControl::cueGoto(double value) { return; } - QMutexLocker lock(&m_mutex); + QMutexLocker lock(&m_trackMutex); // Seek to cue point double cuePoint = m_pCuePoint->get(); @@ -1272,7 +1191,7 @@ void CueControl::cueGotoAndPlay(double value) { } cueGoto(value); - QMutexLocker lock(&m_mutex); + QMutexLocker lock(&m_trackMutex); // Start playing if not already // End previewing to not jump back if a sticking finger on a cue @@ -1321,7 +1240,6 @@ void CueControl::cueCDJ(double value) { // If pressed while stopped and at cue, play while pressed. // If play is pressed while holding cue, the deck is now playing. (Handled in playFromCuePreview().) - QMutexLocker lock(&m_mutex); const auto freely_playing = m_pPlay->toBool() && !getEngineBuffer()->getScratching(); TrackAt trackAt = getTrackAt(); @@ -1334,13 +1252,11 @@ void CueControl::cueCDJ(double value) { // we are already previewing by hotcues // just jump to cue point and continue previewing updateCurrentlyPreviewingIndex(kMainCueIndex); - lock.unlock(); seekAbs(m_pCuePoint->get()); } else if (freely_playing || trackAt == TrackAt::End) { // Jump to cue when playing or when at end position m_pPlay->set(0.0); // Need to unlock before emitting any signals to prvent deadlock. - lock.unlock(); seekAbs(m_pCuePoint->get()); } else if (trackAt == TrackAt::Cue) { // pause at cue point @@ -1353,7 +1269,6 @@ void CueControl::cueCDJ(double value) { // If quantize is enabled, jump to the cue point since it's not // necessarily where we currently are if (m_pQuantizeEnabled->toBool()) { - lock.unlock(); // prevent deadlock. // Enginebuffer will quantize more exactly than we can. seekAbs(m_pCuePoint->get()); } @@ -1362,8 +1277,6 @@ void CueControl::cueCDJ(double value) { updateCurrentlyPreviewingIndex(Cue::kNoHotCue); m_pPlay->set(0.0); // Need to unlock before emitting any signals to prevent deadlock. - lock.unlock(); - seekExact(m_pCuePoint->get()); } @@ -1382,7 +1295,6 @@ void CueControl::cueDenon(double value) { // If pressed while stopped and at cue, play while pressed. // Cue Point is moved by play from pause - QMutexLocker lock(&m_mutex); bool playing = (m_pPlay->toBool()); TrackAt trackAt = getTrackAt(); @@ -1394,23 +1306,17 @@ void CueControl::cueDenon(double value) { // we are already previewing by hotcues // just jump to cue point and continue previewing updateCurrentlyPreviewingIndex(kMainCueIndex); - lock.unlock(); seekAbs(m_pCuePoint->get()); } else if (!playing && trackAt == TrackAt::Cue) { // paused at cue point updateCurrentlyPreviewingIndex(kMainCueIndex); m_pPlay->set(1.0); } else { - // Need to unlock before emitting any signals to prevent deadlock. - lock.unlock(); seekExact(m_pCuePoint->get()); } } else if (m_currentlyPreviewingIndex == kMainCueIndex) { updateCurrentlyPreviewingIndex(Cue::kNoHotCue); m_pPlay->set(0.0); - // Need to unlock before emitting any signals to prevent deadlock. - lock.unlock(); - seekExact(m_pCuePoint->get()); } } @@ -1421,7 +1327,6 @@ void CueControl::cuePlay(double value) { // If not freely playing (i.e. stopped or platter IS being touched), press to go to cue and stop. // On release, start playing from cue point. - QMutexLocker lock(&m_mutex); const auto freely_playing = m_pPlay->toBool() && !getEngineBuffer()->getScratching(); TrackAt trackAt = getTrackAt(); @@ -1431,10 +1336,6 @@ void CueControl::cuePlay(double value) { if (freely_playing) { updateCurrentlyPreviewingIndex(Cue::kNoHotCue); m_pPlay->set(0.0); - - // Need to unlock before emitting any signals to prevent deadlock. - lock.unlock(); - seekAbs(m_pCuePoint->get()); } else if (trackAt == TrackAt::ElseWhere) { // Pause not at cue point and not at end position @@ -1445,7 +1346,6 @@ void CueControl::cuePlay(double value) { // If quantize is enabled, jump to the cue point since it's not // necessarily where we currently are if (m_pQuantizeEnabled->toBool()) { - lock.unlock(); // prevent deadlock. // Enginebuffer will quantize more exactly than we can. seekAbs(m_pCuePoint->get()); } @@ -1453,7 +1353,6 @@ void CueControl::cuePlay(double value) { } else if (trackAt == TrackAt::Cue) { updateCurrentlyPreviewingIndex(Cue::kNoHotCue); m_pPlay->set(1.0); - lock.unlock(); } } @@ -1473,7 +1372,7 @@ void CueControl::cueDefault(double v) { } void CueControl::pause(double v) { - QMutexLocker lock(&m_mutex); + QMutexLocker lock(&m_trackMutex); //qDebug() << "CueControl::pause()" << v; if (v > 0.0) { m_pPlay->set(0.0); @@ -1481,7 +1380,7 @@ void CueControl::pause(double v) { } void CueControl::playStutter(double v) { - QMutexLocker lock(&m_mutex); + QMutexLocker lock(&m_trackMutex); //qDebug() << "playStutter" << v; if (v > 0.0) { if (m_pPlay->toBool()) { @@ -1503,7 +1402,7 @@ void CueControl::introStartSet(double value) { return; } - QMutexLocker lock(&m_mutex); + QMutexLocker lock(&m_trackMutex); double position = getQuantizedCurrentPosition(); @@ -1531,6 +1430,9 @@ void CueControl::introStartSet(double value) { TrackPointer pLoadedTrack = m_pLoadedTrack; lock.unlock(); + // Update Track's cue. + // CO's are update loadCuesFromTrack() + // this can be done outside the locking scope if (pLoadedTrack) { CuePointer pCue = pLoadedTrack->findCueByType(mixxx::CueType::Intro); if (!pCue) { @@ -1547,11 +1449,14 @@ void CueControl::introStartClear(double value) { return; } - QMutexLocker lock(&m_mutex); + QMutexLocker lock(&m_trackMutex); double introEnd = m_pIntroEndPosition->get(); TrackPointer pLoadedTrack = m_pLoadedTrack; lock.unlock(); + // Update Track's cue. + // CO's are update loadCuesFromTrack() + // this can be done outside the locking scope if (pLoadedTrack) { CuePointer pCue = pLoadedTrack->findCueByType(mixxx::CueType::Intro); if (introEnd != Cue::kNoPosition) { @@ -1568,10 +1473,7 @@ void CueControl::introStartActivate(double value) { return; } - QMutexLocker lock(&m_mutex); double introStart = m_pIntroStartPosition->get(); - lock.unlock(); - if (introStart == Cue::kNoPosition) { introStartSet(1.0); } else { @@ -1584,7 +1486,7 @@ void CueControl::introEndSet(double value) { return; } - QMutexLocker lock(&m_mutex); + QMutexLocker lock(&m_trackMutex); double position = getQuantizedCurrentPosition(); @@ -1612,6 +1514,9 @@ void CueControl::introEndSet(double value) { TrackPointer pLoadedTrack = m_pLoadedTrack; lock.unlock(); + // Update Track's cue. + // CO's are update loadCuesFromTrack() + // this can be done outside the locking scope if (pLoadedTrack) { CuePointer pCue = pLoadedTrack->findCueByType(mixxx::CueType::Intro); if (!pCue) { @@ -1628,11 +1533,14 @@ void CueControl::introEndClear(double value) { return; } - QMutexLocker lock(&m_mutex); + QMutexLocker lock(&m_trackMutex); double introStart = m_pIntroStartPosition->get(); TrackPointer pLoadedTrack = m_pLoadedTrack; lock.unlock(); + // Update Track's cue. + // CO's are update loadCuesFromTrack() + // this can be done outside the locking scope if (pLoadedTrack) { CuePointer pCue = pLoadedTrack->findCueByType(mixxx::CueType::Intro); if (introStart != Cue::kNoPosition) { @@ -1649,7 +1557,7 @@ void CueControl::introEndActivate(double value) { return; } - QMutexLocker lock(&m_mutex); + QMutexLocker lock(&m_trackMutex); double introEnd = m_pIntroEndPosition->get(); lock.unlock(); @@ -1665,7 +1573,7 @@ void CueControl::outroStartSet(double value) { return; } - QMutexLocker lock(&m_mutex); + QMutexLocker lock(&m_trackMutex); double position = getQuantizedCurrentPosition(); @@ -1693,6 +1601,9 @@ void CueControl::outroStartSet(double value) { TrackPointer pLoadedTrack = m_pLoadedTrack; lock.unlock(); + // Update Track's cue. + // CO's are update loadCuesFromTrack() + // this can be done outside the locking scope if (pLoadedTrack) { CuePointer pCue = pLoadedTrack->findCueByType(mixxx::CueType::Outro); if (!pCue) { @@ -1709,11 +1620,14 @@ void CueControl::outroStartClear(double value) { return; } - QMutexLocker lock(&m_mutex); + QMutexLocker lock(&m_trackMutex); double outroEnd = m_pOutroEndPosition->get(); TrackPointer pLoadedTrack = m_pLoadedTrack; lock.unlock(); + // Update Track's cue. + // CO's are update loadCuesFromTrack() + // this can be done outside the locking scope if (pLoadedTrack) { CuePointer pCue = pLoadedTrack->findCueByType(mixxx::CueType::Outro); if (outroEnd != Cue::kNoPosition) { @@ -1730,7 +1644,7 @@ void CueControl::outroStartActivate(double value) { return; } - QMutexLocker lock(&m_mutex); + QMutexLocker lock(&m_trackMutex); double outroStart = m_pOutroStartPosition->get(); lock.unlock(); @@ -1746,7 +1660,7 @@ void CueControl::outroEndSet(double value) { return; } - QMutexLocker lock(&m_mutex); + QMutexLocker lock(&m_trackMutex); double position = getQuantizedCurrentPosition(); @@ -1774,6 +1688,9 @@ void CueControl::outroEndSet(double value) { TrackPointer pLoadedTrack = m_pLoadedTrack; lock.unlock(); + // Update Track's cue. + // CO's are update loadCuesFromTrack() + // this can be done outside the locking scope if (pLoadedTrack) { CuePointer pCue = pLoadedTrack->findCueByType(mixxx::CueType::Outro); if (!pCue) { @@ -1790,11 +1707,14 @@ void CueControl::outroEndClear(double value) { return; } - QMutexLocker lock(&m_mutex); + QMutexLocker lock(&m_trackMutex); double outroStart = m_pOutroStartPosition->get(); TrackPointer pLoadedTrack = m_pLoadedTrack; lock.unlock(); + // Update Track's cue. + // CO's are update loadCuesFromTrack() + // this can be done outside the locking scope if (pLoadedTrack) { CuePointer pCue = pLoadedTrack->findCueByType(mixxx::CueType::Outro); if (outroStart != Cue::kNoPosition) { @@ -1811,7 +1731,7 @@ void CueControl::outroEndActivate(double value) { return; } - QMutexLocker lock(&m_mutex); + QMutexLocker lock(&m_trackMutex); double outroEnd = m_pOutroEndPosition->get(); lock.unlock(); @@ -2137,22 +2057,24 @@ void CueControl::setCurrentSavedLoopControlAndActivate(HotcueControl* pControl) if (!pControl) { return; } - - if (!m_pLoadedTrack) { + CuePointer pCue = pControl->getCue(); + VERIFY_OR_DEBUG_ASSERT(pCue) { return; } - CuePointer pCue(pControl->getCue()); + mixxx::CueType type = pCue->getType(); + double startPos = pCue->getPosition(); + double endPos = pCue->getEndPosition(); - VERIFY_OR_DEBUG_ASSERT(pCue && - pCue->getType() == mixxx::CueType::Loop && - pCue->getEndPosition() != Cue::kNoPosition) { + VERIFY_OR_DEBUG_ASSERT( + type == mixxx::CueType::Loop && + endPos != Cue::kNoPosition) { return; } // Set new control as active m_pCurrentSavedLoopControl = pControl; - setLoop(pCue->getPosition(), pCue->getEndPosition(), true); + setLoop(startPos, endPos, true); pControl->setStatus(HotcueControl::Status::Active); } @@ -2187,16 +2109,15 @@ void CueControl::slotLoopUpdated(double startPosition, double endPosition) { return; } - if (!m_pLoadedTrack) { - return; - } - if (m_pCurrentSavedLoopControl->getStatus() != HotcueControl::Status::Active) { slotLoopReset(); return; } - CuePointer pCue(m_pCurrentSavedLoopControl->getCue()); + CuePointer pCue = m_pCurrentSavedLoopControl->getCue(); + if (!pCue) { + return; + } VERIFY_OR_DEBUG_ASSERT(pCue->getType() == mixxx::CueType::Loop) { setCurrentSavedLoopControlAndActivate(nullptr); @@ -2470,6 +2391,7 @@ double HotcueControl::getEndPosition() const { } void HotcueControl::setCue(const CuePointer& pCue) { + DEBUG_ASSERT(!m_pCue); setPosition(pCue->getPosition()); setEndPosition(pCue->getEndPosition()); setColor(pCue->getColor()); diff --git a/src/engine/controls/cuecontrol.h b/src/engine/controls/cuecontrol.h index bd318fd6578..6360012a646 100644 --- a/src/engine/controls/cuecontrol.h +++ b/src/engine/controls/cuecontrol.h @@ -337,24 +337,14 @@ class CueControl : public EngineControl { ControlObject* m_pHotcueFocusColorNext; ControlObject* m_pHotcueFocusColorPrev; - TrackPointer m_pLoadedTrack; // is written from an engine worker thread HotcueControl* m_pCurrentSavedLoopControl; // Tells us which controls map to which hotcue QMap m_controlMap; - // TODO(daschuer): It looks like the whole m_mutex is broken. Originally it - // ensured that the main cue really belongs to the loaded track. Now that - // we have hot cues that are altered outsite this guard this guarantee has - // become void. - // - // We have multiple cases where it locks m_pLoadedTrack and - // pControl->getCue(). This guards the hotcueClear() that could detach the - // cue call, but doesn't protect from cue changes via loadCuesFromTrack() - // which is called outside the mutex lock. - // - // We need to repair this. - QMutex m_mutex; + TrackPointer m_pLoadedTrack; // is written from an engine worker thread + // Must be locked when using the m_pLoadedTrack and it's properties + QMutex m_trackMutex; friend class HotcueControlTest; }; diff --git a/src/track/track.cpp b/src/track/track.cpp index c9d9ae7b2cb..7b896f63536 100644 --- a/src/track/track.cpp +++ b/src/track/track.cpp @@ -756,7 +756,7 @@ void Track::setCuePoint(CuePosition cue) { return; } - // Store the cue point in a load cue + // Store the cue point as main cue CuePointer pLoadCue = findCueByType(mixxx::CueType::MainCue); double position = cue.getPosition(); if (position != -1.0) { From 8b0614dea84726b5d1e05b95bf92fc496d47d83c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sun, 29 Nov 2020 19:13:16 +0100 Subject: [PATCH 05/20] Add function overload to createAndAddCue() to initalize the cue as well --- src/track/cue.cpp | 15 +++++++++++++++ src/track/cue.h | 11 +++++++++++ src/track/track.cpp | 25 +++++++++++++++++++++++++ src/track/track.h | 5 +++++ 4 files changed, 56 insertions(+) diff --git a/src/track/cue.cpp b/src/track/cue.cpp index cb3cd17e9fc..1e1fc0784cc 100644 --- a/src/track/cue.cpp +++ b/src/track/cue.cpp @@ -104,6 +104,21 @@ Cue::Cue( DEBUG_ASSERT(!m_dbId.isValid()); } +/// Initialize new cue points +Cue::Cue( + mixxx::CueType type, + int hotCueIndex, + double sampleStartPosition, + double sampleEndPosition) + : m_bDirty(false), // not yet in database + m_type(type), + m_sampleStartPosition(sampleStartPosition), + m_sampleEndPosition(sampleEndPosition), + m_iHotCue(hotCueIndex), + m_color(mixxx::PredefinedColorPalettes::kDefaultCueColor) { + DEBUG_ASSERT(!m_dbId.isValid()); +} + mixxx::CueInfo Cue::getCueInfo( mixxx::audio::SampleRate sampleRate) const { QMutexLocker lock(&m_mutex); diff --git a/src/track/cue.h b/src/track/cue.h index 69cbc04a77b..5d8dbfbfde3 100644 --- a/src/track/cue.h +++ b/src/track/cue.h @@ -26,11 +26,14 @@ class Cue : public QObject { /// Hot cues are sequentially indexed starting with kFirstHotCue (inclusive) static constexpr int kFirstHotCue = 0; + /// Creates an invalid Cue point Cue(); + /// For roundtrips during tests Cue( const mixxx::CueInfo& cueInfo, mixxx::audio::SampleRate sampleRate, bool setDirty); + /// Load entity from database. Cue( DbId id, @@ -40,6 +43,14 @@ class Cue : public QObject { int hotCue, const QString& label, mixxx::RgbColor color); + + /// Initialize new cue points + Cue( + mixxx::CueType type, + int hotCueIndex, + double sampleStartPosition, + double sampleEndPosition); + ~Cue() override = default; bool isDirty() const; diff --git a/src/track/track.cpp b/src/track/track.cpp index 7b896f63536..7abf13b4d34 100644 --- a/src/track/track.cpp +++ b/src/track/track.cpp @@ -829,6 +829,31 @@ CuePointer Track::createAndAddCue() { return pCue; } +CuePointer Track::createAndAddCue( + mixxx::CueType type, + int hotCueIndex, + double sampleStartPosition, + double sampleEndPosition) { + QMutexLocker lock(&m_qMutex); + CuePointer pCue(new Cue( + type, + hotCueIndex, + sampleStartPosition, + sampleEndPosition)); + // While this method could be called from any thread, + // associated Cue objects should always live on the + // same thread as their host, namely this->thread(). + pCue->moveToThread(thread()); + connect(pCue.get(), + &Cue::updated, + this, + &Track::slotCueUpdated); + m_cuePoints.push_back(pCue); + markDirtyAndUnlock(&lock); + emit cuesUpdated(); + return pCue; +} + CuePointer Track::findCueByType(mixxx::CueType type) const { // This method cannot be used for hotcues because there can be // multiple hotcues and this function returns only a single CuePointer. diff --git a/src/track/track.h b/src/track/track.h index 5e7f3057b33..701c9c000eb 100644 --- a/src/track/track.h +++ b/src/track/track.h @@ -249,6 +249,11 @@ class Track : public QObject { // Calls for managing the track's cue points CuePointer createAndAddCue(); + CuePointer createAndAddCue( + mixxx::CueType type, + int hotCueIndex, + double sampleStartPosition, + double sampleEndPosition); CuePointer findCueByType(mixxx::CueType type) const; // NOTE: Cannot be used for hotcues. CuePointer findCueById(DbId id) const; void removeCue(const CuePointer& pCue); From 3e6775ab3fe30526da4a2cbd8fb0f57aa37894bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sun, 29 Nov 2020 19:35:10 +0100 Subject: [PATCH 06/20] Add Cue::setStartAndEndPosition() for setting both under one locking scope --- src/analyzer/analyzersilence.cpp | 24 ++++++++++++++---------- src/track/cue.cpp | 15 +++++++++++++++ src/track/cue.h | 7 +++++-- 3 files changed, 34 insertions(+), 12 deletions(-) diff --git a/src/analyzer/analyzersilence.cpp b/src/analyzer/analyzersilence.cpp index c07cde2e53f..2307798baeb 100644 --- a/src/analyzer/analyzersilence.cpp +++ b/src/analyzer/analyzersilence.cpp @@ -95,17 +95,21 @@ void AnalyzerSilence::storeResults(TrackPointer pTrack) { CuePointer pAudibleSound = pTrack->findCueByType(mixxx::CueType::AudibleSound); if (pAudibleSound == nullptr) { - pAudibleSound = pTrack->createAndAddCue(); - pAudibleSound->setType(mixxx::CueType::AudibleSound); + pAudibleSound = pTrack->createAndAddCue( + mixxx::CueType::AudibleSound, + Cue::kNoHotCue, + firstSound, + lastSound); + } else { + // The user has no way to directly edit the AudibleSound cue. If the user + // has deleted the Intro or Outro Cue, this analysis will be rerun when + // the track is loaded again. In this case, adjust the AudibleSound Cue's + // positions. This could be helpful, for example, when the track length + // is changed in a different program, or the silence detection threshold + // is changed. + pAudibleSound->setStartPosition(firstSound); + pAudibleSound->setEndPosition(lastSound); } - // The user has no way to directly edit the AudibleSound cue. If the user - // has deleted the Intro or Outro Cue, this analysis will be rerun when - // the track is loaded again. In this case, adjust the AudibleSound Cue's - // positions. This could be helpful, for example, when the track length - // is changed in a different program, or the silence detection threshold - // is changed. - pAudibleSound->setStartPosition(firstSound); - pAudibleSound->setEndPosition(lastSound); CuePointer pIntroCue = pTrack->findCueByType(mixxx::CueType::Intro); diff --git a/src/track/cue.cpp b/src/track/cue.cpp index 1e1fc0784cc..c1c42edddb5 100644 --- a/src/track/cue.cpp +++ b/src/track/cue.cpp @@ -190,6 +190,21 @@ void Cue::setEndPosition(double samplePosition) { emit updated(); } +void Cue::setStartAndEndPosition( + double sampleStartPosition, + double sampleEndPosition) { + QMutexLocker lock(&m_mutex); + if (m_sampleStartPosition == sampleStartPosition && + m_sampleEndPosition == sampleEndPosition) { + return; + } + m_sampleStartPosition = sampleStartPosition; + m_sampleEndPosition = sampleEndPosition; + m_bDirty = true; + lock.unlock(); + emit updated(); +} + void Cue::shiftPositionFrames(double frameOffset) { QMutexLocker lock(&m_mutex); if (m_sampleStartPosition != kNoPosition) { diff --git a/src/track/cue.h b/src/track/cue.h index 5d8dbfbfde3..3e3b407fa84 100644 --- a/src/track/cue.h +++ b/src/track/cue.h @@ -61,9 +61,12 @@ class Cue : public QObject { double getPosition() const; void setStartPosition( - double samplePosition = kNoPosition); + double samplePosition); void setEndPosition( - double samplePosition = kNoPosition); + double samplePosition); + void setStartAndEndPosition( + double sampleStartPosition, + double sampleEndPosition); void shiftPositionFrames(double frameOffset); double getLength() const; From 39ff07894a1373368e46d04c86c9935406112fa0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sun, 29 Nov 2020 20:18:50 +0100 Subject: [PATCH 07/20] Create and initalize Cues at once to avoid sending singlas with halve initalized data --- src/analyzer/analyzersilence.cpp | 21 +++--- src/engine/controls/cuecontrol.cpp | 66 +++++++++------- src/mixer/basetrackplayer.cpp | 11 ++- src/test/analyzersilence_test.cpp | 20 ++--- src/test/cuecontrol_test.cpp | 117 ++++++++++++++++------------- 5 files changed, 133 insertions(+), 102 deletions(-) diff --git a/src/analyzer/analyzersilence.cpp b/src/analyzer/analyzersilence.cpp index 2307798baeb..4b0c0e8b6a1 100644 --- a/src/analyzer/analyzersilence.cpp +++ b/src/analyzer/analyzersilence.cpp @@ -107,8 +107,7 @@ void AnalyzerSilence::storeResults(TrackPointer pTrack) { // positions. This could be helpful, for example, when the track length // is changed in a different program, or the silence detection threshold // is changed. - pAudibleSound->setStartPosition(firstSound); - pAudibleSound->setEndPosition(lastSound); + pAudibleSound->setStartAndEndPosition(firstSound, lastSound); } CuePointer pIntroCue = pTrack->findCueByType(mixxx::CueType::Intro); @@ -130,17 +129,19 @@ void AnalyzerSilence::storeResults(TrackPointer pTrack) { } if (pIntroCue == nullptr) { - pIntroCue = pTrack->createAndAddCue(); - pIntroCue->setType(mixxx::CueType::Intro); - pIntroCue->setStartPosition(introStart); - pIntroCue->setEndPosition(Cue::kNoPosition); + pIntroCue = pTrack->createAndAddCue( + mixxx::CueType::Intro, + Cue::kNoHotCue, + introStart, + Cue::kNoPosition); } CuePointer pOutroCue = pTrack->findCueByType(mixxx::CueType::Outro); if (pOutroCue == nullptr) { - pOutroCue = pTrack->createAndAddCue(); - pOutroCue->setType(mixxx::CueType::Outro); - pOutroCue->setStartPosition(Cue::kNoPosition); - pOutroCue->setEndPosition(lastSound); + pOutroCue = pTrack->createAndAddCue( + mixxx::CueType::Outro, + Cue::kNoHotCue, + Cue::kNoPosition, + lastSound); } } diff --git a/src/engine/controls/cuecontrol.cpp b/src/engine/controls/cuecontrol.cpp index 4360de6b313..270ea529880 100644 --- a/src/engine/controls/cuecontrol.cpp +++ b/src/engine/controls/cuecontrol.cpp @@ -651,10 +651,11 @@ void CueControl::loadCuesFromTrack() { // Note: This is 0:00 for new tracks mainCuePoint = m_pLoadedTrack->getCuePoint(); // Than add the load cue to the list of cue - CuePointer pCue = m_pLoadedTrack->createAndAddCue(); - pCue->setType(mixxx::CueType::MainCue); - pCue->setStartPosition(mainCuePoint.getPosition()); - pCue->setHotCue(Cue::kNoHotCue); + CuePointer pCue = m_pLoadedTrack->createAndAddCue( + mixxx::CueType::MainCue, + Cue::kNoHotCue, + mainCuePoint.getPosition(), + Cue::kNoPosition); } m_pCuePoint->set(quantizeCuePoint(mainCuePoint.getPosition())); } @@ -735,8 +736,6 @@ void CueControl::hotcueSet(HotcueControl* pControl, double value, HotcueSetMode // https://bugs.launchpad.net/mixxx/+bug/1653276 hotcueClear(pControl, value); - CuePointer pCue = m_pLoadedTrack->createAndAddCue(); - double cueStartPosition = Cue::kNoPosition; double cueEndPosition = Cue::kNoPosition; mixxx::CueType cueType = mixxx::CueType::Invalid; @@ -790,11 +789,12 @@ void CueControl::hotcueSet(HotcueControl* pControl, double value, HotcueSetMode int hotcueIndex = pControl->getHotcueIndex(); - pCue->setStartPosition(cueStartPosition); - pCue->setEndPosition(cueEndPosition); - pCue->setHotCue(hotcueIndex); - pCue->setLabel(QString()); - pCue->setType(cueType); + CuePointer pCue = m_pLoadedTrack->createAndAddCue( + cueType, + hotcueIndex, + cueStartPosition, + cueEndPosition); + // TODO(XXX) deal with spurious signals attachCue(pCue, pControl); @@ -1436,11 +1436,14 @@ void CueControl::introStartSet(double value) { if (pLoadedTrack) { CuePointer pCue = pLoadedTrack->findCueByType(mixxx::CueType::Intro); if (!pCue) { - pCue = pLoadedTrack->createAndAddCue(); - pCue->setType(mixxx::CueType::Intro); + pCue = pLoadedTrack->createAndAddCue( + mixxx::CueType::Intro, + Cue::kNoHotCue, + position, + introEnd); + } else { + pCue->setStartAndEndPosition(position, introEnd); } - pCue->setStartPosition(position); - pCue->setEndPosition(introEnd); } } @@ -1520,11 +1523,14 @@ void CueControl::introEndSet(double value) { if (pLoadedTrack) { CuePointer pCue = pLoadedTrack->findCueByType(mixxx::CueType::Intro); if (!pCue) { - pCue = pLoadedTrack->createAndAddCue(); - pCue->setType(mixxx::CueType::Intro); + pCue = pLoadedTrack->createAndAddCue( + mixxx::CueType::Intro, + Cue::kNoHotCue, + introStart, + position); + } else { + pCue->setStartAndEndPosition(introStart, position); } - pCue->setStartPosition(introStart); - pCue->setEndPosition(position); } } @@ -1607,11 +1613,14 @@ void CueControl::outroStartSet(double value) { if (pLoadedTrack) { CuePointer pCue = pLoadedTrack->findCueByType(mixxx::CueType::Outro); if (!pCue) { - pCue = pLoadedTrack->createAndAddCue(); - pCue->setType(mixxx::CueType::Outro); + pCue = pLoadedTrack->createAndAddCue( + mixxx::CueType::Outro, + Cue::kNoHotCue, + position, + outroEnd); + } else { + pCue->setStartAndEndPosition(position, outroEnd); } - pCue->setStartPosition(position); - pCue->setEndPosition(outroEnd); } } @@ -1694,11 +1703,14 @@ void CueControl::outroEndSet(double value) { if (pLoadedTrack) { CuePointer pCue = pLoadedTrack->findCueByType(mixxx::CueType::Outro); if (!pCue) { - pCue = pLoadedTrack->createAndAddCue(); - pCue->setType(mixxx::CueType::Outro); + pCue = pLoadedTrack->createAndAddCue( + mixxx::CueType::Outro, + Cue::kNoHotCue, + outroStart, + position); + } else { + pCue->setStartAndEndPosition(outroStart, position); } - pCue->setStartPosition(outroStart); - pCue->setEndPosition(position); } } diff --git a/src/mixer/basetrackplayer.cpp b/src/mixer/basetrackplayer.cpp index 215ac264f0f..b8f1036bf8f 100644 --- a/src/mixer/basetrackplayer.cpp +++ b/src/mixer/basetrackplayer.cpp @@ -327,11 +327,14 @@ TrackPointer BaseTrackPlayerImpl::unloadTrack() { } } if (!pLoopCue) { - pLoopCue = m_pLoadedTrack->createAndAddCue(); - pLoopCue->setType(mixxx::CueType::Loop); + pLoopCue = m_pLoadedTrack->createAndAddCue( + mixxx::CueType::Loop, + Cue::kNoHotCue, + loopStart, + loopEnd); + } else { + pLoopCue->setStartAndEndPosition(loopStart, loopEnd); } - pLoopCue->setStartPosition(loopStart); - pLoopCue->setEndPosition(loopEnd); } disconnectLoadedTrack(); diff --git a/src/test/analyzersilence_test.cpp b/src/test/analyzersilence_test.cpp index def40554ebe..9d9eb8192b2 100644 --- a/src/test/analyzersilence_test.cpp +++ b/src/test/analyzersilence_test.cpp @@ -171,15 +171,17 @@ TEST_F(AnalyzerSilenceTest, RespectUserEdits) { pTrack->setCuePoint(CuePosition(kManualCuePosition)); - CuePointer pIntroCue = pTrack->createAndAddCue(); - pIntroCue->setType(mixxx::CueType::Intro); - pIntroCue->setStartPosition(kManualIntroPosition); - pIntroCue->setEndPosition(Cue::kNoPosition); - - CuePointer pOutroCue = pTrack->createAndAddCue(); - pOutroCue->setType(mixxx::CueType::Outro); - pOutroCue->setStartPosition(Cue::kNoPosition); - pOutroCue->setEndPosition(kManualOutroPosition); + CuePointer pIntroCue = pTrack->createAndAddCue( + mixxx::CueType::Intro, + Cue::kNoHotCue, + kManualIntroPosition, + Cue::kNoPosition); + + CuePointer pOutroCue = pTrack->createAndAddCue( + mixxx::CueType::Outro, + Cue::kNoHotCue, + Cue::kNoPosition, + kManualOutroPosition); // Fill the first half with silence for (int i = 0; i < nTrackSampleDataLength / 2; i++) { diff --git a/src/test/cuecontrol_test.cpp b/src/test/cuecontrol_test.cpp index d24e86d6765..f5fc8cad63e 100644 --- a/src/test/cuecontrol_test.cpp +++ b/src/test/cuecontrol_test.cpp @@ -82,14 +82,16 @@ class CueControlTest : public BaseSignalPathTest { TEST_F(CueControlTest, LoadUnloadTrack) { TrackPointer pTrack = createTestTrack(); pTrack->setCuePoint(CuePosition(100.0)); - auto pIntro = pTrack->createAndAddCue(); - pIntro->setType(mixxx::CueType::Intro); - pIntro->setStartPosition(150.0); - pIntro->setEndPosition(200.0); - auto pOutro = pTrack->createAndAddCue(); - pOutro->setType(mixxx::CueType::Outro); - pOutro->setStartPosition(250.0); - pOutro->setEndPosition(300.0); + auto pIntro = pTrack->createAndAddCue( + mixxx::CueType::Intro, + Cue::kNoHotCue, + 150.0, + 200.0); + auto pOutro = pTrack->createAndAddCue( + mixxx::CueType::Outro, + Cue::kNoHotCue, + 250.0, + 300.0); loadTrack(pTrack); @@ -119,14 +121,16 @@ TEST_F(CueControlTest, LoadUnloadTrack) { TEST_F(CueControlTest, LoadTrackWithDetectedCues) { TrackPointer pTrack = createTestTrack(); pTrack->setCuePoint(CuePosition(100.0)); - auto pIntro = pTrack->createAndAddCue(); - pIntro->setType(mixxx::CueType::Intro); - pIntro->setStartPosition(100.0); - pIntro->setEndPosition(Cue::kNoPosition); - auto pOutro = pTrack->createAndAddCue(); - pOutro->setType(mixxx::CueType::Outro); - pOutro->setStartPosition(Cue::kNoPosition); - pOutro->setEndPosition(200.0); + auto pIntro = pTrack->createAndAddCue( + mixxx::CueType::Intro, + Cue::kNoHotCue, + 100.0, + Cue::kNoPosition); + auto pOutro = pTrack->createAndAddCue( + mixxx::CueType::Outro, + Cue::kNoHotCue, + Cue::kNoPosition, + 200.0); loadTrack(pTrack); @@ -143,14 +147,16 @@ TEST_F(CueControlTest, LoadTrackWithDetectedCues) { TEST_F(CueControlTest, LoadTrackWithIntroEndAndOutroStart) { TrackPointer pTrack = createTestTrack(); - auto pIntro = pTrack->createAndAddCue(); - pIntro->setType(mixxx::CueType::Intro); - pIntro->setStartPosition(Cue::kNoPosition); - pIntro->setEndPosition(150.0); - auto pOutro = pTrack->createAndAddCue(); - pOutro->setType(mixxx::CueType::Outro); - pOutro->setStartPosition(250.0); - pOutro->setEndPosition(Cue::kNoPosition); + auto pIntro = pTrack->createAndAddCue( + mixxx::CueType::Intro, + Cue::kNoHotCue, + Cue::kNoPosition, + 150.0); + auto pOutro = pTrack->createAndAddCue( + mixxx::CueType::Outro, + Cue::kNoHotCue, + 250.0, + Cue::kNoPosition); loadTrack(pTrack); @@ -178,15 +184,17 @@ TEST_F(CueControlTest, LoadAutodetectedCues_QuantizeEnabled) { pTrack->setCuePoint(CuePosition(1.9 * beatLength)); - auto pIntro = pTrack->createAndAddCue(); - pIntro->setType(mixxx::CueType::Intro); - pIntro->setStartPosition(2.1 * beatLength); - pIntro->setEndPosition(3.7 * beatLength); + auto pIntro = pTrack->createAndAddCue( + mixxx::CueType::Intro, + Cue::kNoHotCue, + 2.1 * beatLength, + 3.7 * beatLength); - auto pOutro = pTrack->createAndAddCue(); - pOutro->setType(mixxx::CueType::Outro); - pOutro->setStartPosition(11.1 * beatLength); - pOutro->setEndPosition(15.5 * beatLength); + auto pOutro = pTrack->createAndAddCue( + mixxx::CueType::Outro, + Cue::kNoHotCue, + 11.1 * beatLength, + 15.5 * beatLength); loadTrack(pTrack); @@ -205,15 +213,17 @@ TEST_F(CueControlTest, LoadAutodetectedCues_QuantizeEnabledNoBeats) { pTrack->setCuePoint(CuePosition(100.0)); - auto pIntro = pTrack->createAndAddCue(); - pIntro->setType(mixxx::CueType::Intro); - pIntro->setStartPosition(250.0); - pIntro->setEndPosition(400.0); + auto pIntro = pTrack->createAndAddCue( + mixxx::CueType::Intro, + Cue::kNoHotCue, + 250.0, + 400.0); - auto pOutro = pTrack->createAndAddCue(); - pOutro->setType(mixxx::CueType::Outro); - pOutro->setStartPosition(550.0); - pOutro->setEndPosition(800.0); + auto pOutro = pTrack->createAndAddCue( + mixxx::CueType::Outro, + Cue::kNoHotCue, + 550.0, + 800.0); loadTrack(pTrack); @@ -232,15 +242,17 @@ TEST_F(CueControlTest, LoadAutodetectedCues_QuantizeDisabled) { pTrack->setCuePoint(CuePosition(240.0)); - auto pIntro = pTrack->createAndAddCue(); - pIntro->setType(mixxx::CueType::Intro); - pIntro->setStartPosition(210.0); - pIntro->setEndPosition(330.0); + auto pIntro = pTrack->createAndAddCue( + mixxx::CueType::Intro, + Cue::kNoHotCue, + 210.0, + 330.0); - auto pOutro = pTrack->createAndAddCue(); - pOutro->setType(mixxx::CueType::Outro); - pOutro->setStartPosition(770.0); - pOutro->setEndPosition(990.0); + auto pOutro = pTrack->createAndAddCue( + mixxx::CueType::Outro, + Cue::kNoHotCue, + 770.0, + 990.0); loadTrack(pTrack); @@ -254,10 +266,11 @@ TEST_F(CueControlTest, LoadAutodetectedCues_QuantizeDisabled) { TEST_F(CueControlTest, SeekOnLoadDefault) { // Default is to load at the intro start TrackPointer pTrack = createTestTrack(); - auto pIntro = pTrack->createAndAddCue(); - pIntro->setType(mixxx::CueType::Intro); - pIntro->setStartPosition(250.0); - pIntro->setEndPosition(400.0); + auto pIntro = pTrack->createAndAddCue( + mixxx::CueType::Intro, + Cue::kNoHotCue, + 250.0, + 400.0); loadTrack(pTrack); From 7159a99477a4dd9973d1ed45d337d631dc4bafdf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sun, 29 Nov 2020 20:20:52 +0100 Subject: [PATCH 08/20] get rid of hotCueFound --- src/library/rekordbox/rekordboxfeature.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/library/rekordbox/rekordboxfeature.cpp b/src/library/rekordbox/rekordboxfeature.cpp index bb452d372c3..f8682767ae6 100644 --- a/src/library/rekordbox/rekordboxfeature.cpp +++ b/src/library/rekordbox/rekordboxfeature.cpp @@ -794,18 +794,15 @@ void setHotCue(TrackPointer track, const QString& label, mixxx::RgbColor::optional_t color) { CuePointer pCue; - bool hotCueFound = false; - const QList cuePoints = track->getCuePoints(); for (const CuePointer& trackCue : cuePoints) { if (trackCue->getHotCue() == id) { pCue = trackCue; - hotCueFound = true; break; } } - if (!hotCueFound) { + if (!pCue) { pCue = CuePointer(track->createAndAddCue()); } From 234f90bffc3cc386cf0f217b3ae3cd25bcfeb160 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sun, 29 Nov 2020 20:51:17 +0100 Subject: [PATCH 09/20] remove now unused createAndAddCue() that creates an invalid Cue --- src/library/rekordbox/rekordboxfeature.cpp | 19 +++++++++++-------- src/track/track.cpp | 16 ---------------- src/track/track.h | 1 - 3 files changed, 11 insertions(+), 25 deletions(-) diff --git a/src/library/rekordbox/rekordboxfeature.cpp b/src/library/rekordbox/rekordboxfeature.cpp index f8682767ae6..ead2250beb8 100644 --- a/src/library/rekordbox/rekordboxfeature.cpp +++ b/src/library/rekordbox/rekordboxfeature.cpp @@ -802,18 +802,21 @@ void setHotCue(TrackPointer track, } } - if (!pCue) { - pCue = CuePointer(track->createAndAddCue()); + mixxx::CueType type = mixxx::CueType::HotCue; + if (endPosition != Cue::kNoPosition) { + type = mixxx::CueType::Loop; } - pCue->setStartPosition(startPosition); - if (endPosition == Cue::kNoPosition) { - pCue->setType(mixxx::CueType::HotCue); + if (!pCue) { + pCue = track->createAndAddCue( + type, + id, + startPosition, + endPosition); } else { - pCue->setType(mixxx::CueType::Loop); - pCue->setEndPosition(endPosition); + pCue->setStartAndEndPosition(startPosition, endPosition); } - pCue->setHotCue(id); + pCue->setLabel(label); if (color) { pCue->setColor(*color); diff --git a/src/track/track.cpp b/src/track/track.cpp index 7abf13b4d34..9a97369c9fc 100644 --- a/src/track/track.cpp +++ b/src/track/track.cpp @@ -813,22 +813,6 @@ void Track::slotCueUpdated() { emit cuesUpdated(); } -CuePointer Track::createAndAddCue() { - QMutexLocker lock(&m_qMutex); - CuePointer pCue(new Cue()); - // While this method could be called from any thread, - // associated Cue objects should always live on the - // same thread as their host, namely this->thread(). - pCue->moveToThread(thread()); - connect(pCue.get(), - &Cue::updated, - this, - &Track::slotCueUpdated); - m_cuePoints.push_back(pCue); - // don't emit cuesUpdated() here, because the cue is still invalid. - return pCue; -} - CuePointer Track::createAndAddCue( mixxx::CueType type, int hotCueIndex, diff --git a/src/track/track.h b/src/track/track.h index 701c9c000eb..ce6681c4469 100644 --- a/src/track/track.h +++ b/src/track/track.h @@ -248,7 +248,6 @@ class Track : public QObject { void analysisFinished(); // Calls for managing the track's cue points - CuePointer createAndAddCue(); CuePointer createAndAddCue( mixxx::CueType type, int hotCueIndex, From c38fd68be5fedc2a1eb2f800c36ad89defaab469 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sun, 29 Nov 2020 21:15:44 +0100 Subject: [PATCH 10/20] Remove unused Cue() default constructor --- src/test/cue_test.cpp | 8 -------- src/track/cue.cpp | 10 ---------- src/track/cue.h | 2 -- src/track/track.cpp | 8 +++++--- 4 files changed, 5 insertions(+), 23 deletions(-) diff --git a/src/test/cue_test.cpp b/src/test/cue_test.cpp index 8e8a34ab19d..3e0af81cdef 100644 --- a/src/test/cue_test.cpp +++ b/src/test/cue_test.cpp @@ -8,14 +8,6 @@ namespace mixxx { -TEST(CueTest, DefaultCueToCueInfoTest) { - const Cue cueObject; - auto cueInfo = cueObject.getCueInfo( - audio::SampleRate(44100)); - cueInfo.setColor(std::nullopt); - EXPECT_EQ(CueInfo(), cueInfo); -} - TEST(CueTest, DefaultCueInfoToCueRoundtrip) { const CueInfo cueInfo1; const Cue cueObject( diff --git a/src/track/cue.cpp b/src/track/cue.cpp index c1c42edddb5..4c361385f33 100644 --- a/src/track/cue.cpp +++ b/src/track/cue.cpp @@ -47,16 +47,6 @@ void CuePointer::deleteLater(Cue* pCue) { } } -Cue::Cue() - : m_bDirty(false), - m_type(mixxx::CueType::Invalid), - m_sampleStartPosition(Cue::kNoPosition), - m_sampleEndPosition(Cue::kNoPosition), - m_iHotCue(Cue::kNoHotCue), - m_color(mixxx::PredefinedColorPalettes::kDefaultCueColor) { - DEBUG_ASSERT(!m_dbId.isValid()); -} - Cue::Cue( DbId id, mixxx::CueType type, diff --git a/src/track/cue.h b/src/track/cue.h index 3e3b407fa84..3abc9773a0c 100644 --- a/src/track/cue.h +++ b/src/track/cue.h @@ -26,8 +26,6 @@ class Cue : public QObject { /// Hot cues are sequentially indexed starting with kFirstHotCue (inclusive) static constexpr int kFirstHotCue = 0; - /// Creates an invalid Cue point - Cue(); /// For roundtrips during tests Cue( const mixxx::CueInfo& cueInfo, diff --git a/src/track/track.cpp b/src/track/track.cpp index 9a97369c9fc..52f1765bc22 100644 --- a/src/track/track.cpp +++ b/src/track/track.cpp @@ -763,13 +763,15 @@ void Track::setCuePoint(CuePosition cue) { if (pLoadCue) { pLoadCue->setStartPosition(position); } else { - pLoadCue = CuePointer(new Cue()); + pLoadCue = CuePointer(new Cue( + mixxx::CueType::MainCue, + Cue::kNoHotCue, + position, + Cue::kNoPosition)); // While this method could be called from any thread, // associated Cue objects should always live on the // same thread as their host, namely this->thread(). pLoadCue->moveToThread(thread()); - pLoadCue->setType(mixxx::CueType::MainCue); - pLoadCue->setStartPosition(position); connect(pLoadCue.get(), &Cue::updated, this, From 15e10bb6fa6e3be0b4fd82ae9d3d37cc2007c32a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sun, 29 Nov 2020 21:22:27 +0100 Subject: [PATCH 11/20] make m_iHotCue const --- src/track/cue.cpp | 12 ------------ src/track/cue.h | 4 +--- 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/src/track/cue.cpp b/src/track/cue.cpp index 4c361385f33..0b37be5cc17 100644 --- a/src/track/cue.cpp +++ b/src/track/cue.cpp @@ -224,18 +224,6 @@ int Cue::getHotCue() const { return m_iHotCue; } -void Cue::setHotCue(int hotCue) { - QMutexLocker lock(&m_mutex); - // TODO(XXX) enforce uniqueness? - if (m_iHotCue == hotCue) { - return; - } - m_iHotCue = hotCue; - m_bDirty = true; - lock.unlock(); - emit updated(); -} - QString Cue::getLabel() const { QMutexLocker lock(&m_mutex); return m_label; diff --git a/src/track/cue.h b/src/track/cue.h index 3abc9773a0c..a9f35bf213d 100644 --- a/src/track/cue.h +++ b/src/track/cue.h @@ -70,8 +70,6 @@ class Cue : public QObject { double getLength() const; int getHotCue() const; - void setHotCue( - int hotCue = kNoHotCue); QString getLabel() const; void setLabel(const QString& label); @@ -99,7 +97,7 @@ class Cue : public QObject { mixxx::CueType m_type; double m_sampleStartPosition; double m_sampleEndPosition; - int m_iHotCue; + const int m_iHotCue; QString m_label; mixxx::RgbColor m_color; From 2d758610846d3eb86f4eba71fd7d1d1e77a1fb19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sun, 29 Nov 2020 21:39:22 +0100 Subject: [PATCH 12/20] Added getStartAndEndPosition() to query both in one locking scope. --- src/track/cue.cpp | 5 +++++ src/track/cue.h | 7 +++++++ 2 files changed, 12 insertions(+) diff --git a/src/track/cue.cpp b/src/track/cue.cpp index 0b37be5cc17..a64ad516dd3 100644 --- a/src/track/cue.cpp +++ b/src/track/cue.cpp @@ -195,6 +195,11 @@ void Cue::setStartAndEndPosition( emit updated(); } +Cue::StartAndEndPositions Cue::getStartAndEndPosition() const { + QMutexLocker lock(&m_mutex); + return {m_sampleStartPosition, m_sampleEndPosition}; +} + void Cue::shiftPositionFrames(double frameOffset) { QMutexLocker lock(&m_mutex); if (m_sampleStartPosition != kNoPosition) { diff --git a/src/track/cue.h b/src/track/cue.h index a9f35bf213d..01fbf4050ca 100644 --- a/src/track/cue.h +++ b/src/track/cue.h @@ -26,6 +26,11 @@ class Cue : public QObject { /// Hot cues are sequentially indexed starting with kFirstHotCue (inclusive) static constexpr int kFirstHotCue = 0; + struct StartAndEndPositions { + double startPosition; + double endPosition; + }; + /// For roundtrips during tests Cue( const mixxx::CueInfo& cueInfo, @@ -79,6 +84,8 @@ class Cue : public QObject { double getEndPosition() const; + StartAndEndPositions getStartAndEndPosition() const; + mixxx::CueInfo getCueInfo( mixxx::audio::SampleRate sampleRate) const; From ffcb727c18abf70e6761c7be4b6b588e82dc1a8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sun, 29 Nov 2020 22:01:32 +0100 Subject: [PATCH 13/20] Use getStartAndEndPosition() to be sure the two values match. --- src/engine/controls/cuecontrol.cpp | 23 +++++++++++++---------- src/library/autodj/autodjprocessor.cpp | 8 ++++---- src/widget/wcuemenupopup.cpp | 13 +++++++------ 3 files changed, 24 insertions(+), 20 deletions(-) diff --git a/src/engine/controls/cuecontrol.cpp b/src/engine/controls/cuecontrol.cpp index 270ea529880..97fef869a6c 100644 --- a/src/engine/controls/cuecontrol.cpp +++ b/src/engine/controls/cuecontrol.cpp @@ -581,8 +581,9 @@ void CueControl::loadCuesFromTrack() { attachCue(pCue, pControl); } else { // If the old hotcue is the same, then we only need to update - pControl->setPosition(pCue->getPosition()); - pControl->setEndPosition(pCue->getEndPosition()); + Cue::StartAndEndPositions pos = pCue->getStartAndEndPosition(); + pControl->setPosition(pos.startPosition); + pControl->setEndPosition(pos.endPosition); pControl->setColor(pCue->getColor()); pControl->setType(pCue->getType()); } @@ -940,7 +941,8 @@ void CueControl::hotcueCueLoop(HotcueControl* pControl, double value) { setCurrentSavedLoopControlAndActivate(pControl); } else { bool loopActive = pControl->getStatus() == HotcueControl::Status::Active; - setLoop(pCue->getPosition(), pCue->getEndPosition(), !loopActive); + Cue::StartAndEndPositions pos = pCue->getStartAndEndPosition(); + setLoop(pos.startPosition, pos.endPosition, !loopActive); } } break; case mixxx::CueType::HotCue: { @@ -981,7 +983,8 @@ void CueControl::hotcueActivate(HotcueControl* pControl, double value, HotcueSet } else { bool loopActive = pControl->getStatus() == HotcueControl::Status::Active; - setLoop(pCue->getPosition(), pCue->getEndPosition(), !loopActive); + Cue::StartAndEndPositions pos = pCue->getStartAndEndPosition(); + setLoop(pos.startPosition, pos.endPosition, !loopActive); } break; default: @@ -2075,18 +2078,17 @@ void CueControl::setCurrentSavedLoopControlAndActivate(HotcueControl* pControl) } mixxx::CueType type = pCue->getType(); - double startPos = pCue->getPosition(); - double endPos = pCue->getEndPosition(); + Cue::StartAndEndPositions pos = pCue->getStartAndEndPosition(); VERIFY_OR_DEBUG_ASSERT( type == mixxx::CueType::Loop && - endPos != Cue::kNoPosition) { + pos.endPosition != Cue::kNoPosition) { return; } // Set new control as active m_pCurrentSavedLoopControl = pControl; - setLoop(startPos, endPos, true); + setLoop(pos.startPosition, pos.endPosition, true); pControl->setStatus(HotcueControl::Status::Active); } @@ -2404,8 +2406,9 @@ double HotcueControl::getEndPosition() const { void HotcueControl::setCue(const CuePointer& pCue) { DEBUG_ASSERT(!m_pCue); - setPosition(pCue->getPosition()); - setEndPosition(pCue->getEndPosition()); + Cue::StartAndEndPositions pos = pCue->getStartAndEndPosition(); + setPosition(pos.startPosition); + setEndPosition(pos.endPosition); setColor(pCue->getColor()); setStatus((pCue->getType() == mixxx::CueType::Invalid) ? HotcueControl::Status::Empty diff --git a/src/library/autodj/autodjprocessor.cpp b/src/library/autodj/autodjprocessor.cpp index 9e4f52c6b79..115de97fc01 100644 --- a/src/library/autodj/autodjprocessor.cpp +++ b/src/library/autodj/autodjprocessor.cpp @@ -1118,10 +1118,10 @@ double AutoDJProcessor::getLastSoundSecond(DeckAttributes* pDeck) { } CuePointer pFromTrackAudibleSound = pTrack->findCueByType(mixxx::CueType::AudibleSound); - if (pFromTrackAudibleSound && pFromTrackAudibleSound->getLength() > 0) { - double lastSound = pFromTrackAudibleSound->getEndPosition(); - if (lastSound > 0) { - return samplePositionToSeconds(lastSound, pDeck); + if (pFromTrackAudibleSound) { + Cue::StartAndEndPositions pos = pFromTrackAudibleSound->getStartAndEndPosition(); + if (pos.endPosition > 0 && (pos.endPosition - pos.startPosition) > 0) { + return samplePositionToSeconds(pos.endPosition, pDeck); } } return getEndSecond(pDeck); diff --git a/src/widget/wcuemenupopup.cpp b/src/widget/wcuemenupopup.cpp index ea92cb56c07..b5ede872137 100644 --- a/src/widget/wcuemenupopup.cpp +++ b/src/widget/wcuemenupopup.cpp @@ -81,13 +81,14 @@ void WCueMenuPopup::setTrackAndCue(TrackPointer pTrack, const CuePointer& pCue) m_pCueNumber->setText(hotcueNumberText); QString positionText = ""; - double startPosition = m_pCue->getPosition(); - double endPosition = m_pCue->getEndPosition(); - if (startPosition != Cue::kNoPosition) { - double startPositionSeconds = startPosition / m_pTrack->getSampleRate() / mixxx::kEngineChannelCount; + Cue::StartAndEndPositions pos = m_pCue->getStartAndEndPosition(); + if (pos.startPosition != Cue::kNoPosition) { + double startPositionSeconds = pos.startPosition / + m_pTrack->getSampleRate() / mixxx::kEngineChannelCount; positionText = mixxx::Duration::formatTime(startPositionSeconds, mixxx::Duration::Precision::CENTISECONDS); - if (endPosition != Cue::kNoPosition) { - double endPositionSeconds = endPosition / m_pTrack->getSampleRate() / mixxx::kEngineChannelCount; + if (pos.endPosition != Cue::kNoPosition) { + double endPositionSeconds = pos.endPosition / + m_pTrack->getSampleRate() / mixxx::kEngineChannelCount; positionText = QString("%1 - %2").arg( positionText, mixxx::Duration::formatTime(endPositionSeconds, mixxx::Duration::Precision::CENTISECONDS) From ad7a2b2439ba07ab2d7ae925e712b1248944db11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sun, 29 Nov 2020 23:33:27 +0100 Subject: [PATCH 14/20] Improve API for storing of the inital preview state. --- src/engine/controls/cuecontrol.cpp | 24 ++++++++++++------------ src/engine/controls/cuecontrol.h | 20 +++++++++++--------- 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/src/engine/controls/cuecontrol.cpp b/src/engine/controls/cuecontrol.cpp index 97fef869a6c..5d982c2fda4 100644 --- a/src/engine/controls/cuecontrol.cpp +++ b/src/engine/controls/cuecontrol.cpp @@ -1008,18 +1008,17 @@ void CueControl::hotcueActivate(HotcueControl* pControl, double value, HotcueSet void CueControl::hotcueActivatePreview(HotcueControl* pControl, double value) { CuePointer pCue = pControl->getCue(); + int index = pControl->getHotcueIndex(); if (value > 0) { - if (pCue) { - double position = pCue->getPosition(); - mixxx::CueType type = pCue->getType(); - int index = pControl->getHotcueIndex(); + if (m_currentlyPreviewingIndex != index) { + pControl->storePreviewingActivateData(); + double position = pControl->getPreviewingPosition(); + mixxx::CueType type = pControl->getPreviewingType(); + ; if (type != mixxx::CueType::Invalid && - position != Cue::kNoPosition && - m_currentlyPreviewingIndex != index) { + position != Cue::kNoPosition) { updateCurrentlyPreviewingIndex(index); m_bypassCueSetByPlay = true; - pControl->setPreviewingType(type); - pControl->setPreviewingPosition(position); if (type == mixxx::CueType::Loop) { setCurrentSavedLoopControlAndActivate(pControl); } else if (pControl->getStatus() == HotcueControl::Status::Set) { @@ -1029,7 +1028,7 @@ void CueControl::hotcueActivatePreview(HotcueControl* pControl, double value) { m_pPlay->set(1.0); } } - } else if (m_currentlyPreviewingIndex == pControl->getHotcueIndex()) { + } else if (m_currentlyPreviewingIndex == index) { // This is a release of a previewing hotcue double position = pControl->getPreviewingPosition(); updateCurrentlyPreviewingIndex(Cue::kNoHotCue); @@ -2169,9 +2168,7 @@ ConfigKey HotcueControl::keyForControl(const QString& name) { HotcueControl::HotcueControl(const QString& group, int hotcueIndex) : m_group(group), m_hotcueIndex(hotcueIndex), - m_pCue(nullptr), - m_previewingType(mixxx::CueType::Invalid), - m_previewingPosition(-1) { + m_pCue(nullptr) { m_hotcuePosition = std::make_unique(keyForControl(QStringLiteral("position"))); connect(m_hotcuePosition.get(), &ControlObject::valueChanged, @@ -2310,6 +2307,9 @@ HotcueControl::HotcueControl(const QString& group, int hotcueIndex) this, &HotcueControl::slotHotcueClear, Qt::DirectConnection); + + m_previewingType.setValue(mixxx::CueType::Invalid); + m_previewingPosition.setValue(Cue::kNoPosition); } HotcueControl::~HotcueControl() = default; diff --git a/src/engine/controls/cuecontrol.h b/src/engine/controls/cuecontrol.h index 6360012a646..dcecd738179 100644 --- a/src/engine/controls/cuecontrol.h +++ b/src/engine/controls/cuecontrol.h @@ -102,16 +102,18 @@ class HotcueControl : public QObject { // Used for caching the preview state of this hotcue control // for the case the cue is deleted during preview. mixxx::CueType getPreviewingType() const { - return m_previewingType; - } - void setPreviewingType(mixxx::CueType type) { - m_previewingType = type; + return m_previewingType.getValue(); } double getPreviewingPosition() const { - return m_previewingPosition; + return m_previewingPosition.getValue(); } - void setPreviewingPosition(double position) { - m_previewingPosition = position; + void storePreviewingActivateData() { + if (m_pCue) { + m_previewingPosition.setValue(m_pCue->getPosition()); + m_previewingType.setValue(m_pCue->getType()); + } else { + m_previewingType.setValue(mixxx::CueType::Invalid); + } } private slots: @@ -176,8 +178,8 @@ class HotcueControl : public QObject { std::unique_ptr m_hotcueActivatePreview; std::unique_ptr m_hotcueClear; - mixxx::CueType m_previewingType; - double m_previewingPosition; + ControlValueAtomic m_previewingType; + ControlValueAtomic m_previewingPosition; }; class CueControl : public EngineControl { From d72f2b4aae9c27499b5bba77b778b1068b72e7c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Wed, 16 Dec 2020 09:10:03 +0100 Subject: [PATCH 15/20] fix typos --- src/engine/controls/cuecontrol.cpp | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/engine/controls/cuecontrol.cpp b/src/engine/controls/cuecontrol.cpp index 5d982c2fda4..cdec7d7ac1c 100644 --- a/src/engine/controls/cuecontrol.cpp +++ b/src/engine/controls/cuecontrol.cpp @@ -30,7 +30,7 @@ constexpr double CUE_MODE_CUP = 5.0; /// This is the position of a fresh loaded tack without any seek constexpr double kDefaultLoadPosition = 0.0; constexpr int kNoHotCueNumber = 0; -/// Used for a common tarcking of the previewing Hotcue in m_currentlyPreviewingIndex +/// Used for a common tracking of the previewing Hotcue in m_currentlyPreviewingIndex constexpr int kMainCueIndex = NUM_HOT_CUES; // Helper function to convert control values (i.e. doubles) into RgbColor @@ -416,7 +416,7 @@ void CueControl::detachCue(HotcueControl* pControl) { pControl->resetCue(); } -// This is called from the EngineWokerThread and ends with the initial seek v +// This is called from the EngineWokerThread and ends with the initial seek // via seekOnLoad(). There is the theoretical and pending issue of a delayed control // command intended for the old track that might be performed instead. void CueControl::trackLoaded(TrackPointer pNewTrack) { @@ -1179,7 +1179,7 @@ void CueControl::cueGoto(double value) { // Seek to cue point double cuePoint = m_pCuePoint->get(); - // Note: We do not mess with ply here, we continue playing or previewing. + // Note: We do not mess with play here, we continue playing or previewing. // Need to unlock before emitting any signals to prevent deadlock. lock.unlock(); @@ -1433,7 +1433,7 @@ void CueControl::introStartSet(double value) { lock.unlock(); // Update Track's cue. - // CO's are update loadCuesFromTrack() + // CO's are updated in loadCuesFromTrack() // this can be done outside the locking scope if (pLoadedTrack) { CuePointer pCue = pLoadedTrack->findCueByType(mixxx::CueType::Intro); @@ -1460,7 +1460,7 @@ void CueControl::introStartClear(double value) { lock.unlock(); // Update Track's cue. - // CO's are update loadCuesFromTrack() + // CO's are updated in loadCuesFromTrack() // this can be done outside the locking scope if (pLoadedTrack) { CuePointer pCue = pLoadedTrack->findCueByType(mixxx::CueType::Intro); @@ -1520,7 +1520,7 @@ void CueControl::introEndSet(double value) { lock.unlock(); // Update Track's cue. - // CO's are update loadCuesFromTrack() + // CO's are updated in loadCuesFromTrack() // this can be done outside the locking scope if (pLoadedTrack) { CuePointer pCue = pLoadedTrack->findCueByType(mixxx::CueType::Intro); @@ -1547,7 +1547,7 @@ void CueControl::introEndClear(double value) { lock.unlock(); // Update Track's cue. - // CO's are update loadCuesFromTrack() + // CO's are updated in loadCuesFromTrack() // this can be done outside the locking scope if (pLoadedTrack) { CuePointer pCue = pLoadedTrack->findCueByType(mixxx::CueType::Intro); @@ -1610,7 +1610,7 @@ void CueControl::outroStartSet(double value) { lock.unlock(); // Update Track's cue. - // CO's are update loadCuesFromTrack() + // CO's are updated in loadCuesFromTrack() // this can be done outside the locking scope if (pLoadedTrack) { CuePointer pCue = pLoadedTrack->findCueByType(mixxx::CueType::Outro); @@ -1637,7 +1637,7 @@ void CueControl::outroStartClear(double value) { lock.unlock(); // Update Track's cue. - // CO's are update loadCuesFromTrack() + // CO's are updated in loadCuesFromTrack() // this can be done outside the locking scope if (pLoadedTrack) { CuePointer pCue = pLoadedTrack->findCueByType(mixxx::CueType::Outro); @@ -1700,7 +1700,7 @@ void CueControl::outroEndSet(double value) { lock.unlock(); // Update Track's cue. - // CO's are update loadCuesFromTrack() + // CO's are updated in loadCuesFromTrack() // this can be done outside the locking scope if (pLoadedTrack) { CuePointer pCue = pLoadedTrack->findCueByType(mixxx::CueType::Outro); @@ -1727,7 +1727,7 @@ void CueControl::outroEndClear(double value) { lock.unlock(); // Update Track's cue. - // CO's are update loadCuesFromTrack() + // CO's are updated in loadCuesFromTrack() // this can be done outside the locking scope if (pLoadedTrack) { CuePointer pCue = pLoadedTrack->findCueByType(mixxx::CueType::Outro); From 90342c4cb202f2aaaa4c1d22a3dc6440aa96d128 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Wed, 16 Dec 2020 09:51:04 +0100 Subject: [PATCH 16/20] Make a local copy of m_pCurrentSavedLoopControl to make sur it doe not become null --- src/engine/controls/cuecontrol.cpp | 50 +++++++++++++++--------------- src/engine/controls/cuecontrol.h | 3 +- 2 files changed, 27 insertions(+), 26 deletions(-) diff --git a/src/engine/controls/cuecontrol.cpp b/src/engine/controls/cuecontrol.cpp index a9ccb90817e..3b3d7ed85e9 100644 --- a/src/engine/controls/cuecontrol.cpp +++ b/src/engine/controls/cuecontrol.cpp @@ -407,10 +407,7 @@ void CueControl::detachCue(HotcueControl* pControl) { } disconnect(pCue.get(), nullptr, this, nullptr); - - if (m_pCurrentSavedLoopControl == pControl) { - m_pCurrentSavedLoopControl = nullptr; - } + m_pCurrentSavedLoopControl.testAndSetRelease(pControl, nullptr); pControl->resetCue(); } @@ -595,9 +592,9 @@ void CueControl::loadCuesFromTrack() { } // Detach all hotcues that are no longer present - for (int hotCue = 0; hotCue < m_iNumHotCues; ++hotCue) { - if (!active_hotcues.contains(hotCue)) { - HotcueControl* pControl = m_hotcueControls.at(hotCue); + for (int hotCueIndex = 0; hotCueIndex < m_iNumHotCues; ++hotCueIndex) { + if (!active_hotcues.contains(hotCueIndex)) { + HotcueControl* pControl = m_hotcueControls.at(hotCueIndex); detachCue(pControl); } } @@ -2059,11 +2056,11 @@ void CueControl::hotcueFocusColorNext(double value) { } void CueControl::setCurrentSavedLoopControlAndActivate(HotcueControl* pControl) { - if (m_pCurrentSavedLoopControl && m_pCurrentSavedLoopControl != pControl) { + HotcueControl* pOldSavedLoopControl = m_pCurrentSavedLoopControl.fetchAndStoreAcquire(nullptr); + if (pOldSavedLoopControl && pOldSavedLoopControl != pControl) { // Disable previous saved loop - DEBUG_ASSERT(m_pCurrentSavedLoopControl->getStatus() != HotcueControl::Status::Empty); - m_pCurrentSavedLoopControl->setStatus(HotcueControl::Status::Set); - m_pCurrentSavedLoopControl = nullptr; + DEBUG_ASSERT(pOldSavedLoopControl->getStatus() != HotcueControl::Status::Empty); + pOldSavedLoopControl->setStatus(HotcueControl::Status::Set); } if (!pControl) { @@ -2084,9 +2081,9 @@ void CueControl::setCurrentSavedLoopControlAndActivate(HotcueControl* pControl) } // Set new control as active - m_pCurrentSavedLoopControl = pControl; setLoop(pos.startPosition, pos.endPosition, true); pControl->setStatus(HotcueControl::Status::Active); + m_pCurrentSavedLoopControl.storeRelease(pControl); } void CueControl::slotLoopReset() { @@ -2094,39 +2091,42 @@ void CueControl::slotLoopReset() { } void CueControl::slotLoopEnabledChanged(bool enabled) { - if (!m_pCurrentSavedLoopControl) { + HotcueControl* pSavedLoopControl = m_pCurrentSavedLoopControl; + if (!pSavedLoopControl) { return; } - DEBUG_ASSERT(m_pCurrentSavedLoopControl->getStatus() != HotcueControl::Status::Empty); + DEBUG_ASSERT(pSavedLoopControl->getStatus() != HotcueControl::Status::Empty); DEBUG_ASSERT( - m_pCurrentSavedLoopControl->getCue() && - m_pCurrentSavedLoopControl->getCue()->getPosition() == + pSavedLoopControl->getCue() && + pSavedLoopControl->getCue()->getPosition() == m_pLoopStartPosition->get()); DEBUG_ASSERT( - m_pCurrentSavedLoopControl->getCue() && - m_pCurrentSavedLoopControl->getCue()->getEndPosition() == + pSavedLoopControl->getCue() && + pSavedLoopControl->getCue()->getEndPosition() == m_pLoopEndPosition->get()); if (enabled) { - m_pCurrentSavedLoopControl->setStatus(HotcueControl::Status::Active); + pSavedLoopControl->setStatus(HotcueControl::Status::Active); } else { - m_pCurrentSavedLoopControl->setStatus(HotcueControl::Status::Set); + pSavedLoopControl->setStatus(HotcueControl::Status::Set); } } void CueControl::slotLoopUpdated(double startPosition, double endPosition) { - if (!m_pCurrentSavedLoopControl) { + HotcueControl* pSavedLoopControl = m_pCurrentSavedLoopControl; + if (!pSavedLoopControl) { return; } - if (m_pCurrentSavedLoopControl->getStatus() != HotcueControl::Status::Active) { + if (pSavedLoopControl->getStatus() != HotcueControl::Status::Active) { slotLoopReset(); return; } - CuePointer pCue = m_pCurrentSavedLoopControl->getCue(); + CuePointer pCue = pSavedLoopControl->getCue(); if (!pCue) { + // this can happen if the cue is deleted while this slot is cued return; } @@ -2139,10 +2139,10 @@ void CueControl::slotLoopUpdated(double startPosition, double endPosition) { DEBUG_ASSERT(endPosition != Cue::kNoPosition); DEBUG_ASSERT(startPosition < endPosition); - DEBUG_ASSERT(m_pCurrentSavedLoopControl->getStatus() == HotcueControl::Status::Active); + DEBUG_ASSERT(pSavedLoopControl->getStatus() == HotcueControl::Status::Active); pCue->setStartPosition(startPosition); pCue->setEndPosition(endPosition); - DEBUG_ASSERT(m_pCurrentSavedLoopControl->getStatus() == HotcueControl::Status::Active); + DEBUG_ASSERT(pSavedLoopControl->getStatus() == HotcueControl::Status::Active); } void CueControl::setHotcueFocusIndex(int hotcueIndex) { diff --git a/src/engine/controls/cuecontrol.h b/src/engine/controls/cuecontrol.h index 9e0b758bec2..ce0c9a66149 100644 --- a/src/engine/controls/cuecontrol.h +++ b/src/engine/controls/cuecontrol.h @@ -1,6 +1,7 @@ #pragma once #include +#include #include #include @@ -335,7 +336,7 @@ class CueControl : public EngineControl { ControlObject* m_pHotcueFocusColorNext; ControlObject* m_pHotcueFocusColorPrev; - HotcueControl* m_pCurrentSavedLoopControl; + QAtomicPointer m_pCurrentSavedLoopControl; // Tells us which controls map to which hotcue QMap m_controlMap; From 512573da70cc7328cac1ad985d3e97c16f21f3fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Wed, 16 Dec 2020 09:56:33 +0100 Subject: [PATCH 17/20] rename storePreviewingActivateData() to cachePreviewingStartState() and add a comment --- src/engine/controls/cuecontrol.cpp | 2 +- src/engine/controls/cuecontrol.h | 12 +++++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/engine/controls/cuecontrol.cpp b/src/engine/controls/cuecontrol.cpp index 3b3d7ed85e9..5cee08910d7 100644 --- a/src/engine/controls/cuecontrol.cpp +++ b/src/engine/controls/cuecontrol.cpp @@ -1006,7 +1006,7 @@ void CueControl::hotcueActivatePreview(HotcueControl* pControl, double value) { int index = pControl->getHotcueIndex(); if (value > 0) { if (m_currentlyPreviewingIndex != index) { - pControl->storePreviewingActivateData(); + pControl->cachePreviewingStartState(); double position = pControl->getPreviewingPosition(); mixxx::CueType type = pControl->getPreviewingType(); ; diff --git a/src/engine/controls/cuecontrol.h b/src/engine/controls/cuecontrol.h index ce0c9a66149..94717d0feec 100644 --- a/src/engine/controls/cuecontrol.h +++ b/src/engine/controls/cuecontrol.h @@ -96,15 +96,21 @@ class HotcueControl : public QObject { void setColor(mixxx::RgbColor::optional_t newColor); mixxx::RgbColor::optional_t getColor() const; - // Used for caching the preview state of this hotcue control - // for the case the cue is deleted during preview. + /// Used for caching the preview state of this hotcue control + /// for the case the cue is deleted during preview. mixxx::CueType getPreviewingType() const { return m_previewingType.getValue(); } + + /// Used for caching the preview state of this hotcue control + /// for the case the cue is deleted during preview. double getPreviewingPosition() const { return m_previewingPosition.getValue(); } - void storePreviewingActivateData() { + + /// Used for caching the preview state of this hotcue control + /// for the case the cue is deleted during preview. + void cachePreviewingStartState() { if (m_pCue) { m_previewingPosition.setValue(m_pCue->getPosition()); m_previewingType.setValue(m_pCue->getType()); From 58f9939859701d93c8378f8d32df2cd229d24df2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Wed, 16 Dec 2020 09:59:15 +0100 Subject: [PATCH 18/20] Reorder member variables --- src/engine/controls/cuecontrol.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/controls/cuecontrol.h b/src/engine/controls/cuecontrol.h index 94717d0feec..0d393a1948f 100644 --- a/src/engine/controls/cuecontrol.h +++ b/src/engine/controls/cuecontrol.h @@ -347,9 +347,9 @@ class CueControl : public EngineControl { // Tells us which controls map to which hotcue QMap m_controlMap; - TrackPointer m_pLoadedTrack; // is written from an engine worker thread // Must be locked when using the m_pLoadedTrack and it's properties QMutex m_trackMutex; + TrackPointer m_pLoadedTrack; // is written from an engine worker thread friend class HotcueControlTest; }; From 4114b160e8ca6bdf2a6e4aaf98948fed36304c13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Wed, 16 Dec 2020 10:01:39 +0100 Subject: [PATCH 19/20] swap if else logic --- src/library/rekordbox/rekordboxfeature.cpp | 7 +++---- src/mixer/basetrackplayer.cpp | 6 +++--- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/library/rekordbox/rekordboxfeature.cpp b/src/library/rekordbox/rekordboxfeature.cpp index 9d1a26dc3dc..4e78a0dc9d1 100644 --- a/src/library/rekordbox/rekordboxfeature.cpp +++ b/src/library/rekordbox/rekordboxfeature.cpp @@ -805,16 +805,15 @@ void setHotCue(TrackPointer track, type = mixxx::CueType::Loop; } - if (!pCue) { + if (pCue) { + pCue->setStartAndEndPosition(startPosition, endPosition); + } else { pCue = track->createAndAddCue( type, id, startPosition, endPosition); - } else { - pCue->setStartAndEndPosition(startPosition, endPosition); } - pCue->setLabel(label); if (color) { pCue->setColor(*color); diff --git a/src/mixer/basetrackplayer.cpp b/src/mixer/basetrackplayer.cpp index d830cad0227..b851e802d8f 100644 --- a/src/mixer/basetrackplayer.cpp +++ b/src/mixer/basetrackplayer.cpp @@ -324,14 +324,14 @@ TrackPointer BaseTrackPlayerImpl::unloadTrack() { break; } } - if (!pLoopCue) { + if (pLoopCue) { + pLoopCue->setStartAndEndPosition(loopStart, loopEnd); + } else { pLoopCue = m_pLoadedTrack->createAndAddCue( mixxx::CueType::Loop, Cue::kNoHotCue, loopStart, loopEnd); - } else { - pLoopCue->setStartAndEndPosition(loopStart, loopEnd); } } From ad9bddb32178d8bfd063ff8437664ba7ec5f45b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Wed, 16 Dec 2020 10:06:18 +0100 Subject: [PATCH 20/20] narrow locking scope --- src/track/track.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/track/track.cpp b/src/track/track.cpp index 78c9376fa94..665cbd8eb08 100644 --- a/src/track/track.cpp +++ b/src/track/track.cpp @@ -815,7 +815,6 @@ CuePointer Track::createAndAddCue( int hotCueIndex, double sampleStartPosition, double sampleEndPosition) { - QMutexLocker lock(&m_qMutex); CuePointer pCue(new Cue( type, hotCueIndex, @@ -829,6 +828,7 @@ CuePointer Track::createAndAddCue( &Cue::updated, this, &Track::slotCueUpdated); + QMutexLocker lock(&m_qMutex); m_cuePoints.push_back(pCue); markDirtyAndUnlock(&lock); emit cuesUpdated();