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

SeekTo max position is limited to current track duration since 2.10.0 #6493

Closed
utwyko opened this issue Oct 1, 2019 · 6 comments
Closed
Assignees
Labels

Comments

@utwyko
Copy link

utwyko commented Oct 1, 2019

[REQUIRED] Searched documentation and issues

I've dived into the source code of ExoPlayerImpl and BasePlayer, searched the docs, issuetracker and SO and investigated the changelogs from2.9.6 to 2.10.0

[REQUIRED] Question

Upgrading from 2.9.6 to 2.10.0+ broke the seeking functionality of an app I am working on. We load in N number of queue items through a MediaControllerCompat that to the user make up a single track. We display the total duration of all the loaded in tracks together through a custom AppCompatSeekBar. The user can seek on this seekbar.

After upgrading to 2.10.0+, when the user seeks outside of the currently loaded track, the seek is done to the end of the currently loaded track. For example:

  • The currently displayed track consists of two different items with both 20 seconds in duration, meaning the total duration is 0:40.
  • The user current duration is 0:05 and seeks to 0:30
  • The seek is done to the end of the first item, which results in a seek to 0:20

From what I see, the commit that caused this was 063f570#diff-b7efe9a0ad2f3e37d9b61516d7d52a33R734 . Instead of dispatching the seek, there is now an extra control that checks if the seek does not exceed the current player duration. However, this duration only looks at the current item.

When adding the queue items, we add the duration of all the individual items by setting the MediaMetadataCompat.METADATA_KEY_DURATION of the item. I think ExoPlayer should be able know the total duration of the track by calculating the total sum of the items. Then, it should limit the seek based on this instead of the current item.

Is there any way we can instruct ExoPlayer to seek without limiting based on the duration of the current item? Or could the behaviour be reverted or changed to look at the total duration of all the items?

A full bug report captured from the device

Link to test content

@kim-vde kim-vde assigned kim-vde and marcbaechinger and unassigned kim-vde Oct 1, 2019
@google google deleted a comment from google-oss-bot Oct 2, 2019
@ojw28 ojw28 added bug and removed question labels Oct 2, 2019
@ojw28 ojw28 assigned ojw28 and unassigned marcbaechinger Oct 3, 2019
@ojw28 ojw28 removed the needs triage label Oct 3, 2019
@ojw28
Copy link
Contributor

ojw28 commented Oct 3, 2019

It's a little unclear to me how your solution works end to end. In particular, how do you handle the seek once it's dispatched (when using 2.9.6)? Are you doing something special in your ControlDispatcher's dispatchSeekTo?

@utwyko
Copy link
Author

utwyko commented Oct 4, 2019

It's a little unclear to me how your solution works end to end. In particular, how do you handle the seek once it's dispatched (when using 2.9.6)? Are you doing something special in your ControlDispatcher's dispatchSeekTo?

Yes. In dispatchSeekTo, we take the newPosition (which previously was the proper duration, but is now limited by the current item duration) and we retrieve the proper windowIndex associated with that duration. We do this based on our own metadata stored in memory. Then we calculate the relative duration within that item and dispatch the seek to the player with player.seekTo(index, relativeDuration).

If there is a smarter way to let ExoPlayer handle this logic without this custom logic, I'd be ecstatic :). We're currently not able to switch to a smarter protocol like HLS so we have to do our own stitching based on a custom protocol.

@tonihei
Copy link
Collaborator

tonihei commented Oct 4, 2019

Do you always want to treat all items as one for the playback? If so, then you would probably benefit from the ConcatenatingMediaSource.isAtomic flag once we implement the enhancement tracked by #4868. That would merge your media together to one piece, so no need for custom seek bars nor for special seek dispatching.

@ojw28 ojw28 added question and removed bug labels Oct 4, 2019
@ojw28
Copy link
Contributor

ojw28 commented Oct 4, 2019

Yes, I think that would help. I guess the other option would be for us to implement the equivalent of PlayerControlView.setShowMultiWindowTimeBar for MediaSessionConnector as well (and PlayerNotificationManager, for completeness).

Removing the bug label because I don't think this is really a bug. It's more a case of application code digging around in a way that we don't anticipate. ExoPlayer has a model of the media, and our UI components (and components like MediaSessionConnector that connect to UI components) are based on that model. What you're doing is, I think, making a superficial change high up in one of those components without modifying the underlying model, which is why things break down.

The issue @tonihei refers to will provide a way to actually adjust the model. My suggestion above would work at the UI layer only, but would provide a more complete solution than what you're currently doing.

@tonihei - What do you think is best to do here? Mark as a duplicate of #4868? Or as an enhancement to add PlayerControlView.setShowMultiWindowTimeBar in our other UI components? Is the latter redundant once #4868 is done? If so I guess it would be wasted effort.

@utwyko - If it would help in the short term, we could probably make something protected in MediaSessionConnector, to let you override the clamping and move forward to a newer release. Let us know if that would be helpful.

@tonihei
Copy link
Collaborator

tonihei commented Oct 4, 2019

Once #4868 is done, the multi window time bar is most likely redundant. At least it's hard to come up with a reason why you would only want to merge sources on UI level and not on actual source level. Maybe some form of chapter-based video where the whole stream should be shown in the UI, but the next/previous buttons just jump to the next/previous chapter? If that's something we plan to support, then keeping the multi-window time bar may be right. Otherwise, if that sounds too niche, we should probably just focus on #4868.

@utwyko
Copy link
Author

utwyko commented Oct 8, 2019

Yes all items should always we be treated as one and are displayed in the UI as such. The reason why they're divided up into separate items is so we can insert personalised content into the audio streams (which are not visible in the UI). #4868 sounds interesting, and we'll gladly get rid of our special seek dispatching.

Thanks for your help. I'm fine with closing this ticket and watching #4868.

@ojw28 ojw28 closed this as completed Oct 16, 2019
@google google locked and limited conversation to collaborators Dec 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

6 participants