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 far seek performance of chunkSampleStream #2318

Merged
merged 2 commits into from Jan 16, 2017

Conversation

WeiChungChang
Copy link
Contributor

@WeiChungChang WeiChungChang commented Jan 12, 2017

Rework to make the patch accordingly by @ojw28 kindly suggestions #as #2247.

Issue report: #2253

Copy link
Contributor

@ojw28 ojw28 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good; thanks! I think you can make a few further simplifications, but otherwise think we're good to get this merged. Please take a look.

* Seeks to the specified position in microseconds.
*
* @param positionUs The seek position in microseconds.
*/
public void seekToUs(long positionUs) {
lastSeekPositionUs = positionUs;
// If we're not pending a reset, see if we can seek within the sample queue.
boolean seekInsideBuffer = !isPendingReset() && sampleQueue.skipToKeyframeBefore(positionUs);
boolean seekInsideBuffer = !isPendingReset() &&
sampleQueue.skipToKeyframeBefore(positionUs, isWithinLastChunk(positionUs));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure you need to be as sophisticated as checking whether the position is within the last chunk. Isn't it sufficient just to pass positionUs < getNextLoadPositionUs() as the second argument? I don't think passing true does any harm in cases where positionUs is earlier than the last chunk.

This allows you to remove your isWithinLastChunk and getCurrentLoadPosition methods ;).

* @return Whether the skip was successful.
*/
public boolean skipToKeyframeBefore(long timeUs) {
long nextOffset = infoQueue.skipToKeyframeBefore(timeUs);
long nextOffset = infoQueue.skipToKeyframeBefore(timeUs, false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can replace the whole body of this method with just:
return skipToKeyframeBefore(timeUs, false)

@WeiChungChang
Copy link
Contributor Author

Dear @ojw28:
Thanks a lot for your suggestion to make it clearer and simpler.
I have made the new patch as 'refine_to_make_flow_simpler':
2214a4c
Thank you!

@ojw28 ojw28 merged commit a418b13 into google:dev-v2 Jan 16, 2017
@WeiChungChang WeiChungChang deleted the far_seek_improvement branch January 20, 2017 06:45
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants