Merge pull request #3532 from Holzhaus/serato-markers-roundtrip-fixes

Serato Markers Roundtrip Fixes

8 files changed, 205 insertions, 22 deletions

diff --git a/src/test/seratotagstest.cpp b/src/test/seratotagstest.cpp
index 4b7963e3a3..f9356ff1e3 100644
--- a/src/test/seratotagstest.cpp
+++ b/src/test/seratotagstest.cpp
@@ -1,8 +1,24 @@
 #include <gtest/gtest.h>
 
+#include <QDir>
+#include <QFile>
+
 #include "track/serato/tags.h"
 #include "util/color/predefinedcolorpalettes.h"
 
+namespace {
+
+bool dumpToFile(const QString& filename, const QByteArray& data) {
+    QFile outfile(filename);
+    const bool openOk = outfile.open(QIODevice::WriteOnly);
+    if (!openOk) {
+        return false;
+    }
+    return data.size() == outfile.write(data);
+}
+
+} // namespace
+
 class SeratoTagsTest : public testing::Test {
   protected:
     void trackColorRoundtrip(mixxx::RgbColor::optional_t displayedColor) {
@@ -181,3 +197,71 @@ TEST_F(SeratoTagsTest, CueColorConversionRoundtrip) {
         EXPECT_EQ(color, displayedColor);
     }
 }
+
+TEST_F(SeratoTagsTest, MarkersParseDumpRoundtrip) {
+    const auto filetype = mixxx::taglib::FileType::MP3;
+    QDir dir(QStringLiteral("src/test/serato/data/mp3/markers_/"));
+    dir.setFilter(QDir::Files);
+    dir.setNameFilters(QStringList() << "*.octet-stream");
+    const QFileInfoList fileList = dir.entryInfoList();
+    for (const QFileInfo& fileInfo : fileList) {
+        mixxx::SeratoTags seratoTags;
+        EXPECT_TRUE(seratoTags.getCueInfos().isEmpty());
+
+        auto file = QFile(fileInfo.filePath());
+        const bool openOk = file.open(QIODevice::ReadOnly);
+        EXPECT_TRUE(openOk);
+        const QByteArray inputData = file.readAll();
+        const bool parseOk = seratoTags.parseMarkers(inputData, filetype);
+        EXPECT_TRUE(parseOk);
+
+        const auto trackColor = seratoTags.getTrackColor();
+        const auto cueInfos = seratoTags.getCueInfos();
+        seratoTags.setTrackColor(trackColor);
+        seratoTags.setCueInfos(cueInfos);
+
+        const QByteArray outputData = seratoTags.dumpMarkers(filetype);
+        EXPECT_EQ(inputData, outputData);
+        if (inputData != outputData) {
+            qWarning() << "parsed" << cueInfos;
+            EXPECT_TRUE(dumpToFile(
+                    file.fileName() + QStringLiteral(".seratotagstest.actual"),
+                    outputData));
+        }
+    }
+}
+
+TEST_F(SeratoTagsTest, Markers2RoundTrip) {
+    const auto filetype = mixxx::taglib::FileType::MP3;
+    QDir dir(QStringLiteral("src/test/serato/data/mp3/markers2/"));
+    dir.setFilter(QDir::Files);
+    dir.setNameFilters(QStringList() << "*.octet-stream");
+    const QFileInfoList fileList = dir.entryInfoList();
+    for (const QFileInfo& fileInfo : fileList) {
+        mixxx::SeratoTags seratoTags;
+        EXPECT_TRUE(seratoTags.getCueInfos().isEmpty());
+
+        auto file = QFile(fileInfo.filePath());
+        const bool openOk = file.open(QIODevice::ReadOnly);
+        EXPECT_TRUE(openOk);
+        const QByteArray inputData = file.readAll();
+        const bool parseOk = seratoTags.parseMarkers2(inputData, filetype);
+        EXPECT_TRUE(parseOk);
+
+        const auto bpmLocked = seratoTags.isBpmLocked();
+        const auto trackColor = seratoTags.getTrackColor();
+        const auto cueInfos = seratoTags.getCueInfos();
+        seratoTags.setBpmLocked(bpmLocked);
+        seratoTags.setTrackColor(trackColor);
+        seratoTags.setCueInfos(cueInfos);
+
+        const QByteArray outputData = seratoTags.dumpMarkers2(filetype);
+        EXPECT_EQ(inputData, outputData);
+        if (inputData != outputData) {
+            qWarning() << "parsed" << cueInfos;
+            EXPECT_TRUE(dumpToFile(
+                    file.fileName() + QStringLiteral(".seratotagstest.actual"),
+                    outputData));
+        }
+    }
+}
diff --git a/src/track/cueinfo.cpp b/src/track/cueinfo.cpp
index 0d678ade6a..688ec28b51 100644
--- a/src/track/cueinfo.cpp
+++ b/src/track/cueinfo.cpp
@@ -9,7 +9,8 @@ CueInfo::CueInfo()
           m_startPositionMillis(std::nullopt),
           m_endPositionMillis(std::nullopt),
           m_hotCueIndex(std::nullopt),
-          m_color(std::nullopt) {
+          m_color(std::nullopt),
+          m_flags(CueFlag::None) {
 }
 
 CueInfo::CueInfo(
@@ -18,13 +19,15 @@ CueInfo::CueInfo(
         std::optional<double> endPositionMillis,
         std::optional<int> hotCueIndex,
         const QString& label,
-        mixxx::RgbColor::optional_t color)
+        mixxx::RgbColor::optional_t color,
+        CueFlags flags)
         : m_type(type),
           m_startPositionMillis(startPositionMillis),
           m_endPositionMillis(endPositionMillis),
           m_hotCueIndex(hotCueIndex),
           m_label(label),
-          m_color(color) {
+          m_color(color),
+          m_flags(flags) {
 }
 
 CueType CueInfo::getType() const {
@@ -129,6 +132,7 @@ QDebug operator<<(QDebug debug, const CueInfo& cueInfo) {
             << ", index=" << cueInfo.getHotCueIndex()
             << ", label=" << cueInfo.getLabel()
             << ", color=" << cueInfo.getColor()
+            << ", flags=" << cueInfo.flags()
             << "]";
     return debug;
 }
diff --git a/src/track/cueinfo.h b/src/track/cueinfo.h
index e027a9e0f5..17090e2965 100644
--- a/src/track/cueinfo.h
+++ b/src/track/cueinfo.h
@@ -1,6 +1,6 @@
 #pragma once
-// cueinfo.h
-// Created 2020-02-28 by Jan Holthuis
+
+#include <QFlags>
 
 #include "audio/signalinfo.h"
 #include "util/color/rgbcolor.h"
@@ -21,6 +21,14 @@ enum class CueType {
                       // sound; not shown to user
 };
 
+enum class CueFlag {
+    None = 0,
+    /// Currently only used when importing locked loops from Serato Metadata.
+    Locked = 1,
+};
+Q_DECLARE_FLAGS(CueFlags, CueFlag);
+Q_DECLARE_OPERATORS_FOR_FLAGS(CueFlags);
+
 /// Hot cues are sequentially indexed starting with kFirstHotCueIndex (inclusive)
 static constexpr int kFirstHotCueIndex = 0;
 
@@ -33,7 +41,8 @@ class CueInfo {
             std::optional<double> endPositionMillis,
             const std::optional<int> hotCueIndex,
             const QString& label,
-            RgbColor::optional_t color);
+            RgbColor::optional_t color,
+            CueFlags flags = CueFlag::None);
 
     CueType getType() const;
     void setType(CueType type);
@@ -58,6 +67,23 @@ class CueInfo {
     void setColor(
             mixxx::RgbColor::optional_t color = std::nullopt);
 
+    CueFlags flags() const {
+        return m_flags;
+    }
+
+    /// Set flags for the cue.
+    ///
+    /// These flags are currently only set during Serato cue import and *not*
+    /// saved in the Database (only used for roundtrip testing purposes).
+    void setFlags(CueFlags flags) {
+        m_flags = flags;
+    }
+
+    /// Checks if the `CueFlag::Locked` flag is set for this cue.
+    bool isLocked() const {
+        return m_flags.testFlag(CueFlag::Locked);
+    }
+
   private:
     CueType m_type;
     std::optional<double> m_startPositionMillis;
@@ -65,6 +91,7 @@ class CueInfo {
     std::optional<int> m_hotCueIndex;
     QString m_label;
     RgbColor::optional_t m_color;
+    CueFlags m_flags;
 };
 
 bool operator==(
diff --git a/src/track/serato/markers.cpp b/src/track/serato/markers.cpp
index 0714446365..eda52ff80a 100644
--- a/src/track/serato/markers.cpp
+++ b/src/track/serato/markers.cpp
@@ -148,6 +148,17 @@ SeratoMarkersEntryPointer SeratoMarkersEntry::parseID3(const QByteArray& data) {
 
     stream >> colorSerato32 >> type >> isLocked;
 
+    if (isLocked && type != static_cast<quint8>(TypeId::Loop)) {
+        // This should never happen with Metadata exported from Serato. We fail
+        // here because this case is unexpected and should not be corrected
+        // silently without knowing what we do. Write a bug if this assumption
+        // is wrong.
+        kLogger.warning() << "Parsing SeratoMarkersEntry failed:"
+                          << "isLocked field is not false for non-loop type"
+                          << static_cast<quint8>(TypeId::Loop);
+        return nullptr;
+    }
+
     const RgbColor color = RgbColor(serato32toUint24(colorSerato32));
 
     // Parse Start Position
@@ -156,6 +167,10 @@ SeratoMarkersEntryPointer SeratoMarkersEntry::parseID3(const QByteArray& data) {
     if (!hasStartPosition) {
         // Start position not set
         if (startPositionSerato32 != kNoPosition) {
+            // This should never happen with Metadata exported from Serato. We fail
+            // here because this case is unexpected and should not be corrected
+            // silently without knowing what we do. Write a bug if this assumption
+            // is wrong.
             kLogger.warning() << "Parsing SeratoMarkersEntry failed:"
                               << "startPosition"
                               << startPosition
@@ -174,6 +189,10 @@ SeratoMarkersEntryPointer SeratoMarkersEntry::parseID3(const QByteArray& data) {
     if (!hasEndPosition) {
         // End position not set
         if (endPositionSerato32 != kNoPosition) {
+            // This should never happen with Metadata exported from Serato. We fail
+            // here because this case is unexpected and should not be corrected
+            // silently without knowing what we do. Write a bug if this assumption
+            // is wrong.
             kLogger.warning() << "Parsing SeratoMarkersEntry failed:"
                               << "endPosition"
                               << endPositionSerato32
@@ -188,6 +207,10 @@ SeratoMarkersEntryPointer SeratoMarkersEntry::parseID3(const QByteArray& data) {
 
     // Make sure that the unknown (and probably unused) bytes have the expected value
     if (strncmp(buffer, "\x00\x7F\x7F\x7F\x7F\x7F", sizeof(buffer)) != 0) {
+        // This should never happen with Metadata exported from Serato. We fail
+        // here because this case is unexpected and should not be corrected
+        // silently without knowing what we do. Write a bug if this assumption
+        // is wrong.
         kLogger.warning() << "Parsing SeratoMarkersEntry failed:"
                           << "Unexpected value at offset 10"
                           << QByteArray::fromRawData(buffer, sizeof(buffer));
@@ -246,10 +269,26 @@ SeratoMarkersEntryPointer SeratoMarkersEntry::parseMP4(const QByteArray& data) {
     }
 
     stream >> colorRed >> colorGreen >> colorBlue >> type >> isLocked;
+
+    if (isLocked && type != static_cast<quint8>(TypeId::Loop)) {
+        // This should never happen with Metadata exported from Serato. We fail
+        // here because this case is unexpected and should not be corrected
+        // silently without knowing what we do. Write a bug if this assumption
+        // is wrong.
+        kLogger.warning() << "Parsing SeratoMarkersEntry failed:"
+                          << "isLocked field is not false for non-loop type"
+                          << static_cast<quint8>(TypeId::Loop);
+        return nullptr;
+    }
+
     const RgbColor color = RgbColor(qRgb(colorRed, colorGreen, colorBlue));
 
     // Make sure that the unknown (and probably unused) bytes have the expected value
     if (strncmp(buffer, "\x00\xFF\xFF\xFF\xFF\x00", sizeof(buffer)) != 0) {
+        // This should never happen with Metadata exported from Serato. We fail
+        // here because this case is unexpected and should not be corrected
+        // silently without knowing what we do. Write a bug if this assumption
+        // is wrong.
         kLogger.warning() << "Parsing SeratoMarkersEntry (MP4) failed:"
                           << "Unexpected value at offset 8"
                           << QByteArray::fromRawData(buffer, sizeof(buffer));
@@ -344,6 +383,10 @@ bool SeratoMarkers::parseID3(
 
         if (i < kNumCueEntries &&
                 pEntry->typeId() != SeratoMarkersEntry::TypeId::Cue) {
+            // This should never happen with Metadata exported from Serato. We fail
+            // here because this case is unexpected and should not be corrected
+            // silently without knowing what we do. Write a bug if this assumption
+            // is wrong.
             kLogger.warning() << "Parsing SeratoMarkers_ failed:"
                               << "Expected cue entry but found type" << pEntry->type();
             return false;
@@ -351,6 +394,10 @@ bool SeratoMarkers::parseID3(
 
         if (i >= kNumCueEntries &&
                 pEntry->typeId() != SeratoMarkersEntry::TypeId::Loop) {
+            // This should never happen with Metadata exported from Serato. We fail
+            // here because this case is unexpected and should not be corrected
+            // silently without knowing what we do. Write a bug if this assumption
+            // is wrong.
             kLogger.warning() << "Parsing SeratoMarkers_ failed:"
                               << "Expected loop entry but found type"
                               << pEntry->type();
@@ -437,6 +484,10 @@ bool SeratoMarkers::parseMP4(
 
         if (i < kNumCueEntries &&
                 pEntry->typeId() != SeratoMarkersEntry::TypeId::Cue) {
+            // This should never happen with Metadata exported from Serato. We fail
+            // here because this case is unexpected and should not be corrected
+            // silently without knowing what we do. Write a bug if this assumption
+            // is wrong.
             kLogger.warning() << "Parsing SeratoMarkers_ (MP4) failed:"
                               << "Expected cue entry but found type" << pEntry->type();
             return false;
@@ -444,6 +495,10 @@ bool SeratoMarkers::parseMP4(
 
         if (i >= kNumCueEntries &&
                 pEntry->typeId() != SeratoMarkersEntry::TypeId::Loop) {
+            // This should never happen with Metadata exported from Serato. We fail
+            // here because this case is unexpected and should not be corrected
+            // silently without knowing what we do. Write a bug if this assumption
+            // is wrong.
             kLogger.warning() << "Parsing SeratoMarkers_ (MP4) failed:"
                               << "Expected loop entry but found type"
                               << pEntry->type();
@@ -461,6 +516,10 @@ bool SeratoMarkers::parseMP4(
     RgbColor trackColor = RgbColor(qRgb(colorRed, colorGreen, colorBlue));
 
     if (field1 != 0x00) {
+        // This should never happen with Metadata exported from Serato. We fail
+        // here because this case is unexpected and should not be corrected
+        // silently without knowing what we do. Write a bug if this assumption
+        // is wrong.
         kLogger.warning() << "Parsing SeratoMarkers_ (MP4) failed:"
                           << "Unexpected value before track color"
                           << field1;
@@ -588,7 +647,8 @@ QList<CueInfo> SeratoMarkers::getCues() const {
                         std::nullopt,
                         cueIndex,
                         QString(),
-                        pEntry->getColor());
+                        pEntry->getColor(),
+                        CueFlag::None);
                 cueInfos.append(cueInfo);
             }
             cueIndex++;
@@ -609,7 +669,8 @@ QList<CueInfo> SeratoMarkers::getCues() const {
                         pEntry->getEndPosition(),
                         loopIndex,
                         QString(),
-                        std::nullopt);
+                        std::nullopt,
+                        pEntry->isLocked() ? CueFlag::Locked : CueFlag::None);
                 cueInfos.append(loopInfo);
                 // TODO: Add support for the "locked" attribute
             }
@@ -639,9 +700,6 @@ void SeratoMarkers::setCues(const QList<CueInfo>& cueInfos) {
         VERIFY_OR_DEBUG_ASSERT(hotcueIndex >= kFirstHotCueIndex) {
             continue;
         }
-        VERIFY_OR_DEBUG_ASSERT(cueInfo.getColor()) {
-            continue;
-        }
         VERIFY_OR_DEBUG_ASSERT(cueInfo.getStartPositionMillis()) {
             continue;
         }
@@ -667,7 +725,7 @@ void SeratoMarkers::setCues(const QList<CueInfo>& cueInfos) {
         const CueInfo cueInfo = cueMap.value(i);
 
         SeratoMarkersEntryPointer pEntry;
-        if (cueInfo.getStartPositionMillis()) {
+        if (cueInfo.getStartPositionMillis() && cueInfo.getColor()) {
             pEntry = std::make_shared<SeratoMarkersEntry>(
                     true,
                     static_cast<int>(*cueInfo.getStartPositionMillis()),
@@ -699,9 +757,13 @@ void SeratoMarkers::setCues(const QList<CueInfo>& cueInfos) {
                     static_cast<int>(*cueInfo.getStartPositionMillis()),
                     true,
                     static_cast<int>(*cueInfo.getEndPositionMillis()),
-                    *cueInfo.getColor(),
+                    // TODO: In Serato, saved loops always have a fixed color.
+                    // We *could* export the actual color here if we also
+                    // import the blue-ish default color in the code above, but
+                    // it will not be used by Serato.
+                    SeratoTags::kFixedLoopColor,
                     static_cast<int>(SeratoMarkersEntry::TypeId::Loop),
-                    false);
+                    cueInfo.isLocked());
         } else {
             pEntry = std::make_shared<SeratoMarkersEntry>(
                     false,
diff --git a/src/track/serato/markers.h b/src/track/serato/markers.h
index d4a32a4a53..370cbd48c3 100644
--- a/src/track/serato/markers.h
+++ b/src/track/serato/markers.h
@@ -19,7 +19,7 @@ class SeratoMarkersEntry {
   public:
     /// We didn't encounter other type IDs as those listed here (e.g. "2") yet.
     /// Apparently these are not used.
-    enum class TypeId : int {
+    enum class TypeId : quint8 {
         /// Used for unset cue points
         Unknown = 0,
         /// Used for set cue points
@@ -34,7 +34,7 @@ class SeratoMarkersEntry {
             bool hasEndPosition,
             int endPosition,
             RgbColor color,
-            int type,
+            quint8 type,
             bool isLocked)
             : m_color(color),
               m_hasStartPosition(hasStartPosition),
@@ -52,7 +52,7 @@ class SeratoMarkersEntry {
     static SeratoMarkersEntryPointer parseID3(const QByteArray& data);
     static SeratoMarkersEntryPointer parseMP4(const QByteArray& data);
 
-    int type() const {
+    quint8 type() const {
         return m_type;
     }
 
@@ -103,7 +103,7 @@ class SeratoMarkersEntry {
     bool m_isLocked;
     quint32 m_startPosition;
     quint32 m_endPosition;
-    int m_type;
+    quint8 m_type;
 };
 
 inline bool operator==(const SeratoMarkersEntry& lhs, const SeratoMarkersEntry& rhs) {
diff --git a/src/track/serato/markers2.cpp b/src/track/serato/markers2.cpp
index 6ca642b077..550c1bc906 100644
--- a/src/track/serato/markers2.cpp
+++ b/src/track/serato/markers2.cpp
@@ -2,6 +2,7 @@
 
 #include <QtEndian>
 
+#include "track/serato/tags.h"
 #include "util/logger.h"
 
 namespace {
@@ -617,7 +618,8 @@ QList<CueInfo> SeratoMarkers2::getCues() const {
                 std::nullopt,
                 pCueEntry->getIndex(),
                 pCueEntry->getLabel(),
-                pCueEntry->getColor());
+                pCueEntry->getColor(),
+                CueFlag::None);
         cueInfos.append(cueInfo);
     }
 
@@ -639,7 +641,8 @@ QList<CueInfo> SeratoMarkers2::getCues() const {
                 pLoopEntry->getEndPosition(),
                 pLoopEntry->getIndex(),
                 pLoopEntry->getLabel(),
-                std::nullopt); // Serato's Loops don't have a color
+                std::nullopt, // Serato's Loops don't have a color
+                pLoopEntry->isLocked() ? CueFlag::Locked : CueFlag::None);
         // TODO: Add support for "locked" loops
         cueInfos.append(loopInfo);
     }
@@ -714,8 +717,8 @@ void SeratoMarkers2::setCues(const QList<CueInfo>& cueInfos) {
                 *cueInfo.getHotCueIndex(),
                 *cueInfo.getStartPositionMillis(),
                 *cueInfo.getEndPositionMillis(),
-                *cueInfo.getColor(),
-                false,
+                SeratoTags::kFixedLoopColor,
+                cueInfo.isLocked(),
                 cueInfo.getLabel());
         newEntries.append(pEntry);
     }
diff --git a/src/track/serato/tags.cpp b/src/track/serato/tags.cpp
index 9f682613ca..8033337038 100644
--- a/src/track/serato/tags.cpp
+++ b/src/track/serato/tags.cpp
@@ -330,6 +330,7 @@ QList<CueInfo> SeratoTags::getCueInfos() const {
         newCueInfo.setStartPositionMillis(cueInfo.getStartPositionMillis());
         newCueInfo.setEndPositionMillis(cueInfo.getEndPositionMillis());
         newCueInfo.setHotCueIndex(index);
+        newCueInfo.setFlags(cueInfo.flags());
 
         RgbColor::optional_t color = cueInfo.getColor();
         if (color) {
@@ -385,6 +386,7 @@ void SeratoTags::setCueInfos(const QList<CueInfo>& cueInfos, double timingOffset
         if (cueInfo.getEndPositionMillis()) {
             newCueInfo.setEndPositionMillis(*cueInfo.getEndPositionMillis() - timingOffsetMillis);
         }
+        newCueInfo.setFlags(cueInfo.flags());
 
         switch (cueInfo.getType()) {
         case CueType::HotCue:
diff --git a/src/track/serato/tags.h b/src/track/serato/tags.h
index 2db3a599d9..6071c9001d 100644
--- a/src/track/serato/tags.h
+++ b/src/track/serato/tags.h
@@ -15,6 +15,7 @@ class SeratoTags final {
   public:
     static constexpr RgbColor kDefaultTrackColor = RgbColor(0xFF9999);
     static constexpr RgbColor kDefaultCueColor = RgbColor(0xCC0000);
+    static constexpr RgbColor kFixedLoopColor = RgbColor(0x27AAE1);
 
     SeratoTags() = default;