PlayPauseButtonState has a likely race

[bubenheimer]

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

media/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/PlayPauseButtonState.kt

Lines 40 to 41 in 51efcad

	val playPauseButtonState = remember(player) { PlayPauseButtonState(player) }
	LaunchedEffect(player) { playPauseButtonState.observe() }

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 [email protected] after filing this issue.

[bubenheimer]

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]

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:

@Composable
internal fun PlayPauseButton(player: Player, modifier: Modifier = Modifier) {

  LaunchedEffect(player) { player.playWhenReady = !player.playWhenReady }

  val state = rememberPlayPauseButtonState(player)

  IconButton(onClick = state::onClick, modifier, state.isEnabled) {
    Icon(icon, contentDescription)
  }

which is

@Composable
internal fun PlayPauseButton(player: Player, modifier: Modifier = Modifier) {

 LaunchedEffect(player) { player.playWhenReady = !player.playWhenReady } // 1

 val playPauseButtonState = remember(player) { PlayPauseButtonState(player) } 
 LaunchedEffect(player) { playPauseButtonState.observe() }  // 2

 IconButton(onClick = state::onClick, modifier, state.isEnabled) {
    Icon(icon, contentDescription)
  }

.. showed that the ordering of commands was:

• initialise PlayPauseButtonState(player)
• player.playWhenReady = !player.playWhenReady from LaunchedEffect1
• PlayPauseButtonState.observe() from LaunchedEffect2
• UI icon is out of sync with what the player is doing

Since 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:

class PlayPauseButtonState(private val player: Player) {
  var isEnabled by mutableStateOf(shouldEnablePlayPauseButton(player))
  var showPlay by mutableStateOf(shouldShowPlayButton(player))

  suspend fun observe(): Nothing {
    showPlay = shouldShowPlayButton(player)  // New code
    isEnabled = shouldEnablePlayPauseButton(player)  // New code
    player.listen { events -> if (events.containsAny(...)) 
     {
        showPlay = shouldShowPlayButton(this)
        isEnabled = shouldEnablePlayPauseButton(this)
      }
    }
  }
}

I raised a PR internally and you should soon see it merged into the main branch!

[bubenheimer]

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]

@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.

@Composable
fun rememberPlayerErrorState(player: Player): PlayerErrorState {
    val rememberObserver = remember(player) { PlayerErrorStateRememberObserver(player) }

    return rememberObserver.playerErrorState
}

@Stable
interface PlayerErrorState {
    val error: PlaybackException?
}

private class WritableErrorState(initialError: PlaybackException?) : PlayerErrorState {
    override var error by mutableStateOf(initialError)
}

private class PlayerErrorStateRememberObserver(private val player: Player) : RememberObserver {
    val playerErrorState = WritableErrorState(player.playerError)

    private val listener = object : Player.Listener {
        override fun onPlayerErrorChanged(error: PlaybackException?) {
            playerErrorState.error = error
        }
    }

    override fun onRemembered() = player.addListener(listener)

    override fun onForgotten() = player.removeListener(listener)

    override fun onAbandoned() {}
}

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]

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]

Less clunky:

@Composable
fun rememberPlayerErrorState(player: Player): PlayerErrorState {
    val rememberObserver = remember(player) { PlayerErrorStateRememberObserver(player) }

    return rememberObserver.playerErrorState
}

@Stable
interface PlayerErrorState {
    val error: PlaybackException?
}

private class PlayerErrorStateRememberObserver(
    private val player: Player
) : RememberObserver {
    val playerErrorState: PlayerErrorState

    private val listener: Player.Listener

    init {
        val mutableState = mutableStateOf(player.playerError)

        playerErrorState = object : PlayerErrorState {
            override val error by mutableState
        }

        listener = object : Player.Listener {
            override fun onPlayerErrorChanged(error: PlaybackException?) {
                mutableState.value = error
            }
        }
    }

    override fun onRemembered() = player.addListener(listener)

    override fun onForgotten() = player.removeListener(listener)

    override fun onAbandoned() {}
}

[oceanjules]

Added the workaround in ab6b0f6

[bubenheimer]

@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]

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]

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

	public void setPlayer(@Nullable Player player) {
	Assertions.checkState(Looper.myLooper() == Looper.getMainLooper());
	Assertions.checkArgument(
	player == null || player.getApplicationLooper() == Looper.getMainLooper());

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]

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]

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.