Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix export of extra metadata #2301

Merged
merged 3 commits into from
Sep 27, 2019
Merged

Fix export of extra metadata #2301

merged 3 commits into from
Sep 27, 2019

Conversation

uklotzde
Copy link
Contributor

  • Some __EXTRA_METADATA__ ifdefs were missing
  • Switching between the old and new ID3v2 grouping tag mapping was broken. A single missing ! causes writing the new tag GRP1 instead of TIT1 as intended.

An additional fix that hides all extra properties in AlbumInfo and TrackInfo will follow. This should then prevent any unintended usage in the future. It revealed the missing ifdefs that are fixed in this PR.

@uklotzde uklotzde added this to the 2.3.0 milestone Sep 27, 2019
@uklotzde uklotzde requested a review from daschuer September 27, 2019 13:17
@daschuer
Copy link
Member

Ups... Yes thank you.

Just to verify I was looking a bit closer to this I have found:

Bei mp4-Dateien hat Apple übrigens diesen Fehler nicht gemacht, möglicherweise ist es also ein Versehen.

https://de.m.wikipedia.org/wiki/ID3-Tag?wprov=sfla1

Are we sure that the GPR1 frame is exclusively used for that type of tracks?

// Stick to traditional mapping if the new GRP1
if (
#if defined(__EXTRA_METADATA__)
trackMetadata.getTrackInfo().getWork().isNull() &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this condition true?
I wonder if iTunes use the work tag without GRP1 tag for mp4 files.
What do others, if we consider the iTunes way as a hotfix for TIT1 missuse?

Copy link
Contributor Author

@uklotzde uklotzde Sep 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The corresponding (custom) MP4 atoms are well defined without any context-sensitive switching.

Maybe we should use the new mapping if work is available or movement is available or GRP1 is present in the file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. The new version of the export code now clearly distinguishes between either TIT1 or GRP1/TIT1/MVNM.

@daschuer
Copy link
Member

LGTM, Thank you.
I have played a bit with iTunes 12.9.6.3 and it works. It is unfortunately not as smart as Mixxx and does not import tracks with only TIT1 as grouping.

@daschuer daschuer merged commit 674a5b5 into mixxxdj:master Sep 27, 2019
@uklotzde uklotzde deleted the extra_metadata branch September 28, 2019 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants