From 0473d2845988e8ed65dacb86a87b627da35d9964 Mon Sep 17 00:00:00 2001 From: Marcel van der Veldt Date: Tue, 24 Feb 2026 21:55:48 +0100 Subject: [PATCH] Fix double restart of playback when switching output protocol --- .../controllers/players/controller.py | 56 ++++---- .../controllers/players/protocol_linking.py | 2 +- music_assistant/providers/airplay/player.py | 2 - tests/core/test_protocol_linking.py | 133 +++++++++++++++++- 4 files changed, 157 insertions(+), 36 deletions(-) diff --git a/music_assistant/controllers/players/controller.py b/music_assistant/controllers/players/controller.py index 2b15bad8..fd47bccd 100644 --- a/music_assistant/controllers/players/controller.py +++ b/music_assistant/controllers/players/controller.py @@ -2392,13 +2392,6 @@ class PlayerController(ProtocolLinkingMixin, CoreController): ) return # For regular players, handle protocol selection and translation - # Store playback state before changing members to detect protocol changes - was_playing = parent_player.playback_state in ( - PlaybackState.PLAYING, - PlaybackState.PAUSED, - ) - previous_protocol = parent_player.active_output_protocol if was_playing else None - await self._handle_set_members_with_protocols( parent_player, final_player_ids_to_add, final_player_ids_to_remove ) @@ -2406,28 +2399,6 @@ class PlayerController(ProtocolLinkingMixin, CoreController): if should_stop: # Stop playback on the player if it is being removed from itself await self._handle_cmd_stop(parent_player.player_id) - return - - # Check if protocol changed due to member change and restart playback if needed - if not should_stop and was_playing: - # Determine which protocol would be used now with new members - _new_target_player, new_protocol = self._select_best_output_protocol(parent_player) - new_protocol_id = new_protocol.output_protocol_id if new_protocol else "native" - previous_protocol_id = previous_protocol or "native" - - # If protocol changed, restart playback - if new_protocol_id != previous_protocol_id: - self.logger.info( - "Protocol changed from %s to %s due to member change, restarting playback", - previous_protocol_id, - new_protocol_id, - ) - # Restart playback on the new protocol using resume - await self.cmd_resume( - parent_player.player_id, - parent_player.state.active_source, - parent_player.state.current_media, - ) async def _handle_set_members_with_protocols( self, @@ -2753,8 +2724,25 @@ class PlayerController(ProtocolLinkingMixin, CoreController): if media.source_id: player.set_active_mass_source(media.source_id) - # Select best output protocol for playback - target_player, output_protocol = self._select_best_output_protocol(player) + # Determine output protocol to use: + # If player already has an active protocol set, prefer that. + # Otherwise, select best protocol based on current state. + if ( + player.active_output_protocol + and player.active_output_protocol != "native" + and (target_player := self.get_player(player.active_output_protocol)) + ): + # Use the already-set protocol directly + output_protocol = next( + ( + p + for p in player.linked_output_protocols + if p.output_protocol_id == player.active_output_protocol + ), + None, + ) + else: + target_player, output_protocol = self._select_best_output_protocol(player) if target_player.player_id != player.player_id: # Playing via linked protocol - update active output protocol @@ -2891,7 +2879,11 @@ class PlayerController(ProtocolLinkingMixin, CoreController): # handle command on player(protocol) directly await target_player.stop() - player.set_active_output_protocol(None) # also clear active protocol if any + # Only clear active protocol if the protocol player has no remaining group members. + # If there are still protocol group members, keep the protocol active so that + # when playback resumes it continues on the same protocol. + if target_player.player_id == player.player_id or len(target_player.group_members) <= 1: + player.set_active_output_protocol(None) async def _handle_cmd_play(self, player_id: str) -> None: """ diff --git a/music_assistant/controllers/players/protocol_linking.py b/music_assistant/controllers/players/protocol_linking.py index dcf86621..396eb3f8 100644 --- a/music_assistant/controllers/players/protocol_linking.py +++ b/music_assistant/controllers/players/protocol_linking.py @@ -1442,7 +1442,7 @@ class ProtocolLinkingMixin: parent_protocol_player.provider.domain, ) # Use resume to restart from current position - await self.mass.players.cmd_resume(parent_player.player_id) + await self.mass.players._handle_cmd_resume(parent_player.player_id) self.logger.debug( "After set_members, protocol player %s state: group_members=%s, synced_to=%s", diff --git a/music_assistant/providers/airplay/player.py b/music_assistant/providers/airplay/player.py index ea0723ec..b8b76ae7 100644 --- a/music_assistant/providers/airplay/player.py +++ b/music_assistant/providers/airplay/player.py @@ -605,8 +605,6 @@ class AirPlayPlayer(Player): if not child_player_to_add: # should not happen, but guard against it continue - if child_player_to_add.synced_to and child_player_to_add.synced_to != self.player_id: - raise RuntimeError("Player is already synced to another player") # ensure the child does not have an existing stream session active if child_player_to_add := cast( diff --git a/tests/core/test_protocol_linking.py b/tests/core/test_protocol_linking.py index 14431d0a..8440fdb4 100644 --- a/tests/core/test_protocol_linking.py +++ b/tests/core/test_protocol_linking.py @@ -6,10 +6,11 @@ from unittest.mock import MagicMock import pytest from music_assistant_models.enums import ( IdentifierType, + PlaybackState, PlayerFeature, PlayerType, ) -from music_assistant_models.player import OutputProtocol +from music_assistant_models.player import OutputProtocol, PlayerMedia from music_assistant.controllers.players import PlayerController from music_assistant.helpers.throttle_retry import Throttler @@ -99,6 +100,31 @@ class MockPlayer(Player): async def stop(self) -> None: """Stop playback - required abstract method.""" + async def set_members( + self, + player_ids_to_add: list[str] | None = None, + player_ids_to_remove: list[str] | None = None, + ) -> None: + """Mock implementation of set_members.""" + current_members = set(getattr(self, "_attr_group_members", [])) + + if player_ids_to_add: + current_members.update(player_ids_to_add) + + if player_ids_to_remove: + current_members.difference_update(player_ids_to_remove) + + # Always include self as first member if there are members + if current_members: + self._attr_group_members = [self.player_id] + [ + pid for pid in current_members if pid != self.player_id + ] + else: + self._attr_group_members = [] + + # Clear cache to reflect changes + self._cache.clear() + @pytest.fixture def mock_mass() -> MagicMock: @@ -1965,6 +1991,111 @@ class TestProtocolSwitchingDuringPlayback: assert output_protocol is not None assert output_protocol.output_protocol_id == "airplay_sonos" + async def test_no_restart_from_handle_set_members(self, mock_mass: MagicMock) -> None: + """Test that _handle_set_members does NOT restart playback. + + Protocol switching and playback restarts are handled in _forward_protocol_set_members, + not in _handle_set_members. This test verifies that _handle_set_members doesn't + trigger any redundant playback restarts. + """ + controller = PlayerController(mock_mass) + + sonos_provider = MockProvider("sonos", instance_id="sonos_instance", mass=mock_mass) + airplay_provider = MockProvider("airplay", instance_id="airplay_instance", mass=mock_mass) + + # Create Sonos player currently playing via AirPlay + sonos_player = MockPlayer( + sonos_provider, + "sonos_123", + "Living Room", + identifiers={IdentifierType.MAC_ADDRESS: "AA:BB:CC:DD:EE:01"}, + ) + sonos_player._attr_supported_features.add(PlayerFeature.PLAY_MEDIA) + sonos_player._attr_supported_features.add(PlayerFeature.SET_MEMBERS) + sonos_player._attr_playback_state = PlaybackState.PLAYING + sonos_player._attr_group_members = ["sonos_123", "sonos_456"] + + # Create another Sonos player in the group (member of sonos_123's group) + sonos_player_b = MockPlayer( + sonos_provider, + "sonos_456", + "Kitchen", + identifiers={IdentifierType.MAC_ADDRESS: "AA:BB:CC:DD:EE:02"}, + ) + sonos_player_b._attr_supported_features.add(PlayerFeature.SET_MEMBERS) + # sonos_player_b's synced_to is derived from group_members, not a direct attribute + + # Create AirPlay protocol player (was used for grouping) + sonos_airplay = MockPlayer( + airplay_provider, + "airplay_sonos", + "Living Room (AirPlay)", + player_type=PlayerType.PROTOCOL, + identifiers={IdentifierType.MAC_ADDRESS: "AA:BB:CC:DD:EE:01"}, + ) + sonos_airplay._attr_supported_features.add(PlayerFeature.SET_MEMBERS) + sonos_airplay.set_protocol_parent_id("sonos_123") + + sonos_player.set_linked_output_protocols( + [ + OutputProtocol( + output_protocol_id="airplay_sonos", + name="AirPlay", + protocol_domain="airplay", + priority=10, + available=True, + ) + ] + ) + + mock_mass.players = controller + controller._players = { + "sonos_123": sonos_player, + "sonos_456": sonos_player_b, + "airplay_sonos": sonos_airplay, + } + controller._player_throttlers = { + "sonos_123": Throttler(1, 0.05), + "sonos_456": Throttler(1, 0.05), + "airplay_sonos": Throttler(1, 0.05), + } + + # Update state and set active output protocol AFTER registering with controller + sonos_player.update_state(signal_event=False) + sonos_player_b.update_state(signal_event=False) + sonos_airplay.update_state(signal_event=False) + + # Set active output protocol (must be done after controller is set up) + sonos_player.set_active_output_protocol("airplay_sonos") + + # Track if cmd_resume was called + resume_called = False + + async def mock_cmd_resume( + player_id: str, # noqa: ARG001 + source: str | None = None, # noqa: ARG001 + media: PlayerMedia | None = None, # noqa: ARG001 + ) -> None: + nonlocal resume_called + resume_called = True + + controller.cmd_resume = mock_cmd_resume # type: ignore[method-assign] + + # Remove member - now only the parent player is left + # After removal, _select_best_output_protocol would return native + sonos_player._attr_group_members = ["sonos_123"] + sonos_player._cache.clear() + + # Call _handle_set_members directly to trigger the protocol change check + await controller._handle_set_members( + sonos_player, + player_ids_to_add=None, + player_ids_to_remove=["sonos_456"], + ) + + # Playback should NOT have been restarted because we're going back to native + assert not resume_called, "cmd_resume should not be called when switching to native" + class TestNativeProtocolPlayerGrouping: """Tests for grouping with native protocol players (e.g., native AirPlay like Apple TV).""" -- 2.34.1