Invitation to comment: Player threading model

[tonihei]

The current threading model came up in multiple issues recently and we are considering options to improve the current situation. If someone feels strongly for/against a solution, please let us know.

In particular, if you are using ExoPlayer on a background thread we would be interested why you feel this is needed.

---

Background

The current player threading model requires that the player is accessed from a single thread. The player also calls all listener callbacks on this thread. This thread is the thread the player is created on or the main thread if that thread doesn't have a Looper.

This model may cause problems in the following situations:

• When executing code that must be called on the main thread in one of the listener callbacks (e.g. the UI components and ImaAdsLoader do that). If the listener callbacks happen on a background thread, the code will throw (see ExoPlayerSimpleView updating not on the ui thread and crushing the app #4439).
• When code calls the player from another thread (see e.g. Allow specifying a Looper when instantiating ExoPlayer #4278 and Crash reported pointing internal to player. #4408):
  This may happen if:
  • the developer is unsure which thread certain methods run on
  • or just because calling the player directly is more convenient than posting to another thread

The first example causes problems, although the player is used in the documented way and only ExoPlayer's own UI components violate the contract. The second example violates the documented rules outright, but it's easy to get wrong and developers may feel that having to post onto the correct thread is unnecessarily complicated.

Options for improvement

There are four main options to improve the current model:

1. Keep it as it is and only amend documentation for UI components and other components relying on being called on the main thread. May also add assertions to enforce correct usage.

• Pro: No code change needed. Keeps current behaviour.
• Con: This would prevent all people using the UI components from accessing the player on a background thread (=violation between what we advertise and what we actually support).

2. Require all implementations to use the main thread to access the player unless they explicitly specify a Looper for player access when creating the player.

• Pro: Prevents the problems above by moving most users to the main thread automatically. Simplifies threading rules.
• Con: Still prevents the combination of background thread and using UI components, but at least makes it an explicit choice. May silently change the current behaviour of implementations (unless we enforce using the explicit Looper parameter with assertions).

3. Require all implementations to use the main thread to access the player.

• Pro: Prevents the problems listed above. Also easy to reason about and simplifies bug search.
• Con: May be too restrictive in case there are legitimate reasons where the player needs to be accessed on a background thread. May also require many developers to change their code to adhere to these rules.

4. Synchronize all access to the player + allow code registering a listener to specify the Looper thread on which the listener is called.

• Pro: Prevents the problems above by allowing a more flexible use of the player. Supports other use cases where the player is accessed in parallel. Prevents accidental developer bugs as it's by definition not possible to use the player in a wrong way.
• Con: Introduces complications with maintaining the player. Difficult to reason about. Listener callbacks may see unexpected state changes if the player changes while processing one action. May also need a way to support transactional player changes (or another external way to lock access to the player). May create deadlocks in existing code if external locking is used for player/callback access.

Option 4 is the least preferred option, as it involves many changes and may introduce new problems.

Options 1-3 are all feasible. We need to establish whether using the player on a background thread is (a) necessary for any case or (b) widely used. If neither is true, option 2 or 3 may be best.

[markmckenna]

We have two use cases for a multithreaded player:

• Prepare playback on multiple streams in parallel, but then only start one of them, so the user got good performance no matter which they selected
• Protect player responsiveness from other developers blocking the main thread. We maintain a library that is integrated into applications controlled by other teams. This means that app developers who block the main thread can interfere with the behaviour of the player, and vice-versa. Among other things we want to avoid 'blame game' situations where app devs block the main thread, then file unreproducible bugs saying that the player is unresponsive.

We ran into a few issues:

• Most responses from the player took place on message response, but the occasional one (onStateChanged from playWhenReady change, IIRC) seems to happen immediately, which meant that queued actions occurred in an unexpected order. This isn't directly related to the threading model but did interfere with our implementation.
• if you prepare the MediaSource on one thread, then pass it to the player which is tied to a different thread, it will sometimes crash. The background loading was happening unprotected on whatever thread started prepare, but once you hand it to an ExoPlayer instance it gets picked up by the player's internal thread and those two start to conflict. We fixed this by always creating a new ExoPlayer instance for every new unit of content.
• With separate ExoPlayer instances, there are (were?) un-preparation messaging problems. When you close the player it seems to close the internal thread right away, even if messages are queued (including the message that completes unload of player allocated resources). This would sometimes result in memory leaks, particularly when linked with an Android SDK bug on older devices (pre-22?) that left the last delivered message in the Looper hanging around. We worked around this by calling stop(), then posting close() with a few-second delay.

For our purposes a single-threaded player works fine, as long as that thread isn't mandated to be the main thread.

Also note that Android thinks it's OK to interact with UI components from a non-main thread, as long as the components aren't tied directly to an external view hierarchy. I don't know how that interacts with input messages, but it might be possible to resolve the UI component thread problem by rendering them on a custom drawing thread and posting the render to a View asynchronously. We did this in apps a couple of times to allow for custom cached rendering.

Another thought... would it be reasonable to design a thread-safe wrapper that minimizes the complexity of the problem through a single (or a few) immutable State object(s)? So the user constructs a player instance on whatever thread and uses a static factory to build the wrapper (SynchronizedPlayer Player.synchronized(Player player)), then they interact strictly through
a couple calls like State getState(), setState(State new), onStateChange(StateListener l)? This should make adding thread safety a lot easier without requiring so much drilling into the core.

[tonihei]

@markmckenna Thanks for the feedback!

Just some initial thoughts:

• Preparing multiple players on the same thread shouldn't be an issue because each gets its own playback thread anyway.
• Keeping the player on a background thread to prevent interference from other sources sounds interesting. I suspect it's not strictly needed because all operations performed on the player are non-blocking and the main part of the work is done on background threads anyway. In any case, I acknowledge that mandating to use the main thread may feel restrictive.
• We already have this single-thread policy but do not enforce it. So, for example, setPlayWhenReady is supposed to be called on the same thread as the player was created. Meaning that the issue you describe is actually not supposed to happen.
• Preparing media source before passing them to the player is something on our agenda, but actually not supported yet. How do prevent the player from preparing the same source again?
• Remaining messages are deleted when the player is released, so sending the clean-up message first sounds right. You can wait for the message with message.blockUntilDelivered if needed before releasing the player. That's maybe safer than using a timer. If you have a specific feature request for the messaging system, it would be great if you can open a new issue to track that.

[markmckenna]

@tonihei all good points, thank you :)

• True that preparing several players on the same thread isn't really an issue. That's something we can consider doing if/when we have to change what we've got.
• Avoiding blocking on main thread between app and library has more to do with our code and plugins, than it does with ExoPlayer directly. Exo has never been the cause of blocking in my experience (kudos for that :)), but it would be tricky for us to do what we do without also having Exo be controlled by a thread of our own design.
• I might have explained the setPlayWhenReady problem poorly. It was a couple of years ago and I'm having a hard time remembering the exact details. I'll come back with a better response if/when I can remember exactly what the problem was.
• No worries about pre-preparing the MediaSource. This was an issue more with Exo 1.x when there was a prepare() call straight on the object you called player.prepare() with. I see that that' gone, so the API clearly indicates that we shouldn't be doing that.
• I could add blockUntilDelivered() but that would require spinning up a thread that I'm allowed to block. The timer is pretty safe given that I can wait a fairly ample length of time. It seems like a good idea for Exo to make sure it's released resources that it allocated before it throws away the queue that is responsible for doing the releasing; but having said that, with the Looper bug closed, I don't know of another outstanding resource leak connected to ExoPlayer (Audio tracks, video decoders maybe?)

