Improve stream URL handling with failover support#2996
Improve stream URL handling with failover support#2996benklop wants to merge 5 commits intomusic-assistant:devfrom
Conversation
…e giving up I'm finding that prem2 (often the first URL in in the list) returns 403 'Too many clients'. Failing over to the next URL works and plays the stream.
There was a problem hiding this comment.
Pull request overview
Adds mirrored stream URL support for the Digitally Incorporated provider so playback can fail over to the next URL on errors, and updates serialization/helpers to handle the new stream path types.
Changes:
- Digitally Incorporated provider now returns multiple stream URLs as mirrors and uses HTTP stream type.
- Audio streaming helper now supports mirrored URL lists via a new
get_mirror_streampath. - JSON serialization helper enhanced to better handle dataclasses/containers; adds a small serialization test.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
tests/test_json_serialization.py |
Adds a regression test ensuring lists of MultiPartPath can be JSON serialized. |
music_assistant/providers/digitally_incorporated/__init__.py |
Refactors stream URL retrieval to return multiple StreamMirror entries and switches to HTTP stream type. |
music_assistant/helpers/json.py |
Extends get_serializable_value to handle dataclasses and adds defensive to_json logging. |
music_assistant/helpers/audio.py |
Adds mirror-stream handling and introduces get_mirror_stream for URL failover. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| isinstance(obj, list | set | filter | tuple | dict_values | dict_keys | dict_values) | ||
| or obj.__class__ == "dict_valueiterator" |
There was a problem hiding this comment.
This check is ineffective: obj.__class__ is a type, so comparing it to the string "dict_valueiterator" will always be False. If you need to special-case dict value iterators, compare type(obj).__name__ (or use an Iterator/Iterable check) instead. Also, dict_values is listed twice in the isinstance union.
| isinstance(obj, list | set | filter | tuple | dict_values | dict_keys | dict_values) | |
| or obj.__class__ == "dict_valueiterator" | |
| isinstance(obj, list | set | filter | tuple | dict_values | dict_keys) | |
| or type(obj).__name__ == "dict_valueiterator" |
| return stream_url | ||
| # Log all available URLs | ||
| for i, url in enumerate(stream_list): | ||
| self.logger.debug("%s: Available stream URL %d: %s", self.domain, i + 1, url) |
There was a problem hiding this comment.
stream_list contains StreamMirror objects, but the debug log prints the object (%s, url) rather than the actual URL string. This makes logs harder to read and can omit the URL if StreamMirror.__str__ isn’t implemented. Log url.path (and optionally url.priority) instead.
| self.logger.debug("%s: Available stream URL %d: %s", self.domain, i + 1, url) | |
| self.logger.debug( | |
| "%s: Available stream URL %d: %s (priority: %s)", | |
| self.domain, | |
| i + 1, | |
| url.path, | |
| url.priority, | |
| ) |
| @use_cache(CACHE_STREAM_URLS) | ||
| async def _get_stream_urls(self, network_key: str, channel_key: str) -> list[StreamMirror]: | ||
| """Get the streaming URLs for a channel.""" | ||
| self.logger.debug("%s: Getting stream URL for %s:%s", self.domain, network_key, channel_key) |
There was a problem hiding this comment.
This debug line still says "Getting stream URL" even though the method now returns multiple URLs. Consider updating the message to "stream URLs" to match the new behavior and avoid confusion when troubleshooting.
| # Get the stream URL | ||
| stream_url = await self._get_stream_url(network_key, channel_key) | ||
| stream_url = await self._get_stream_urls(network_key, channel_key) | ||
|
|
There was a problem hiding this comment.
stream_url now holds a list of StreamMirror entries (multiple URLs). Renaming to something like stream_urls/mirrors would make the type/intent clearer and avoid confusion for future readers.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| media_type=MediaType.RADIO, | ||
| stream_type=StreamType.ICY, | ||
| # Use HTTP stream type with mirrors so we can try multiple URLs | ||
| stream_type=StreamType.HTTP, | ||
| path=stream_url, |
There was a problem hiding this comment.
Changing the Digitally Incorporated radio stream from StreamType.ICY to StreamType.HTTP will bypass the ICY-specific streaming path (get_icy_radio_stream) and likely disables ICY metadata (stream title) updates. If these streams provide ICY metadata, consider keeping StreamType.ICY and adding mirror failover support for ICY streams (or enhancing the ICY streamer to accept a mirror list) instead of downgrading to plain HTTP.
|
Could you have a look at the failing tests and open co pilot suggestions? Marking this PR as draft so we can keep track of which PRs needs our attention |
|
I will try to update this PR this weekend. sorry for the delays, been a crazy week. |
4 participants
Enhance the Digitally Incorporated provider to handle 403 (and other) errors by attempting the next URL in the list. Refactor stream URL handling to support multiple URL types and improve error logging. Include tests for JSON serialization of stream details.
This change requires an update to music-assistant/server, music-assistant/models#155
Supercedes #2984