Fix playback continuation on external controller command involving search (through the onPlayFromSearch) #2280
Conversation
This enables external controllers other than android auto to resume the playback
After an external playback start, app launch and exit the session can be left in the released state until process finish
|
Hmm, I'm not sure if this is the correct solution. The code is only a weird workaround. Usually connecting to a media controller should not prepare anything. Instead, the client connecting to a session should issue preparing of the corresponding media. |
|
I am considering the scenario when the controller connects and issues a play command. Most apps continue to play the media they played last, Voice now picks a book from those which have not been started yet. Do you mean that there should be an extra action between the connection and the play command initiating loading the playback state? Or should it be loaded on service start? |
|
I'm hesitant on this change; especially around re-injecting the service. I think this is not how playback resumption is mean to work with media3. I've raised a question here: androidx/media#1064 Could you add the details from your original discussion to this issue so the information is less spread and media3 developers have an easier time? |
Instead of loading the playback position on any external controller connection it is now loaded on service creation
|
I have moved the position loading from |
|
I would assume that this would lead to a permanent notification which has been the case in the past. |
|
I see no extra notifications on my phone |
|
Well, now I see some notifications but not permanent: they sometimes appear on app launch (is is a problem?) and swiping it away from the recent app list (trying to figure out why it happens and how to filter it out). |
|
Can you check Marcs answer? From what he explained it looks like this change should not be necessary and another controller should just connect and then call play() |
Legacy controllers have onPlayFromSearch method that is called for play button and expected to play something meaningful. For media3 sessions it delegates to getSearchResult. In this app search was already returning the last position for calls with null as query, but in the mentioned scenario it is called with an empty string, so, instead of manually loading the last book played it is now returned for empty query searches
|
Yes, I have explored what's happening again and see another solution. SimpleWear, with which I face the issue, uses legacy controller's |
| @@ -44,7 +44,7 @@ class BookSearchHandler | |||
|
|
|||
| // Look for anything that might match the query | |||
| private suspend fun searchUnstructured(query: String?): Book? { | |||
| if (query != null) { | |||
| if (query != null && query != "") { | |||
There was a problem hiding this comment.
Nitpick: you can use isNullOrBlank
|
Very cool! Could you merge main and update the PR title to describe what it fixed? |
ASGusev
changed the title
|
Will it do? |
| } | ||
| override fun onGetSession(controllerInfo: MediaSession.ControllerInfo): MediaLibrarySession { | ||
| if (session.invokeIsReleased) | ||
| rootComponentAs<PlaybackComponent.Provider>() |
There was a problem hiding this comment.
And are you sure this is necessary? I'm hesitant to merge this because this will literally create new instances of the whole player stuff - while not releasing the old things.
I fear that this might lead to several players playing at the same time under some circumstances
There was a problem hiding this comment.
A new session is only created in a situation when the service is not destroyed after a release call. I face this situation when I run Voice from SimpleWear and then close it. Now in such a situation playback cannot be started even by the play button in the player activity (or the play button in the books list). I think this behaviour is not more desirable.
By the way, in the comment in onTaskRemoved you have written that releasing the session is required to prevent showing a dead notification. Later, in a comment to my previous PR, you've mentioned having fixed the notification being unresponsive. So, is the dead notification still an issue, and is the release call in onTaskRemoved still required? It seems to me that only calling release from onDestroy would obviate the sole need to check if the session state.
There was a problem hiding this comment.
And which conditions do you mean? Is that onGetSession being called from another thread than onCreate that does not see the session created there?
There was a problem hiding this comment.
Also, I don't quite understand what you mean by 'not releasing the old stuff' - a new session is only created if the old one has already been released.
In #2215 I have mentioned a limitation of external controller support and described a possible solution. So, here I propose some changes to widen the support for external controllers.