[natez0r]

Wanted to chime in on mark's suggestion:

Another thought... would it be reasonable to design a thread-safe wrapper that minimizes the complexity of the problem through a single (or a few) immutable State object(s)? So the user constructs a player instance on whatever thread and uses a static factory to build the wrapper (SynchronizedPlayer Player.synchronized(Player player)), then they interact strictly through
a couple calls like State getState(), setState(State new), onStateChange(StateListener l)? This should make adding thread safety a lot easier without requiring so much drilling into the core.

We use a similar pattern in our app, but it is to interact with our class that ultimately manages interactions with exoplayers. Any call to that class is done through a wrapper that always posts messages to a dedicated thread for the manager and works well to hide the threading concerns from the clients of our manager.

About our threading model:

We have 3 main threads to worry about:

1 - Player internal thread (the one described above), all actions to our manager are posted to this thread and all exoplayer messages are delivered on this thread. Most of the large interactions with the player occur on this thread. (play/pause/stop/etc...)

2 - ExoPlayer creation thread. We create the exoplayer on a thread that's literally dedicated for this in the app (lives for the entire life of the app and used exclusively for this purpose). This is most likely not required anymore, but we introduced it for a few legacy reasons. The first reason was the thread safety of MediaCodec access on certain older devices (i believe Galaxy S5), we would see native crashes in the media stack if we did not create all of the Android MediaPlayer objects and do any MediaCodec interrogation (checking of codec supports main/high profile, etc...) and also release any android MediaPlayer instances. We used to have both the exoplayer and MediaPlayer alongside each other, so this has carried over.

3 - Ticking thread. We spawn a thread that is responsible for firing a precisely timed event that we use to track analytics. This thread calls getCurrentPosition() on the player VERY frequently. I am fairly certain that this is the reason why we're seeing the threading issues linked in this thread.

I think we could get rid of thread number 2 already, but thread 1 is definitely staying. We'd love to get rid of thread 3, but exoplayer does not have a supported way to notify when time passes, so we had to implement something on our own. I'd love to see exoplayer support events on a cadence and then we could get rid of this.

[tonihei]

@natez0r Thanks for the insight. Really helpful to see how ExoPlayer is used - especially the parts we never intended to be used like that :)

The short answer is: All three threads should be just one. The player is simply not written to be safely accessed from multiple threads.

