Skip to content

Formal API to allow apps to handle playback resumption themselves #2458

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nift4
Copy link

@nift4 nift4 commented May 22, 2025

This could previously be achieved by inspecting stack trace of
onPlaybackResumption() and then returning settable future that will
never be set, but this API adds a cleaner way and makes the caveats
of this clearer.

Issue: #1764

@tonihei tonihei self-assigned this May 30, 2025
@tonihei
Copy link
Collaborator

tonihei commented Jun 30, 2025

Thanks for the suggestions! This PR currently mixes multiple things together, and it may be helpful to untangle this a bit. As far as I can see, there are 3 proposals here:

  1. Update check for whether this is the boot time SysUI query for playback resumption to use !playWhenReady instead state==IDLE.
    • This makes sense for the scenario you describe, but may fail if System UI checks playback resumption at a different time. The player may reasonably be in a paused state for example, which would trigger the wrong path here. I guess a better distinction is to check whether System UI started the service just for this check (vs connecting to an already existing service). Do you want to file a new issue for this so we can see if that's possible to implement?
  2. Add new parameter to the onPlaybackResumption callback to distinguish the "information only" from the "playback will be started soon" case. This is actually quite useful to avoid work and I think we can use this PR to merge this part of the change.
  3. Add a way to intercept playback resumption handling entirely to do something else. It would be worth figuring out what "something else" is to see how it fits into the picture. The general assumption from our perspective was that you'd be able to call additional setup methods from onPlaybackResumption. In any case, it looks like this isn't implemented by this PR fully (or I missed the part where the new exception is thrown) and it's better to handle that separately.

@nift4
Copy link
Author

nift4 commented Jun 30, 2025

Hi @tonihei, thanks for your feedback.

This makes sense for the scenario you describe, but may fail if System UI checks playback resumption at a different time. The player may reasonably be in a paused state for example, which would trigger the wrong path here.

Yes, I don't think it's a problem to do that though as while the wrong path might be more expensive, it will not be functionally wrong. I also did some basic testing and didn't see SystemUI querying playback resumption while paused during that testing. Though I admit that it'd be better to detect that, I am not sure how that would work. I'll file an issue (edit: #2578) to discuss further and drop it from the PR for now. If we find a better way, great, and if not I can push the commit again into a new PR.

it would be worth figuring out what "something else" is to see how it fits into the picture.

My usecase is actually just setShuffleOrder. Can't do that before setMediaItems (I get IllegalArgumentException iirc), but if library calls setMediaItems I don't see any non-racy way to add a setShuffleOrder call after that. For now I've hacked my ShuffleOrder to just return a predefined result in cloneAndInsert if something is loaded to disk, but it's rather suboptimal hack.
An alternative solution I can imagine is adding ShuffleOrder to MediaItemsWithStartPosition, but that would require going back to the drawing board in order to refactor for #325, which I do not see happening anytime soon (it's a hard problem - how do you serialize shuffle order and their possible mutations for media controllers sending them via Binder? if you want to support setting shuffle orders via Player interface, you'd need to handle that).

or I missed the part where the new exception is thrown

The idea is that app code calls setException on the SettableFuture it returns from onPlaybackResumption when it wants to manually do playback resumption.

Add new parameter to the onPlaybackResumption callback to distinguish the "information only" from the "playback will be started soon" case. This is actually quite useful to avoid work and I think we can use this PR to merge this part of the change.

I agree this is useful on it's own, but I specifically added it to go hand-in-hand with manually handling playback resumption, as manual handling only makes sense if we want to actually play stuff. I am not opposed to adding that first if you think the manual handling makes no sense or needs to be iterated upon first, though.

Let me know whether I should split out manual handling as well.

This could previously be achieved by inspecting stack trace of
onPlaybackResumption() and then returning settable future that will
never be set, but this API adds a cleaner way and makes the caveats
of this clearer.

Issue: androidx#1764
@nift4 nift4 changed the title Fix a bug and add a feature for media resumption Formal API to allow apps to handle playback resumption themselves Jun 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants