- 
          
 - 
                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?
Conversation
| 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 | ||
| ): | 
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.
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 comment
The 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 comment
The 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
| f"Audiobook streaming failed on {self.instance_id}: " | ||
| f"{e}. Trying other instances..." | ||
| ) | 
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.
I really no not like this approach, please inform me why this is needed this way.
If content is tied to a specific account (such as playlists) we should adjust core for that
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.
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 comment
The 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 comment
The 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 comment
The 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 ?
If so, I wonder if there is any check/flag in the api that allows us to detect this. Its pointless to report audiobooks support if you can not play it
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.
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 comment
The 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 comment
The 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 comment
The 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 comment
The 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
| if (item and item["id"] and item["name"]) | ||
| ] | ||
| searchresult.artists = [*searchresult.artists, *artists] | ||
| items_received += len(api_result["artists"]["items"]) | 
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()
| 
           Marked as draft until we figure out how to handle this.  | 
    
The 10 sec timeout generally was a bit long so that has been shortened.
Then for Audiobooks as people might have multiple instances where some can play audiobooks and some cant then we need to try the other instances if there is a stream failure. Also to further speed up the audiobook start time there is now an option to further reduce the stream timeout.