From 1810e435bbe93b702149853011c21fda7e339a0b Mon Sep 17 00:00:00 2001 From: Marcel van der Veldt Date: Sat, 6 Jul 2024 00:14:06 +0200 Subject: [PATCH] A collection of small bugfixes and optimizations (#1451) --- .../common/models/config_entries.py | 6 ++- music_assistant/common/models/media_items.py | 18 ++++--- music_assistant/server/controllers/config.py | 2 +- music_assistant/server/helpers/tags.py | 7 ++- .../server/providers/airplay/__init__.py | 20 +++++++- .../server/providers/dlna/helpers.py | 14 ++++-- .../server/providers/filesystem_local/base.py | 37 ++++++++------ .../providers/filesystem_smb/__init__.py | 2 + .../server/providers/fully_kiosk/__init__.py | 15 ++++-- .../server/providers/hass/__init__.py | 3 +- .../server/providers/hass_players/__init__.py | 9 ++-- music_assistant/server/server.py | 49 ++++++++++--------- 12 files changed, 119 insertions(+), 63 deletions(-) diff --git a/music_assistant/common/models/config_entries.py b/music_assistant/common/models/config_entries.py index 7dc60b1d..84479dee 100644 --- a/music_assistant/common/models/config_entries.py +++ b/music_assistant/common/models/config_entries.py @@ -54,10 +54,10 @@ ConfigValueType = ( | int | float | bool + | list[tuple[int, int]] | tuple[int, int] | list[str] | list[int] - | list[tuple[int, int]] | Enum | None ) @@ -479,6 +479,10 @@ CONF_ENTRY_ENFORCE_MP3 = ConfigEntry( category="audio", ) +CONF_ENTRY_ENFORCE_MP3_DEFAULT_ENABLED = ConfigEntry.from_dict( + {**CONF_ENTRY_ENFORCE_MP3.to_dict(), "default_value": True} +) + CONF_ENTRY_SYNC_ADJUST = ConfigEntry( key=CONF_SYNC_ADJUST, type=ConfigEntryType.INTEGER, diff --git a/music_assistant/common/models/media_items.py b/music_assistant/common/models/media_items.py index 28a3a4cd..9a01a93a 100644 --- a/music_assistant/common/models/media_items.py +++ b/music_assistant/common/models/media_items.py @@ -479,12 +479,18 @@ class AlbumTrack(Track): ) -> AlbumTrack: """Cast Track to AlbumTrack.""" album_track = track.to_dict() - if album is None and track.album: - album_track["album"] = track.album - if disc_number is None: - album_track["disc_number"] = track.disc_number - if track_number is None: - album_track["track_number"] = track.track_number + if album_track["album"] is None: + if not album: + raise InvalidDataError("AlbumTrack requires an album") + album_track["album"] = album.to_dict() + if album_track["disc_number"] is None: + if disc_number is None: + raise InvalidDataError("AlbumTrack requires a disc_number") + album_track["disc_number"] = disc_number + if album_track["track_number"] is None: + if track_number is None: + raise InvalidDataError("AlbumTrack requires a track_number") + album_track["track_number"] = track_number # let mushmumaro instantiate a new object - this will ensure that valididation takes place return AlbumTrack.from_dict(album_track) diff --git a/music_assistant/server/controllers/config.py b/music_assistant/server/controllers/config.py index b378a091..85c45732 100644 --- a/music_assistant/server/controllers/config.py +++ b/music_assistant/server/controllers/config.py @@ -837,7 +837,7 @@ class ConfigController: deps.add(dep_prov.instance_id) await self.mass.unload_provider(dep_prov.instance_id) # (re)load the provider - await self.mass.load_provider(config.instance_id) + await self.mass.load_provider_config(config) # reload any dependants for dep in deps: conf = await self.get_provider_config(dep) diff --git a/music_assistant/server/helpers/tags.py b/music_assistant/server/helpers/tags.py index a28e72c7..8a882546 100644 --- a/music_assistant/server/helpers/tags.py +++ b/music_assistant/server/helpers/tags.py @@ -431,7 +431,12 @@ async def parse_tags( if not tags.duration and tags.raw.get("format", {}).get("duration"): tags.duration = float(tags.raw["format"]["duration"]) - if file_path.endswith(".mp3") and "musicbrainzrecordingid" not in tags.tags: + if ( + not file_path.startswith("http") + and file_path.endswith(".mp3") + and "musicbrainzrecordingid" not in tags.tags + and await asyncio.to_thread(os.path.isfile, file_path) + ): # eyed3 is able to extract the musicbrainzrecordingid from the unique file id # this is actually a bug in ffmpeg/ffprobe which does not expose this tag # so we use this as alternative approach for mp3 files diff --git a/music_assistant/server/providers/airplay/__init__.py b/music_assistant/server/providers/airplay/__init__.py index 6899e015..f5de17fd 100644 --- a/music_assistant/server/providers/airplay/__init__.py +++ b/music_assistant/server/providers/airplay/__init__.py @@ -115,6 +115,15 @@ AIRPLAY_PCM_FORMAT = AudioFormat( CONF_ENTRY_SAMPLE_RATES_AIRPLAY = create_sample_rates_config_entry(44100, 16, 44100, 16, True) +# TODO: Airplay provider +# - split up and cleanup the code into more digestable parts +# - Implement authentication for Apple TV +# - Implement volume control for Apple devices using pyatv +# - Implement metadata for Apple Apple devices using pyatv +# - Use pyatv for communicating with original Apple devices +# and use cliraop for actual streaming + + async def setup( mass: MusicAssistant, manifest: ProviderManifest, config: ProviderConfig ) -> ProviderInstanceType: @@ -975,14 +984,21 @@ class AirplayProvider(PlayerProvider): elif path in ("/ctrl-int/1/pause", "/ctrl-int/1/discrete-pause"): self.mass.create_task(self.mass.player_queues.pause(active_queue.queue_id)) elif "dmcp.device-volume=" in path: + if mass_player.device_info.manufacturer.lower() == "apple": + # Apple devices only report their (new) volume level, they dont request it + return raop_volume = float(path.split("dmcp.device-volume=", 1)[-1]) volume = convert_airplay_volume(raop_volume) - if abs(volume - mass_player.volume_level) > 2: + if volume != mass_player.volume_level: self.mass.create_task(self.cmd_volume_set(player_id, volume)) + # optimistically set the new volume to prevent bouncing around + mass_player.volume_level = volume elif "dmcp.volume=" in path: volume = int(path.split("dmcp.volume=", 1)[-1]) - if abs(volume - mass_player.volume_level) > 2: + if volume != mass_player.volume_level: self.mass.create_task(self.cmd_volume_set(player_id, volume)) + # optimistically set the new volume to prevent bouncing around + mass_player.volume_level = volume elif "device-prevent-playback=1" in path: # device switched to another source (or is powered off) if active_stream := airplay_player.active_stream: diff --git a/music_assistant/server/providers/dlna/helpers.py b/music_assistant/server/providers/dlna/helpers.py index 88161a12..f59f98de 100644 --- a/music_assistant/server/providers/dlna/helpers.py +++ b/music_assistant/server/providers/dlna/helpers.py @@ -5,6 +5,7 @@ from __future__ import annotations from typing import TYPE_CHECKING from aiohttp.web import Request, Response +from async_upnp_client.const import HttpRequest from async_upnp_client.event_handler import UpnpEventHandler, UpnpNotifyServer if TYPE_CHECKING: @@ -28,13 +29,18 @@ class DLNANotifyServer(UpnpNotifyServer): async def _handle_request(self, request: Request) -> Response: """Handle incoming requests.""" - headers = request.headers - body = await request.text() - if request.method != "NOTIFY": return Response(status=405) - status = await self.event_handler.handle_notify(headers, body) + # transform aiohttp request to async_upnp_client request + http_request = HttpRequest( + method=request.method, + url=request.url, + headers=request.headers, + body=await request.text(), + ) + + status = await self.event_handler.handle_notify(http_request) return Response(status=status) diff --git a/music_assistant/server/providers/filesystem_local/base.py b/music_assistant/server/providers/filesystem_local/base.py index c217664b..ec508b18 100644 --- a/music_assistant/server/providers/filesystem_local/base.py +++ b/music_assistant/server/providers/filesystem_local/base.py @@ -887,22 +887,29 @@ class FileSystemProviderBase(MusicProvider): elif await self.exists(name.title()): artist_path = name.title() + if artist_path: # noqa: SIM108 + # prefer the path as id + item_id = artist_path + else: + # simply use the album name as item id + item_id = name + artist = Artist( - item_id=artist_path or name, + item_id=item_id, provider=self.instance_id, name=name, sort_name=sort_name, provider_mappings={ ProviderMapping( - item_id=artist_path or name, + item_id=item_id, provider_domain=self.domain, provider_instance=self.instance_id, - url=artist_path or name, + url=artist_path, ) }, ) - if not await self.exists(artist_path): + if artist_path is None or not await self.exists(artist_path): # return basic object if there is no dedicated artist folder await self.mass.cache.set(cache_key, artist, expiration=120) return artist @@ -946,24 +953,26 @@ class FileSystemProviderBase(MusicProvider): cache_key = f"{self.instance_id}-albumdata-{name}-{album_path}" if cache := await self.mass.cache.get(cache_key): return cache - # create fake path if needed - if not album_path and artists: - album_path = artists[0].name + os.sep + name - elif not album_path: - album_path = name - if not name: - name = album_path.split(os.sep)[-1] + if album_path: + # prefer the path as id + item_id = album_path + elif artists: + # create fake item_id based on artist + album + item_id = artists[0].name + os.sep + name + else: + # simply use the album name as item id + album_path = name album = Album( - item_id=album_path, + item_id=item_id, provider=self.instance_id, name=name, sort_name=sort_name, artists=artists, provider_mappings={ ProviderMapping( - item_id=album_path, + item_id=item_id, provider_domain=self.domain, provider_instance=self.instance_id, url=album_path, @@ -976,7 +985,7 @@ class FileSystemProviderBase(MusicProvider): # hunt for additional metadata and images in the folder structure extra_path = os.path.dirname(track_path) if (track_path and not album_path) else None for folder_path in (disc_path, album_path, extra_path): - if not folder_path: + if not folder_path or not await self.exists(folder_path): continue nfo_file = os.path.join(folder_path, "album.nfo") if await self.exists(nfo_file): diff --git a/music_assistant/server/providers/filesystem_smb/__init__.py b/music_assistant/server/providers/filesystem_smb/__init__.py index 4fb2cf15..4ff43ee6 100644 --- a/music_assistant/server/providers/filesystem_smb/__init__.py +++ b/music_assistant/server/providers/filesystem_smb/__init__.py @@ -2,6 +2,7 @@ from __future__ import annotations +import os import platform from collections.abc import AsyncGenerator from typing import TYPE_CHECKING @@ -221,6 +222,7 @@ class SMBFileSystemProvider(LocalFileSystemProvider): [m.replace(password, "########") if password else m for m in mount_cmd], ) env_vars = { + **os.environ, "USER": username, } if password: diff --git a/music_assistant/server/providers/fully_kiosk/__init__.py b/music_assistant/server/providers/fully_kiosk/__init__.py index 575b1dbc..d7ea3fbd 100644 --- a/music_assistant/server/providers/fully_kiosk/__init__.py +++ b/music_assistant/server/providers/fully_kiosk/__init__.py @@ -12,7 +12,7 @@ from fullykiosk import FullyKiosk from music_assistant.common.models.config_entries import ( CONF_ENTRY_CROSSFADE, CONF_ENTRY_CROSSFADE_DURATION, - CONF_ENTRY_ENFORCE_MP3, + CONF_ENTRY_ENFORCE_MP3_DEFAULT_ENABLED, CONF_ENTRY_FLOW_MODE_ENFORCED, ConfigEntry, ConfigValueType, @@ -25,7 +25,13 @@ from music_assistant.common.models.enums import ( ) from music_assistant.common.models.errors import PlayerUnavailableError, SetupFailedError from music_assistant.common.models.player import DeviceInfo, Player, PlayerMedia -from music_assistant.constants import CONF_IP_ADDRESS, CONF_PASSWORD, CONF_PORT, VERBOSE_LOG_LEVEL +from music_assistant.constants import ( + CONF_ENFORCE_MP3, + CONF_IP_ADDRESS, + CONF_PASSWORD, + CONF_PORT, + VERBOSE_LOG_LEVEL, +) from music_assistant.server.models.player_provider import PlayerProvider if TYPE_CHECKING: @@ -35,7 +41,6 @@ if TYPE_CHECKING: from music_assistant.server.models import ProviderInstanceType AUDIOMANAGER_STREAM_MUSIC = 3 -CONF_ENFORCE_MP3 = "enforce_mp3" async def setup( @@ -164,7 +169,7 @@ class FullyKioskProvider(PlayerProvider): CONF_ENTRY_FLOW_MODE_ENFORCED, CONF_ENTRY_CROSSFADE, CONF_ENTRY_CROSSFADE_DURATION, - CONF_ENTRY_ENFORCE_MP3, + CONF_ENTRY_ENFORCE_MP3_DEFAULT_ENABLED, ) async def cmd_volume_set(self, player_id: str, volume_level: int) -> None: @@ -188,7 +193,7 @@ class FullyKioskProvider(PlayerProvider): ) -> None: """Handle PLAY MEDIA on given player.""" player = self.mass.players.get(player_id) - if self.mass.config.get_raw_player_config_value(player_id, CONF_ENFORCE_MP3, False): + if self.mass.config.get_raw_player_config_value(player_id, CONF_ENFORCE_MP3, True): media.uri = media.uri.replace(".flac", ".mp3") await self._fully.playSound(media.uri, AUDIOMANAGER_STREAM_MUSIC) player.current_media = media diff --git a/music_assistant/server/providers/hass/__init__.py b/music_assistant/server/providers/hass/__init__.py index 22ab0921..113599ac 100644 --- a/music_assistant/server/providers/hass/__init__.py +++ b/music_assistant/server/providers/hass/__init__.py @@ -165,7 +165,8 @@ class HomeAssistant(PluginProvider): try: await self.hass.connect() except BaseHassClientError as err: - raise SetupFailedError from err + err_msg = str(err) or err.__class__.__name__ + raise SetupFailedError(err_msg) from err self._listen_task = self.mass.create_task(self._hass_listener()) async def unload(self) -> None: diff --git a/music_assistant/server/providers/hass_players/__init__.py b/music_assistant/server/providers/hass_players/__init__.py index 1e303372..d24780a6 100644 --- a/music_assistant/server/providers/hass_players/__init__.py +++ b/music_assistant/server/providers/hass_players/__init__.py @@ -15,7 +15,7 @@ from music_assistant.common.helpers.datetime import from_iso_string from music_assistant.common.models.config_entries import ( CONF_ENTRY_CROSSFADE_DURATION, CONF_ENTRY_CROSSFADE_FLOW_MODE_REQUIRED, - CONF_ENTRY_ENFORCE_MP3, + CONF_ENTRY_ENFORCE_MP3_DEFAULT_ENABLED, CONF_ENTRY_FLOW_MODE_DEFAULT_ENABLED, ConfigEntry, ConfigValueOption, @@ -89,9 +89,6 @@ class MediaPlayerEntityFeature(IntFlag): CONF_ENFORCE_MP3 = "enforce_mp3" -CONF_ENTRY_ENFORCE_MP3_DEFAULT_ENABLED = ConfigEntry.from_dict( - {**CONF_ENTRY_ENFORCE_MP3.to_dict(), "default_value": True} -) PLAYER_CONFIG_ENTRIES = ( CONF_ENTRY_CROSSFADE_FLOW_MODE_REQUIRED, @@ -233,7 +230,7 @@ class HomeAssistantPlayers(PlayerProvider): async def play_media(self, player_id: str, media: PlayerMedia) -> None: """Handle PLAY MEDIA on given player.""" - if self.mass.config.get_raw_player_config_value(player_id, CONF_ENFORCE_MP3, False): + if self.mass.config.get_raw_player_config_value(player_id, CONF_ENFORCE_MP3, True): media.uri = media.uri.replace(".flac", ".mp3") await self.hass_prov.hass.call_service( domain="media_player", @@ -264,7 +261,7 @@ class HomeAssistantPlayers(PlayerProvider): async def enqueue_next_media(self, player_id: str, media: PlayerMedia) -> None: """Handle enqueuing of the next queue item on the player.""" - if self.mass.config.get_raw_player_config_value(player_id, CONF_ENFORCE_MP3, False): + if self.mass.config.get_raw_player_config_value(player_id, CONF_ENFORCE_MP3, True): media.uri = media.uri.replace(".flac", ".mp3") await self.hass_prov.hass.call_service( domain="media_player", diff --git a/music_assistant/server/server.py b/music_assistant/server/server.py index 65955443..28165736 100644 --- a/music_assistant/server/server.py +++ b/music_assistant/server/server.py @@ -420,29 +420,28 @@ class MusicAssistant: prov_conf: ProviderConfig, ) -> None: """Try to load a provider and catch errors.""" + # cancel existing (re)load timer if needed + task_id = f"load_provider_{prov_conf.instance_id}" + if existing := self._tracked_timers.pop(task_id, None): + existing.cancel() + try: await self._load_provider(prov_conf) # pylint: disable=broad-except except Exception as exc: - if isinstance(exc, MusicAssistantError): - LOGGER.error( - "Error loading provider(instance) %s: %s", - prov_conf.name or prov_conf.domain, - str(exc), - ) - else: - # log full stack trace on unhandled/generic exception - LOGGER.exception( - "Error loading provider(instance) %s", - prov_conf.name or prov_conf.domain, - ) + LOGGER.error( + "Error loading provider(instance) %s: %s", + prov_conf.name or prov_conf.instance_id, + str(exc) or exc.__class__.__name__, + # log full stack trace if debug logging is enabled + exc_info=exc if LOGGER.isEnabledFor(logging.DEBUG) else None, + ) raise async def load_provider( self, instance_id: str, - raise_on_error: bool = False, - schedule_retry: int | None = 10, + allow_retry: bool = False, ) -> None: """Try to load a provider and catch errors.""" try: @@ -455,25 +454,31 @@ class MusicAssistant: # Was disabled before we could run return + # cancel existing (re)load timer if needed + task_id = f"load_provider_{instance_id}" + if existing := self._tracked_timers.pop(task_id, None): + existing.cancel() + try: await self.load_provider_config(prov_conf) # pylint: disable=broad-except except Exception as exc: - if raise_on_error: - raise # if loading failed, we store the error in the config object # so we can show something useful to the user prov_conf.last_error = str(exc) self.config.set(f"{CONF_PROVIDERS}/{instance_id}/last_error", str(exc)) - # auto schedule a retry if the (re)load failed - if schedule_retry: + + # auto schedule a retry if the (re)load failed (handled exceptions only) + if isinstance(exc, MusicAssistantError) and allow_retry: self.call_later( - schedule_retry, + 300, self.load_provider, instance_id, - raise_on_error, - min(schedule_retry + 10, 600), + allow_retry, + task_id=task_id, ) + else: + raise async def unload_provider(self, instance_id: str) -> None: """Unload a provider.""" @@ -536,7 +541,7 @@ class MusicAssistant: for prov_conf in prov_configs: if not prov_conf.enabled: continue - tg.create_task(self.load_provider(prov_conf.instance_id)) + tg.create_task(self.load_provider(prov_conf.instance_id, allow_retry=True)) async def _load_provider(self, conf: ProviderConfig) -> None: """Load (or reload) a provider.""" -- 2.34.1