Skip to content
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

Idiomatic kotlin API for Player Events (media3-common-ktx) #2027

Open
Ethan1983 opened this issue Jan 8, 2025 · 2 comments
Open

Idiomatic kotlin API for Player Events (media3-common-ktx) #2027

Ethan1983 opened this issue Jan 8, 2025 · 2 comments
Assignees

Comments

@Ethan1983
Copy link

Ethan1983 commented Jan 8, 2025

Use case description

Please provide a idiomatic kotlin API for player events using Flow in androidx.media3:media3-common-ktx

Proposed solution

Current API is a suspend API with a callback. suspend API is typically meant for a one-time async request. For a stream of player events, please provide a Flow<Player.Events> using callbackFlow.

suspend fun Player.listen(onEvents: Player.(Player.Events) -> Unit): Nothing {
  if (Looper.myLooper() == applicationLooper) {
    listenImpl(onEvents)
  } else
    withContext(HandlerCompat.createAsync(applicationLooper).asCoroutineDispatcher()) {
      listenImpl(onEvents)
    }
}

Alternatives considered

// use callbackFlow to convert callback into Flow

val Player.eventsFlow: Flow<Player.Events> get() = callbackFlow {
  val listener = object: Player.Listener {
    override fun onEvents(player: Player, events: Player.Events) {
      trySend(events)
    }
  }

  addListener(listener)

  awaitClose {
    removeListener(listener)
  }
}
@oceanjules
Copy link
Contributor

Hi @Ethan1983,

Thank you very much for your suggestion. Please first take a look at the KDoc we provided for the listenImpl (I admit it's a private function, so you might not have seen it):

/**
* Implements the core listening logic for [Player.Events].
*
* This function creates a cancellable coroutine that listens to [Player.Events] in an infinite
* loop. The coroutine can be cancelled externally, or it can terminate if the [onEvents] lambda
* throws an exception.
*
* Given that `invokeOnCancellation` block can be called at any time, we provide the thread-safety
* guarantee by:
* * unregistering the callback (i.e. removing the listener) in the finally block to keep on the
* calling context which was previously ensured to be the application thread
* * marking the listener as cancelled using `AtomicBoolean` to ensure that this value will be
* visible immediately on any non-calling thread due to a memory barrier.
*
* A note on [callbackFlow] vs [suspendCancellableCoroutine]:
*
* Despite [callbackFlow] being recommended for a multi-shot API (like [Player]'s), a
* [suspendCancellableCoroutine] is a lower-level construct that allows us to overcome the
* limitations of [Flow]'s buffered dispatch. In our case, we will not be waiting for a particular
* callback to resume the continuation (i.e. the common single-shot use of
* [suspendCancellableCoroutine]), but rather handle incoming Events indefinitely. This approach
* controls the timing of dispatching events to the caller more tightly than [Flow]s. Such timing
* guarantees are critical for responding to events with frame-perfect timing and become more
* relevant in the context of front-end UI development (e.g. using Compose).
*/
private suspend fun Player.listenImpl(onEvents: Player.(Player.Events) -> Unit): Nothing {

As you can see, there is a note on why we chose to go ahead with a suspend function vs a Flow.

However, I understand where you are coming from - Kotlin documentation on https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines.flow/callback-flow.html and https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines/suspend-cancellable-coroutine.html is definitely clear in that respect and it might look like we are abusing some sort of a loophole with suspendCancellableCoroutine. There have been countless design discussions about this particular issue and the conclusion was based on the combination of what the Player object is like, plus the fact that we are dealing with UI events. The current implementation as is ensures the fastest reaction to a Player event. You can subscribe to those changes and listen indefinitely from multiple Compose elements without being afraid that they end up in an illegal state.

A Flow with all the events would also not adhere to a single responsibility principle, each consumer would still have to filter out the ones that they want. At this point you might wonder why not create 30 flows for all sorts of events, which is when timing guarantees become debatable - having to use https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines.flow/combine.html again leads you to potentially illegal states.

We understand that, as app developers, you will have received a lot of Android guidance on using Flows. It is still the case that Flows and Channels are great structures for a data-business layer exchange. However, as library developers, we have to work around Player - this live object is responsible for a lot of things and acts both as a session-like object which you can "attach" to, but also a bit of a mini-database of certain values to be extracted for UI display, while also needing to ensure you catch all the events in order.

One thing to mention is that with the current implementation, you can create the Flows that you need (if you dare to go down that path after all I've explained above), since you have a lower-level construct. However, you might notice how easy it is to use Player.listen in the upcoming 1.6.0-alpha02 release. If you look at various UI state holders in https://github.com/androidx/media/tree/main/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state

suspend fun catchEvents(): Nothing =
    player.listen { events ->
      if (
        events.containsAny(
          Player.EVENT_A_CHANGED,
          Player.EVENT_B_CHANGED,
          Player.EVENT_AVAILABLE_COMMANDS_CHANGED,
        )
      ) {
        someBoolean = getBooleanOutOfPlayer(player=this)
        isEnabled = this.isCommandAvailable(COMMAND_D)
      }
    }

@Ethan1983
Copy link
Author

Thanks for the Kdoc. Is there a way to reproduce the delay when using Flow? Would a SharedFlow (without any replay) have helped when dealing with multiple consumers?

A Flow with all the events would also not adhere to a single responsibility principle, each consumer would still have to filter out the ones that they want.

Isn't this already the case even with the callback? Multiple consumers are effectively filtering with the contains check. How different is this from using a filter on a flow?

class PreviousButtonState(private val player: Player) {
  ...
  suspend fun observe(): Nothing =
    player.listen { events ->
      if (events.contains(Player.EVENT_AVAILABLE_COMMANDS_CHANGED)) {
        isEnabled = isCommandAvailable(Player.COMMAND_SEEK_TO_PREVIOUS)
      }
    }
}

class NextButtonState(private val player: Player) {
  ...
  suspend fun observe(): Nothing =
    player.listen { events ->
      if (events.contains(Player.EVENT_AVAILABLE_COMMANDS_CHANGED)) {
        isEnabled = isCommandAvailable(Player.COMMAND_SEEK_TO_NEXT)
      }
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants