Serato Markers Integration for Track Color/BPM lock #2499

Holzhaus · 2020-02-13T18:04:37Z

This adds support for reading the Serato Markers_ and Serato Markers2 tag that are written by Serato and contains Cues, Loops, Track Color, etc.

This PR supersedes #2473 and depends on #2495. As @daschuer suggested I removed export support for now.

IMPORTANT: You need to apply this patch to enable the actual integration:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 59564fa2c0..614b65a808 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -698,6 +698,7 @@ add_library(mixxx-lib STATIC EXCLUDE_FROM_ALL
 set_target_properties(mixxx-lib PROPERTIES AUTOMOC ON AUTOUIC ON)
 target_include_directories(mixxx-lib PUBLIC src "${CMAKE_CURRENT_BINARY_DIR}/src")
 target_compile_definitions(mixxx-lib PRIVATE SETTINGS_FILE="mixxx.cfg")
+target_compile_definitions(mixxx-lib PUBLIC __EXTRA_METADATA__)
 if(UNIX AND NOT APPLE)
   target_compile_definitions(mixxx-lib PRIVATE SETTINGS_PATH=".mixxx/")
 endif()

Holzhaus · 2020-02-13T18:10:46Z

I need some help here. For importing cue positons I need the track's sample rate. However, I cannot use track->getSampleRate() when the file tags are read, because according to a comment in the source code the sample rate will be adjusted by the audio decoder after import from file tags. Any ideas how to solve this? Can we postpone the parsing of the serato tags until after this has happend?

uklotzde · 2020-02-13T20:57:41Z

I need some help here. For importing cue positons I need the track's sample rate. However, I cannot use track->getSampleRate() when the file tags are read, because according to a comment in the source code the sample rate will be adjusted by the audio decoder after import from file tags. Any ideas how to solve this? Can we postpone the parsing of the serato tags until after this has happend?

Mixxx's format for storing the position of cue points in samples is unfortunate. I guess there will be no easy solution other than some dirty hack. In the long term we need to migrate from samples (integer) to time units (floating-point).

The sample rate from the tags is not reliable and may be missing or differ from the sample rate of the actual audio stream. This property is only available after actually opening the stream with a decoder.

uklotzde · 2020-02-13T21:00:54Z

The correct sample rate must then be passed to the tag reader as an additional parameter for performing these conversions. The tag reader must not make any assumptions how this sample rate is determined.

src/track/track.cpp

src/track/serato/markers2.h

src/track/serato/markers2.cpp

src/track/serato/markers2.h

src/track/serato/markers.h

Holzhaus · 2020-02-14T01:22:40Z

@uklotzde thanks for reviewing. Note that only the last 3 commits are new on this branch. Most of the changes actually belong to #2495, so I makes sense to review that one too. I'll probably rebase sooner or later this branch and it would be a shame if comments get lost.

Holzhaus · 2020-02-14T14:47:31Z

The sample rate from the tags is not reliable and may be missing or differ from the sample rate of the actual audio stream. This property is only available after actually opening the stream with a decoder.

I'm still struggling to find the proper spot for doing this. Seems wrong to read metadata here: https://github.com/mixxxdj/mixxx/blob/master/src/sources/soundsourceproxy.cpp#L526

uklotzde · 2020-02-15T09:24:08Z

The sample rate from the tags is not reliable and may be missing or differ from the sample rate of the actual audio stream. This property is only available after actually opening the stream with a decoder.

I'm still struggling to find the proper spot for doing this. Seems wrong to read metadata here: https://github.com/mixxxdj/mixxx/blob/master/src/sources/soundsourceproxy.cpp#L526

I agree that this is an ugly side effect. But it is the only opportunity where we could update the metadata with the actual properties. Any ideas how to improve this? Return the audio properties to the caller?

We need to ensure somehow that the Track object gets updated when the corresponding audio stream is opened for the first time! Otherwise the audio properties have to be considered stale.

The SoundSourceProxy needs to hold a (smart) pointer to the track to ensure that no file tags are written while the audio stream is read.

uklotzde · 2020-02-16T12:08:55Z

The sample rate from the tags is not reliable and may be missing or differ from the sample rate of the actual audio stream. This property is only available after actually opening the stream with a decoder.

I'm still struggling to find the proper spot for doing this. Seems wrong to read metadata here: https://github.com/mixxxdj/mixxx/blob/master/src/sources/soundsourceproxy.cpp#L526

Extracting the audio stream properties from TrackRecord turns out to be more tedious than expected. This is required as a first step to sort out the dependencies.

Holzhaus · 2020-02-16T23:08:48Z

Apart from the problem that the sample rate is incorrect, I also need a way to get the Track dependency out of the Serato classes.

I was planning to have a QList<CuePointer> getCues(int sampleRate) function that returns cues, then set them via Track::setCuePoints(QList<CuePointer>)); from inside the Track object. That way we wouldn't need to depend on the Track Object at all. That doesn't work though because

The Cue constructor is private, and
it also requires passing the TrackId.

I'm not sure how to solve this. Should I create a new CueInfo class that is used by Cue like Track uses TrackInfo? Or is there a better way to do this?

Holzhaus · 2020-02-23T13:27:11Z

@uklotzde What do you think?

uklotzde · 2020-02-25T21:22:03Z

@uklotzde What do you think?

Difficult question. I'm biased after I already failed and finally gave up on fixing the cue control code 😬

Holzhaus · 2020-03-01T17:52:38Z

Merge? This is ifdef'ed behind __EXTRA_METADATA__ anyway.

src/track/serato/tags.cpp

uklotzde · 2020-03-01T18:13:18Z

src/track/serato/tags.cpp

+        return RgbColor(0x999998);
+    }
+
+    if (value == 0x99999a) {


Many of those magic codes/numbers appear repeatedly and in pairs. Would it make sense to define some constants for them?

I thought about that, too, but decided against it - mainly because I couldn't find fitting names ;-)

src/track/serato/tags.cpp

src/track/serato/tags.h

src/track/trackmetadatataglib.cpp

uklotzde

TODO: Test that dumping empty markers produces empty byte arrays. This is required when exporting the markers into file tags.

Holzhaus · 2020-03-01T23:31:21Z

TODO: Test that dumping empty markers produces empty byte arrays. This is required when exporting the markers into file tags.

Like this? 27871cb

Holzhaus · 2020-03-03T09:09:09Z

TODO: Test that dumping empty markers produces empty byte arrays. This is required when exporting the markers into file tags.

I fixed the SeratoMarkers2 default constructor and added tests to make sure that empty QByteArrays are returned when calling dump().

Holzhaus · 2020-03-04T11:25:18Z

Anything else to do here?

src/track/serato/markers2.cpp

uklotzde · 2020-03-05T10:12:16Z

LGTM. No CI failures expected. Thank you Jan!!

Holzhaus · 2020-03-05T13:17:57Z

@uklotzde Thanks for all the thorough reviews!

Holzhaus added the library label Feb 13, 2020

Holzhaus self-assigned this Feb 13, 2020

Holzhaus mentioned this pull request Feb 13, 2020

[WIP] Add support for importing/exporting the SeratoMarkers2 tag #2473

Closed

4 tasks