The slightly longer answer:

• Thread 2 (the "creation thread") may have been needed for MediaPlayer, but shouldn't be needed for ExoPlayer. ExoPlayer's constructors as well as the prepare method are both doing nothing on the calling thread and are safe to be called without blocking. The problem arising from that thread is that all event callbacks will be delivered on this thread as well. We added an option recently to specify the callback thread at the time of creation if another thread should be used. So if you feel it's better or easier to keep this thread, please specify Thread 1 as the callback thread.
  This is also the most likely cause of the reported issue: setPlayWhenReady is called on Thread 1 (and tries to change the state), while the player handles an internal state change on its callback Thread 2.
• I acknowledge that using a background thread for all player commands (Thread 1) is common place and Mark already mentioned he'd like to keep it that way. We believe it's actually not strictly necessary as none of the player operations are blocking. Anyway, this probably rules out option 3 of my original proposal (=forcing everyone to use the main thread).
• Thread 3 is interesting. As said above, it's probably not the cause for the issue you are seeing. Still, calling getCurrentPosition from another thread is not safe. It may "just work" if you never have dynamic changes to playlists and the like, but I wouldn't rely on that. Some thoughts about that:
  • How VERY frequently is getCurrentPosition being called? And how accurately timed need the call to be? It seems to me that it should be fine to post regular messages on Thread 1 to obtain the position without any major problems (as none of the other methods called on this thread are blocking). Note that the "current position" in itself is not exact science as it only gets updated roughly every 10ms anyway and given the fluid nature of the rendering pipeline and the A/V synchronization etc. it's doubtful that requesting the position at exact moments in time gives you any meaningful advantage over just calling it less often at less accurately timed moments.
  • Besides that, I'd suggest to use the existing ExoPlayer callbacks to calculate analytics values. For example, you can listen to state changes to know how long a video has been played, how long a user spend buffering and so on. We also plan to provide a simple implementation for that ourselves, but it should be relatively straight-forward to implement that yourself. The obvious advantage is that there is much less message handling, and player queries etc. needed to (probably) get the same analytics values.

[markmckenna]

Awesome discussion!

@tonihei regarding using the background thread, note that we actually use that to take asynchronous methods and make them synchronous--for example pairing up a seek event with the event that indicates the seek has completed successfully, so we don't run into situations such as the user requests a seek just as content is changing, and the seek action ends up carrying over into the new piece of content because things happened in an unexpected order. This is a bit of a trivial example in that I can see other ways to fix it, but it gets more complex with SSAI and corrective seeking, our plugin architecture, when handing the player lib off to other developers to control who may not be familiar with the ins and outs of player development, and so on. It's probable that we could've solved everything by queueing on the main thread, but I'm not sure it would've been a better solution (...I'm also not sure it wouldn't be a better solution, mind you).

[natez0r]

@tonihei thanks for the response! (sorry I was OOO since I posed my message). We call getCurrentPosition every 10ms in our "ticking" code.

There are a bunch of components that are based on the Ticks. We convert them into events and all of our underlying analytics code uses the ticks to collect various statistics on a set cadence as well as simpler things like a throttled (roughly 20FPS) event that the UI elements use to be informed of the progress of the player.

We're not married to the 10ms cadence, it was just a value that was chosen across platforms so we'd have similar accuracy on iOS/Android. Could you you elaborate on "listening for how long a video has been played" using the AnalyticsCollector? I'd be willing to try something new, considering I do NOT like the separate thread ticking because we have to be hyper aware about the state of the player and make sure we've actually made progress during this 'tick'

[tonihei]

@natez0r
If I understand you correctly, you want to report the position of the player and whether it's playing at regular 10ms intervals. What I meant is that it's possible to listen to events which change the state without loosing any information. For example, listen to onPlayerStateChanged, onPositionDiscontinuity and onTimelineChanged (which may include a discontinuity) and possibly also onPlaybackParametersChanged and check the current position in each callback. Between each of these events, the player was either playing (STATE_READY and playWhenReady=true) and you can assume the position progressed linearly from its last position (with the current playback speed) OR it stayed where it was if the player is not playing. This should be enough information to reconstruct every 10ms of playbacks if you like, but without actually calling a method every 10ms. Does that make sense? Or is there something missing?

[tonihei]

We also decided to move forward with option 1 above, but without actual assertions for now.

• This keeps the possibility of using ExoPlayer from a background thread.
• Access to the player needs to happen on one thread only (i.e. no change to the current situation). Parallel access is too error-prone and we may introduce deadlocks into existing code when we start adding synchronized keywords for all methods and callbacks. The proposed immutable state object would work but is too much of an API change for now.
• It will log warnings if the player is accessed from another thread. That is to alert developers of the potential issue. It won't throw an exception for now to ensure we don't break people unnecessarily.

[tonihei]

Closing this issue because we settled on an implementation option, added documentation to reduce confusion, and added warnings if player is used from the wrong thread unintentionally.