Additional logic and fixes for new group handling (#1706)
authorMarcel van der Veldt <m.vanderveldt@outlook.com>
Sat, 12 Oct 2024 01:23:56 +0000 (03:23 +0200)
committerGitHub <noreply@github.com>
Sat, 12 Oct 2024 01:23:56 +0000 (03:23 +0200)
music_assistant/server/controllers/player_queues.py
music_assistant/server/controllers/players.py
music_assistant/server/models/player_provider.py
music_assistant/server/providers/chromecast/__init__.py
music_assistant/server/providers/player_group/__init__.py
music_assistant/server/providers/sonos_s1/__init__.py

index 017cd6b90ff6aa99d24da16e748a80c500a9f0ae..5c4e41771d371d247324c95dcbbd783a21cfd452 100644 (file)
@@ -1,4 +1,15 @@
-"""Logic to play music from MusicProviders to supported players."""
+"""
+MusicAssistant Player Queues Controller.
+
+Handles all logic to PLAY Media Items, provided by Music Providers to supported players.
+
+It is loosely coupled to the MusicAssistant Music Controller and Player Controller.
+A Music Assistant Player always has a PlayerQueue associated with it
+which holds the queue items and state.
+
+The PlayerQueue is in that case the active source of the player,
+but it can also be something else, hence the loose coupling.
+"""
 
 from __future__ import annotations
 
@@ -848,6 +859,17 @@ class PlayerQueuesController(CoreController):
             raise PlayerUnavailableError("Queue {target_queue_id} is not available")
         if auto_play is None:
             auto_play = source_queue.state == PlayerState.PLAYING
+
+        target_player = self.mass.players.get(target_queue_id)
+        if target_player.active_group or target_player.synced_to:
+            # edge case: the user wants to move playback from the group as a whole, to a single
+            # player in the group or it is grouped and the command targeted at the single player.
+            # We need to dissolve the group first.
+            await self.mass.players.cmd_power(
+                target_player.active_group or target_player.synced_to, False
+            )
+            await asyncio.sleep(3)
+
         source_items = self._queue_items[source_queue_id]
         target_queue.repeat_mode = source_queue.repeat_mode
         target_queue.shuffle_enabled = source_queue.shuffle_enabled
@@ -858,6 +880,7 @@ class PlayerQueuesController(CoreController):
             target_queue.current_item = source_queue.current_item
             target_queue.current_item.queue_id = target_queue_id
         self.clear(source_queue_id)
+
         self.load(target_queue_id, source_items, keep_remaining=False, keep_played=False)
         for item in source_items:
             item.queue_id = target_queue_id
index 7d21b8203d16e5a0cce4f3d59962bb5f84faf250..b886274571ac7e2dff8be985c5307fd43b0bd51b 100644 (file)
@@ -1,4 +1,10 @@
-"""Logic to play music from MusicProviders to supported players."""
+"""
+MusicAssistant Players Controller.
+
+Handles all logic to control supported players,
+which are provided by Player Providers.
+
+"""
 
 from __future__ import annotations
 
@@ -253,7 +259,9 @@ class PlayerController(CoreController):
 
     @api_command("players/cmd/power")
     @handle_player_command
-    async def cmd_power(self, player_id: str, powered: bool, skip_redirect: bool = False) -> None:
+    async def cmd_power(
+        self, player_id: str, powered: bool, skip_redirect: bool = False, skip_update: bool = False
+    ) -> None:
         """Send POWER command to given player.
 
         - player_id: player_id of the player to handle the command.
@@ -264,13 +272,17 @@ class PlayerController(CoreController):
         if player.powered == powered:
             return  # nothing to do
 
-        # redirect to active group player if player is group child
-        if not skip_redirect and player.active_group:
-            if group_player_provider := self.get_player_provider(player.active_group):
-                async with self._player_throttlers[player.active_group]:
-                    await group_player_provider.on_group_child_power(
-                        player.active_group, player_id, powered
-                    )
+        if player.active_group and not powered and not skip_redirect:
+            # this is simply not possible (well, not without major headaches)
+            # the player is part of a permanent (sync)group and the user tries to power off
+            # one child player... we can't allow this, as it would break the group so we
+            # power off the whole group instead.
+            self.logger.info(
+                "Detected a power OFF command to player %s which is part of a (active) group. "
+                "This command will be redirected to the entire group.",
+                player.name,
+            )
+            await self.cmd_power(player.active_group, False)
             return
 
         # always stop player at power off
@@ -282,14 +294,13 @@ class PlayerController(CoreController):
             await self.cmd_stop(player_id)
 
         # unsync player at power off
-        if not powered and (
-            player.synced_to or (player.type == PlayerType.PLAYER and player.group_childs)
-        ):
+        if not powered and (player.synced_to):
             await self.cmd_unsync(player_id)
-        # elif not powered and player.type == PlayerType.PLAYER and player.group_childs:
-        #     async with TaskManager(self.mass) as tg:
-        #         for member in self.iter_group_members(player, True):
-        #             tg.create_task(self.cmd_power(member.player_id, False))
+        # power off all synced childs when player is a sync leader
+        elif not powered and player.type == PlayerType.PLAYER and player.group_childs:
+            async with TaskManager(self.mass) as tg:
+                for member in self.iter_group_members(player, True):
+                    tg.create_task(self.cmd_power(member.player_id, False))
 
         # handle actual power command
         if PlayerFeature.POWER in player.supported_features:
@@ -307,7 +318,9 @@ class PlayerController(CoreController):
         # reset active source on power off
         if not powered:
             player.active_source = None
-        self.update(player_id)
+
+        if not skip_update:
+            self.update(player_id)
 
         # handle 'auto play on power on' feature
         if (
@@ -515,6 +528,7 @@ class PlayerController(CoreController):
         finally:
             player.announcement_in_progress = False
 
+    @handle_player_command
     async def play_media(
         self, player_id: str, media: PlayerMedia, skip_redirect: bool = False
     ) -> None:
@@ -573,20 +587,68 @@ class PlayerController(CoreController):
             return
         if not (player.synced_to or player.group_childs):
             return  # nothing to do
+        if player.active_group:
+            raise PlayerCommandFailed(
+                "Command denied: player %s is part of (active) group %s",
+                player.display_name,
+                player.active_group,
+            )
+
+        if player.active_group:
+            # this is simply not possible (well, not without major headaches)
+            # the player is part of a permanent (sync)group and the user tries to unsync
+            # one child player... we can't allow this, as it would break the group so we
+            # power unsync the whole group instead.
+            self.logger.info(
+                "Detected a power OFF command to player %s which is part of a (active) group. "
+                "This command will be redirected by turning off the entire group!",
+                player.name,
+            )
+            await self.cmd_power(player.active_group, False)
+            return
+
+        # handle (edge)case where un unsync command is sent to a sync leader;
+        # we dissolve the entire syncgroup in this case.
+        # while maybe not strictly needed to do this for all player providers,
+        # we do this to keep the functionality consistent across all providers
+        if player.group_childs:
+            self.logger.warning(
+                "Detected unsync command to player %s which is a sync(group) leader, "
+                "all sync members will be unsynced!",
+                player.name,
+            )
+            async with TaskManager(self.mass) as tg:
+                for group_child_id in player.group_childs:
+                    if group_child_id == player_id:
+                        continue
+                    tg.create_task(self.cmd_unsync(group_child_id))
+            return
 
-        # reset active source player if it is unsynced
+        # (optimistically) reset active source player if it is unsynced
         player.active_source = None
+
         # forward command to the player provider
         if player_provider := self.get_player_provider(player_id):
             await player_provider.cmd_unsync(player_id)
+        # if the command succeeded we optimistically reset the sync state
+        # this is to prevent race conditions and to update the UI as fast as possible
+        player.synced_to = None
 
     @api_command("players/cmd/sync_many")
     async def cmd_sync_many(self, target_player: str, child_player_ids: list[str]) -> None:
         """Create temporary sync group by joining given players to target player."""
         parent_player: Player = self.get(target_player, True)
         if PlayerFeature.SYNC not in parent_player.supported_features:
-            msg = f"Player {parent_player.name} does not support (un)sync commands"
+            msg = f"Player {parent_player.name} does not support sync commands"
             raise UnsupportedFeaturedException(msg)
+
+        if parent_player.synced_to:
+            # guard edge case: player already synced to another player
+            raise PlayerCommandFailed(
+                f"Player {parent_player.name} is already synced to another player on its own, "
+                "you need to unsync it first before you can join other players to it.",
+            )
+
         # filter all player ids on compatibility and availability
         final_player_ids: UniqueList[str] = UniqueList()
         for child_player_id in child_player_ids:
@@ -596,25 +658,32 @@ class PlayerController(CoreController):
                 self.logger.warning("Player %s is not available", child_player_id)
                 continue
             if PlayerFeature.SYNC not in child_player.supported_features:
-                self.logger.warning(
-                    "Player %s does not support (un)sync commands", child_player.name
-                )
+                # this should not happen, but just in case bad things happen, guard it
+                self.logger.warning("Player %s does not support sync commands", child_player.name)
                 continue
             if child_player.synced_to and child_player.synced_to == target_player:
                 continue  # already synced to this target
-            elif child_player.synced_to:
+
+            if child_player.group_childs:
+                # guard edge case: childplayer is already a sync leader on its own
+                raise PlayerCommandFailed(
+                    f"Player {child_player.name} is already synced with other players, "
+                    "you need to unsync it first before you can join it to another player.",
+                )
+            if child_player.synced_to:
                 # player already synced to another player, unsync first
                 self.logger.warning(
-                    "Player %s is already synced, unsyncing first", child_player.name
+                    "Player %s is already synced to another player, unsyncing first",
+                    child_player.name,
                 )
                 await self.cmd_unsync(child_player.player_id)
             # power on the player if needed
             if not child_player.powered:
-                await self.cmd_power(child_player.player_id, True)
+                await self.cmd_power(child_player.player_id, True, skip_update=True)
             # if we reach here, all checks passed
             final_player_ids.append(child_player_id)
             # set active source if player is synced
-            child_player.active_source = parent_player.active_source
+            child_player.active_source = parent_player.player_id
 
         # forward command to the player provider after all (base) sanity checks
         player_provider = self.get_player_provider(target_player)
index e1afab7b65adf09f0f47ce1072f209bbcfb5c509..dbbead7f3ab6cc90a356ae0ceb1d5dc13df13146 100644 (file)
@@ -166,17 +166,6 @@ class PlayerProvider(Provider):
             # default implementation, simply call the cmd_sync for all child players
             await self.cmd_sync(child_id, target_player)
 
-    async def on_group_child_power(
-        self, group_player_id: str, child_player_id: str, powered: bool
-    ) -> None:
-        """Call when a child player of a group player is powered on/off."""
-        # default implementation, simply redirect the request to the group player
-        self.logger.warning(
-            "Detected a player power command to a player that is part of a group. "
-            "Redirecting to group player..."
-        )
-        await self.mass.players.cmd_power(group_player_id, powered)
-
     async def poll_player(self, player_id: str) -> None:
         """Poll player for state updates.
 
index 5121f71e7072aac998f1f968a57a668367e02577..84d7dd434ba02546fd8e4ed0f02c96879899cb47 100644 (file)
@@ -18,6 +18,7 @@ from pychromecast.discovery import CastBrowser, SimpleCastListener
 from pychromecast.socket_client import CONNECTION_STATUS_CONNECTED, CONNECTION_STATUS_DISCONNECTED
 
 from music_assistant.common.models.config_entries import (
+    BASE_PLAYER_CONFIG_ENTRIES,
     CONF_ENTRY_CROSSFADE_DURATION,
     CONF_ENTRY_CROSSFADE_FLOW_MODE_REQUIRED,
     ConfigEntry,
@@ -172,9 +173,13 @@ class ChromecastProvider(PlayerProvider):
     async def get_player_config_entries(self, player_id: str) -> tuple[ConfigEntry, ...]:
         """Return all (provider/player specific) Config Entries for the given player (if any)."""
         cast_player = self.castplayers.get(player_id)
+        if cast_player and cast_player.player.type == PlayerType.GROUP:
+            return (
+                *BASE_PLAYER_CONFIG_ENTRIES,
+                *PLAYER_CONFIG_ENTRIES,
+                CONF_ENTRY_SAMPLE_RATES_CAST_GROUP,
+            )
         base_entries = await super().get_player_config_entries(player_id)
-        if cast_player and cast_player.cast_info.is_audio_group:
-            return (*base_entries, *PLAYER_CONFIG_ENTRIES, CONF_ENTRY_SAMPLE_RATES_CAST_GROUP)
         return (*base_entries, *PLAYER_CONFIG_ENTRIES, CONF_ENTRY_SAMPLE_RATES_CAST)
 
     def on_player_config_changed(
@@ -207,9 +212,17 @@ class ChromecastProvider(PlayerProvider):
         castplayer = self.castplayers[player_id]
         if powered:
             await self._launch_app(castplayer)
-            return
-        # handle power off
-        await asyncio.to_thread(castplayer.cc.quit_app)
+        else:
+            castplayer.player.active_group = None
+            castplayer.player.active_source = None
+            await asyncio.to_thread(castplayer.cc.quit_app)
+        # optimistically update the group childs
+        if castplayer.player.type == PlayerType.GROUP:
+            active_group = castplayer.player.active_group or castplayer.player.player_id
+            for child_id in castplayer.player.group_childs:
+                if child := self.castplayers.get(child_id):
+                    child.player.powered = powered
+                    child.player.active_group = active_group if powered else None
 
     async def cmd_volume_set(self, player_id: str, volume_level: int) -> None:
         """Send VOLUME_SET command to given player."""
@@ -382,7 +395,7 @@ class ChromecastProvider(PlayerProvider):
             self.castplayers[player_id] = castplayer
 
             castplayer.status_listener = CastStatusListener(self, castplayer, self.mz_mgr)
-            if cast_info.is_audio_group and not cast_info.is_multichannel_group:
+            if castplayer.player.type == PlayerType.GROUP:
                 mz_controller = MultizoneController(cast_info.uuid)
                 castplayer.cc.register_handler(mz_controller)
                 castplayer.mz_controller = mz_controller
@@ -474,16 +487,18 @@ class ChromecastProvider(PlayerProvider):
 
         # active source
         if group_player:
-            castplayer.player.active_source = group_player.player.active_source
-            castplayer.player.active_group = group_player.player.player_id
+            castplayer.player.active_source = (
+                group_player.player.active_source or group_player.player.player_id
+            )
+            castplayer.player.active_group = (
+                group_player.player.active_group or group_player.player.player_id
+            )
         elif castplayer.cc.app_id == MASS_APP_ID:
             castplayer.player.active_source = castplayer.player_id
-            castplayer.player.active_group = None
         else:
             castplayer.player.active_source = castplayer.cc.app_display_name
-            castplayer.player.active_group = None
 
-        if status.content_id:
+        if status.content_id and not status.player_is_idle:
             castplayer.player.current_media = PlayerMedia(
                 uri=status.content_id,
                 title=status.title,
@@ -496,7 +511,24 @@ class ChromecastProvider(PlayerProvider):
         else:
             castplayer.player.current_media = None
 
-        # current media
+        # weird workaround which is needed for multichannel group childs
+        # (e.g. a stereo pair within a cast group)
+        # where it does not receive updates from the group,
+        # so we need to update the group child(s) manually
+        if castplayer.player.type == PlayerType.GROUP and castplayer.player.powered:
+            for child_id in castplayer.player.group_childs:
+                if child := self.castplayers.get(child_id):
+                    if not child.cast_info.is_multichannel_group:
+                        continue
+                    child.player.state = castplayer.player.state
+                    child.player.current_media = castplayer.player.current_media
+                    child.player.elapsed_time = castplayer.player.elapsed_time
+                    child.player.elapsed_time_last_updated = (
+                        castplayer.player.elapsed_time_last_updated
+                    )
+                    child.player.active_source = castplayer.player.active_source
+                    child.player.active_group = castplayer.player.active_group
+
         self.mass.loop.call_soon_threadsafe(self.mass.players.update, castplayer.player_id)
 
     def on_new_connection_status(self, castplayer: CastPlayer, status: ConnectionStatus) -> None:
@@ -527,7 +559,7 @@ class ChromecastProvider(PlayerProvider):
                 manufacturer=castplayer.cast_info.manufacturer,
             )
             self.mass.loop.call_soon_threadsafe(self.mass.players.update, castplayer.player_id)
-            if new_available and not castplayer.cast_info.is_audio_group:
+            if new_available and castplayer.player.type != PlayerType.GROUP:
                 # Poll current group status
                 for group_uuid in self.mz_mgr.get_multizone_memberships(castplayer.cast_info.uuid):
                     group_media_controller = self.mz_mgr.get_multizone_mediacontroller(group_uuid)
index 8d08bf1ede48d3383e83dfb54acc4428fc8fc2a1..421c2c3d24f7cfe1eae49cbd129853cc9f6e417a 100644 (file)
@@ -8,6 +8,7 @@ allowing the user to create 'presets' of players to sync together (of the same t
 from __future__ import annotations
 
 from collections.abc import Callable
+from contextlib import suppress
 from time import time
 from typing import TYPE_CHECKING, Final, cast
 
@@ -37,6 +38,7 @@ from music_assistant.common.models.enums import (
     ProviderFeature,
 )
 from music_assistant.common.models.errors import (
+    PlayerUnavailableError,
     ProviderUnavailableError,
     UnsupportedFeaturedException,
 )
@@ -132,11 +134,6 @@ async def get_config_entries(
 class PlayerGroupProvider(PlayerProvider):
     """Base/builtin provider for creating (permanent) player groups."""
 
-    @property
-    def supported_features(self) -> tuple[ProviderFeature, ...]:
-        """Return the features supported by this Provider."""
-        return ()
-
     def __init__(
         self, mass: MusicAssistant, manifest: ProviderManifest, config: ProviderConfig
     ) -> None:
@@ -393,10 +390,10 @@ class PlayerGroupProvider(PlayerProvider):
         """Handle PLAY MEDIA on given player."""
         group_player = self.mass.players.get(player_id)
         # power on (or resync) if needed
-        if not group_player.powered:
-            await self.cmd_power(player_id, True)
-        elif player_id.startswith(SYNCGROUP_PREFIX):
+        if group_player.powered and player_id.startswith(SYNCGROUP_PREFIX):
             await self._sync_syncgroup(group_player)
+        else:
+            await self.cmd_power(player_id, True)
 
         # set the state optimistically
         group_player.current_media = media
@@ -463,6 +460,7 @@ class PlayerGroupProvider(PlayerProvider):
                             title=group_player.display_name,
                             queue_id=group_player.player_id,
                         ),
+                        skip_redirect=True,
                     )
                 )
 
@@ -473,7 +471,7 @@ class PlayerGroupProvider(PlayerProvider):
             # this shouldn't happen, but just in case
             raise UnsupportedFeaturedException("Command is not supported for UGP players")
         if sync_leader := self._get_sync_leader(group_player):
-            await self.enqueue_next_media(
+            await self.mass.players.enqueue_next_media(
                 sync_leader.player_id,
                 media=media,
             )
@@ -526,18 +524,23 @@ class PlayerGroupProvider(PlayerProvider):
                 continue  # already registered
             members = player_config.get_value(CONF_GROUP_MEMBERS)
             group_type = player_config.get_value(CONF_GROUP_TYPE)
-            self._register_group_player(
-                player_config.player_id,
-                group_type,
-                player_config.name or player_config.default_name,
-                members,
-            )
+            with suppress(PlayerUnavailableError):
+                self._register_group_player(
+                    player_config.player_id,
+                    group_type,
+                    player_config.name or player_config.default_name,
+                    members,
+                )
 
     def _register_group_player(
         self, group_player_id: str, group_type: str, name: str, members: Iterable[str]
     ) -> Player:
         """Register a syncgroup player."""
         player_features = {PlayerFeature.POWER, PlayerFeature.VOLUME_SET}
+
+        if not (self.mass.players.get(x) for x in members):
+            raise PlayerUnavailableError("One or more members are not available!")
+
         if group_type == GROUP_TYPE_UNIVERSAL:
             model_name = "Universal Group"
             manufacturer = self.name
@@ -552,17 +555,14 @@ class PlayerGroupProvider(PlayerProvider):
                 player_provider = cast(PlayerProvider, player_provider)
             model_name = "Sync Group"
             manufacturer = self.mass.get_provider(group_type).name
-            if child_player := next((x for x in player_provider.players), None):
-                for feature in (
-                    PlayerFeature.PAUSE,
-                    PlayerFeature.VOLUME_MUTE,
-                ):
-                    if feature in child_player.supported_features:
-                        player_features.add(feature)
+            for feature in (
+                PlayerFeature.PAUSE,
+                PlayerFeature.VOLUME_MUTE,
+            ):
+                if all(x for x in player_provider.players if feature in x.supported_features):
+                    player_features.add(feature)
         else:
-            # this may happen if the provider is not available yet
-            model_name = "Sync Group"
-            manufacturer = self.name
+            raise PlayerUnavailableError(f"Provider for syncgroup {group_type} is not available!")
 
         player = Player(
             player_id=group_player_id,
index 6d6d075cfbcca738ac025263d08c909c1c2df170..fb49555b55b4c30e46a98f680190fd511e97e778 100644 (file)
@@ -76,7 +76,6 @@ async def setup(
 ) -> ProviderInstanceType:
     """Initialize provider(instance) with given configuration."""
     soco_config.EVENTS_MODULE = events_asyncio
-    soco_config.REQUEST_TIMEOUT = 9.5
     zonegroupstate.EVENT_CACHE_TIMEOUT = SUBSCRIPTION_TIMEOUT
     prov = SonosPlayerProvider(mass, manifest, config)
     # set-up soco logging