Open
Description
Version
Media3 1.6.0
More version details
No response
Devices that reproduce the issue
found via code inspection
Devices that do not reproduce the issue
No response
Reproducible in the demo app?
Not tested
Reproduction steps
The problem here is a gap between setting the initial button state in PlayPauseButtonState() and observing a change in state. The state could change in this gap without us observing it.
Android composition appears to use Dispatchers.Main for CoroutineContexts, implying that Player/Controller state could change prior to the LaunchedEffect coroutine starting.
Expected result
N/A
Actual result
N/A
Media
N/A
Bug Report
- You will email the zip file produced by
adb bugreport
to android-media-github@google.com after filing this issue.
Metadata
Metadata
Assignees
Type
Projects
Milestone
Relationships
Development
No branches or pull requests
Activity
bubenheimer commentedon Apr 7, 2025
Other button state Composables use this same pattern and would be similarly affected. The pattern is also highlighted in developer.android.com reference documentation and would need to be corrected there as well.
oceanjules commentedon Apr 17, 2025
Hi @bubenheimer,
Thank you very much for your interest in our new
ui-compose
module. We are excited for any new adopters!You are also absolutely right about the race. It wasn't obvious to me that the LaunchedEffect (with state.observe/event listening) will only get scheduled for after the full PlayPauseButton Composable was returned. Testing it out with:
which is
.. showed that the ordering of commands was:
PlayPauseButtonState(player)
player.playWhenReady = !player.playWhenReady
from LaunchedEffect1PlayPauseButtonState.observe()
from LaunchedEffect2Since we cannot guarantee the atomicity of
PlayPauseButtonState
creation and event-listening registration, the best we can do it to ensure that we start listening from the correct state. That means, even if there were 5 LaunchedEffects with a bunch of modifications to the Player, we bring PlayPauseButtonState to the correct, most up-to-date, post-all-intermediate-events configuration. How? With a pre-emptive reading of relevant values outside of the "listen" coroutine:I raised a PR internally and you should soon see it merged into the main branch!
bubenheimer commentedon Apr 17, 2025
Thank you @oceanjules for taking a look, testing, and iterating on the pattern. Looks like it will work as long as the applicationLooper is the main looper, which is pretty much a must anyway at this point.
I have started to use a different pattern that may be preferable. I will plan to sketch it out when I get a chance in the coming days.
bubenheimer commentedon Apr 17, 2025
@oceanjules here is a version of the pattern I have in mind. It is essentially a streamlined combination of state remember + DisposableEffect, instead of LaunchedEffect coroutine. This specific example tracks the Player's error state.
This works as is only when the Player.applicationLooper is the main thread. My impression of the media3 compose-ui code is that it largely assumes this precondition.
AFAIK composition (for Compose UI) currently runs uninterrupted until after applying effects without a chance for something else to swoop in on the same (main) thread. Chuck Jazdzewski has been working on a "PausableComposition" prototype, which may change this behavior, but I don't think its fate is clear at this point, so I don't think that such a hypothetical change can or should be taken into account at this point.
Alternatively one can addListener in an init block without waiting for onRemembered, plus an additional removeListener in onAbandoned. Its primary drawback is addListener typically using synchronization, which may be undesirable during initial composition. In case of a "PausableComposition" this may have other drawbacks.
bubenheimer commentedon Apr 18, 2025
For user-facing documentation, as opposed to a library-provided optimized implementation, it may be better to use a less streamlined but functionally equivalent pattern with separate remember & DisposableEffect. Using the RememberObserver pattern directly as above has some subtle gotchas that may trip up developers when applying the pattern.
bubenheimer commentedon Apr 18, 2025
Less clunky:
oceanjules commentedon Apr 23, 2025
Added the workaround in ab6b0f6
bubenheimer commentedon Apr 23, 2025
@oceanjules I can't say that I find the current pattern compelling in general.
What about the public visibility of
observe()
? That does not seem to make much sense.bubenheimer commentedon May 5, 2025
The bigger problem with the current pattern in 1.6.1 and the shared commit is implicit reliance on applicationLooper being the main looper, otherwise there will be races and inconsistent Player states. There are ways around it, but this pattern would have to change quite a bit.
oceanjules commentedon May 6, 2025
I see what you are saying, but our views based solution also had such a limitation:
media/libraries/ui/src/main/java/androidx/media3/ui/PlayerView.java
Lines 650 to 653 in 839c4a9
While we do technically allow the player to run on a non-main thread, we don't recommend it because it means you need a new thread-hopping layer to talk to the background thread player from the UI for example. Are you worried about this because your application uses such threading?
For now, we will specify that we are building the UI module for players running on the main thread and will be considering any bugs that can be reproduced under such conditions until we reach an MVP. Expansion to other threading models will be marked as an enhancement.
bubenheimer commentedon May 6, 2025
Thank you for this valuable information. My assumption has been that threading was an oversight due to no statement on the subject. Without specification I cannot determine correctness, or suggest viable solutions.
You mused elsewhere about how to tame the complexity of the Player API for Compose. Using snapshots of Player state could be a more foundational approach to address data races and threading.
bubenheimer commentedon May 9, 2025
I suspect that the current approach is not fully compatible with PausableCompositions, as composition would not know when Player state changes while composition is paused. This could be remedied by extracting Player state into composition snapshot state.