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

feat: Support Parallel Segment Fetching #4784

Merged
merged 12 commits into from
Jan 31, 2023

Conversation

tyrelltle
Copy link
Contributor

@tyrelltle tyrelltle commented Dec 3, 2022

closes #4658.

This solution is inspired by abandoned PR #2809, which implements segment prefetching ahead of current play head.

image

@tyrelltle tyrelltle changed the title Parallelfetching feat: Support Parallel Segment Fetching Dec 3, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 3, 2022

Incremental code coverage: 98.17%

@avelad avelad added type: enhancement New feature or request priority: P2 Smaller impact or easy workaround labels Dec 5, 2022
@avelad avelad added this to the v4.4 milestone Dec 5, 2022
@avelad avelad requested review from joeyparrish and theodab December 5, 2022 08:05
@tyrelltle tyrelltle marked this pull request as ready for review December 6, 2022 01:07
@avelad avelad self-requested a review December 7, 2022 11:05
@avelad
Copy link
Member

avelad commented Dec 7, 2022

Thanks for your patience, I'll try to review it throughout this week.

externs/shaka/player.js Outdated Show resolved Hide resolved
externs/shaka/player.js Outdated Show resolved Hide resolved
lib/media/streaming_engine.js Outdated Show resolved Hide resolved
lib/media/streaming_engine.js Outdated Show resolved Hide resolved
lib/media/streaming_engine.js Outdated Show resolved Hide resolved
@joeyparrish
Copy link
Member

I skimmed this PR, and it looks interesting and well-constructed. You've designed something that appears to address most of my concerns, it defaults to 0 (good for low-memory devices), and the prefetcher cleans up after itself so it doesn't hold onto an unbounded number of segments. This could also be useful in a future implementation of #880 (preload API).

I haven't read it in detail, and I have some deadlines to get AV sync fixes done this week before we go on holidays at Google. But I will come back to this in January for a detailed review, and I expect this is something that will ultimately ship in Shaka Player.

Thanks for your contribution!

@tyrelltle
Copy link
Contributor Author

tyrelltle commented Dec 13, 2022

Thanks @joeyparrish !
Assuming this change goes into main branch in January, when a new version that has the change will be released ?
Just trying to understand the overall release process.

Thanks again for helping on the code reviews !

@joeyparrish
Copy link
Member

We can make a feature release at any time. We don't have a fixed release cycle.

@avelad
Copy link
Member

avelad commented Dec 16, 2022

@tyrelltle can you rebase and resolve the conflicts? Thanks!

@tyrelltle
Copy link
Contributor Author

@avelad yes I just rebased now

avelad
avelad previously approved these changes Jan 5, 2023
demo/locales/source.json Outdated Show resolved Hide resolved
externs/shaka/player.js Outdated Show resolved Hide resolved
lib/media/segment_prefetch.js Outdated Show resolved Hide resolved
lib/media/segment_prefetch.js Outdated Show resolved Hide resolved
lib/media/segment_prefetch.js Outdated Show resolved Hide resolved
lib/media/streaming_engine.js Show resolved Hide resolved
lib/media/streaming_engine.js Outdated Show resolved Hide resolved
lib/media/streaming_engine.js Outdated Show resolved Hide resolved
lib/media/streaming_engine.js Outdated Show resolved Hide resolved
test/test/util/fake_segment_prefetch.js Outdated Show resolved Hide resolved
@tyrelltle
Copy link
Contributor Author

@theodab thanks for the comments !
Agree all of them and will make sure address them early next week

@tyrelltle
Copy link
Contributor Author

@theodab thanks for the comments ! Agree all of them and will make sure address them early next week

addressed some of the comments, will finish other ones by tomorrow

Copy link
Contributor

@theodab theodab left a comment

Choose a reason for hiding this comment

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

Thanks for the work, by the way.

lib/media/segment_prefetch.js Outdated Show resolved Hide resolved
test/test/util/fake_segment_prefetch.js Outdated Show resolved Hide resolved
@tyrelltle
Copy link
Contributor Author

Thanks for the work, by the way.

Thanks for the comments I updated all of them. Let me know if any other suggestions :)

@tyrelltle tyrelltle requested review from theodab and removed request for joeyparrish January 16, 2023 00:24
@avelad avelad self-requested a review January 22, 2023 11:15
@avelad avelad merged commit de6abde into shaka-project:main Jan 31, 2023
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority: P2 Smaller impact or easy workaround status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for fetching multiple segments in parallel
4 participants