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

HLS/MP3: SeekTo does not seek to exact location #6155

Closed
kschults opened this issue Jul 9, 2019 · 26 comments
Closed

HLS/MP3: SeekTo does not seek to exact location #6155

kschults opened this issue Jul 9, 2019 · 26 comments
Assignees
Labels

Comments

@kschults
Copy link

kschults commented Jul 9, 2019

[REQUIRED] Issue description

Calling seekTo when playing from an HLS playlist does not always seek to the exact location. After calling seekTo, playback appears to begin at the beginning of the playlist chunk containing the requested location. From the docs, it appears that exact seeking should be supported.

[REQUIRED] Reproduction steps

  1. Load the playlist into the sample player
  2. Seek from the beginning to (e.g.) 270757ms (I accomplished this by hardcoding the seek location into PlayerControlView.seekToTimeBarPosition)
  3. Playback resumes at approximately 240047ms
  4. Once the playhead is within the current chunk, seeking to that location behaves as expected.

[REQUIRED] Link to test content

Sample playlist has been sent to the dev.exoplayer email.

[REQUIRED] A full bug report captured from the device

Bug report has been sent with the sample playlist.

[REQUIRED] Version of ExoPlayer being used

I have reproduced this on 2.9.4 and 2.10

[REQUIRED] Device(s) and version(s) of Android being used

I am currently using a Pixel 3 running Android 9. We've had a couple instances of it happening on a Pixel 1 running 9 as well, but less frequently. I've been thus far unable to reproduce it on an emulator.

@ojw28
Copy link
Contributor

ojw28 commented Jul 10, 2019

I cannot reproduce this. What are you looking at to determine (3)?

@ojw28 ojw28 self-assigned this Jul 10, 2019
@kschults
Copy link
Author

kschults commented Jul 10, 2019

In the demo application, I'm seeing that the seekbar goes to ~4:30 (270757ms) when I drop it (I hardcoded the seekbar to use that number - this is replicating a seek based on chapter metadata), but then it jumps back to ~4:00 (240047ms) when playback resumes.

In my own app, I've verified the location where playback resumes by polling the current state of the player.

@ojw28
Copy link
Contributor

ojw28 commented Jul 10, 2019

I do not observe this behavior with 2.9.4, 2.10.0 or the current dev branch, using the sample provided. The seek bar goes to 4:30, and then stays there. When playback resumes the seek bar goes to 4:31, 4:32 etc as expected.

@kschults
Copy link
Author

That's really odd. I can consistently reproduce this using the most up-to-date version of the demo library. To expand my steps:

  • I changed PlayerActivity L 333 to hardcode a list containing my playlist URI
  • I changed PlayerControlView and inserted positionMs = 270757; as L 1013
  • I launch the app, and select the first HLS track.
  • Once playback starts, I drag the seekbar to anywhere in the middle and release

When I do that, the seekbar first snaps to 4:31 as it requests the seek, and then jumps back to 4:00 as playback jumps to that location. A second seek to that location works correctly, but the issue reproduces every time I close and re-open the player activity.

I attached a debugger in my own application, and followed the seekTo request, and determined that internally, exoplayer is calling seekTo with the desired value, but I was unable to follow any further than determining that it started loading the chunk that contains 4:30.

Would it be helpful if I provided a video of the behavior?

@ojw28
Copy link
Contributor

ojw28 commented Jul 11, 2019

I tried on a few different devices and this doesn't reproduce for me. A video would be useful just to see what you're seeing, however I'm not sure what we'll be able to do if we're still unable to reproduce!

@kschults
Copy link
Author

Here's a gif of the screen recording, since GitHub doesn't allow mp4 attachments. It lost a little of the framerate in the conversion, but it still shows the issue. You an see where when I release the seekbar, it snaps to 4:31 as it calls seekTo(270757), and then jump to 4:00 as playback changes to there.

