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: prefetch audio languages. #6139
feat: prefetch audio languages. #6139
Conversation
Implements the behavior outlines in shaka-project#6128. Adds `streaming.prefetchAudioLanguages` and `streaming.disableVideoPrefetch` options. Does not currently have `disableAudioPrefetch` and `disableTextPrefetch`.
This is a draft PR because some tests are failing (they pass in our fork) but I wanted to get this out asap to get some eyes on them. I'll take a look into why tests are failing and fix them before adding in support for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it is not complete, I leave a first review with the things I have seen, but it looks good!
Tests fixed. I also ran a quick check locally and seems to be working as expected, still. Next steps for this PR:
|
Noticed that if I enable |
Also, right now, segment prefetch never runs for text. shaka-player/lib/media/streaming_engine.js Lines 899 to 902 in 701ec9b
Given this, I'm inclined to skip |
I think this PR is ready for a proper review now. |
* The maximum number of segments for each active stream to be prefetched | ||
* ahead of playhead in parallel. | ||
* If <code>0</code>, the segments will be fetched sequentially. | ||
* Defaults to <code>0</code>. | ||
* @property {!Array<string>} prefetchAudioLanguages | ||
* The audio languages to prefetch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching is a lot faster if rebufferingGoal is set to a low value. Is this something worth noting here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think it's necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to push changes to your branch to correct these comments. Thank you!
lib/media/segment_prefetch.js
Outdated
if (!reference) { | ||
return; | ||
} | ||
if (reference.initSegmentReference) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if should be moved inside the while loop and take into account whether prefetchAllowed (I would move it to line 110-111)
lib/media/segment_prefetch.js
Outdated
return; | ||
} | ||
const currTime = startReference.startTime; | ||
const maxTime = Math.max(currTime, this.prefetchPosTime_); | ||
const iterator = this.stream_.segmentIndex.getIteratorForTime(maxTime); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized that this would not work for partial segments in HLS, so I am going to make a change so that it works as well.
Please fix the tests |
Tests fixed. |
Implements the behavior outlines in #6128.
Adds
streaming.prefetchAudioLanguages
andstreaming.disableVideoPrefetch
options.Does not currently have
disableAudioPrefetch
anddisableTextPrefetch
.