Skip to content

Commit

Permalink
Merge pull request #11466 from daschuer/gh11283
Browse files Browse the repository at this point in the history 
Don't use garbage Cues from Serato Tags or library.
  • Loading branch information
@Swiftb0y
Swiftb0y committed May 2, 2023
2 parents e8cedac + 452b04a commit 0d5b4a7
Show file tree
Hide file tree
Showing 7 changed files with 154 additions and 102 deletions.
16 changes: 13 additions & 3 deletions src/library/dao/cuedao.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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,
Expand Down Expand Up @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion src/test/cue_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
Binary file modified src/test/serato/data/mp3/markers_/very-long-names.octet-stream
View file
Binary file not shown.
32 changes: 30 additions & 2 deletions src/track/cueinfo.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -28,6 +55,7 @@ CueInfo::CueInfo(
m_label(label),
m_color(color),
m_flags(flags) {
assertEndPosition(type, endPositionMillis);
}

CueType CueInfo::getType() const {
Expand All @@ -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 {
Expand All @@ -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;
}

Expand Down
3 changes: 1 addition & 2 deletions src/track/cueinfo.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
174 changes: 95 additions & 79 deletions src/track/serato/tags.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand All @@ -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());
Expand All @@ -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:
Expand Down
Loading

0 comments on commit 0d5b4a7

Please sign in to comment.