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.

Sign up for GitHub

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

Jump to bottom

Asynchronous onAddMediaItems immediately get skipped when seeking to them #85

Closed
lukel97 opened this issue Jun 12, 2022 · 3 comments
Closed

Asynchronous onAddMediaItems immediately get skipped when seeking to them #85

lukel97 opened this issue Jun 12, 2022 · 3 comments
Assignees
@tonihei
Labels
bug

Comments

@lukel97
Copy link

lukel97 commented Jun 12, 2022 •
edited
Loading

Hi there, we’ve been trying out the main branch for the onAddMediaItems callback which is exactly what we require for our use case:
Our app adds a media item to the playlist via the controller but it only knows the media id of the item, the session itself then resolves the URL and metadata by communicating to an external server.

It’s been working great so far but the one edge case we’ve found so far is that when seeking to a media item that was just added, but not yet resolved, the player seems to immediately skip over it.

For example, if your playlist looks like

A C
^

where A is the currently playing media item, and then you add an “unresolved" media item B so it then becomes

A B C
^

If you seek to the next media item via seekToNextMediaItem, it immediately skips past B to C:

A B C
    ^

I’ve tested that this doesn’t happen if onAddMediaItems resolves immediately with a URI (via Futures.immediateFuture).

Is this by design? Or are we maybe misusing onAddMediaItems here?

The revision this is happening on is a105d03. (We’ve built snapshot artifacts here if anyone else is interested in riding the bleeding edge)
@tonihei
Copy link
Collaborator

tonihei commented Jun 15, 2022

Thanks for reporting! We are actually aware that this may an issue that needs to be fixed.

The reason this is happening is the asynchronous nature of the method call. After calling addMediaItem, the media controller pretends the playlist already looks like A B C. The session, however, is running the asynchonous media item resolution (triggered by onAddMediaItems) and its playlist still looks like A C. The seek command will be resolved immediately, so that the session seek to its next item (C) first and only once the new item is resolved it will insert it into the playlist.

The solution will be to better enforce the ordering of commands even if some of them need asynchronous resolution (this applies to other asynchronous commands as well, e.g. setRating and custom commands).

rohitjoins pushed a commit that referenced this issue Jul 21, 2022
@tonihei @rohitjoins
Use Futures for MediaSession command queue instead of Runnables
dee8078 
Some commands may be asynchronous and subsequent commands need to
wait for them to complete before running. This change updates the
queue to use (and listen to) Futures instead of calling Runnables
directly. The commands are currently still added as Runanbles
though, so this change is a no-op.

Also moves the permission check in MediaSessionImpl to before
queueing the command because the permission should be check at
the time of calling the method.

When executing the comamnds in the queue, we need to be careful
to avoid recursion in the same thread (which happens when both
the Future is immediate and running on the correct thread already).
To avoid recursion, we detect this case and loop the commands
instead.

Issue: #85
PiperOrigin-RevId: 461827264
rohitjoins pushed a commit that referenced this issue Jul 21, 2022
@tonihei @rohitjoins
Properly chain commands in MediaSessionStub
45f1f5b 
The commands currently use a task and a postTask that are chained
together manually. In some cases, e.g. when adding MediaItems,
the postTask is already a chain of commands in itself.

To allow using the entire command handling as a single task
(for simplified queueing), we can change the implementation to
always create a single task. If multiple subtasks need to be
chained together, we can do that by wrapping the method calls.
In case a task is asynchronous, we can also use Futures to
chain them together.

Overall, this is just a refactoring and changes no logic.

Issue: #85
PiperOrigin-RevId: 462085724
rohitjoins pushed a commit that referenced this issue Jul 21, 2022
@tonihei @rohitjoins
Run MediaSessionStub commands in order
7cb7636 
Some commands are run asynchronously and subsequent commands need
to wait until the previous one finished. This can be supported
by returning a Future for each command and using the existing
command execution logic to wait for each Future to complete.

As some MediaSessionStub code is now executed delayed to when it
was originally created, we also need to check if the session is
not released before triggering any actions or sending result codes.

Issue: #85
PiperOrigin-RevId: 462101136
This was referenced Aug 16, 2022
Closed
Closed
Merged
@tonihei tonihei closed this as completed Oct 4, 2022
microkatz pushed a commit that referenced this issue Nov 22, 2022
@tonihei @microkatz
Use Futures for MediaSession command queue instead of Runnables
481205a 
Some commands may be asynchronous and subsequent commands need to
wait for them to complete before running. This change updates the
queue to use (and listen to) Futures instead of calling Runnables
directly. The commands are currently still added as Runanbles
though, so this change is a no-op.

Also moves the permission check in MediaSessionImpl to before
queueing the command because the permission should be check at
the time of calling the method.

When executing the comamnds in the queue, we need to be careful
to avoid recursion in the same thread (which happens when both
the Future is immediate and running on the correct thread already).
To avoid recursion, we detect this case and loop the commands
instead.

Issue: #85
PiperOrigin-RevId: 461827264
(cherry picked from commit dee8078)
microkatz pushed a commit that referenced this issue Nov 22, 2022
@tonihei @microkatz
Properly chain commands in MediaSessionStub
d84662e 
The commands currently use a task and a postTask that are chained
together manually. In some cases, e.g. when adding MediaItems,
the postTask is already a chain of commands in itself.

To allow using the entire command handling as a single task
(for simplified queueing), we can change the implementation to
always create a single task. If multiple subtasks need to be
chained together, we can do that by wrapping the method calls.
In case a task is asynchronous, we can also use Futures to
chain them together.

Overall, this is just a refactoring and changes no logic.

Issue: #85
PiperOrigin-RevId: 462085724
(cherry picked from commit 45f1f5b)
microkatz pushed a commit that referenced this issue Nov 22, 2022
@tonihei @microkatz
Run MediaSessionStub commands in order
eb823a9 
Some commands are run asynchronously and subsequent commands need
to wait until the previous one finished. This can be supported
by returning a Future for each command and using the existing
command execution logic to wait for each Future to complete.

As some MediaSessionStub code is now executed delayed to when it
was originally created, we also need to check if the session is
not released before triggering any actions or sending result codes.

Issue: #85
PiperOrigin-RevId: 462101136
(cherry picked from commit 7cb7636)
@luizgrp
Copy link

luizgrp commented Dec 7, 2022

@tonihei could you please confirm in which release version this issue was fixed?

@marcbaechinger
Copy link
Contributor

If you use the latest version that is 1.0.0-beta03 then this is included.

@androidx androidx locked and limited conversation to collaborators Mar 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@tonihei tonihei

Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@marcbaechinger @luizgrp @lukel97 @tonihei