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

improve the performance of short seek #2237

Closed

Conversation

WeiChungChang
Copy link
Contributor

@WeiChungChang WeiChungChang commented Dec 21, 2016

http://programmingmemojohnchang.blogspot.tw/2016/12/improve-performance-of-short-seek-of.html

Please read in detail by the link above which includes the result of experimental comparison.

For streaming, current flow may hit the case where the whole pre-loaded data will be flushed. However, sometimes it is unnecessary to do so. An example is the short seek to the place a little ahead current playback position (ex, current playback time is 15 second & seek to 18 seconds).

Once all pre-loaded data is flushed, ex: for 4K movie, a painful delay will happen.
Also, it hurts for users who do not have unlimited network data plan.

Some MPEG DASH links such as YouTube are made according to the settings of the max interval between key frames is over 5 seconds. Hence the short seek may probably hit the claimed case.

To improve it, we may take the same way applied in GStreamer which filters the decoded-only samples at renderer.

The experimental result shows:

  1. We save a lot of data download compared to original flow.
  2. The seek response time is near to be the same as seamless; compared to originally it has a noticeable delay.

The experiment is based on branch = release-v2.
The only problem is that I DO NOT understand the flow in branch dev-v2 which forces to add RENDERER_TIMESTAMP_OFFSET_US to renderer offset.
Hence we will get a biased value from onProcessedOutputBuffer.
Since I believe we should report the right time instead, I still put the patch here without taking the offset into account in this patch.

Thanks.

@WeiChungChang
Copy link
Contributor Author

Dear Sir:
Please help to check why the cla is missed here.
I wonder why previous request is O.K but it fails here.
Thanks a lot~

@ojw28
Copy link
Contributor

ojw28 commented Jan 5, 2017

Whilst the long seek case has an obviously correct solution, the best thing to do for short seeks is less clear. I'm worried about anything that adds complexity to the boundary between sources (including sample streams) and the renderers that consume them, since any complexity on that boundary is additional cost (and potential for introducing bugs) that's incurred for all source and renderer implementations.

A more obviously correct solution, that can work for all sources and doesn't change the mentioned boundary, would be to change DefaultTrackOutput so that instead of discarding a sample as soon as it's read, it instead retains samples from the previous key-frame. When a new key-frame is read all samples up to it can then be discarded. When a short seek occurs you'll then get optimized performance with the existing seeking logic, since retaining from the previous key-frame will mean that skipToKeyframeBefore returns true rather than false.

The downside of this approach is that some additional memory will be required, since samples are retained in the queue for a bit longer. I think it's worth it for implementation sanity, however.

Thoughts?

@ojw28
Copy link
Contributor

ojw28 commented Jan 5, 2017

Note that retaining samples from the previous key-frame is useful for resolving some other issues we currently face as well. For example when you replace the Surface, which requires the decoder to be re-initialized, which requires you to feed samples from the previous key-frame through the decoder in order to get back to the frame you were displaying. Right now I think the renderer skips ahead to the next key-frame, which is something we should try and fix at some point.

@WeiChungChang WeiChungChang deleted the improve-short-seek branch January 20, 2017 06:49
@google google locked and limited conversation to collaborators Jun 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants