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

daschuer · 2023-04-10T23:27:28Z

This fixes the debug assert in #11283 by simply discarding implausible cues. During my tests, I was not able to produce these cue entries. So I assume it is a 2.4 bug. I keep investigating. This is a band-aid for not seeing such cues in the GUI.

…sition

src/track/cueinfo.cpp

src/track/serato/tags.cpp

Swiftb0y · 2023-04-11T12:47:05Z

src/library/dao/cuedao.cpp

+    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();
+    }


is there no better way to remove these invalid cuepoints from the db? maybe some sort of sanity check at startup or db migration?

I was currently not able to re-add this issue. Maybe it is a pending 2.4 bug (keep investigating).
If this is a case a library migration is not possible.

A startup code will involve a startup penalty, because we need to check the whole library. So I think this on-the-fly removal is already a good option.

It could be an issue if we keep removing real cues that fit to the pattern.

A startup code will involve a startup penalty, because we need to check the whole library. So I think this on-the-fly removal is already a good option.

My issue is that this will only apply to tracks once they're loaded, in huge libraries, tracks with the deficiency could linger around for years until they've been loaded again. So this cleanup workaround will also have to stay forever. With a DB migration or something similar, the workaround could be removed after a couple versions when we can be certain that people that introduced this issue into their DB while testing the beta have cleaned it up by running a proper 2.4 release version once.

Maybe this concern is overengineering, just sharing my thoughts. I'm not suggesting another approach would be better.

Yes, in understand, let's look at it case by case.

The fist condition makes always sense and is IMHO no candidate for removal. A loop Cue without a length will cause some side effects. So rejecting it from the library is kind of reasonable interdependent from the issue that causes such an entry. Downgrade it to a Hotcue would be the alternative, but this will increase the complexity.

The second condition is more special. Does a black hotcue at position 0 make sense in future? I cannot think of it.
Do you?

The black hotcues at 0 do no harm other than populating the cue buttons and the file metadata. So we may consider to leave them alone.

If we have discovered the reason for these garbage cues, we may reconsider a one time action, that we can provide for a fix.

Ideas?

Well I think both types of cues are reasonable to be removed, my complaint is just that it happens lazily on track load while I would prefer a one-time cleanup action.

In any case, we should definitely try to find the source of the garbage data. I'm not sure how to do that. A sanity check after the export could be an option, but that has some overhead associated with it. Do you have a better idea?

I cannot tell yet. I need to investigate, maybe we should hold this PR back until we know more.

The lazy clean up is simple and works without any other conditions. The only drawback I see is that we may have an issue if a user uses a version without this clean up code or a third party tool reading the Mixxx database. Not sure if that rectifies a more complex solution.

I'd be in favor of finding the cause before trying to fix its symptoms.

Swiftb0y · 2023-04-26T14:28:01Z

src/track/serato/tags.cpp

+        std::optional<int> pIndex = findIndexForCueInfo(cueInfo);
+        if (!pIndex) {


why the pIndex? The result is not a pointer. Its just that the dereferencing operator will "unwrap" the optional.

The variable is not the index itself the name pIndex makes this visible.
Yes, it is not a pointer, but thanks to operator*() and operator->() it can be used exactly like a pointer or smart pointer.
From that point of view, it is a pointer to the value in itself.
In this case the p prefix is a nice well known reminder for "don't forget to deference it" and "caution: it can be null"
I have stumbled over during development.

I still don't think we should use that prefix for anything but a pointer (actually we should prefix any variable simply based on its type or other attributes but thats outside the scope of the discussion). IMO the only legitimate reason to prefix pointers with p is because they have pointer semantics which make it much more difficult to reason about. std::optional has value semantics and is thus much easier to deal with. Adding p to something that has nothing to do with pointers will only cause confusing (and the next person looking at this code will likely change it back to the prefix-less version anyways with the same reason as before that its not a pointer).

In this case the p prefix is a nice well known reminder for "don't forget to deference it" and "caution: it can be null"
I have stumbled over during development.

How did you stumble over this? It should be nothing more than a compile error.

How about introduce o prefix for this issue?
oIndex can than easy destingushed from index that can be used without dereference.
index = *oIndex

Possible, but I'm not a fan of prefixes and I don't think we need more of them. They have little benefit. They're only additional (arbitrary) rules for new contributors and they only get in the way when refactoring since now a change in type also needs a change in name. "I don't know how to use this common type so I need to remind myself how to use it every time" is not a good argument for type-based prefixes (the same applies to visibility prefixes m_ but that's out of scope again).

Oh, no please beware! One more custom convention to follow.

Those type-prefixes are not recommended for new code because the type system has become more complex. Too many conflicts and ambiguities. There already are inconsistent prefixes everywhere in the code, e.g. pList for a QList of pointers 🤡

I'm aware its a drop in the bucket, but if we just introduce more and more, its never going to get better.

There already are inconsistent prefixes everywhere in the code, e.g. pList for a QList of pointers

Where should this be?

Looks like these are already gone. But I remember that I stumbled across various inconsistent and arbitrary usage of type prefixes, sometimes even misleading.

Neither the compiler nor linter care. Better avoid them and focus on naming to communicate intentions, not to redundantly encode type information.

Swiftb0y · 2023-04-26T14:34:42Z

src/track/serato/tags.cpp

@@ -299,6 +326,11 @@ void SeratoTags::setCueInfos(const QList<CueInfo>& cueInfos, double timingOffset
            cueMap.insert(hotcueIndex, newCueInfo);
            break;
        case CueType::Loop:
+            if (!newCueInfo.getEndPositionMillis()) {


Can we prefer the has_value over the to bool conversion? I think it improves legibility quite a bit.

Suggested change

if (!newCueInfo.getEndPositionMillis()) {

if (!newCueInfo.getEndPositionMillis().has_value()) {

Also, this could be a verify or debug assert instead.

We used the boolean operator in the past because has_value() was not available for the old macOS C++ toolchain.

Good to know, thank you. Does that limitation still apply?

Swiftb0y · 2023-04-26T14:37:20Z

src/track/serato/tags.cpp

+                    *cueInfo.getEndPositionMillis() == 0)) {
+        // These entries are likely added via issue #11283
+        qWarning() << "Discard loop cue" << cueInfo.getHotCueIndex()
+                   << "found in markers2 with length of 0";


This is still left over from the deduplication and could be misleading.

Suggested change

<< "found in markers2 with length of 0";

<< "with length of 0";

daschuer · 2023-04-26T20:01:50Z

Done.

Swiftb0y

LGTM. what are your plans for a follow up PR to actually nail down the actual issue thats producing these cues?

daschuer · 2023-04-27T05:24:06Z

I will do some brief tests with switching between this branch and 2.4 to recreate the garbage cues. I hope this will reveal the original issue.

By the way, can you also confirm the issue?

We need to consider if we want to hold back 2.3.5 over this, just merge it or postpone this PR.

Swiftb0y · 2023-04-27T09:38:39Z

By the way, can you also confirm the issue?

It hasn't occurred to me yet, but I also didn't take any time to try and recreate it. From the way I structure my various DBs and mixxx version, its unlikely this would occur for me.

Swiftb0y · 2023-04-27T09:39:40Z

We need to consider if we want to hold back 2.3.5 over this, just merge it or postpone this PR.

Not sure, merging it would mask the issue so it would get more complicated to find the root cause, right?

daschuer · 2023-04-30T00:47:38Z

I think I have understand the issue. When there is no Serato Cue at all the metadata has 5 empty cues. They are discarded when they are not visible in the Markers_. Unfortunately the function was exited early if Markes_ is empty.
The sanity check introduced here fixes that. In addition there was an issue that the Loop cues are duplicated and shifted around and an issue that intro and outro are deleted when the metadata is reloaded without the track loaded into a deck.
These issues is now fixed as well.
The test has been adjusted.

Swiftb0y · 2023-04-30T14:22:42Z

When there is no Serato Cue at all the metadata has 5 empty cues. They are discarded when they are not visible in the Markers_. Unfortunately the function was exited early if Markes_ is empty.

But those 5 empty cues are only set in Markers_, so if thats empty, there are no empty cues. I don't understand how that behavior results in buggy behavior.

In addition there was an issue that the Loop cues are duplicated and shifted around and an issue that intro and outro are deleted when the metadata is reloaded without the track loaded into a deck.
These issues is now fixed as well.

I can't spot anything in the changes that would fix those. Can point me to their fixes or explain what changes exactly have fixed these issues?

src/track/serato/tags.cpp

daschuer · 2023-05-01T20:11:34Z

When there is no Serato Cue at all the metadata has 5 empty cues. They are discarded when they are not visible in the Markers_. Unfortunately the function was exited early if Markes_ is empty.

But those 5 empty cues are only set in Markers_, so if thats empty, there are no empty cues. I don't understand how that behavior results in buggy behavior.

The buggy behavior happens do to the early exit if the Markers_ are empty. Let's say Markers2 have 3 cues and Markers only one, the result is one. If Markers_ is entirely empty, all three cues of Markers2 reach the database and may contain invalid data. I am not able to reproduce the issue, but this at least explains the resulting garbage cues.

In addition there was an issue that the Loop cues are duplicated and shifted around and an issue that intro and outro are deleted when the metadata is reloaded without the track loaded into a deck. These issues is now fixed as well.

I can't spot anything in the changes that would fix those. Can point me to their fixes or explain what changes exactly have fixed these issues?

This has been fixed in 383f682
The original code blindly adds an offset of kFirstLoopIndex = 8 to the Loop index during reading. But does not alter the index during writing. This way the +8 offset applied on every round-trip.

Swiftb0y · 2023-05-02T12:14:46Z

If Markers_ is entirely empty, all three cues of Markers2 reach the database and may contain invalid data.

But that's the behavior observed by serato according to this comment:

mixxx/src/track/serato/tags.cpp

Lines 216 to 221 in f36cd4f

    
           // 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(); 
        
           }

So iiuc, the problem is that Markers2_ contain garbage data while Markers_ is empty. It seems like serato doesn't produce this kind of data but Mixxx does. So its not the import that should be fixed but the export. Wdyt?

This has been fixed in 383f682
The original code blindly adds an offset of kFirstLoopIndex = 8 to the Loop index during reading. But does not alter the index during writing. This way the +8 offset applied on every round-trip.

I see how the loopcue index has been fixed but not how this fixed the intro/outro issue.

daschuer · 2023-05-02T12:24:20Z

I see how the loopcue index has been fixed but not how this fixed the intro/outro issue.

The intro/outro delete issue is fixed in 2ffed46

daschuer · 2023-05-02T12:29:33Z

It seems like serato doesn't produce this kind of data but Mixxx does. So its not the import that should be fixed but the export. Wdyt?

We must not trust any imported data. So fixing the importer is mandatory.

Do you have a hint for you assumption that it is Mixxxs fault? The issue is that I am not able to reproduce to create these faulty files. I can only confirm that this PR prevents to spread the garbage even more.

Swiftb0y · 2023-05-02T12:43:27Z

We must not trust any imported data. So fixing the importer is mandatory.

I agree, though the fixed importer will differ in behavior from serato, though in this case it still makes sense to fix the importer.

Do you have a hint for you assumption that it is Mixxxs fault? The issue is that I am not able to reproduce to create these faulty files. I can only confirm that this PR prevents to spread the garbage even more.

Not really, its just my logical conclusion since you previously said that the only origin of serato metadata in your files could be from mixxx. So this PR is masking the faulty metadata on import, but mixxx could still produce it on export and external software such as serato could then import that faulty data.
I'd really like to take a look at the metadata of the files you exported, but I assume you don't have them anymore? If so lets just merge this fix and worry about bugs in the export when other users report issues with external software.

daschuer · 2023-05-02T13:06:05Z

The only file I have is a well formated file with black cues at zero. There it must have been not that well formated before, and then got "corrected" due to the Mixxx database roundtrip.

Swiftb0y · 2023-05-02T15:22:57Z

Unfortunate, so do you agree it would be a good strategy merge and wait for user reports in case similar issues still occur?

daschuer · 2023-05-02T18:32:01Z

Yes, we have no better chance to make progress.

Holzhaus · 2023-05-02T19:39:15Z

src/track/serato/tags.cpp

+    // 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.


This sounds like a regression to me. Now the ordering of hotcues and loops is quite unpredictable to the regular user.
The main target audience of this feature is people switching from Serato, where these banks are separate. In Mixxx, they are suddenly mixed.

In Serato, you have the following cues:

Cues:

Hotcue A

Hotcue B

Hotcue D

...

Loops:

Loop A

Loop B

Loop C

Loop D

...

When importing into Mixxx, this will now suddenly become:

Hotcue A

Hotcue B

Loop C

Hotcue D

...

...

...

...

Loop A

Loop B

Loop D

...

This is IMHO unexpected and should be reverted. The fixed offset of 8. makes more sense: If you have more than 8 cues and loops total, you need access to cues with index > 8 anyway.

Also, users that previously used Serato are likely to have a Serato-Style DJ controller, which features separate Hotcue and Loop banks. For example, the Roland DJ-505 mapping uses cues 1-8 for hotcues and 9-16 for loop cues. This way, these users can continue using their controller and tracks as expected.

oof, bad timing... this was just merged :( In my defense, from the PR's title I expected this to be a bugfix, not a behavior change.

I'm sorry for the bad timing

This is IMHO unexpected and should be reverted. The fixed offset of 8. makes more sense: If you have more than 8 cues and loops total, you need access to cues with index > 8 anyway.

I fully agree, but how would you keep the offset index from accumulating then? How would tell loop cues interspersed with hotcues by mixxx and loopcues from serato apart?

I think the export is still marked as experimental, and if we have to make a compromise, then it should be in the export logic, not the import logic. It's a format for a different software, so some downside is to be expected.

Serato does not have interspersed hotcues/loop cues, so the order will have to change on export anyway.
One simple way would be to restore the previous import behavior and check if all loop cues have an index > 8. If so, subtract 8 from the index during export.
This would at least allow to preserve empty loop cue slots.

Example:

Cues:

Hotcue A

Hotcue B

Hotcue D

...

Loops:

Loop A

Loop C

Loop D

...

Becomes this in Mixxx:

Hotcue A

Hotcue B

Hotcue D

...

...

...

...

Loop A

Loop C

Loop D

...

And on export the original is restored:

Cues:

Hotcue A

Hotcue B

Hotcue D

...

Loops:

Loop A

Loop C

Loop D

...

Yes, this will be suboptimal for people who want to move away from Mixxx to Serato, but if we have to choose which kind of user we want to accomodate, it should be the user group that wants to migrate from Serato to Mixxx, instead of the group that wants to leave Mixxx anyway.

It looks like this are two different demands. The issue in Mixxx is that we have only 8 buttons in the GUI. So from your example only Loop C is usable for all users. For me the current solution is a good compromise that at least fixes the round-trip starting from Mixxx. I am afraid a full solution requires to store the Mixxx index and the Serato index separate. This is out of scope of this PR. Do you have an alternative solution?
Another open issue is that the white loop cues are becoming orange after the round-trip. Do you have an idea how to fix that?

A regular Mixxx user has probably

Hotcue A

Hotcue B

...

Hotcue D

Loop A

Loop B

Loop D

This is exported now and before as

Hotcues

Hotcue A

Hotcue B

...

Hotcue D

Loops

Loop A

Loop B

...

Loop D

After reading it back from the file it becomes before that PR:

Hotcue A

Hotcue B

...

Hotcue D

...

...

...

...

Loop A

Loop B

Loop D

The Loops are no longer accessible. And after another round-trip it is:

Hotcue A

Hotcue B

...

Hotcue D

...

...

...

...

...

...

...

...

...

...

..

...

Loop A

Loop B

Loop D

For me the current solution is a good compromise that at least fixes the round-trip starting from Mixxx.

Again, I'd argue that our focus should be on the people who want to switch from Serato to Mixxx, not the people who want to switch from Mixxx to Serato. The primary purpose of the experimental export feature is to avoid lock-in. Due to differences between Mixxx and Serato, this format is not suited for lossless transfor of cues between two Mixxx instances.

For Mixxx-only users, the proper solution would be to devise a custom format, e.g. something bson-based maybe. This could be stored in a different tag.

Another open issue is that the white loop cues are becoming orange after the round-trip.

Loop cues don't have colors in Serato, they are all blue. I hypothesized that there is a color field and just the GUI support is missing in Serato, because there is a field that would indicate the color blue, but setting a different "color" in the metadata did not work. We should probably remove color support for loop cues altogether, because it is not supported.

daschuer · 2023-05-02T20:41:46Z

Again, I'd argue that our focus should be on the people who want to switch from Serato to Mixxx.

A user that switches from Serato to Mixxx does not see the loop cues, because they are shifted into a not visible region.
This PR fixes this.

But I am happy with any other solution you come up that does not suffer the described shifting issue.

Holzhaus · 2023-05-02T21:10:33Z

Again, I'd argue that our focus should be on the people who want to switch from Serato to Mixxx.

A user that switches from Serato to Mixxx does not see the loop cues, because they are shifted into a not visible region. This PR fixes this.

But I am happy with any other solution you come up that does not suffer the described shifting issue.

There is no proper way to fix this. Because Mixxx and Serato are just different. If you have 8 hotcues assigned, the loop cues will still be inaccessable, so another solution (e.g., a dialog to reorder cues) is needed. But this change breaks Serato cue import for people who have a skin or mapping with more than 8 hotcues. And even for everyone else, it's very confusing and non-obvious that the cues are shuffled.

This will only get worse if we ever get Serato Flip (Macro) support in a mergeable state. Serato also has a dedicated bank for that, too. I'd argue that we should import them with index offset 16. If we also use the approach from this change, then import indices will become even more unpredictable.

daschuer · 2023-05-03T05:51:10Z

I have created #11530 to discuss a solution.

daschuer added 4 commits April 11, 2023 01:13

Add assertion for a valid cue end position depending on the type

0ee7dba

Fix test CueTest.ConvertCueInfoToCueRoundtrip

a349501

Issue a warning for more details in case the a loop cue has no end po…

be7c91a

…sition

Discard garbage cues that have been introduced by issue mixxxdj#11283

dc6afec

github-actions bot added code quality library labels Apr 10, 2023

This was referenced Apr 10, 2023

Serato tags: DEBUG_ASSERT in markers.cpp:718 #11283

Closed

Serato Cue refactorings #11467

Merged

Swiftb0y reviewed Apr 11, 2023

View reviewed changes

daschuer added this to the 2.3.5 milestone Apr 17, 2023

daschuer added 2 commits April 18, 2023 07:18

Remove inline keyword from a function that is inlined anyway

ba071de

Introduce isCueInfoValid() to deduplicate code

f9b33c5

Swiftb0y reviewed Apr 26, 2023

View reviewed changes

daschuer added 2 commits April 26, 2023 21:53

Improve variable names and comments

bf67fcf

make use of value_or()

6244044

daschuer force-pushed the gh11283 branch from 7336c39 to 6244044 Compare April 26, 2023 19:55

make use of has_value()

a306792

Swiftb0y approved these changes Apr 26, 2023

View reviewed changes

daschuer added 7 commits April 29, 2023 22:41

Add missing std::move

50a6f6e

Remove and assert the not existing isEmpty() case

2e2dfc5

Only remove cues early, that are part or the pending importer

2ffed46

reserve memory before append in a loop

c7f7ec1

Make sure Loop cue Indexes are pereserved in a file metadata roundtrip

383f682

Add debug output to: SeratoTags::getCueInfos()

eed65dc

Fix SeratoTagsTest.MarkersParseDumpRoundtrip test

95d876d

daschuer force-pushed the gh11283 branch from fbd9669 to 95d876d Compare April 30, 2023 00:34

daschuer requested a review from Swiftb0y April 30, 2023 00:47

Swiftb0y reviewed Apr 30, 2023

View reviewed changes

src/track/serato/tags.cpp Outdated Show resolved Hide resolved

check size() for > 0

452b04a

Swiftb0y approved these changes May 2, 2023

View reviewed changes

Swiftb0y merged commit 0d5b4a7 into mixxxdj:2.3 May 2, 2023

Holzhaus requested changes May 2, 2023

View reviewed changes

daschuer mentioned this pull request May 3, 2023

Serato cue import: Risk of loosing data #11530

Closed

		std::optional<int> pIndex = findIndexForCueInfo(cueInfo);
		if (!pIndex) {

	if (!newCueInfo.getEndPositionMillis()) {
	if (!newCueInfo.getEndPositionMillis().has_value()) {

	<< "found in markers2 with length of 0";
	<< "with length of 0";

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

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

Conversation

daschuer commented Apr 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Apr 26, 2023

Swiftb0y left a comment

Choose a reason for hiding this comment

daschuer commented Apr 27, 2023

Swiftb0y commented Apr 27, 2023

Swiftb0y commented Apr 27, 2023

daschuer commented Apr 30, 2023 • edited Loading

Swiftb0y commented Apr 30, 2023

daschuer commented May 1, 2023

Swiftb0y commented May 2, 2023

daschuer commented May 2, 2023

daschuer commented May 2, 2023

Swiftb0y commented May 2, 2023

daschuer commented May 2, 2023

Swiftb0y commented May 2, 2023

daschuer commented May 2, 2023

Holzhaus May 2, 2023 • edited Loading

Choose a reason for hiding this comment

Holzhaus May 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holzhaus May 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holzhaus May 2, 2023 • edited Loading

Choose a reason for hiding this comment

daschuer commented May 2, 2023

Holzhaus commented May 2, 2023

daschuer commented May 3, 2023

Swiftb0y Apr 26, 2023 •

edited

Loading

Swiftb0y Apr 26, 2023 •

edited

Loading

uklotzde Apr 26, 2023 •

edited

Loading

uklotzde Apr 26, 2023 •

edited

Loading

daschuer commented Apr 30, 2023 •

edited

Loading

Holzhaus May 2, 2023 •

edited

Loading

Holzhaus May 2, 2023 •

edited

Loading

Holzhaus May 2, 2023 •

edited

Loading

Holzhaus May 2, 2023 •

edited

Loading