Don't use garbage Cues from Serato Tags or library.

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<mixxx::CueType>(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<mixxx::CueType>(type),
+            type,
             position,
             length,
             hotcue,
@@ -81,7 +91,7 @@ QList<CuePointer> CueDAO::getCuesForTrack(TrackId trackId) const {
     QMap<int, CuePointer> hotCuesByNumber;
     while (query.next()) {
         CuePointer pCue = cueFromRow(query.record());
-        VERIFY_OR_DEBUG_ASSERT(pCue) {
+        if (!pCue) {
             continue;
         }
         int hotCueNumber = pCue->getHotCue();

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));

src/track/cueinfo.cpp

@@ -4,6 +4,33 @@
 
 namespace mixxx {
 
+namespace {
+
+void assertEndPosition(
+        CueType type,
+        std::optional<double> 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<double> CueInfo::getStartPositionMillis() const {
 
 void CueInfo::setEndPositionMillis(std::optional<double> positionMillis) {
     m_endPositionMillis = positionMillis;
+    assertEndPosition(m_type, m_endPositionMillis);
 }
 
 std::optional<double> CueInfo::getEndPositionMillis() const {
@@ -58,8 +87,7 @@ std::optional<int> CueInfo::getHotCueIndex() const {
     return m_hotCueIndex;
 }
 
-void CueInfo::setHotCueIndex(const std::optional<int> hotCueIndex) {
-    DEBUG_ASSERT(!hotCueIndex || *hotCueIndex >= kFirstHotCueIndex);
+void CueInfo::setHotCueIndex(int hotCueIndex) {
     m_hotCueIndex = hotCueIndex;
 }
 

src/track/cueinfo.h

@@ -56,8 +56,7 @@ class CueInfo {
             std::optional<double> positionMillis = std::nullopt);
 
     std::optional<int> getHotCueIndex() const;
-    void setHotCueIndex(
-            const std::optional<int> hotCueIndex = std::nullopt);
+    void setHotCueIndex(int hotCueIndex);
 
     QString getLabel() const;
     void setLabel(

src/track/serato/tags.cpp

@@ -40,35 +40,26 @@ QString getPrimaryDecoderNameForFilePath(const QString& filePath) {
 constexpr int kFirstLoopIndex = mixxx::kFirstHotCueIndex + 8;
 constexpr int kNumCuesInMarkersTag = 5;
 
-std::optional<int> findIndexForCueInfo(const mixxx::CueInfo& cueInfo) {
-    VERIFY_OR_DEBUG_ASSERT(cueInfo.getHotCueIndex()) {
-        qWarning() << "SeratoTags::getCues: Cue without number found!";
-        return std::nullopt;
+bool isCueInfoValid(const mixxx::CueInfo& cueInfo) {
+    if (cueInfo.getType() == mixxx::CueType::Loop &&
+            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;
     }
-
-    int index = *cueInfo.getHotCueIndex();
-    VERIFY_OR_DEBUG_ASSERT(index >= mixxx::kFirstHotCueIndex) {
-        qWarning() << "SeratoTags::getCues: Cue with number < 0 found!";
-        return std::nullopt;
+    if (cueInfo.getType() == mixxx::CueType::HotCue &&
+            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";
+        return false;
     }
-
-    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;
+    VERIFY_OR_DEBUG_ASSERT(cueInfo.getHotCueIndex()) {
+        return false;
     }
-
-    return index;
+    return true;
 }
 
 } // namespace
@@ -200,70 +191,90 @@ QList<CueInfo> SeratoTags::getCueInfos() const {
     // "Serato Markers_" and "Serato Markers2" contradict each other,
     // Serato will use the values from "Serato Markers_").
 
-    QMap<int, CueInfo> cueMap;
+    QMap<int, CueInfo> hotCueMap;
+    QMap<int, CueInfo> loopCueMap;
     const QList<CueInfo> cuesMarkers2 = m_seratoMarkers2.getCues();
     for (const CueInfo& cueInfo : cuesMarkers2) {
-        std::optional<int> index = findIndexForCueInfo(cueInfo);
-        if (!index) {
+        if (!isCueInfoValid(cueInfo)) {
             continue;
         }
-
-        CueInfo newCueInfo(cueInfo);
-        newCueInfo.setHotCueIndex(index);
-        cueMap.insert(*index, newCueInfo);
+        if (cueInfo.getType() == CueType::Loop) {
+            loopCueMap.insert(*cueInfo.getHotCueIndex(), cueInfo);
+        } else {
+            hotCueMap.insert(*cueInfo.getHotCueIndex(), cueInfo);
+        }
     };
 
-    // 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<int> unsetCuesInMarkersTag;
-    for (int i = 0; i < kNumCuesInMarkersTag; i++) {
-        unsetCuesInMarkersTag.insert(i);
-    }
-
     const QList<CueInfo> cuesMarkers = m_seratoMarkers.getCues();
-    for (const CueInfo& cueInfo : cuesMarkers) {
-        std::optional<int> index = findIndexForCueInfo(cueInfo);
-        if (!index) {
-            continue;
+    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_",
+        // 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<int> 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);
+        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);
+        }
+    }
 
-        // This cue is set in the "Serato Markers_" tag, so remove it from the
-        // set of unset cues
-        unsetCuesInMarkersTag.remove(*index);
-    };
+    // 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);
+        }
+    }
 
-    // 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);
+    const QList<CueInfo> cueInfos = hotCueMap.values();
+    qDebug() << "SeratoTags::getCueInfos()";
+    for (const CueInfo& cueInfo : cueInfos) {
+        qDebug() << cueInfo;
     }
 
-    return cueMap.values();
+    return cueInfos;
 }
 
 void SeratoTags::setCueInfos(const QList<CueInfo>& cueInfos, double timingOffsetMillis) {
@@ -283,13 +294,13 @@ void SeratoTags::setCueInfos(const QList<CueInfo>& 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());
@@ -299,6 +310,11 @@ void SeratoTags::setCueInfos(const QList<CueInfo>& cueInfos, double timingOffset
             cueMap.insert(hotcueIndex, newCueInfo);
             break;
         case CueType::Loop:
+            if (!newCueInfo.getEndPositionMillis().has_value()) {
+                qWarning() << "Loop Cue" << hotcueIndex << "has no end position";
+                DEBUG_ASSERT(false);
+                continue;
+            }
             loopMap.insert(hotcueIndex, newCueInfo);
             break;
         default: