Merge branch 'master' into coverimagehash

mixxxdj · Mar 5, 2020 · 323d894 · 323d894
2 parents 43c5167 + fdaacca
commit 323d894
Show file tree

Hide file tree

Showing 16 changed files with 295 additions and 50 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -523,6 +523,7 @@ add_library(mixxx-lib STATIC EXCLUDE_FROM_ALL
   src/track/replaygain.cpp
   src/track/serato/markers.cpp
   src/track/serato/markers2.cpp
+  src/track/serato/tags.cpp
   src/track/track.cpp
   src/track/trackfile.cpp
   src/track/trackinfo.cpp
@@ -980,6 +981,7 @@ add_executable(mixxx-test
   src/test/searchqueryparsertest.cpp
   src/test/seratomarkerstest.cpp
   src/test/seratomarkers2test.cpp
+  src/test/seratotagstest.cpp
   src/test/signalpathtest.cpp
   src/test/skincontext_test.cpp
   src/test/softtakeover_test.cpp

diff --git a/build/depends.py b/build/depends.py
@@ -1216,6 +1216,7 @@ def sources(self, build):
                    "src/track/replaygain.cpp",
                    "src/track/serato/markers.cpp",
                    "src/track/serato/markers2.cpp",
+                   "src/track/serato/tags.cpp",
                    "src/track/track.cpp",
                    "src/track/globaltrackcache.cpp",
                    "src/track/trackfile.cpp",

diff --git a/src/test/seratomarkers2test.cpp b/src/test/seratomarkers2test.cpp
@@ -277,4 +277,12 @@ TEST_F(SeratoMarkers2Test, ParseMarkers2Data) {
     }
 }
 
+TEST_F(SeratoMarkers2Test, ParseEmptyData) {
+    QByteArray inputValue;
+    mixxx::SeratoMarkers2 seratoMarkers2;
+    mixxx::SeratoMarkers2::parse(&seratoMarkers2, inputValue);
+    QByteArray outputValue = seratoMarkers2.dump();
+    EXPECT_EQ(inputValue, outputValue);
+}
+
 }  // namespace
diff --git a/src/test/seratomarkerstest.cpp b/src/test/seratomarkerstest.cpp
@@ -155,4 +155,12 @@ TEST_F(SeratoMarkersTest, ParseMarkersData) {
     }
 }
 
+TEST_F(SeratoMarkersTest, ParseEmptyData) {
+    QByteArray inputValue;
+    mixxx::SeratoMarkers seratoMarkers;
+    mixxx::SeratoMarkers::parse(&seratoMarkers, inputValue);
+    QByteArray outputValue = seratoMarkers.dump();
+    EXPECT_EQ(inputValue, outputValue);
+}
+
 } // namespace
