-
Notifications
You must be signed in to change notification settings - Fork 539
session: fix IllegalStateException when calling setSessionExtras() #2265
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
Conversation
@marcbaechinger ping since it has been a month, any chance you could take a look at this PR? |
@@ -1134,7 +1134,7 @@ public void onSessionExtrasChanged(int seq, Bundle sessionExtras) { | |||
PlayerWrapper playerWrapper = sessionImpl.getPlayerWrapper(); | |||
playerWrapper.setLegacyExtras(sessionExtras); | |||
sessionCompat.setExtras(playerWrapper.getLegacyExtras()); | |||
sessionCompat.setPlaybackState(sessionImpl.getPlayerWrapper().createPlaybackStateCompat()); |
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.
Can you point me to the line where the IllegalStateException
is originating?
I can't find a line where this would be caused by sessionCompat.setPlaybackState(state)
. Similarly, for playerWrapper.createPlaybackStateCompat()
. I probably just haven't find it.
Best would be a stack trace of when this IllegalStateException happens.
I'm asking because if that would be an issue then there is a general problem in other methods around that call site I think. We'd need to investigate this some further, so a stack trace would be very helpful.
You say this is for when calling from a thread different than the main thread. The session is using the application thread which which the player was build [1], so if there was an issue regarding main thread vs. application thread, then we either have a more fundamental problem to discover, or then the player/session may not have been setup properly in the app.
I'd be great to clarify this first to see whether and where we need changes.
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.
Hi @marcbaechinger,
the full stack trace is:
2025-05-16 11:00:55.166 4788-4788 AndroidRuntime androidx.media3.demo.session E FATAL EXCEPTION: main
Process: androidx.media3.demo.session, PID: 4788
java.lang.RuntimeException: Unable to create service androidx.media3.demo.session.PlaybackService: java.lang.IllegalStateException
at android.app.ActivityThread.handleCreateService(ActivityThread.java:5295)
at android.app.ActivityThread.-$$Nest$mhandleCreateService(Unknown Source:0)
at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2597)
at android.os.Handler.dispatchMessage(Handler.java:110)
at android.os.Looper.loopOnce(Looper.java:248)
at android.os.Looper.loop(Looper.java:338)
at android.app.ActivityThread.main(ActivityThread.java:9067)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:593)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:932)
Caused by: java.lang.IllegalStateException
at androidx.media3.common.util.Assertions.checkState(Assertions.java:85)
at androidx.media3.session.PlayerWrapper.verifyApplicationThread(PlayerWrapper.java:1294)
at androidx.media3.session.PlayerWrapper.getPlayerError(PlayerWrapper.java:253)
at androidx.media3.session.PlayerWrapper.createPlaybackStateCompat(PlayerWrapper.java:1049)
at androidx.media3.session.MediaSessionLegacyStub$ControllerLegacyCbForBroadcast.onSessionExtrasChanged(MediaSessionLegacyStub.java:1150)
at androidx.media3.session.MediaSessionImpl.lambda$setSessionExtras$8(MediaSessionImpl.java:565)
at androidx.media3.session.MediaSessionImpl$$ExternalSyntheticLambda35.run(D8$$SyntheticClass:0)
at androidx.media3.session.MediaSessionImpl.dispatchRemoteControllerTaskWithoutReturn(MediaSessionImpl.java:1143)
at androidx.media3.session.MediaLibrarySessionImpl.dispatchRemoteControllerTaskWithoutReturn(MediaLibrarySessionImpl.java:366)
at androidx.media3.session.MediaSessionImpl.setSessionExtras(MediaSessionImpl.java:564)
at androidx.media3.session.MediaSession.setSessionExtras(MediaSession.java:1218)
at androidx.media3.demo.session.DemoPlaybackService.initializeSessionAndPlayer(DemoPlaybackService.kt:172)
at androidx.media3.demo.session.DemoPlaybackService.onCreate(DemoPlaybackService.kt:122)
at android.app.ActivityThread.handleCreateService(ActivityThread.java:5282)
at android.app.ActivityThread.-$$Nest$mhandleCreateService(Unknown Source:0)
at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2597)
at android.os.Handler.dispatchMessage(Handler.java:110)
at android.os.Looper.loopOnce(Looper.java:248)
at android.os.Looper.loop(Looper.java:338)
at android.app.ActivityThread.main(ActivityThread.java:9067)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:593)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:932)
I have edited the demo app to create the player on a different thread. However, the MediaSession will still be created on the main thread, and setSessionExtras will be called on the main thread, too.
I have thought about wheter it is just incorrect to call this on the main thread if the application thread is not the main thread, but:
- Some methods in MediaSession do say in the javadoc they must be called on the application thread. This is not one of them.
- The method javadoc specifies it is asynchronous. Running a part of the method on another thread will not break the method semantics.
- Almost every other method in MediaSessionLegacyStub uses
updateLegacySessionPlaybackState
which is a postOrRun variant of this. Only onSessionExtrasChanged does not use postOrRun which leads to this crash. Aligning this with the other methods makes the crash go away.
Based on the fact that:
- Other methods use the same pattern and hence are fine to call from any thread
- This method doesn't seem to be intended to enforce application thread as it does not enforce it itself
- The javadoc doesn't mention thread restrictions either
I think this makes sense to fix like this, instead of calling setSessionExtras on another thread. Many generic methods like the constructor of MediaSession work on any thread, and it makes the app code simpler.
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.
Many thanks for the response and the stack trace to help me understand the issue better. It's coming from calling methods on the player in createPlaybackStateCompat
.
I figure then we'd have the same issue when calling for instance MediaSession.sendError()
that is calling through to ControllerLegacyCbForBroadcast.onError()
which then calls playerWrapper.createPlaybackStateCompat()
from the main thread as well.
Ok, thanks for reporting. I'm going to take that PR, send it for review and then will file an internal bug, so we can look into other possible cases we have to cover. Thank you.
...from thread which isn't application thread. This change makes the demo app work with player using a non-main application thread.
d4b0217
to
5d102d1
Compare
I'm going to send this for internal review now. You may see some more commits being added as I make changes in response to review feedback. Please refrain from pushing any more substantive changes as it will complicate the internal review - thanks! |
Thanks :) |
...from thread which isn't application thread. This change makes the demo app work with player using a non-main application thread.
(There is no related issue, I believe.)