Interface with macOS's media controls and now playing info in Control Center #4809

fwcd · 2022-06-18T04:05:55Z

This branch exposes Mixxx's playback state to macOS through the Media Player framework, making it externally controllable (e.g. via control center, media keys etc.):

Some notes on the implementation:

The implementation of MacOSMediaPlayerService is written in Objective-C++. This should not be an issue, given that Clang handles ObjC++ natively pretty well, but something to be aware of (I needed to tweak the clang-format config, since it would otherwise fail when trying to format some ObjC constructs)
The API of the MacOSMediaPlayerService closely follows the ScrobblingService from expose current track metadata to external APIs #3483, to make integration with the more general abstractions from the other PR for exposing playback metadata easy (in the future).
- Ideally, all of the general challenges surrounding exposing playback metadata should be handled abstractly (e.g. figuring out which decks to play/pause etc.), so the service implementations can focus on the platform-specifics (e.g. macOS in this case or MPRIS, ...)

To do

Future Directions

Compute correct rate from bpm adjustment in slotBroadcastCurrentTrack. This isn't that urgent though since the playback position corrects itself ~every second in slotPlayPositionChanged and might just be nice to have a smoother slider.
Implement ScrobblingService interface from expose current track metadata to external APIs #3483 (once ready)

daschuer

Nice!1 Thank you very much for picking this up.

src/broadcast/macos/macosmediaplayerservice.mm

github-actions · 2022-09-18T00:09:12Z

This PR is marked as stale because it has been open 90 days with no activity.

fwcd · 2023-03-18T01:52:50Z

src/broadcast/macos/macosmediaplayerservice.mm

+MacOSMediaPlayerService::~MacOSMediaPlayerService() {
+    for (DeckAttributes* attributes : qAsConst(m_deckAttributes)) {
+        delete attributes;
+    }
+}


Implementation note: This approach of managing memory manually is based on

mixxx/src/broadcast/mpris/mprisplayer.cpp

Lines 76 to 80 in 50c981c

MprisPlayer::~MprisPlayer() {

for (DeckAttributes* attrib : qAsConst(m_deckAttributes)) {

delete attrib;

}

}

from #3483, I haven't found a better way to do this though since DeckAttributes doesn't seem to provide an easy way to hook itself into the QObject graph.

Swiftb0y · 2023-03-18T12:34:12Z

Since this involves Obj-C code again, I'd step up to review this, but I have a little too much on my plate right now. Feel free to ping me sometimes so I don't forget about this and can address it when I have my time. Thank you.

Swiftb0y · 2023-03-30T15:05:35Z

There are still unaddressed comments from previous reviews. Also IMO, we should really introduce an abstraction for these external player concepts first. Otherwise we end up duplicating lots of code. I think #3483 contains that but its quite big for review. It would be nice if that abstraction could be factored out first...

fwcd · 2023-04-19T15:32:13Z

There are still unaddressed comments from previous reviews.

Do you mean #4809 (comment)?

It would be nice if that abstraction could be factored out first...

I agree, would be really cool to have the more general abstraction for this first.

Swiftb0y · 2023-04-19T16:58:40Z

There are still unaddressed comments from previous reviews.

Do you mean #4809 (comment)?

Yes.

It would be nice if that abstraction could be factored out first...

I agree, would be really cool to have the more general abstraction for this first.

Do you have interest in working on that?

fwcd · 2023-04-19T17:41:05Z

Do you have interest in working on that?

I am afraid I don't really have the time to dig into that and I'm not really familiar with the relevant design and implementation choices there.

Yes.

As mentioned, I think the current solution should work fine, since we create a new dictionary per track. The isNull case can occur

mixxx/src/library/coverartcache.cpp

Line 97 in 6b1844e

emit coverFound(pRequestor, coverInfo, QPixmap(), requestedCacheKey, false);

An assert wouldn't work therefore. So if there's anything to do there, I'd appreciate some more feedback.

daschuer · 2023-10-16T20:21:01Z

What is the sate here? Is that still working?
Can you resolve the conflict?

fwcd · 2023-10-16T20:46:49Z

Sure! My hope was that we could get the interface from #3483 merged first, but otherwise the PR should be pretty much ready (though it could of course use some battle-testing).

fwcd · 2023-10-16T20:52:22Z

src/broadcast/macos/macosmediaplayerservice.mm

+} // anonymous namespace
+
+MacOSMediaPlayerService::MacOSMediaPlayerService(
+        PlayerManagerInterface& pPlayerManager) {


There are a few design questions that might be interesting though, especially for the "interface PR": Given that this service also handles control in both directions (play/pause, seeking), the question would be if the scrobbling service interface should support that too. Currently the MacOSMediaPlayerService simply takes a play manager and then wires itself up to that, but perhaps we'd want to take the opportunity and abstract over that too.

- Stub out MacOSMediaPlayerService - Broadcast now playing info to MPNowPlayingInfoCenter - Wire up MacOSMediaPlayerService - Remove unused dependency - Set now playing playback state - Define __MACOS_MEDIAPLAYER__ publicly - Set asset URL and media type for now playing - Set some dummy values for playback rate and time - Set MPNowPlayingInfoPropertyIsLiveStream - Register play command in MPRemoteCommandCenter - Successfully load cover art asynchronously to macOS media player - Handle case where broadcasted track is null - Add moc include - Use new CoverArtCache API

acolombier

This is a great feature! Just a small suggestion, but happy to approve and merge otherwise!

acolombier · 2025-02-05T18:38:29Z

src/broadcast/macos/macosmediaplayerservice.mm

+        const CoverInfo& coverInfo,
+        const QPixmap& pixmap) {
+    Q_UNUSED(coverInfo);


Suggested change

const CoverInfo& coverInfo,

const QPixmap& pixmap) {

Q_UNUSED(coverInfo);

const CoverInfo&,

const QPixmap& pixmap) {

As per our new guideline on this, introduced a few hours ago ;)

src/broadcast/macos/macosmediaplayerservice.mm

acolombier · 2025-02-05T18:41:42Z

src/coreservices.cpp

+#ifdef __MACOS_MEDIAPLAYER__
+    m_pMacOSMediaPlayerService = std::make_shared<MacOSMediaPlayerService>(*m_pPlayerManager);
+#endif


This means that this feature will always be enabled for user (since I assume we will probably ship a Mixxx release with MACOS_MEDIAPLAYER=on), however I can imagine some user not wanting this integration to be on, so would you consider adding a user setting for this? Note that I'm not a Mac user, so happy to ignore that one!

Good idea, I'm not sure where to put that though. Ideally, this should probably go into DlgPrefMetadata as proposed in https://github.com/mixxxdj/mixxx/pull/3483/files#diff-25906de66775af0b1495dd1a528cc4256dd1de4778a23a494db27fbb4c164a76

Yeah I think it sounds sensible to unify it with #3483 !

github-actions bot added the build label Jun 18, 2022

daschuer reviewed Jun 19, 2022

View reviewed changes

src/broadcast/macos/macosmediaplayerservice.mm Outdated Show resolved Hide resolved

src/broadcast/macos/macosmediaplayerservice.mm Show resolved Hide resolved

src/broadcast/macos/macosmediaplayerservice.mm Show resolved Hide resolved

github-actions bot added the stale Stale issues that haven't been updated for a long time. label Sep 18, 2022

fwcd force-pushed the macos-media-player-service branch from 9670686 to 1a303bd Compare October 3, 2022 14:27

ronso0 added macos and removed stale Stale issues that haven't been updated for a long time. labels Oct 3, 2022

fwcd mentioned this pull request Feb 8, 2023

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

Closed

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-media-player-service branch 3 times, most recently from 328b63f to 8f5ccb8 Compare March 18, 2023 01:46

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

fwcd commented Mar 18, 2023

View reviewed changes

fwcd marked this pull request as ready for review March 18, 2023 02:18

fwcd force-pushed the macos-media-player-service branch from fb7b792 to dd58d98 Compare March 30, 2023 00:10