diff --git a/src/test/seratotagstest.cpp b/src/test/seratotagstest.cpp
@@ -0,0 +1,43 @@
+#include <gtest/gtest.h>
+
+#include "track/serato/tags.h"
+
+namespace {
+
+class SeratoTagsTest : public testing::Test {
+  protected:
+    void trackColorRoundtrip(mixxx::RgbColor storedColor, mixxx::RgbColor::optional_t displayedColor) {
+        mixxx::RgbColor::optional_t actualDisplayedColor = mixxx::SeratoTags::storedToDisplayedTrackColor(storedColor);
+        EXPECT_EQ(displayedColor, actualDisplayedColor);
+
+        mixxx::RgbColor actualStoredColor = mixxx::SeratoTags::displayedToStoredTrackColor(actualDisplayedColor);
+        EXPECT_EQ(actualStoredColor, storedColor);
+    }
+};
+
+TEST_F(SeratoTagsTest, ParseTrackColor) {
+    trackColorRoundtrip(mixxx::RgbColor(0xFF99FF), mixxx::RgbColor::optional(0x993399));
+    trackColorRoundtrip(mixxx::RgbColor(0xFF99DD), mixxx::RgbColor::optional(0x993377));
+    trackColorRoundtrip(mixxx::RgbColor(0xFF99BB), mixxx::RgbColor::optional(0x993355));
+    trackColorRoundtrip(mixxx::RgbColor(0xFF9999), mixxx::RgbColor::optional(0x993333));
+    trackColorRoundtrip(mixxx::RgbColor(0xFFBB99), mixxx::RgbColor::optional(0x995533));
+    trackColorRoundtrip(mixxx::RgbColor(0xFFDD99), mixxx::RgbColor::optional(0x997733));
+    trackColorRoundtrip(mixxx::RgbColor(0xFFFF99), mixxx::RgbColor::optional(0x999933));
+    trackColorRoundtrip(mixxx::RgbColor(0xDDFF99), mixxx::RgbColor::optional(0x779933));
+    trackColorRoundtrip(mixxx::RgbColor(0xBBFF99), mixxx::RgbColor::optional(0x559933));
+    trackColorRoundtrip(mixxx::RgbColor(0x99FF99), mixxx::RgbColor::optional(0x339933));
+    trackColorRoundtrip(mixxx::RgbColor(0x99FFBB), mixxx::RgbColor::optional(0x339955));
+    trackColorRoundtrip(mixxx::RgbColor(0x99FFDD), mixxx::RgbColor::optional(0x339977));
+    trackColorRoundtrip(mixxx::RgbColor(0x99FFFF), mixxx::RgbColor::optional(0x339999));
+    trackColorRoundtrip(mixxx::RgbColor(0x99DDFF), mixxx::RgbColor::optional(0x337799));
+    trackColorRoundtrip(mixxx::RgbColor(0x99BBFF), mixxx::RgbColor::optional(0x335599));
+    trackColorRoundtrip(mixxx::RgbColor(0x9999FF), mixxx::RgbColor::optional(0x333399));
+    trackColorRoundtrip(mixxx::RgbColor(0xBB99FF), mixxx::RgbColor::optional(0x553399));
+    trackColorRoundtrip(mixxx::RgbColor(0xDD99FF), mixxx::RgbColor::optional(0x773399));
+    trackColorRoundtrip(mixxx::RgbColor(0x000000), mixxx::RgbColor::optional(0x333333));
+    trackColorRoundtrip(mixxx::RgbColor(0xBBBBBB), mixxx::RgbColor::optional(0x555555));
+    trackColorRoundtrip(mixxx::RgbColor(0x999999), mixxx::RgbColor::optional(0x090909));
+    trackColorRoundtrip(mixxx::RgbColor(0xFFFFFF), std::nullopt);
+}
+
+} // namespace
diff --git a/src/track/serato/markers.cpp b/src/track/serato/markers.cpp
@@ -2,15 +2,14 @@
 
 #include <QtEndian>
 
-#include "util/color/rgbcolor.h"
+#include "track/serato/tags.h"
 
 namespace {
 
 const int kNumEntries = 14;
 const int kLoopEntryStartIndex = 5;
 const int kEntrySize = 22;
 const quint16 kVersion = 0x0205;
-constexpr mixxx::RgbColor kDefaultTrackColor = mixxx::RgbColor(0xFF9999);
 
 // These functions conversion between the 4-byte "Serato Markers_" color format
 // and RgbColor (3-Byte RGB, transparency disabled).
@@ -252,6 +251,11 @@ bool SeratoMarkers::parse(SeratoMarkers* seratoMarkers, const QByteArray& data)
 
 QByteArray SeratoMarkers::dump() const {
     QByteArray data;
+    if (isEmpty()) {
+        // Return empty QByteArray
+        return data;
+    }
+
     data.resize(sizeof(quint16) + 2 * sizeof(quint32) + kEntrySize * m_entries.size());
 
     QDataStream stream(&data, QIODevice::WriteOnly);
@@ -262,7 +266,7 @@ QByteArray SeratoMarkers::dump() const {
         SeratoMarkersEntryPointer pEntry = m_entries.at(i);
         stream.writeRawData(pEntry->dump(), kEntrySize);
     }
-    stream << seratoColorFromRgb(m_trackColor.value_or(kDefaultTrackColor));
+    stream << seratoColorFromRgb(m_trackColor.value_or(SeratoTags::kDefaultTrackColor));
     return data;
 }
 

diff --git a/src/track/serato/markers.h b/src/track/serato/markers.h
@@ -11,7 +11,6 @@
 
 namespace mixxx {
 
-// Forward declaration
 class SeratoMarkersEntry;
 typedef std::shared_ptr<SeratoMarkersEntry> SeratoMarkersEntryPointer;
 

diff --git a/src/track/serato/markers2.cpp b/src/track/serato/markers2.cpp
@@ -372,6 +372,18 @@ bool SeratoMarkers2::parse(SeratoMarkers2* seratoMarkers2, const QByteArray& out
 
 QByteArray SeratoMarkers2::dump() const {
     QByteArray data;
+
+    // To reduce disk fragmentation, Serato pre-allocates at least 470 bytes
+    // for the "Markers2" tag. Unused bytes are filled with null-bytes.
+    // Hence, it's possible to have a valid tag that does not contain actual
+    // marker information. The allocated size is set after successfully parsing
+    // the tag, so if the tag is valid but does not contain entries we
+    // shouldn't delete the tag content.
+    if (isEmpty() && getAllocatedSize() == 0) {
+        // Return empty QByteArray
+        return data;
+    }
+
     QDataStream stream(&data, QIODevice::WriteOnly);
     stream.setVersion(QDataStream::Qt_5_0);
     stream.setByteOrder(QDataStream::BigEndian);
@@ -426,4 +438,34 @@ QByteArray SeratoMarkers2::dump() const {
     return outerData.leftJustified(size, '\0');
 }
 
+RgbColor::optional_t SeratoMarkers2::getTrackColor() const {
+    qDebug() << "Reading track color from 'Serato Markers2' tag data...";
+
+    for (auto& pEntry : m_entries) {
+        DEBUG_ASSERT(pEntry);
+        if (pEntry->typeId() != SeratoMarkers2Entry::TypeId::Color) {
+            continue;
+        }
+        const SeratoMarkers2ColorEntry* pColorEntry = static_cast<SeratoMarkers2ColorEntry*>(pEntry.get());
+        return RgbColor::optional(pColorEntry->getColor());
+    }
+
+    return std::nullopt;
+}
+
+bool SeratoMarkers2::isBpmLocked() const {
+    qDebug() << "Reading bpmlock state from 'Serato Markers2' tag data...";
+
+    for (auto& pEntry : m_entries) {
+        DEBUG_ASSERT(pEntry);
+        if (pEntry->typeId() != SeratoMarkers2Entry::TypeId::Bpmlock) {
+            continue;
+        }
+        const SeratoMarkers2BpmlockEntry* pBpmlockEntry = static_cast<SeratoMarkers2BpmlockEntry*>(pEntry.get());
+        return pBpmlockEntry->isLocked();
+    }
+
+    return false;
+}
+
 } //namespace mixxx
diff --git a/src/track/serato/markers2.h b/src/track/serato/markers2.h
@@ -9,11 +9,6 @@
 #include "util/color/rgbcolor.h"
 #include "util/types.h"
 
-namespace {
-constexpr mixxx::RgbColor kDefaultTrackColor = mixxx::RgbColor(0xFF9999);
-constexpr mixxx::RgbColor kDefaultCueColor = mixxx::RgbColor(0xCC0000);
-} // namespace
-
 namespace mixxx {
 
 // Enum values need to appear in the same order as the corresponding entries
@@ -61,6 +56,7 @@ class SeratoMarkers2UnknownEntry : public SeratoMarkers2Entry {
             : m_type(std::move(type)),
               m_data(std::move(data)) {
     }
+    SeratoMarkers2UnknownEntry() = delete;
     ~SeratoMarkers2UnknownEntry() override = default;
 
     QString type() const override {
@@ -89,10 +85,7 @@ class SeratoMarkers2BpmlockEntry : public SeratoMarkers2Entry {
     SeratoMarkers2BpmlockEntry(bool locked)
             : m_locked(locked) {
     }
-
-    SeratoMarkers2BpmlockEntry()
-            : m_locked(false) {
-    }
+    SeratoMarkers2BpmlockEntry() = delete;
 
     static SeratoMarkers2EntryPointer parse(const QByteArray& data);
 
@@ -139,10 +132,7 @@ class SeratoMarkers2ColorEntry : public SeratoMarkers2Entry {
     SeratoMarkers2ColorEntry(RgbColor color)
             : m_color(color) {
     }
-
-    SeratoMarkers2ColorEntry()
-            : m_color(kDefaultTrackColor) {
-    }
+    SeratoMarkers2ColorEntry() = delete;
 
     static SeratoMarkers2EntryPointer parse(const QByteArray& data);
 
@@ -192,13 +182,7 @@ class SeratoMarkers2CueEntry : public SeratoMarkers2Entry {
               m_color(color),
               m_label(label) {
     }
-
-    SeratoMarkers2CueEntry()
-            : m_index(0),
-              m_position(0),
-              m_color(kDefaultCueColor),
-              m_label(QString("")) {
-    }
+    SeratoMarkers2CueEntry() = delete;
 
     static SeratoMarkers2EntryPointer parse(const QByteArray& data);
 
@@ -282,14 +266,7 @@ class SeratoMarkers2LoopEntry : public SeratoMarkers2Entry {
               m_locked(locked),
               m_label(label) {
     }
-
-    SeratoMarkers2LoopEntry()
-            : m_index(0),
-              m_startposition(0),
-              m_endposition(0),
-              m_locked(false),
-              m_label(QString("")) {
-    }
+    SeratoMarkers2LoopEntry() = delete;
 
     static SeratoMarkers2EntryPointer parse(const QByteArray& data);
 
@@ -386,7 +363,9 @@ inline QDebug operator<<(QDebug dbg, const SeratoMarkers2LoopEntry& arg) {
 //
 class SeratoMarkers2 final {
   public:
-    SeratoMarkers2() = default;
+    SeratoMarkers2()
+            : m_allocatedSize(0) {
+    }
     explicit SeratoMarkers2(
             QList<std::shared_ptr<SeratoMarkers2Entry>> entries)
             : m_allocatedSize(0),
@@ -419,6 +398,9 @@ class SeratoMarkers2 final {
         m_entries = std::move(entries);
     }
 
+    RgbColor::optional_t getTrackColor() const;
+    bool isBpmLocked() const;
+
   private:
     int m_allocatedSize;
     QList<std::shared_ptr<SeratoMarkers2Entry>> m_entries;

diff --git a/src/track/serato/tags.cpp b/src/track/serato/tags.cpp
@@ -0,0 +1,81 @@
+#include "track/serato/tags.h"
+
+namespace mixxx {
+
+RgbColor::optional_t SeratoTags::storedToDisplayedTrackColor(RgbColor color) {
+    // Serato stores Track colors differently from how they are displayed in
+    // the library column. Instead of the color from the library view, the
+    // value from the color picker is stored instead (which is different).
+    // To make sure that the track looks the same in both Mixxx' and Serato's
+    // libraries, we need to convert between the two values.
+    //
+    // See this for details:
+    // https://github.com/Holzhaus/serato-tags/blob/master/docs/colors.md#track-colors
+
+    if (color == 0xFFFFFF) {
+        return RgbColor::nullopt();
+    }
+
+    if (color == 0x999999) {
+        return RgbColor::optional(0x090909);
+    }
+
+    if (color == 0x000000) {
+        return RgbColor::optional(0x333333);
+    }
+
+    RgbColor::code_t colorCode = color;
+    colorCode = (colorCode < 0x666666) ? colorCode + 0x99999A : colorCode - 0x666666;
+    return RgbColor::optional(colorCode);
+}
+
+RgbColor SeratoTags::displayedToStoredTrackColor(RgbColor::optional_t color) {
+    if (!color) {
+        return RgbColor(0xFFFFFF);
+    }
+
+    RgbColor::code_t colorCode = *color;
+
+    if (colorCode == 0x090909) {
+        return RgbColor(0x999999);
+    }
+
+    if (colorCode == 0x333333) {
+        return RgbColor(0x000000);
+    }
+
+    // Special case: 0x999999 and 0x99999a are not representable as Serato
+    // track color We'll just modify them a little, so that the look the
+    // same in Serato.
+    if (colorCode == 0x999999) {
+        return RgbColor(0x999998);
+    }
+
+    if (colorCode == 0x99999a) {
+        return RgbColor(0x99999b);
+    }
+
+    colorCode = (colorCode < 0x99999A) ? colorCode + 0x666666 : colorCode - 0x99999A;
+    return RgbColor(colorCode);
+}
+
+RgbColor::optional_t SeratoTags::getTrackColor() const {
+    RgbColor::optional_t color = m_seratoMarkers.getTrackColor();
+
+    if (!color) {
+        // Markers_ is empty, but we may have a color in Markers2
+        color = m_seratoMarkers2.getTrackColor();
+    }
+
+    if (color) {
+        color = SeratoTags::storedToDisplayedTrackColor(*color);
+    }
+
+    return color;
+}
+
+bool SeratoTags::isBpmLocked() const {
+    return m_seratoMarkers2.isBpmLocked();
+}
+
+} // namespace mixxx