Move legacy session calls to a new thread

[nift4]

Issue: https://github.com/androidx/media/issues/2577 Test: add Thread.sleep(5000) to setMetadata(), app stays responsive

[tonihei]

Thanks for providing the PR - I probably need a bit of time to review it in more detail.

Some comments from a quick glance:

• There is blocking call in onMediaButtonEvent which we shouldn't keep if the goal is to remove all potentially blocking calls
• Introducing the new thread on this level in MediaSessionLegacyStub makes it harder to reason about the flow as we suddenly need to account for the possibility of more than one thread in the same class. I wonder if it would be much cleaner to handle the threading in its own dedicated place (e.g. inside MediaSessionCompat?).
• Eventually, all of the binder calls triggered from the media session module (including the media3-only ones!) should be done from a shared background thread. We can start with more isolated cases like the calls to the platform MediaSession, but that also means it's fine to create the background thread high-up in the chain if needed.
• One potential concern is about delicate timing issues - setting the platform media session state and updating the notification for example may now happen in a slightly different order. The underlying risk is that some other logic may accidentally rely on a certain execution order at the moment and break. This kind of issue is really hard to predict or avoid, but just noting that this is one of the reasons I haven't touched this part yet.

[nift4]

Hi,

There is blocking call in onMediaButtonEvent which we shouldn't keep if the goal is to remove all potentially blocking calls

It blocks the media session compat interaction thread, not the main thread, so it shouldn't be too much of a problem (may delay media button clicks but ig we're processing an earlier event, we have to delay anyway).

Introducing the new thread on this level in MediaSessionLegacyStub makes it harder to reason about the flow as we suddenly need to account for the possibility of more than one thread in the same class. I wonder if it would be much cleaner to handle the threading in its own dedicated place (e.g. inside MediaSessionCompat?).

Yes, the callbacks from legacy session side (onMediaButtonEvent, onPlay, etc) are called on the legacy session interaction thread, while the ControllerLegacyCbForBroadcast callbacks are called on session application thread. But we need to know when setMetadata is done in order to make sure notification update is only posted after that.

Eventually, all of the binder calls triggered from the media session module (including the media3-only ones!) should be done from a shared background thread. We can start with more isolated cases like the calls to the platform MediaSession, but that also means it's fine to create the background thread high-up in the chain if needed.

I think we should start with individual threads for now and revisit later.

One potential concern is about delicate timing issues - setting the platform media session state and updating the notification for example may now happen in a slightly different order. The underlying risk is that some other logic may accidentally rely on a certain execution order at the moment and break. This kind of issue is really hard to predict or avoid, but just noting that this is one of the reasons I haven't touched this part yet.

I sidestepped this by not reacting to events in the media notification controller and instead call onNotificationRequestRequired() when calls like setMetadata are actually done.

I experimented with delaying media3 controller calls until platform session is done but it added 500 lines to the diff and caused hard-to-reproduce crashes, so I dropped it.

[nift4]

rebased and pushed a small patch to be less spammy with notification updates if multiple events happen in one Looper iteration, similar to how onEvents() worked

[nift4]

Hi @tonihei, I noticed I had not really addressed any of the review comments properly before, sorry about that. I now uploaded a refactored version, could you please take a look again? I'm trying to get my huge stack of patches upstreamed (or otherwise the need resolved), because it takes quite some time to maintain. Thanks a lot!

There is blocking call in onMediaButtonEvent which we shouldn't keep if the goal is to remove all potentially blocking calls

This is now gone because the callback thread changed to session thread.

Introducing the new thread on this level in MediaSessionLegacyStub makes it harder to reason about the flow as we suddenly need to account for the possibility of more than one thread in the same class. I wonder if it would be much cleaner to handle the threading in its own dedicated place (e.g. inside MediaSessionCompat?).

It's moved to MediaSessionCompat with ListenableFuture in the few places where we need to wait for it to be complete, and you're right, it looks a lot better.

Eventually, all of the binder calls triggered from the media session module (including the media3-only ones!) should be done from a shared background thread. We can start with more isolated cases like the calls to the platform MediaSession, but that also means it's fine to create the background thread high-up in the chain if needed.

Introduced a backgroundThread in MediaSessionImpl for that purpose.

One potential concern is about delicate timing issues - setting the platform media session state and updating the notification for example may now happen in a slightly different order. The underlying risk is that some other logic may accidentally rely on a certain execution order at the moment and break. This kind of issue is really hard to predict or avoid, but just noting that this is one of the reasons I haven't touched this part yet.

I had shipped the previous version of this patch in beta channel for months without issues now. :) At least the subset of media3 that my app uses appears to be fine.