iTunes: Modularize importer and use `iTunesLibrary` on macOS for compatibility with `Music.app` #11353

fwcd · 2023-03-10T02:45:39Z

This fixes

Support the .musicdb generated by Apple's Music.app (which replaces iTunes in newer macOS versions) #11256
Legacy iTunes Integration (XML) #9688 (potentially, though a cross-platform solution might still be nice in the future)

Description

This branch modularizes the iTunes library integration into importers:

ITunesMacOSImporter, using Apple's iTunesLibrary framework
- Used by default on macOS
- Supports regular and smart playlists, along with (nested) folders
- Supports both the old and the new database format used by Music.app
- The framework, however, only lets us read the user's default music library, therefore we'll fall back to the XML importer whenever a custom db path is provided on macOS
ITunesXMLImporter, the current XML parser
- Used on non-macOS platforms or whenever a custom db path is provided

This modularity should also make it easy to drop in new importers in the future, e.g. a cross-platform parser for the new database format.

To do

Future Directions

Adapt the folder import logic (ingesting the parent playlist relationships into a multimap and building the TreeItem afterwards) for the XML importer
- This should address issues like
  - Mixxx does not show iTunes playlist folder heirarchy #9576
  - iTunes folders should be imported as crates #8148
Read cover art via the iTunesLibrary framework too
- iTunes and Music.app in many cases do not store the cover art in the track metadata, making it inaccessible to Mixxx
- However, there is an API for fetching the cover art via an in-memory NSImage from an ITLibMediaItem: https://developer.apple.com/documentation/ituneslibrary/itlibmediaitem/1810529-artwork
- Since cover art isn't stored in the database currently, however, we might either have to
  - modify the schema for this to either store the artwork in the database (possibly space-inefficient for large libraries) or...
  - modify the cover art loader to interface with the iTunesLibrary framework (e.g. querying the iTunes ID for a given track), which might be undesirable since this way the iTunes integration would become a leaky abstraction
Optimize the persistent ID -> database ID mapping
- We need this since iTunes' persistent IDs use unsigned 64-bit ints, but SQLite only lets us store signed 64-bit ints
- Currently we use a dictionary and generate a sequence of incrementing indices
  - One advantage is that we get reasonably deterministic indices (0, 1, 2, 3, ...)
  - This requires storing all IDs in memory though (not sure if this is an issue for very large libraries)
- Potential solutions:
  - Cast the unsigned 64-bit int to a signed 64-bit int for storage in SQLite
    - This would however frequently generate negative indices, which might confuse users and the ORM
  - Perform some form of hashing and investigate how to avoid collisions
    - If we limit ourselves to the positive integers that SQLite can represent, we'd have an output space of 63 bits, which is probably enough, but we'd still want to check the math whether the likelihood of a birthday problem is acceptable
  - Update the database schema
    - Altering the iTunes id column type is a breaking change though, which we'd like to avoid
    - Adding another (unique-constrained) column for persistent IDs is possible without bumping the minimum compatible version, but would require querying the database for every mapping, which would result in a lot of database calls with large playlists

src/library/itunes/itunesmacosimporter.mm

Swiftb0y · 2023-03-14T18:30:27Z

Do you have a good write-up on Obj-C(++) for C++ developers? Also, can you try to keep the Obj-C code minimal? introducing more programming languages increases the risk of pulling in code that we rely on but can't maintain. Especially when that programming language is not used outside the apple ecosystem (which we already don't have access to since none of the active core devs uses macos).

fwcd · 2023-03-14T21:37:41Z

Also, can you try to keep the Obj-C code minimal?

Yeah, that's something I'm already trying to do, also because C++/Qt containers tend to be quite a bit more performant than ObjC types, the latter rely heavily on boxing.

Do you have a good write-up on Obj-C(++) for C++ developers?

http://www.jonhoyle.com/maccompanion/columns/200809.html might be worth looking into.

Since we don't use any of the 'fancy' ObjC features here and only what's necessary to interact with the APIs, this should boil down to learning the syntactic differences (only), e.g. [object method] instead of object.method(). One thing to note is that ObjC objects are always passed as raw pointers, the compiler still performs automatic memory management on them under the hood though (automatic reference counting/ARC to be precise, effectively what a shared_ptr does).

daschuer · 2023-03-15T07:51:45Z

I am unsure what to do with this PR. Is the iTunes integration in 2.3 broken, after a user has received the music.app?

In the unlike case that this PR does any harm, is it possible to disable it and use the XML importer?

I am considering to merge it to 2.4. Do you think the 3 month beta is good enough for testing. Since I am on Linux, I am personally not able to do a sorrowly test. I think we need a real iTunes user to verify details.

Any thoughts?

Swiftb0y · 2023-03-15T10:46:21Z

http://www.jonhoyle.com/maccompanion/columns/200809.html might be worth looking into.

Thank you, I'll familiarize and try to review ASAP.

fwcd · 2023-03-15T11:40:29Z

Is the iTunes integration in 2.3 broken, after a user has received the music.app?

Kind of. Music.app no longer automatically generates an iTunes XML and keeps it in sync. This means the user might see an outdated view of their library, unless they manually export an XML through the UI, which is still possible through Music.app

In the unlike case that this PR does any harm, is it possible to disable it and use the XML importer?

The user should (even if Music.app is installed) always be able to select an XML file in which case we will fall back to the XML importer. The macOS importer is only used when "default library" is selected.

Do you think the 3 month beta is good enough for testing

Since the API surface that we use is relatively low and we reuse a large part of the existing DB infrastructure, I would say that is sufficient. Ideally some users, especially with large libraries, could test this integration.

The iTunesLibrary framework only allows us to inspect the default library, so we might as well reflect that in our API.

Swiftb0y

last comment. lgtm otherwise. I'll have to see whether I can conduct a manual test.

Swiftb0y · 2023-03-23T14:14:55Z

src/library/itunes/itunesfeature.cpp

+#ifdef __MACOS_ITUNES_LIBRARY__
+    if (m_dbfile.isEmpty()) {
+        importer = std::make_unique<ITunesMacOSImporter>(this, m_database, m_cancelImport);
+    } else {
+        importer = std::make_unique<ITunesXMLImporter>(
+                this, m_dbfile, m_database, m_pathMapping, m_cancelImport);


I think we should log which importer was chosen here. Choosing the importer simply based on whether we found the db file seems somewhat brittle (though idk what else would be more robust).

The idea is that e.g. a user on macOS can explicit select an XML file, but also click Use Default Library in the context menu to switch back to the native iTunes integration. Perhaps we should make this more explicit?

I agree that logging the importer is probably a good idea.

I don't quite understand the workflow, what context menu exactly? Is "native iTunes integration" the XML-file or the iTunesLibrary Framework solution?

Is "native iTunes integration" the XML-file or the iTunesLibrary Framework solution?

The iTunesLibrary framework solution.

I don't quite understand the workflow, what context menu exactly?

Selecting Use Default Library sets the dbfile path to an empty string which we interpret as "use the iTunesLibrary importer". If the dbfile path is set to an XML file, we use the XML importer instead. This should also handle existing users of the XML file integration on macOS gracefully (e.g. someone who has an iTunes XML on an external drive etc.)

Mhmm, maybe add a comment that we interpret it that way idk. You're free to improve it in a future PR.

I've added a short doc comment to m_dbfile and a short log message

fwcd · 2023-03-29T14:21:50Z

I have made some minor stylistic improvements here and there and tested the XML importer more extensively, fixing a small regression that surfaced in the guessMusicLibraryMountpoint method (see 16b699e). Thus the path translation for relocated iTunes libraries should work correctly again.

Swiftb0y

small comment, personally, i think the changes look good, but I'd like some more testing, preferably by another 3rd party as I currently don't have access to apple hardware for testing.

src/library/itunes/itunesfeature.cpp

Swiftb0y · 2023-03-30T11:27:10Z

@daschuer what do you think? Should we just merge for 2.4 and see what bug reports we get in the beta?

fwcd · 2023-04-03T14:00:41Z

Any update on this? As mentioned in the original post, there are some things I have in the pipeline for this (e.g. adding the folder logic to the XML importer too), but would move into a separate PR since they change the semantics of the existing importer rather than "just" copying the logic into a separate file.

Swiftb0y · 2023-04-03T15:50:00Z

Well, I would've liked to know @daschuer's opinion before merging since they're usually the ones taking care of coordinating the releases.

If you want to work on other things based on this PR, why don't just do it in a separate branch and occasionally rebase in case changes have been done here? Or am I misunderstanding?

daschuer · 2023-04-03T21:09:23Z

I support merging this into 2.4. I think we need the confirmation that the legacy import is still working before merging.
Has this been tested?

daschuer

I have just fired up my win 10 virtual machine and can confirm that this does not creates obvious regression with iTunes 12.12.7.1

So lets merge this and collect bugs if there are any from beta testers.

Thank you for the work.

uklotzde · 2023-04-04T22:28:39Z

So lets merge this and collect bugs if there are any from beta testers.

That's the right attitude for making progress and keeping trustworthy developers motivated ;)

ronso0 · 2023-04-23T11:51:22Z

src/library/itunes/itunesfeature.h

@@ -51,22 +55,31 @@ class ITunesFeature : public BaseExternalLibraryFeature {
    void clearTable(const QString& table_name);


Some of the method declarations above are obselete now.
I removed them in #11208 when merging 2.4
85c9209
85c9209#diff-380635a0ce3c43f060770fd4001274c29a8142ebba2d56abdef1a9cf06620133

github-actions bot added build library labels Mar 10, 2023

fwcd changed the title ~~ITunesFeature: Modularize importer and use iTunesLibrary on macOS by default~~ iTunes: Modularize importer and use iTunesLibrary on macOS for compatibility with Music.app Mar 10, 2023

daschuer reviewed Mar 11, 2023

View reviewed changes

src/library/itunes/itunesmacosimporter.mm Outdated Show resolved Hide resolved

src/library/itunes/itunesmacosimporter.mm Outdated Show resolved Hide resolved

fwcd marked this pull request as ready for review March 11, 2023 14:28

This was referenced Mar 14, 2023

Mixxx snapshots do not ask for microphone access on macOS (regression) #11365

Closed

clang-format: Handle Objective-C(++) in config #11369

Merged

fwcd force-pushed the macos-itunes-library branch from b86318e to 4284f0f Compare March 14, 2023 16:06

fwcd force-pushed the macos-itunes-library branch from 4284f0f to ad7f428 Compare March 14, 2023 22:46

github-actions bot added the code quality label Mar 14, 2023

fwcd force-pushed the macos-itunes-library branch 2 times, most recently from 8e5e707 to 2c146c3 Compare March 15, 2023 00:49

fwcd added 13 commits March 15, 2023 13:08

ITunesFeature: Factor out ITunesXMLImporter

a79ff45

ITunesMacOSImporter: Stub out native iTunes importer for macOS

1e8be34

ITunesFeature: Conditionally use ITunesMacOSImporter on macOS

4f7b4f0

ITunesFeature: Extract showOpenDialog

8fbaf4e

ITunesMacOSImporter: Implement media item import

f5163e3

ITunesMacOSImporter: Fix imported iTunes durations

48e2910

ITunesMacOSImporter: Remove file

6a88ce7

The iTunesLibrary framework only allows us to inspect the default library, so we might as well reflect that in our API.

ITunesFeature: Fall back to XML importer for custom paths

51296ce

ITunesMacOSImporter: Fix style issue

0c35713

ITunesMacOSImporter: Import playlists

9878d81

ITunesMacOSImporter: Use long long for ids

bfa9206

ITunesMacOSImporter: Factor out state

0437f29

Factor out ImporterImpl

fad5cc4

fwcd force-pushed the macos-itunes-library branch from c8da607 to fb3cc2a Compare March 22, 2023 18:41

Swiftb0y reviewed Mar 23, 2023

View reviewed changes

fwcd added 2 commits March 24, 2023 03:26

ITunesFeature: Clarify m_dbfile

9c9241e

ITunesMacOSImporter: Add doc comment

bb11d5e

fwcd force-pushed the macos-itunes-library branch from 42209cd to bb11d5e Compare March 26, 2023 23:31

fwcd added 4 commits March 29, 2023 15:49

ITunesMacOSImporter: Skip DRM and remote tracks

3b97f61

ITunesXMLImporter: Add missing m_dbfile initialization

16b699e

ITunesXMLImporter: Use better names for various members

ffe6216

ITunesXMLImporter: Use camelCase more consistently

de5188d

fwcd force-pushed the macos-itunes-library branch from f33c67d to de5188d Compare March 29, 2023 14:16

Swiftb0y reviewed Mar 29, 2023

View reviewed changes

src/library/itunes/itunesfeature.cpp Outdated Show resolved Hide resolved

ITunesFeature: Factor out makeImporter and guard macOS version

6e80d16

fwcd commented Mar 30, 2023

View reviewed changes

src/library/itunes/itunesfeature.cpp Show resolved Hide resolved

ITunesFeature: Comment on __builtin_available

f4ab748

fwcd force-pushed the macos-itunes-library branch from 28828bb to f4ab748 Compare March 30, 2023 17:51

daschuer approved these changes Apr 4, 2023

View reviewed changes

daschuer merged commit d6e7553 into mixxxdj:2.4 Apr 4, 2023

fwcd deleted the macos-itunes-library branch April 4, 2023 22:05

This was referenced Apr 4, 2023

Support the .musicdb generated by Apple's Music.app (which replaces iTunes in newer macOS versions) #11256

Closed

Legacy iTunes Integration (XML) #9688

Closed

fwcd mentioned this pull request Apr 5, 2023

iTunes: Fix playlist folder ordering in macOS importer #11444

Merged

ronso0 reviewed Apr 23, 2023

View reviewed changes

fwcd added the itunes label Oct 14, 2023

fwcd mentioned this pull request Oct 14, 2023

iTunes library opens finder and loops without loading #10738

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iTunes: Modularize importer and use `iTunesLibrary` on macOS for compatibility with `Music.app` #11353

iTunes: Modularize importer and use `iTunesLibrary` on macOS for compatibility with `Music.app` #11353

fwcd commented Mar 10, 2023 •

edited

Loading

Swiftb0y commented Mar 14, 2023 •

edited

Loading

fwcd commented Mar 14, 2023 •

edited

Loading

daschuer commented Mar 15, 2023

Swiftb0y commented Mar 15, 2023

fwcd commented Mar 15, 2023

Swiftb0y left a comment

Swiftb0y Mar 23, 2023

fwcd Mar 23, 2023

Swiftb0y Mar 23, 2023

fwcd Mar 23, 2023

Swiftb0y Mar 23, 2023

fwcd Mar 24, 2023

fwcd commented Mar 29, 2023

Swiftb0y left a comment

Swiftb0y commented Mar 30, 2023

fwcd commented Apr 3, 2023

Swiftb0y commented Apr 3, 2023

daschuer commented Apr 3, 2023

daschuer left a comment

uklotzde commented Apr 4, 2023

ronso0 Apr 23, 2023 •

edited

Loading

		@@ -51,22 +55,31 @@ class ITunesFeature : public BaseExternalLibraryFeature {
		void clearTable(const QString& table_name);

iTunes: Modularize importer and use iTunesLibrary on macOS for compatibility with Music.app #11353

iTunes: Modularize importer and use iTunesLibrary on macOS for compatibility with Music.app #11353

Conversation

fwcd commented Mar 10, 2023 • edited Loading

Description

To do

Future Directions

Swiftb0y commented Mar 14, 2023 • edited Loading

fwcd commented Mar 14, 2023 • edited Loading

daschuer commented Mar 15, 2023

Swiftb0y commented Mar 15, 2023

fwcd commented Mar 15, 2023

Swiftb0y left a comment

Choose a reason for hiding this comment

Swiftb0y Mar 23, 2023

Choose a reason for hiding this comment

fwcd Mar 23, 2023

Choose a reason for hiding this comment

Swiftb0y Mar 23, 2023

Choose a reason for hiding this comment

fwcd Mar 23, 2023

Choose a reason for hiding this comment

Swiftb0y Mar 23, 2023

Choose a reason for hiding this comment

fwcd Mar 24, 2023

Choose a reason for hiding this comment

fwcd commented Mar 29, 2023

Swiftb0y left a comment

Choose a reason for hiding this comment

Swiftb0y commented Mar 30, 2023

fwcd commented Apr 3, 2023

Swiftb0y commented Apr 3, 2023

daschuer commented Apr 3, 2023

daschuer left a comment

Choose a reason for hiding this comment

uklotzde commented Apr 4, 2023

ronso0 Apr 23, 2023 • edited Loading

Choose a reason for hiding this comment

iTunes: Modularize importer and use `iTunesLibrary` on macOS for compatibility with `Music.app` #11353

iTunes: Modularize importer and use `iTunesLibrary` on macOS for compatibility with `Music.app` #11353

fwcd commented Mar 10, 2023 •

edited

Loading

Swiftb0y commented Mar 14, 2023 •

edited

Loading

fwcd commented Mar 14, 2023 •

edited

Loading

ronso0 Apr 23, 2023 •

edited

Loading