If I pause the player before seeking, it stays at 4:31 until I hit play, at which point it jumps to 4:00.

seek

@ojw28
Copy link
Contributor

ojw28 commented Jul 11, 2019

Hmm! Weird. I do not see that. Do you definitely see this across all devices? I tested on a Galaxy S8 and a Pixel 2 so far.

@kschults
Copy link
Author

@ojw28 Sorry, I had thought from our QA team that it was happening across devices. I just double-checked with them, and it's more limited.

It's happening extremely frequently to myself and to our QA lead. We're both using Pixel 3 (9.0). One other QA person has experienced it on a Pixel 1 (9.0), but only infrequently. We don't have any Pixel devices on other versions, so we haven't been able to test that out.

I also just tested across a range of emulators, and couldn't reproduce it on any of them, including a "Pixel 3" 9.0.

I'll update the ticket that it seems to be limited to Pixel devices, mostly Pixel 3, and I'll get the rest of the team to keep trying other devices.

@kschults
Copy link
Author

The QA team has also been able to reproduce it consistently on a Nokia 6 (8.1.0) and a Note 4 (6.0.1)

@ojw28
Copy link
Contributor

ojw28 commented Jul 15, 2019

I've filed an internal bug about this [Internal ref: 137524007]. The problem is that the MP3 decoder on affected devices is adjusting buffer presentation timestamps in a way that we don't expect.

That said, it may be possible (and a good idea) for us to adjust the way we're handling this type of seeking so that the implementation is not sensitive to these types of decoder adjustment.

We currently associate a decodeOnly flag with each buffer, and we pass these flags around the side of the decoder as metadata, using the buffer timestamp as the key. Hence any modifications to the buffer timestamps by the decoder prevent correct re-association of the metadata on the output side. A simpler (but less flexible) alternative is to have the renderer know what the seek position is, and apply decodeOnly too all buffers up to the one immediately proceeding that position.

@kschults
Copy link
Author

I'm glad you were able to track that down.

I think it definitely makes sense to protect that timestamp from modifications by the underlying implementations. While it's only on a small number of devices, it's definitely a very bad user experience for those that are affected. We layer some chapter metadata on top of the audio, and for devices that are affected, it means that skipping to the next chapter or using the table of contents doesn't have the desired effect.

@ojw28
Copy link
Contributor

ojw28 commented Jul 15, 2019

As an additional point, it's also sub-optimal that we're feeding all of the media from the start of the segment through the decoder as decodeOnly. For MP3 we could just discard right up to the first sample that's required in the source.

For DASH and SmoothStreaming I'm pretty sure we don't do this. HLS using MPEG-TS makes it a lot harder to do the same thing for HLS, but we can probably do it specifically for tracks where we know that every sample is a sync sample (typically true for audio tracks). We should take a look at this as well.

@kschults
Copy link
Author

@ojw28 Is this fix something that it would be possible to get into an upcoming release?

@kschults
Copy link
Author

@ojw28 Any updates on this? Thanks

@kschults
Copy link
Author

kschults commented Nov 7, 2019

We currently associate a decodeOnly flag with each buffer, and we pass these flags around the side of the decoder as metadata, using the buffer timestamp as the key. Hence any modifications to the buffer timestamps by the decoder prevent correct re-association of the metadata on the output side. A simpler (but less flexible) alternative is to have the renderer know what the seek position is, and apply decodeOnly too all buffers up to the one immediately proceeding that position.

@ojw28 I don't really know enough about media playback to try to figure this out myself. Is it possible to have this fix prioritized?

@andrewlewis
Copy link
Collaborator

This is on our list of things to look at but I'm afraid we likely won't be able to get to it soon.

@kschults
Copy link
Author

@andrewlewis Thanks for the follow-up. I get that this is probably low priority, but I'd definitely love to see it fixed.

@ojw28 ojw28 changed the title SeekTo does not seek to exact location HLS/MP3: SeekTo does not seek to exact location Nov 13, 2019
@ojw28
Copy link
Contributor

ojw28 commented Dec 20, 2019

I've been digging into this a little and I think I can might have spotted a relatively straightforward workaround. Is it possible for you to provide a new test stream so that I can check whether it works? Thanks!

@kschults
Copy link
Author

@ojw28 Thanks for taking a look! I've sent a new stream URL to dev.exoplayer@google.com

@ojw28
Copy link
Contributor

ojw28 commented Jan 10, 2020

We have an initial fix for this, but are discussing a few different options that might be a bit nicer.

If it would be helpful for you to have the fix ahead of that discussion being resolved, please let me know and I can put the initial fix that we have somewhere for you to patch (note: you'll need to build ExoPlayer from source to make use of it, rather than using regular gradle dependencies).

@kschults
Copy link
Author

What's the timeframe for the other options making it into a release? Days? Months?

@ojw28
Copy link
Contributor

ojw28 commented Jan 16, 2020

The final solution looks like it's going to be quite involved, and will likely be included in 2.12.0. In the meantime, I plan to put a temporary fix into the next 2.11.X release. The fix should make it into the dev branch this week, however the release itself will probably in approximately 2 weeks time.

@kschults
Copy link
Author

That's perfect, thank you so much!

ojw28 added a commit that referenced this issue Jan 16, 2020
This method has two use cases:

1. Seeking. Calls are immediately preceded by a call to rewind(), and
   the returned value isn't important unless it's ADVANCED_FAILED (i.e.
   the caller is only interested in success and failure).
2. Advancing. The return value is important unless it's ADVANCED_FAILED,
   in which case the caller wants to treat it as 0.

This change creates separate methods for each use case. The new seekTo
methods automatically rewind and return a boolean. The updated advanceTo
method returns 0 directly in cases where ADVANCED_FAILED was returned.
Arguments that were always hard-coded to true by callers have also been
removed.

This change is a step toward one possible solution for #6155. How we'll
solve that issue is still up for discussion, but this change seems like
one we should make regardless!

Issue: #6155
PiperOrigin-RevId: 290053743
ojw28 added a commit that referenced this issue Jan 16, 2020
Issue: #6155
PiperOrigin-RevId: 290117324
@ojw28
Copy link
Contributor

ojw28 commented Jan 16, 2020

Should be fixed in the dev-v2 branch.

ojw28 added a commit that referenced this issue Jan 17, 2020
This method has two use cases:

1. Seeking. Calls are immediately preceded by a call to rewind(), and
   the returned value isn't important unless it's ADVANCED_FAILED (i.e.
   the caller is only interested in success and failure).
2. Advancing. The return value is important unless it's ADVANCED_FAILED,
   in which case the caller wants to treat it as 0.

This change creates separate methods for each use case. The new seekTo
methods automatically rewind and return a boolean. The updated advanceTo
method returns 0 directly in cases where ADVANCED_FAILED was returned.
Arguments that were always hard-coded to true by callers have also been
removed.

This change is a step toward one possible solution for #6155. How we'll
solve that issue is still up for discussion, but this change seems like
one we should make regardless!

Issue: #6155
PiperOrigin-RevId: 290053743
ojw28 added a commit that referenced this issue Jan 17, 2020
Issue: #6155
PiperOrigin-RevId: 290117324
@ojw28 ojw28 closed this as completed Jan 17, 2020
@kschults
Copy link
Author

@ojw28 I was able to verify that it's fixed in the dev-v2 branch. Hoping that it's able to get into a release shortly. Thanks so much for all the effort you put into it.

@google google locked and limited conversation to collaborators Mar 18, 2020
@ojw28
Copy link
Contributor

ojw28 commented Mar 23, 2020

@kschults - We're getting around to implementing a better fix for this. Would it be possible for you to send us another test stream, so we can make sure the final fix still works properly? Thanks!

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

4 participants