fwcd commented Oct 16, 2023

View reviewed changes

fwcd force-pushed the macos-media-player-service branch from a30cf7a to 915dfcf Compare October 27, 2023 12:03

github-actions bot added the broadcast Bugs pertaining to streaming radio broadcaster use-case label Oct 27, 2023

fwcd mentioned this pull request Nov 6, 2023

(WIP) Axka changes #12250

Draft

JoergAtGithub added the needs review label Dec 10, 2023

fwcd force-pushed the macos-media-player-service branch from e7502ca to e4029b8 Compare July 23, 2024 00:03

fwcd and others added 11 commits July 23, 2024 02:50

MacOSMediaPlayerService: Move connects into constructor

1ebca06

MacOSMediaPlayerService: Report playback position

99ee5d3

MacOSMediaPlayerService: Throttle play position updates

214bbcc

MacOSMediaPlayerService: Implement external controls

60d0f27

MacOSMediaPlayerService: Consolidate anonymous namespaces

d458f58

MacOSMediaPlayerService: Implement AutoDJ-based skipping

e5d5087

MacOSMediaPlayerService: Add doc comments

ce8df1d

MacOSMediaPlayerService: Factor out command handlers

1bc1796

MacOSMediaPlayerService: Fix style issue

73c0f72

MacOSMediaPlayerService: Add Q_UNUSED to suppress warnings

dce0c4c

fwcd force-pushed the macos-media-player-service branch from e4029b8 to dce0c4c Compare July 23, 2024 00:51

fwcd mentioned this pull request Jul 23, 2024

clang-format: Indent Objective-C blocks with 4 spaces #13503

Merged

acolombier reviewed Feb 5, 2025

View reviewed changes

JoergAtGithub mentioned this pull request Feb 18, 2025

expose current track metadata to external APIs #3483

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interface with macOS's media controls and now playing info in Control Center #4809

Interface with macOS's media controls and now playing info in Control Center #4809

fwcd commented Jun 18, 2022 •

edited

Loading

daschuer left a comment

github-actions bot commented Sep 18, 2022

fwcd Mar 18, 2023

Swiftb0y commented Mar 18, 2023

Swiftb0y commented Mar 30, 2023

fwcd commented Apr 19, 2023

Swiftb0y commented Apr 19, 2023

fwcd commented Apr 19, 2023

daschuer commented Oct 16, 2023

fwcd commented Oct 16, 2023

fwcd Oct 16, 2023

acolombier left a comment

acolombier Feb 5, 2025

acolombier Feb 5, 2025

acolombier Feb 5, 2025

fwcd Feb 18, 2025

acolombier Feb 18, 2025

	MprisPlayer::~MprisPlayer() {
	for (DeckAttributes* attrib : qAsConst(m_deckAttributes)) {
	delete attrib;
	}
	}

Interface with macOS's media controls and now playing info in Control Center #4809

Are you sure you want to change the base?

Interface with macOS's media controls and now playing info in Control Center #4809

Conversation

fwcd commented Jun 18, 2022 • edited Loading

To do

Future Directions

daschuer left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 18, 2022

fwcd Mar 18, 2023

Choose a reason for hiding this comment

Swiftb0y commented Mar 18, 2023

Swiftb0y commented Mar 30, 2023

fwcd commented Apr 19, 2023

Swiftb0y commented Apr 19, 2023

fwcd commented Apr 19, 2023

daschuer commented Oct 16, 2023

fwcd commented Oct 16, 2023

fwcd Oct 16, 2023

Choose a reason for hiding this comment

acolombier left a comment

Choose a reason for hiding this comment

acolombier Feb 5, 2025

Choose a reason for hiding this comment

acolombier Feb 5, 2025

Choose a reason for hiding this comment

acolombier Feb 5, 2025

Choose a reason for hiding this comment

fwcd Feb 18, 2025

Choose a reason for hiding this comment

acolombier Feb 18, 2025

Choose a reason for hiding this comment

fwcd commented Jun 18, 2022 •

edited

Loading