Track colors are lost when importing metadata

[istepaniuk]

Bug Description

When doing "Metadata > Import From File Tags" on an MP3 track in the library, the colors I've applied to my track are reset.

Since colors are not stored in MP3 files' tags, I expect them to stay as they set in Mixxx's DB, the same way that the BPM lock, the Date Added, etc. stay unchanged.

Adding tags (e.g. a comment) outside Mixxx during preparation and then importing the metadata should be possible without unexpectedly loosing the assigned track colors.

Version

2.3.3

OS

Debian bookworm

[ronso0]

Do you have checked any of the Serato options in Preferences > Library?

[istepaniuk]

Do you have checked any of the Serato options in Preferences > Library?

No, I don't use the Serato options.

I tried enabling them to see what happens and the behavior is similar, colors get reset when reimporting metadata, even if the files did not change at all.

The exception is that with "Write Serato metadata to files" enabled, I can now keep the colors when reimporting metadata only as long as I do "Export to file tags" first.

This stores the colors in the Serato tags within the file and I though it could be a viable workaround. Unfortunately, other tools (puddletag, easytag, etc.) strip the Serato data when modifying tags so I still can't add a comment outside Mixxx without loosing the colors.

[Holzhaus]

Hmm, if there aren't any serato tags in the file IMHO the track color should not be reset. The color should only be reset if there is serato data in the file, and that data says that the color is "no color".

[istepaniuk]

I can reproduce this every time:

• Fresh start (deleted my ~/.mixxx directory, reinstalled mixxx by remove --purge + install
• Open Mixxx and point the library to a folder containing .mp3 files, only standard ID3v2.3 tags.
• Set a color to the tracks.
• Metadata > "Import From File Tags" on the colored track

Colors gone.

I just compiled and tested 2.5.0-alpha (current master, 09b2f61) and I get the same results.

EDIT: While at it, I am trying to debug Mixxx. The code at track.cpp:208 seems to agree with what @Holzhaus describes: the expected behavior is to do nothing:

// Import track color from Serato tags if available
const auto newColor = m_record.getMetadata().getTrackInfo().getSeratoTags().getTrackColor();
const bool colorModified = compareAndSet(m_record.ptrColor(), newColor);
modified |= colorModified;
DEBUG_ASSERT(!colorModified || m_record.getColor() == newColor);

This code runs even without "Synchronize Serato tag metadata" enabled and with no Serato metadata in these files. The newColor variable is an optional<> and has no value, but colorModified is set to true anyways. compareAndSet() is setting the default color value to the track, resetting its color.

[Holzhaus]

I think we need an additional hasTrackColor() on the Serato tags object, because currently we cannot distinguish "the track color in the Serato tags object is set to 'no color'" (=> serato track color = 0xFFFFFF) from "there is no track color stored in the tags":
https://github.com/mixxxdj/mixxx/blob/main/src/track/serato/tags.cpp#L430

[istepaniuk]

The optional<> allows for simply:

const bool colorModified = newColor.has_value() && compareAndSet(m_record.ptrColor(), newColor);

I tried this and it solves the issue for me. Not owning any Serato products to test the ramifications of this (or being certain that it's kosher C++) I reckon that a PR would just get in the way.

[Holzhaus]

This is not a proper fix because newColor might be nullopt even though there is a serato track color present in the tags (but that color is 0xFFFFFF which means "no color"). In that case, reimporting metadata should indeed reset the track color. (see https://github.com/mixxxdj/mixxx/blob/main/src/track/serato/tags.cpp#L88-L112)

Hence, we either need a helper function to check if a track color is present in the tags or change the signature of SeratoTags::getTrackColor to make it return a nested optional (std::optional<RgbColor::optional_t>).