-
-
Notifications
You must be signed in to change notification settings - Fork 198
Improve Spotify stream start time and fallback #2502
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -202,7 +202,6 @@ async def get_library_playlists(self) -> AsyncGenerator[Playlist, None]: | |
| if item and item["id"]: | ||
| yield parse_playlist(item, self) | ||
|
|
||
| # ruff: noqa: PLR0915 | ||
| @use_cache() | ||
| async def search( | ||
| self, search_query: str, media_types: list[MediaType] | None = None, limit: int = 5 | ||
|
|
@@ -214,9 +213,32 @@ async def search( | |
| :param limit: Number of items to return in the search (per type). | ||
| """ | ||
| searchresult = SearchResults() | ||
| searchtypes = [] | ||
| if media_types is None: | ||
| return searchresult | ||
|
|
||
| searchtype = self._build_search_types(media_types) | ||
| if not searchtype: | ||
| return searchresult | ||
|
|
||
| search_query = search_query.replace("'", "") | ||
| offset = 0 | ||
| page_limit = min(limit, 50) | ||
|
|
||
| while True: | ||
| api_result = await self._get_data( | ||
| "search", q=search_query, type=searchtype, limit=page_limit, offset=offset | ||
| ) | ||
| items_received = self._process_search_results(api_result, searchresult) | ||
|
|
||
| offset += page_limit | ||
| if offset >= limit or items_received < page_limit: | ||
| break | ||
|
|
||
| return searchresult | ||
|
|
||
| def _build_search_types(self, media_types: list[MediaType]) -> str: | ||
| """Build comma-separated search types string from media types.""" | ||
| searchtypes = [] | ||
| if MediaType.ARTIST in media_types: | ||
| searchtypes.append("artist") | ||
| if MediaType.ALBUM in media_types: | ||
|
|
@@ -229,75 +251,76 @@ async def search( | |
| searchtypes.append("show") | ||
| if MediaType.AUDIOBOOK in media_types and self.audiobooks_supported: | ||
| searchtypes.append("audiobook") | ||
| if not searchtypes: | ||
| return searchresult | ||
| searchtype = ",".join(searchtypes) | ||
| search_query = search_query.replace("'", "") | ||
| offset = 0 | ||
| page_limit = min(limit, 50) | ||
| while True: | ||
| items_received = 0 | ||
| api_result = await self._get_data( | ||
| "search", q=search_query, type=searchtype, limit=page_limit, offset=offset | ||
| ) | ||
| if "artists" in api_result: | ||
| artists = [ | ||
| parse_artist(item, self) | ||
| for item in api_result["artists"]["items"] | ||
| if (item and item["id"] and item["name"]) | ||
| ] | ||
| searchresult.artists = [*searchresult.artists, *artists] | ||
| items_received += len(api_result["artists"]["items"]) | ||
| if "albums" in api_result: | ||
| albums = [ | ||
| parse_album(item, self) | ||
| for item in api_result["albums"]["items"] | ||
| if (item and item["id"]) | ||
| ] | ||
| searchresult.albums = [*searchresult.albums, *albums] | ||
| items_received += len(api_result["albums"]["items"]) | ||
| if "tracks" in api_result: | ||
| tracks = [ | ||
| parse_track(item, self) | ||
| for item in api_result["tracks"]["items"] | ||
| if (item and item["id"]) | ||
| ] | ||
| searchresult.tracks = [*searchresult.tracks, *tracks] | ||
| items_received += len(api_result["tracks"]["items"]) | ||
| if "playlists" in api_result: | ||
| playlists = [ | ||
| parse_playlist(item, self) | ||
| for item in api_result["playlists"]["items"] | ||
| if (item and item["id"]) | ||
| ] | ||
| searchresult.playlists = [*searchresult.playlists, *playlists] | ||
| items_received += len(api_result["playlists"]["items"]) | ||
| if "shows" in api_result: | ||
| podcasts = [] | ||
| for item in api_result["shows"]["items"]: | ||
| if not (item and item["id"]): | ||
| continue | ||
| # Filter out audiobooks - they have a distinctive description format | ||
| description = item.get("description", "") | ||
| if description.startswith("Author(s):") and "Narrator(s):" in description: | ||
| continue | ||
| podcasts.append(parse_podcast(item, self)) | ||
| searchresult.podcasts = [*searchresult.podcasts, *podcasts] | ||
| items_received += len(api_result["shows"]["items"]) | ||
| if "audiobooks" in api_result and self.audiobooks_supported: | ||
| audiobooks = [ | ||
| parse_audiobook(item, self) | ||
| for item in api_result["audiobooks"]["items"] | ||
| if (item and item["id"]) | ||
| ] | ||
| searchresult.audiobooks = [*searchresult.audiobooks, *audiobooks] | ||
| items_received += len(api_result["audiobooks"]["items"]) | ||
| offset += page_limit | ||
| if offset >= limit: | ||
| break | ||
| if items_received < page_limit: | ||
| break | ||
| return searchresult | ||
| return ",".join(searchtypes) | ||
|
|
||
| def _process_search_results( | ||
| self, api_result: dict[str, Any], searchresult: SearchResults | ||
| ) -> int: | ||
| """Process API search results and update searchresult object. | ||
|
|
||
| Returns the total number of items received. | ||
| """ | ||
| items_received = 0 | ||
|
|
||
| if "artists" in api_result: | ||
| artists = [ | ||
| parse_artist(item, self) | ||
| for item in api_result["artists"]["items"] | ||
| if (item and item["id"] and item["name"]) | ||
| ] | ||
| searchresult.artists = [*searchresult.artists, *artists] | ||
| items_received += len(api_result["artists"]["items"]) | ||
|
|
||
| if "albums" in api_result: | ||
| albums = [ | ||
| parse_album(item, self) | ||
| for item in api_result["albums"]["items"] | ||
| if (item and item["id"]) | ||
| ] | ||
| searchresult.albums = [*searchresult.albums, *albums] | ||
| items_received += len(api_result["albums"]["items"]) | ||
|
|
||
| if "tracks" in api_result: | ||
| tracks = [ | ||
| parse_track(item, self) | ||
| for item in api_result["tracks"]["items"] | ||
| if (item and item["id"]) | ||
| ] | ||
| searchresult.tracks = [*searchresult.tracks, *tracks] | ||
| items_received += len(api_result["tracks"]["items"]) | ||
|
|
||
| if "playlists" in api_result: | ||
| playlists = [ | ||
| parse_playlist(item, self) | ||
| for item in api_result["playlists"]["items"] | ||
| if (item and item["id"]) | ||
| ] | ||
| searchresult.playlists = [*searchresult.playlists, *playlists] | ||
| items_received += len(api_result["playlists"]["items"]) | ||
|
|
||
| if "shows" in api_result: | ||
| podcasts = [] | ||
| for item in api_result["shows"]["items"]: | ||
| if not (item and item["id"]): | ||
| continue | ||
| # Filter out audiobooks - they have a distinctive description format | ||
| description = item.get("description", "") | ||
| if description.startswith("Author(s):") and "Narrator(s):" in description: | ||
| continue | ||
| podcasts.append(parse_podcast(item, self)) | ||
| searchresult.podcasts = [*searchresult.podcasts, *podcasts] | ||
| items_received += len(api_result["shows"]["items"]) | ||
|
|
||
| if "audiobooks" in api_result and self.audiobooks_supported: | ||
| audiobooks = [ | ||
| parse_audiobook(item, self) | ||
| for item in api_result["audiobooks"]["items"] | ||
| if (item and item["id"]) | ||
| ] | ||
| searchresult.audiobooks = [*searchresult.audiobooks, *audiobooks] | ||
| items_received += len(api_result["audiobooks"]["items"]) | ||
|
|
||
| return items_received | ||
|
|
||
| @use_cache() | ||
| async def get_artist(self, prov_artist_id: str) -> Artist: | ||
|
|
@@ -642,20 +665,13 @@ async def get_similar_tracks(self, prov_track_id: str, limit: int = 25) -> list[ | |
| async def get_stream_details(self, item_id: str, media_type: MediaType) -> StreamDetails: | ||
| """Return content details for the given track/episode/audiobook when it will be streamed.""" | ||
| if media_type == MediaType.AUDIOBOOK and self.audiobooks_supported: | ||
| chapters_data = await self._get_audiobook_chapters_data(item_id) | ||
| if not chapters_data: | ||
| raise MediaNotFoundError(f"No chapters found for audiobook {item_id}") | ||
|
|
||
| # Calculate total duration and convert to seconds for StreamDetails | ||
| total_duration_ms = sum(chapter.get("duration_ms", 0) for chapter in chapters_data) | ||
| duration_seconds = total_duration_ms // 1000 | ||
|
|
||
| # Create chapter URIs for streaming | ||
| chapter_uris = [] | ||
| for chapter in chapters_data: | ||
| chapter_id = chapter["id"] | ||
| chapter_uri = f"spotify://episode:{chapter_id}" | ||
| chapter_uris.append(chapter_uri) | ||
| # Don't fetch chapters here - do it lazily during streaming | ||
| # Just get basic info for duration estimate | ||
| try: | ||
| audiobook_obj = await self._get_data(f"audiobooks/{item_id}") | ||
| duration_seconds = audiobook_obj.get("duration_ms", 0) // 1000 | ||
| except Exception: | ||
| duration_seconds = 0 | ||
|
|
||
| return StreamDetails( | ||
| item_id=item_id, | ||
|
|
@@ -666,7 +682,7 @@ async def get_stream_details(self, item_id: str, media_type: MediaType) -> Strea | |
| allow_seek=True, | ||
| can_seek=True, | ||
| duration=duration_seconds, | ||
| data={"chapters": chapter_uris, "chapters_data": chapters_data}, | ||
| data={"fetch_chapters_on_stream": True}, # Flag to fetch during streaming | ||
| ) | ||
|
|
||
| # For all other media types (tracks, podcast episodes) | ||
|
|
@@ -685,20 +701,23 @@ async def get_audio_stream( | |
| ) -> AsyncGenerator[bytes, None]: | ||
| """Get audio stream from Spotify via librespot.""" | ||
| if streamdetails.media_type == MediaType.AUDIOBOOK and isinstance(streamdetails.data, dict): | ||
| chapter_uris = streamdetails.data.get("chapters", []) | ||
| chapters_data = streamdetails.data.get("chapters_data", []) | ||
| # Fetch chapters NOW if not already provided | ||
| if streamdetails.data.get("fetch_chapters_on_stream"): | ||
| chapters_data = await self._get_audiobook_chapters_data(streamdetails.item_id) | ||
| chapter_uris = [f"spotify://episode:{ch['id']}" for ch in chapters_data] | ||
| else: | ||
| chapter_uris = streamdetails.data.get("chapters", []) | ||
| chapters_data = streamdetails.data.get("chapters_data", []) | ||
|
|
||
| # Calculate which chapter to start from based on seek_position | ||
| # Calculate start chapter | ||
| seek_position_ms = seek_position * 1000 | ||
| current_seek_ms = seek_position_ms | ||
| start_chapter = 0 | ||
|
|
||
| if seek_position > 0 and chapters_data: | ||
| accumulated_duration_ms = 0 | ||
|
|
||
| for i, chapter_data in enumerate(chapters_data): | ||
| chapter_duration_ms = chapter_data.get("duration_ms", 0) | ||
|
|
||
| if accumulated_duration_ms + chapter_duration_ms > seek_position_ms: | ||
| start_chapter = i | ||
| current_seek_ms = seek_position_ms - accumulated_duration_ms | ||
|
|
@@ -708,20 +727,59 @@ async def get_audio_stream( | |
| start_chapter = len(chapter_uris) - 1 | ||
| current_seek_ms = 0 | ||
|
|
||
| # Convert back to seconds for librespot | ||
| current_seek_seconds = int(current_seek_ms // 1000) | ||
|
|
||
| # Stream chapters starting from the calculated position | ||
| for i in range(start_chapter, len(chapter_uris)): | ||
| chapter_uri = chapter_uris[i] | ||
| chapter_seek = current_seek_seconds if i == start_chapter else 0 | ||
|
|
||
| try: | ||
| async for chunk in self.streamer.stream_spotify_uri(chapter_uri, chapter_seek): | ||
| # Try streaming with quick_fail for faster fallback detection | ||
| first_chunk_received = False | ||
| try: | ||
| for i in range(start_chapter, len(chapter_uris)): | ||
| chapter_uri = chapter_uris[i] | ||
| chapter_seek = current_seek_seconds if i == start_chapter else 0 | ||
|
|
||
| # Use quick_fail=True for first chapter to enable faster fallback | ||
| use_quick_fail = i == start_chapter | ||
| async for chunk in self.streamer.stream_spotify_uri( | ||
| chapter_uri, chapter_seek, quick_fail=use_quick_fail | ||
| ): | ||
| first_chunk_received = True | ||
| yield chunk | ||
| except Exception as e: | ||
| self.logger.error(f"Chapter {i + 1} streaming failed: {e}") | ||
| continue | ||
|
|
||
| # If we haven't received any chunks by now, trigger fallback | ||
| if not first_chunk_received: | ||
| raise Exception(f"No audio data received for chapter {i + 1}") | ||
|
|
||
| return # Success | ||
|
|
||
| except Exception as e: | ||
| # Try fallback if we haven't successfully streamed any data yet | ||
| if not first_chunk_received: | ||
| self.logger.warning( | ||
| f"Audiobook streaming failed on {self.instance_id}: " | ||
| f"{e}. Trying other instances..." | ||
| ) | ||
|
Comment on lines
+752
to
+754
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really no not like this approach, please inform me why this is needed this way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The way spotify works with the family plan is that the "owner" has X minutes of audiobook streaming per month included. So I can listen to an Audiobook with my wifes account but not mine. So I have added both accounts to MA but sometimes the audiobook plays and sometimes it doesnt depending on which Spotify provider starts first. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, so for the second spotify account, te whole audiobooks support may not have been enabled in the first place. So the audiobooks check need to be fixed maybe ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No the audiobook check passes as I’m in a supported country. It’s only at the point of actually trying to play the book that it fails. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, but if I understand you correctly, only the main account has rights to play audiobooks ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was looking at the account situation today for a different reason and the further complication is I could pay for X minutes of audiobook playback for my account and then it would work! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...hmmmm.... so we should explore the api if there is any way to get this info back so we can set a flag if streaming is possible or not. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see any obvious API call to check that specifically. The only thing I can think of is to pick some audiobook and try and play a few seconds of it? edit: and of course even if we did this at provider start I could purchase some hours of playback but my account still wouldnt work. I can't see any way around it except testing at the point of playback? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you know if there is a very specific error being returned by librespot ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, one idea I have in mind is to raise a specific exception (which could as well also just be AudioError) and then let the core logic deal with that by retrying with another provider |
||
|
|
||
| for provider in self.mass.music.providers: | ||
| if ( | ||
| provider.domain == "spotify" | ||
| and provider.instance_id != self.instance_id | ||
| and isinstance(provider, SpotifyProvider) | ||
| and provider.audiobooks_supported | ||
| ): | ||
|
Comment on lines
+756
to
+762
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, no, we can not have a provider doing logic like this. A provider can only provide its own data, not interact with other providers directly! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How should this be resolved then that the audiobook fails to play due to the first provider being used? (So its actually inconsistent whether it fails in my case as I have two and for whatever reason the startup order does change) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If content is tied to a specific instance, we should mark it that way. Maybe we need to adjust core logic to account for this...hmmmm.... You would expect that audiobooks/podcasts are generic per streaming provider |
||
| try: | ||
| async for chunk in provider.get_audio_stream( | ||
| streamdetails, seek_position | ||
| ): | ||
| yield chunk | ||
| return | ||
| except Exception as retry_error: | ||
| self.logger.debug( | ||
| f"Instance {provider.instance_id} also failed: {retry_error}" | ||
| ) | ||
| continue | ||
|
|
||
| raise UnsupportedFeaturedException( | ||
| f"No Spotify instance can play this audiobook. Original error: {e}" | ||
| ) | ||
| else: | ||
| # Handle normal tracks and podcast episodes | ||
| async for chunk in self.streamer.get_audio_stream(streamdetails, seek_position): | ||
|
|
@@ -747,6 +805,7 @@ async def login(self, force_refresh: bool = False) -> dict[str, Any]: | |
| "refresh_token": refresh_token, | ||
| "client_id": client_id, | ||
| } | ||
| err = "Unknown error" | ||
| for _ in range(2): | ||
| async with self.mass.http_session.post( | ||
| "https://accounts.spotify.com/api/token", data=params | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this part seems to be completely irrelevant to the PR ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a change except for breaking out into smaller functions as mypy was complaining about too many statements in search()