From 0ee7dbaeffd47ffe3de8a615e4c44e3aba3eaa11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sun, 9 Apr 2023 21:47:12 +0200 Subject: [PATCH 01/17] Add assertion for a valid cue end position depending on the type --- src/track/cueinfo.cpp | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/track/cueinfo.cpp b/src/track/cueinfo.cpp index 688ec28b512..0142d53015e 100644 --- a/src/track/cueinfo.cpp +++ b/src/track/cueinfo.cpp @@ -4,6 +4,33 @@ namespace mixxx { +namespace { + +inline void assertEndPosition( + CueType type, + std::optional endPositionMillis) { + switch (type) { + case CueType::HotCue: + case CueType::MainCue: + DEBUG_ASSERT(!endPositionMillis); + break; + case CueType::Loop: + case CueType::Jump: + case CueType::AudibleSound: + DEBUG_ASSERT(endPositionMillis); + break; + case CueType::Intro: + case CueType::Outro: + case CueType::Invalid: + break; + case CueType::Beat: // unused + default: + DEBUG_ASSERT(!"Unknown Loop Type"); + } +} + +} // namespace + CueInfo::CueInfo() : m_type(CueType::Invalid), m_startPositionMillis(std::nullopt), @@ -28,6 +55,7 @@ CueInfo::CueInfo( m_label(label), m_color(color), m_flags(flags) { + assertEndPosition(type, endPositionMillis); } CueType CueInfo::getType() const { @@ -48,6 +76,7 @@ std::optional CueInfo::getStartPositionMillis() const { void CueInfo::setEndPositionMillis(std::optional positionMillis) { m_endPositionMillis = positionMillis; + assertEndPosition(m_type, m_endPositionMillis); } std::optional CueInfo::getEndPositionMillis() const { From a3495014fb4de02fd3878077cd6d82c0d7366feb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sun, 9 Apr 2023 22:07:05 +0200 Subject: [PATCH 02/17] Fix test CueTest.ConvertCueInfoToCueRoundtrip --- src/test/cue_test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/cue_test.cpp b/src/test/cue_test.cpp index 8e8a34ab19d..fa9d4625b25 100644 --- a/src/test/cue_test.cpp +++ b/src/test/cue_test.cpp @@ -35,7 +35,7 @@ TEST(CueTest, ConvertCueInfoToCueRoundtrip) { const auto cueInfo1 = CueInfo( CueType::HotCue, std::make_optional(1.0 * 44100 * mixxx::kEngineChannelCount), - std::make_optional(2.0 * 44100 * mixxx::kEngineChannelCount), + std::nullopt, std::make_optional(3), QStringLiteral("label"), RgbColor::optional(0xABCDEF)); From be7c91ab6fb136c0effa41a648275a63b411e5fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sun, 9 Apr 2023 21:52:23 +0200 Subject: [PATCH 03/17] Issue a warning for more details in case the a loop cue has no end position --- src/track/serato/tags.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/track/serato/tags.cpp b/src/track/serato/tags.cpp index 26c510a0386..de16901de21 100644 --- a/src/track/serato/tags.cpp +++ b/src/track/serato/tags.cpp @@ -299,6 +299,11 @@ void SeratoTags::setCueInfos(const QList& cueInfos, double timingOffset cueMap.insert(hotcueIndex, newCueInfo); break; case CueType::Loop: + if (!newCueInfo.getEndPositionMillis()) { + qWarning() << "Loop Cue" << hotcueIndex << "has no end position"; + DEBUG_ASSERT(false); + continue; + } loopMap.insert(hotcueIndex, newCueInfo); break; default: From dc6afecd28a568836df117c91be3c4f37e5db28a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Tue, 11 Apr 2023 01:02:01 +0200 Subject: [PATCH 04/17] Discard garbage cues that have been introduced by issue #11283 --- src/library/dao/cuedao.cpp | 16 +++++++++++++--- src/track/serato/tags.cpp | 33 ++++++++++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/src/library/dao/cuedao.cpp b/src/library/dao/cuedao.cpp index f73f17767db..596cdd7f248 100644 --- a/src/library/dao/cuedao.cpp +++ b/src/library/dao/cuedao.cpp @@ -39,7 +39,7 @@ inline QString labelFromQVariant(const QVariant& value) { CuePointer cueFromRow(const QSqlRecord& row) { const auto id = DbId(row.value(row.indexOf("id"))); TrackId trackId(row.value(row.indexOf("track_id"))); - int type = row.value(row.indexOf("type")).toInt(); + auto type = static_cast(row.value(row.indexOf("type")).toInt()); double position = row.value(row.indexOf("position")).toDouble(); int length = row.value(row.indexOf("length")).toInt(); int hotcue = row.value(row.indexOf("hotcue")).toInt(); @@ -48,9 +48,19 @@ CuePointer cueFromRow(const QSqlRecord& row) { VERIFY_OR_DEBUG_ASSERT(color) { return CuePointer(); } + if (type == mixxx::CueType::Loop && length == 0) { + // These entries are likely added via issue #11283 + qWarning() << "Discard loop cue" << hotcue << "found in database with length of 0"; + return CuePointer(); + } + if (type == mixxx::CueType::HotCue && position == 0 && *color == mixxx::RgbColor(0)) { + // These entries are likely added via issue #11283 + qWarning() << "Discard black hot cue" << hotcue << "found in database at position 0"; + return CuePointer(); + } CuePointer pCue(new Cue(id, trackId, - static_cast(type), + type, position, length, hotcue, @@ -81,7 +91,7 @@ QList CueDAO::getCuesForTrack(TrackId trackId) const { QMap hotCuesByNumber; while (query.next()) { CuePointer pCue = cueFromRow(query.record()); - VERIFY_OR_DEBUG_ASSERT(pCue) { + if (!pCue) { continue; } int hotCueNumber = pCue->getHotCue(); diff --git a/src/track/serato/tags.cpp b/src/track/serato/tags.cpp index de16901de21..71905f31414 100644 --- a/src/track/serato/tags.cpp +++ b/src/track/serato/tags.cpp @@ -207,7 +207,22 @@ QList SeratoTags::getCueInfos() const { if (!index) { continue; } - + if (cueInfo.getType() == mixxx::CueType::Loop && + (!cueInfo.getEndPositionMillis() || + *cueInfo.getEndPositionMillis() == 0)) { + // These entries are likely added via issue #11283 + qWarning() << "Discard loop cue" << index << "found in markers2 with length of 0"; + continue; + } + if (cueInfo.getType() == mixxx::CueType::HotCue && + (!cueInfo.getStartPositionMillis() || + *cueInfo.getStartPositionMillis() == 0) && + (!cueInfo.getColor() || + *cueInfo.getColor() == mixxx::RgbColor(0))) { + // These entries are likely added via issue #11283 + qWarning() << "Discard black hot cue" << index << "found in markers2 at position 0"; + continue; + } CueInfo newCueInfo(cueInfo); newCueInfo.setHotCueIndex(index); cueMap.insert(*index, newCueInfo); @@ -237,6 +252,22 @@ QList SeratoTags::getCueInfos() const { if (!index) { continue; } + if (cueInfo.getType() == mixxx::CueType::Loop && + (!cueInfo.getEndPositionMillis() || + *cueInfo.getEndPositionMillis() == 0)) { + // These entries are likely added via issue #11283 + qWarning() << "Discard loop cue" << index << "found in markers with length of 0"; + continue; + } + if (cueInfo.getType() == mixxx::CueType::HotCue && + (!cueInfo.getStartPositionMillis() || + *cueInfo.getStartPositionMillis() == 0) && + (!cueInfo.getColor() || + *cueInfo.getColor() == mixxx::RgbColor(0))) { + // These entries are likely added via issue #11283 + qWarning() << "Discard black hot cue" << index << "found in markers at position 0"; + continue; + } // Take a pre-existing CueInfo object that was read from // "SeratoMarkers2" from the CueMap (or a default constructed CueInfo From ba071de6cc4bf4e06a2ff247a0dd684e2998e80f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Tue, 18 Apr 2023 07:18:52 +0200 Subject: [PATCH 05/17] Remove inline keyword from a function that is inlined anyway --- src/track/cueinfo.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/track/cueinfo.cpp b/src/track/cueinfo.cpp index 0142d53015e..dd5e9ce8e13 100644 --- a/src/track/cueinfo.cpp +++ b/src/track/cueinfo.cpp @@ -6,7 +6,7 @@ namespace mixxx { namespace { -inline void assertEndPosition( +void assertEndPosition( CueType type, std::optional endPositionMillis) { switch (type) { From f9b33c58a364e19122c9bcc96c4c0cca154d6b97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Wed, 26 Apr 2023 08:19:53 +0200 Subject: [PATCH 06/17] Introduce isCueInfoValid() to deduplicate code --- src/track/cueinfo.cpp | 3 +- src/track/cueinfo.h | 3 +- src/track/serato/tags.cpp | 72 ++++++++++++++++++--------------------- 3 files changed, 36 insertions(+), 42 deletions(-) diff --git a/src/track/cueinfo.cpp b/src/track/cueinfo.cpp index dd5e9ce8e13..595ffc524a9 100644 --- a/src/track/cueinfo.cpp +++ b/src/track/cueinfo.cpp @@ -87,8 +87,7 @@ std::optional CueInfo::getHotCueIndex() const { return m_hotCueIndex; } -void CueInfo::setHotCueIndex(const std::optional hotCueIndex) { - DEBUG_ASSERT(!hotCueIndex || *hotCueIndex >= kFirstHotCueIndex); +void CueInfo::setHotCueIndex(int hotCueIndex) { m_hotCueIndex = hotCueIndex; } diff --git a/src/track/cueinfo.h b/src/track/cueinfo.h index 17090e2965f..4ea48279b90 100644 --- a/src/track/cueinfo.h +++ b/src/track/cueinfo.h @@ -56,8 +56,7 @@ class CueInfo { std::optional positionMillis = std::nullopt); std::optional getHotCueIndex() const; - void setHotCueIndex( - const std::optional hotCueIndex = std::nullopt); + void setHotCueIndex(int hotCueIndex); QString getLabel() const; void setLabel( diff --git a/src/track/serato/tags.cpp b/src/track/serato/tags.cpp index 71905f31414..ffdd87f9711 100644 --- a/src/track/serato/tags.cpp +++ b/src/track/serato/tags.cpp @@ -71,6 +71,28 @@ std::optional findIndexForCueInfo(const mixxx::CueInfo& cueInfo) { return index; } +bool isCueInfoValid(const mixxx::CueInfo& cueInfo) { + if (cueInfo.getType() == mixxx::CueType::Loop && + (!cueInfo.getEndPositionMillis() || + *cueInfo.getEndPositionMillis() == 0)) { + // These entries are likely added via issue #11283 + qWarning() << "Discard loop cue" << cueInfo.getHotCueIndex() + << "found in markers2 with length of 0"; + return false; + } + if (cueInfo.getType() == mixxx::CueType::HotCue && + (!cueInfo.getStartPositionMillis() || + *cueInfo.getStartPositionMillis() == 0) && + (!cueInfo.getColor() || + *cueInfo.getColor() == mixxx::RgbColor(0))) { + // These entries are likely added via issue #11283 + qWarning() << "Discard black hot cue" << cueInfo.getHotCueIndex() + << "found in markers2 at position 0"; + return false; + } + return true; +} + } // namespace namespace mixxx { @@ -203,29 +225,16 @@ QList SeratoTags::getCueInfos() const { QMap cueMap; const QList cuesMarkers2 = m_seratoMarkers2.getCues(); for (const CueInfo& cueInfo : cuesMarkers2) { - std::optional index = findIndexForCueInfo(cueInfo); - if (!index) { + if (!isCueInfoValid(cueInfo)) { continue; } - if (cueInfo.getType() == mixxx::CueType::Loop && - (!cueInfo.getEndPositionMillis() || - *cueInfo.getEndPositionMillis() == 0)) { - // These entries are likely added via issue #11283 - qWarning() << "Discard loop cue" << index << "found in markers2 with length of 0"; - continue; - } - if (cueInfo.getType() == mixxx::CueType::HotCue && - (!cueInfo.getStartPositionMillis() || - *cueInfo.getStartPositionMillis() == 0) && - (!cueInfo.getColor() || - *cueInfo.getColor() == mixxx::RgbColor(0))) { - // These entries are likely added via issue #11283 - qWarning() << "Discard black hot cue" << index << "found in markers2 at position 0"; + std::optional pIndex = findIndexForCueInfo(cueInfo); + if (!pIndex) { continue; } CueInfo newCueInfo(cueInfo); - newCueInfo.setHotCueIndex(index); - cueMap.insert(*index, newCueInfo); + newCueInfo.setHotCueIndex(*pIndex); + cueMap.insert(*pIndex, newCueInfo); }; // If the "Serato Markers_" tag does not exist at all, Serato DJ Pro just @@ -248,24 +257,11 @@ QList SeratoTags::getCueInfos() const { const QList cuesMarkers = m_seratoMarkers.getCues(); for (const CueInfo& cueInfo : cuesMarkers) { - std::optional index = findIndexForCueInfo(cueInfo); - if (!index) { - continue; - } - if (cueInfo.getType() == mixxx::CueType::Loop && - (!cueInfo.getEndPositionMillis() || - *cueInfo.getEndPositionMillis() == 0)) { - // These entries are likely added via issue #11283 - qWarning() << "Discard loop cue" << index << "found in markers with length of 0"; + if (!isCueInfoValid(cueInfo)) { continue; } - if (cueInfo.getType() == mixxx::CueType::HotCue && - (!cueInfo.getStartPositionMillis() || - *cueInfo.getStartPositionMillis() == 0) && - (!cueInfo.getColor() || - *cueInfo.getColor() == mixxx::RgbColor(0))) { - // These entries are likely added via issue #11283 - qWarning() << "Discard black hot cue" << index << "found in markers at position 0"; + std::optional pIndex = findIndexForCueInfo(cueInfo); + if (!pIndex) { continue; } @@ -274,18 +270,18 @@ QList SeratoTags::getCueInfos() const { // object if none exists) and use it as template for the new CueInfo // object. Then overwrite all object values that are present in the // "SeratoMarkers_"tag. - CueInfo newCueInfo = cueMap.value(*index); + CueInfo newCueInfo = cueMap.value(*pIndex); newCueInfo.setType(cueInfo.getType()); newCueInfo.setStartPositionMillis(cueInfo.getStartPositionMillis()); newCueInfo.setEndPositionMillis(cueInfo.getEndPositionMillis()); - newCueInfo.setHotCueIndex(index); + newCueInfo.setHotCueIndex(*pIndex); newCueInfo.setFlags(cueInfo.flags()); newCueInfo.setColor(cueInfo.getColor()); - cueMap.insert(*index, newCueInfo); + cueMap.insert(*pIndex, newCueInfo); // This cue is set in the "Serato Markers_" tag, so remove it from the // set of unset cues - unsetCuesInMarkersTag.remove(*index); + unsetCuesInMarkersTag.remove(*pIndex); }; // Now that we know which cues should be present in the "Serato Markers_" From bf67fcf84b0497442396014c8d5111edc8db14da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Wed, 26 Apr 2023 21:41:39 +0200 Subject: [PATCH 07/17] Improve variable names and comments --- src/track/serato/tags.cpp | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/track/serato/tags.cpp b/src/track/serato/tags.cpp index ffdd87f9711..2126a29be44 100644 --- a/src/track/serato/tags.cpp +++ b/src/track/serato/tags.cpp @@ -77,7 +77,7 @@ bool isCueInfoValid(const mixxx::CueInfo& cueInfo) { *cueInfo.getEndPositionMillis() == 0)) { // These entries are likely added via issue #11283 qWarning() << "Discard loop cue" << cueInfo.getHotCueIndex() - << "found in markers2 with length of 0"; + << "with length of 0"; return false; } if (cueInfo.getType() == mixxx::CueType::HotCue && @@ -87,7 +87,7 @@ bool isCueInfoValid(const mixxx::CueInfo& cueInfo) { *cueInfo.getColor() == mixxx::RgbColor(0))) { // These entries are likely added via issue #11283 qWarning() << "Discard black hot cue" << cueInfo.getHotCueIndex() - << "found in markers2 at position 0"; + << "at position 0"; return false; } return true; @@ -228,13 +228,13 @@ QList SeratoTags::getCueInfos() const { if (!isCueInfoValid(cueInfo)) { continue; } - std::optional pIndex = findIndexForCueInfo(cueInfo); - if (!pIndex) { + std::optional index = findIndexForCueInfo(cueInfo); + if (!index) { continue; } CueInfo newCueInfo(cueInfo); - newCueInfo.setHotCueIndex(*pIndex); - cueMap.insert(*pIndex, newCueInfo); + newCueInfo.setHotCueIndex(*index); + cueMap.insert(*index, newCueInfo); }; // If the "Serato Markers_" tag does not exist at all, Serato DJ Pro just @@ -260,8 +260,8 @@ QList SeratoTags::getCueInfos() const { if (!isCueInfoValid(cueInfo)) { continue; } - std::optional pIndex = findIndexForCueInfo(cueInfo); - if (!pIndex) { + std::optional index = findIndexForCueInfo(cueInfo); + if (!index) { continue; } @@ -270,18 +270,18 @@ QList SeratoTags::getCueInfos() const { // object if none exists) and use it as template for the new CueInfo // object. Then overwrite all object values that are present in the // "SeratoMarkers_"tag. - CueInfo newCueInfo = cueMap.value(*pIndex); + CueInfo newCueInfo = cueMap.value(*index); newCueInfo.setType(cueInfo.getType()); newCueInfo.setStartPositionMillis(cueInfo.getStartPositionMillis()); newCueInfo.setEndPositionMillis(cueInfo.getEndPositionMillis()); - newCueInfo.setHotCueIndex(*pIndex); + newCueInfo.setHotCueIndex(*index); newCueInfo.setFlags(cueInfo.flags()); newCueInfo.setColor(cueInfo.getColor()); - cueMap.insert(*pIndex, newCueInfo); + cueMap.insert(*index, newCueInfo); // This cue is set in the "Serato Markers_" tag, so remove it from the // set of unset cues - unsetCuesInMarkersTag.remove(*pIndex); + unsetCuesInMarkersTag.remove(*index); }; // Now that we know which cues should be present in the "Serato Markers_" From 6244044d421e5d5d4d8476d4f36a44bdb2b9c170 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Wed, 26 Apr 2023 21:53:49 +0200 Subject: [PATCH 08/17] make use of value_or() --- src/track/serato/tags.cpp | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/track/serato/tags.cpp b/src/track/serato/tags.cpp index 2126a29be44..8f76827c991 100644 --- a/src/track/serato/tags.cpp +++ b/src/track/serato/tags.cpp @@ -73,18 +73,15 @@ std::optional findIndexForCueInfo(const mixxx::CueInfo& cueInfo) { bool isCueInfoValid(const mixxx::CueInfo& cueInfo) { if (cueInfo.getType() == mixxx::CueType::Loop && - (!cueInfo.getEndPositionMillis() || - *cueInfo.getEndPositionMillis() == 0)) { + cueInfo.getEndPositionMillis().value_or(0) == 0) { // These entries are likely added via issue #11283 qWarning() << "Discard loop cue" << cueInfo.getHotCueIndex() << "with length of 0"; return false; } if (cueInfo.getType() == mixxx::CueType::HotCue && - (!cueInfo.getStartPositionMillis() || - *cueInfo.getStartPositionMillis() == 0) && - (!cueInfo.getColor() || - *cueInfo.getColor() == mixxx::RgbColor(0))) { + cueInfo.getStartPositionMillis().value_or(0) == 0 && + cueInfo.getColor().value_or(mixxx::RgbColor(0)) == mixxx::RgbColor(0)) { // These entries are likely added via issue #11283 qWarning() << "Discard black hot cue" << cueInfo.getHotCueIndex() << "at position 0"; From a3067921747107a77928d205d3cda05948ba0b2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Wed, 26 Apr 2023 22:00:00 +0200 Subject: [PATCH 09/17] make use of has_value() --- src/track/serato/tags.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/track/serato/tags.cpp b/src/track/serato/tags.cpp index 8f76827c991..a6fc2870f71 100644 --- a/src/track/serato/tags.cpp +++ b/src/track/serato/tags.cpp @@ -307,13 +307,13 @@ void SeratoTags::setCueInfos(const QList& cueInfos, double timingOffset } CueInfo newCueInfo(cueInfo); - if (!cueInfo.getStartPositionMillis()) { + if (!cueInfo.getStartPositionMillis().has_value()) { continue; } newCueInfo.setStartPositionMillis( *cueInfo.getStartPositionMillis() - timingOffsetMillis); - if (cueInfo.getEndPositionMillis()) { + if (cueInfo.getEndPositionMillis().has_value()) { newCueInfo.setEndPositionMillis(*cueInfo.getEndPositionMillis() - timingOffsetMillis); } newCueInfo.setFlags(cueInfo.flags()); @@ -323,7 +323,7 @@ void SeratoTags::setCueInfos(const QList& cueInfos, double timingOffset cueMap.insert(hotcueIndex, newCueInfo); break; case CueType::Loop: - if (!newCueInfo.getEndPositionMillis()) { + if (!newCueInfo.getEndPositionMillis().has_value()) { qWarning() << "Loop Cue" << hotcueIndex << "has no end position"; DEBUG_ASSERT(false); continue; From 50a6f6e458eae1d9b2a382005d6ff8b52745ddad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sat, 29 Apr 2023 22:41:44 +0200 Subject: [PATCH 10/17] Add missing std::move --- 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 a8aa63f2650..32fc97e1779 100644 --- a/src/track/track.cpp +++ b/src/track/track.cpp @@ -1050,7 +1050,7 @@ Track::ImportStatus Track::importCueInfos( return ImportStatus::Complete; } DEBUG_ASSERT(!m_pCueInfoImporterPending); - m_pCueInfoImporterPending = pCueInfoImporter; + m_pCueInfoImporterPending = std::move(pCueInfoImporter); if (m_pCueInfoImporterPending->isEmpty()) { // Just return the current import status without clearing any // existing cue points. From 2e2dfc5dd65ee2ba239911dfe2f0aa236c4c0600 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sat, 29 Apr 2023 22:48:00 +0200 Subject: [PATCH 11/17] Remove and assert the not existing isEmpty() case --- src/track/track.cpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/track/track.cpp b/src/track/track.cpp index 32fc97e1779..66808f09533 100644 --- a/src/track/track.cpp +++ b/src/track/track.cpp @@ -1046,17 +1046,14 @@ bool Track::tryImportPendingBeatsMarkDirtyAndUnlock( Track::ImportStatus Track::importCueInfos( mixxx::CueInfoImporterPointer pCueInfoImporter) { QMutexLocker lock(&m_qMutex); - VERIFY_OR_DEBUG_ASSERT(pCueInfoImporter) { + VERIFY_OR_DEBUG_ASSERT(pCueInfoImporter && !pCueInfoImporter->isEmpty()) { + // Just return the current import status without clearing any + // existing cue points. return ImportStatus::Complete; } DEBUG_ASSERT(!m_pCueInfoImporterPending); m_pCueInfoImporterPending = std::move(pCueInfoImporter); - if (m_pCueInfoImporterPending->isEmpty()) { - // Just return the current import status without clearing any - // existing cue points. - m_pCueInfoImporterPending.reset(); - return ImportStatus::Complete; - } else if (m_record.hasStreamInfoFromSource()) { + if (m_record.hasStreamInfoFromSource()) { // Replace existing cue points with imported cue // points immediately importPendingCueInfosMarkDirtyAndUnlock(&lock); From 2ffed467ea6698dfac9c456e92442354b3bb757a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sat, 29 Apr 2023 23:07:34 +0200 Subject: [PATCH 12/17] Only remove cues early, that are part or the pending importer --- src/track/track.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/track/track.cpp b/src/track/track.cpp index 66808f09533..295b5824fb4 100644 --- a/src/track/track.cpp +++ b/src/track/track.cpp @@ -1065,9 +1065,15 @@ Track::ImportStatus Track::importCueInfos( << "cue(s) is pending until the actual sample rate becomes available"; // Clear all existing cue points, that are supposed // to be replaced with the imported cue points soon. + QList cuePoints; + for (const CuePointer& pCue : qAsConst(m_cuePoints)) { + if (!m_pCueInfoImporterPending->hasCueOfType(pCue->getType())) { + cuePoints.append(pCue); + } + } setCuePointsMarkDirtyAndUnlock( &lock, - QList{}); + cuePoints); return ImportStatus::Pending; } } @@ -1084,11 +1090,6 @@ bool Track::setCuePointsWhileLocked(const QList& cuePoints) { // Nothing to do return false; } - // Prevent inconsistencies between cue infos that have been queued - // and are waiting to be imported and new cue points. At least one - // of these two collections must be empty. - DEBUG_ASSERT(cuePoints.isEmpty() || !m_pCueInfoImporterPending || - m_pCueInfoImporterPending->isEmpty()); // disconnect existing cue points for (const auto& pCue : qAsConst(m_cuePoints)) { disconnect(pCue.get(), nullptr, this, nullptr); @@ -1611,7 +1612,6 @@ void Track::updateStreamInfoFromSource( auto cuesImported = false; if (importCueInfos) { - DEBUG_ASSERT(m_cuePoints.isEmpty()); kLogger.debug() << "Finishing deferred import of" << m_pCueInfoImporterPending->size() From c7f7ec188c4c2e42d6af20a0c023aa156f804ab1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sat, 29 Apr 2023 23:13:58 +0200 Subject: [PATCH 13/17] reserve memory before append in a loop --- src/track/track.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/track/track.cpp b/src/track/track.cpp index 295b5824fb4..8a0953bfad0 100644 --- a/src/track/track.cpp +++ b/src/track/track.cpp @@ -1066,6 +1066,7 @@ Track::ImportStatus Track::importCueInfos( // Clear all existing cue points, that are supposed // to be replaced with the imported cue points soon. QList cuePoints; + cuePoints.reserve(m_cuePoints.size()); for (const CuePointer& pCue : qAsConst(m_cuePoints)) { if (!m_pCueInfoImporterPending->hasCueOfType(pCue->getType())) { cuePoints.append(pCue); @@ -1398,6 +1399,7 @@ bool Track::exportSeratoMetadata() { const mixxx::audio::SampleRate sampleRate = streamInfo->getSignalInfo().getSampleRate(); QList cueInfos; + cueInfos.reserve(m_cuePoints.size()); for (const CuePointer& pCue : qAsConst(m_cuePoints)) { cueInfos.append(pCue->getCueInfo(sampleRate)); } From 383f6820985d18599eb89a9a6f10c46107e3379c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sun, 30 Apr 2023 01:21:35 +0200 Subject: [PATCH 14/17] Make sure Loop cue Indexes are pereserved in a file metadata roundtrip --- src/track/serato/tags.cpp | 153 +++++++++++++++++--------------------- 1 file changed, 67 insertions(+), 86 deletions(-) diff --git a/src/track/serato/tags.cpp b/src/track/serato/tags.cpp index a6fc2870f71..968abc87568 100644 --- a/src/track/serato/tags.cpp +++ b/src/track/serato/tags.cpp @@ -40,36 +40,6 @@ QString getPrimaryDecoderNameForFilePath(const QString& filePath) { constexpr int kFirstLoopIndex = mixxx::kFirstHotCueIndex + 8; constexpr int kNumCuesInMarkersTag = 5; -std::optional findIndexForCueInfo(const mixxx::CueInfo& cueInfo) { - VERIFY_OR_DEBUG_ASSERT(cueInfo.getHotCueIndex()) { - qWarning() << "SeratoTags::getCues: Cue without number found!"; - return std::nullopt; - } - - int index = *cueInfo.getHotCueIndex(); - VERIFY_OR_DEBUG_ASSERT(index >= mixxx::kFirstHotCueIndex) { - qWarning() << "SeratoTags::getCues: Cue with number < 0 found!"; - return std::nullopt; - } - - switch (cueInfo.getType()) { - case mixxx::CueType::HotCue: - if (index >= kFirstLoopIndex) { - qWarning() - << "SeratoTags::getCues: Non-loop Cue with number >=" - << kFirstLoopIndex << "found!"; - return std::nullopt; - } - break; - case mixxx::CueType::Loop: - index += kFirstLoopIndex; - break; - default: - return std::nullopt; - } - - return index; -} bool isCueInfoValid(const mixxx::CueInfo& cueInfo) { if (cueInfo.getType() == mixxx::CueType::Loop && @@ -87,6 +57,9 @@ bool isCueInfoValid(const mixxx::CueInfo& cueInfo) { << "at position 0"; return false; } + VERIFY_OR_DEBUG_ASSERT(cueInfo.getHotCueIndex()) { + return false; + } return true; } @@ -219,75 +192,83 @@ QList SeratoTags::getCueInfos() const { // "Serato Markers_" and "Serato Markers2" contradict each other, // Serato will use the values from "Serato Markers_"). - QMap cueMap; + QMap hotCueMap; + QMap loopCueMap; const QList cuesMarkers2 = m_seratoMarkers2.getCues(); for (const CueInfo& cueInfo : cuesMarkers2) { if (!isCueInfoValid(cueInfo)) { continue; } - std::optional index = findIndexForCueInfo(cueInfo); - if (!index) { - continue; + if (cueInfo.getType() == CueType::Loop) { + loopCueMap.insert(*cueInfo.getHotCueIndex(), cueInfo); + } else { + hotCueMap.insert(*cueInfo.getHotCueIndex(), cueInfo); } - CueInfo newCueInfo(cueInfo); - newCueInfo.setHotCueIndex(*index); - cueMap.insert(*index, newCueInfo); }; - // If the "Serato Markers_" tag does not exist at all, Serato DJ Pro just - // takes data from the "Serato Markers2" tag, so we can exit early - // here. If the "Serato Markers_" exists, its data will take precedence. - if (m_seratoMarkers.isEmpty()) { - return cueMap.values(); - } - - // The "Serato Markers_" tag always contains entries for the first five - // cues. If a cue is not set, that entry is present but empty. - // If a cue is set in "Serato Markers2" but not in "Serato Markers_", - // Serato DJ Pro considers it as "not set" and ignores it. - // To mirror the behaviour of Serato, we need to remove from the output of - // this function. - QSet unsetCuesInMarkersTag; - for (int i = 0; i < kNumCuesInMarkersTag; i++) { - unsetCuesInMarkersTag.insert(i); - } - const QList cuesMarkers = m_seratoMarkers.getCues(); - for (const CueInfo& cueInfo : cuesMarkers) { - if (!isCueInfoValid(cueInfo)) { - continue; - } - std::optional index = findIndexForCueInfo(cueInfo); - if (!index) { - continue; + if (cuesMarkers.size()) { + // The "Serato Markers_" tag always contains entries for the first five + // cues. If a cue is not set, that entry is present but empty. + // If a cue is set in "Serato Markers2" but not in "Serato Markers_", + // Serato DJ Pro considers it as "not set" and ignores it. + // To mirror the behaviour of Serato, we need to remove from the output of + // this function. + QSet unsetCuesInMarkersTag; + unsetCuesInMarkersTag.reserve(kNumCuesInMarkersTag); + for (int i = 0; i < kNumCuesInMarkersTag; i++) { + unsetCuesInMarkersTag.insert(i); } - // Take a pre-existing CueInfo object that was read from - // "SeratoMarkers2" from the CueMap (or a default constructed CueInfo - // object if none exists) and use it as template for the new CueInfo - // object. Then overwrite all object values that are present in the - // "SeratoMarkers_"tag. - CueInfo newCueInfo = cueMap.value(*index); - newCueInfo.setType(cueInfo.getType()); - newCueInfo.setStartPositionMillis(cueInfo.getStartPositionMillis()); - newCueInfo.setEndPositionMillis(cueInfo.getEndPositionMillis()); - newCueInfo.setHotCueIndex(*index); - newCueInfo.setFlags(cueInfo.flags()); - newCueInfo.setColor(cueInfo.getColor()); - cueMap.insert(*index, newCueInfo); - - // This cue is set in the "Serato Markers_" tag, so remove it from the - // set of unset cues - unsetCuesInMarkersTag.remove(*index); - }; - - // Now that we know which cues should be present in the "Serato Markers_" - // tag but aren't, remove them from the set. - for (const int index : unsetCuesInMarkersTag) { - cueMap.remove(index); + for (const CueInfo& cueInfo : cuesMarkers) { + if (!isCueInfoValid(cueInfo)) { + continue; + } + // Take a pre-existing CueInfo object that was read from + // "SeratoMarkers2" from the CueMap (or a default constructed CueInfo + // object if none exists) and use it as template for the new CueInfo + // object. Then overwrite all object values that are present in the + // "SeratoMarkers_"tag. + CueInfo& newCueInfo = cueInfo.getType() == CueType::Loop + ? loopCueMap[*cueInfo.getHotCueIndex()] + : hotCueMap[*cueInfo.getHotCueIndex()]; + newCueInfo.setType(cueInfo.getType()); + newCueInfo.setStartPositionMillis(cueInfo.getStartPositionMillis()); + newCueInfo.setEndPositionMillis(cueInfo.getEndPositionMillis()); + newCueInfo.setHotCueIndex(*cueInfo.getHotCueIndex()); + newCueInfo.setFlags(cueInfo.flags()); + newCueInfo.setColor(cueInfo.getColor()); + + // This cue is set in the "Serato Markers_" tag, so remove it from the + // set of unset cues + unsetCuesInMarkersTag.remove(*cueInfo.getHotCueIndex()); + }; + + // Now that we know which cues should be present in the "Serato Markers_" + // tag but aren't, remove them from the set. + for (const int index : unsetCuesInMarkersTag) { + hotCueMap.remove(index); + } } - return cueMap.values(); + // Serato has a separate indexing for loop and hot cues. + // We sort the Loops Cues into the HotCue map if the given index is free, + // add an offset of 8 otherwise or loop until we find a free index. + // Cues above index 8 are not accessible in Mixxx, only visible in waveforms + // During export the Mixxx indexes are kept to allow a perfect round-trip. + for (const CueInfo& loopCueInfo : qAsConst(loopCueMap)) { + if (hotCueMap.contains(*loopCueInfo.getHotCueIndex())) { + CueInfo cueInfo = loopCueInfo; + cueInfo.setHotCueIndex(*loopCueInfo.getHotCueIndex() + kFirstLoopIndex); + while (hotCueMap.contains(*cueInfo.getHotCueIndex())) { + cueInfo.setHotCueIndex(*cueInfo.getHotCueIndex() + 1); + } + hotCueMap.insert(*cueInfo.getHotCueIndex(), cueInfo); + } else { + hotCueMap.insert(*loopCueInfo.getHotCueIndex(), loopCueInfo); + } + } + return hotCueMap.values(); } void SeratoTags::setCueInfos(const QList& cueInfos, double timingOffsetMillis) { From eed65dc531f2132724c277c1a9aab31279842fd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sun, 30 Apr 2023 01:31:55 +0200 Subject: [PATCH 15/17] Add debug output to: SeratoTags::getCueInfos() --- src/track/serato/tags.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/track/serato/tags.cpp b/src/track/serato/tags.cpp index 968abc87568..30e5e3dacae 100644 --- a/src/track/serato/tags.cpp +++ b/src/track/serato/tags.cpp @@ -268,7 +268,14 @@ QList SeratoTags::getCueInfos() const { hotCueMap.insert(*loopCueInfo.getHotCueIndex(), loopCueInfo); } } - return hotCueMap.values(); + + const QList cueInfos = hotCueMap.values(); + qDebug() << "SeratoTags::getCueInfos()"; + for (const CueInfo& cueInfo : cueInfos) { + qDebug() << cueInfo; + } + + return cueInfos; } void SeratoTags::setCueInfos(const QList& cueInfos, double timingOffsetMillis) { From 95d876dd56b60a9626fdb7350459156e96c6920a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sun, 30 Apr 2023 02:05:15 +0200 Subject: [PATCH 16/17] Fix SeratoTagsTest.MarkersParseDumpRoundtrip test --- .../mp3/markers_/very-long-names.octet-stream | Bin 318 -> 318 bytes src/track/serato/tags.cpp | 1 - 2 files changed, 1 deletion(-) diff --git a/src/test/serato/data/mp3/markers_/very-long-names.octet-stream b/src/test/serato/data/mp3/markers_/very-long-names.octet-stream index 92e7638c89e0a95f3a7ad030896824332c291f3b..072eb6f898937fcbf51c9be5451b52c3bed8cd8e 100644 GIT binary patch literal 318 zcmZQ#Wnf_717Ze-dJuq+Yz7Prj9@_&Q6m8?qCk~CSkxLzU{lKs5(Qg?WE;pLW}1mY YyurXApa!vmQ7$x*874@9C_B&x0K=0{3;+NC delta 25 ecmdnTw2x^*$;3wylld5RKzQN;F&6gv`g#C+Y6!ak diff --git a/src/track/serato/tags.cpp b/src/track/serato/tags.cpp index 30e5e3dacae..81c6d2f14f7 100644 --- a/src/track/serato/tags.cpp +++ b/src/track/serato/tags.cpp @@ -40,7 +40,6 @@ QString getPrimaryDecoderNameForFilePath(const QString& filePath) { constexpr int kFirstLoopIndex = mixxx::kFirstHotCueIndex + 8; constexpr int kNumCuesInMarkersTag = 5; - bool isCueInfoValid(const mixxx::CueInfo& cueInfo) { if (cueInfo.getType() == mixxx::CueType::Loop && cueInfo.getEndPositionMillis().value_or(0) == 0) { From 452b04af43fc3f001b77ae5ef2737edabf37dcc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Mon, 1 May 2023 21:55:02 +0200 Subject: [PATCH 17/17] check size() for > 0 --- src/track/serato/tags.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/track/serato/tags.cpp b/src/track/serato/tags.cpp index 81c6d2f14f7..588f03cd9fa 100644 --- a/src/track/serato/tags.cpp +++ b/src/track/serato/tags.cpp @@ -206,7 +206,7 @@ QList SeratoTags::getCueInfos() const { }; const QList cuesMarkers = m_seratoMarkers.getCues(); - if (cuesMarkers.size()) { + if (cuesMarkers.size() > 0) { // The "Serato Markers_" tag always contains entries for the first five // cues. If a cue is not set, that entry is present but empty. // If a cue is set in "Serato Markers2" but not in "Serato Markers_",