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

fix: llhls syncing fixes #1125

Merged
merged 36 commits into from
May 26, 2021
Merged

fix: llhls syncing fixes #1125

merged 36 commits into from
May 26, 2021

Conversation

brandonocasey
Copy link
Contributor

@brandonocasey brandonocasey commented Apr 30, 2021

  • Adds partIndex to getMediaInfoForTime, sync-controller and other Playlist functions.
  • Adds part level timing info and fixes segment level timing info with parts

@codecov
Copy link

codecov bot commented May 7, 2021

Codecov Report

Merging #1125 (ac9de31) into main (1c7a63b) will increase coverage by 0.06%.
The diff coverage is 94.23%.

❗ Current head ac9de31 differs from pull request most recent head 18d06c5. Consider uploading reports for the commit 18d06c5 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1125      +/-   ##
==========================================
+ Coverage   86.20%   86.27%   +0.06%     
==========================================
  Files          39       39              
  Lines        9289     9342      +53     
  Branches     2127     2149      +22     
==========================================
+ Hits         8008     8060      +52     
- Misses       1281     1282       +1     
Impacted Files Coverage Δ
src/playlist.js 94.11% <89.18%> (-0.39%) ⬇️
src/playlist-loader.js 93.06% <92.64%> (+1.43%) ⬆️
src/manifest.js 97.87% <100.00%> (+0.02%) ⬆️
src/segment-loader.js 95.82% <100.00%> (+<0.01%) ⬆️
src/sync-controller.js 96.89% <100.00%> (+0.16%) ⬆️
src/media-groups.js 98.90% <0.00%> (-0.07%) ⬇️
src/videojs-http-streaming.js 90.54% <0.00%> (-0.05%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce03f66...18d06c5. Read the comment docs.

@brandonocasey brandonocasey changed the title WIP: fix: llhls syncing fixes fix: llhls syncing fixes May 7, 2021
* @return {number} the number of seconds between startIndex and endIndex
*/
export const sumDurations = function(playlist, startIndex, endIndex) {
export const sumDurations = function({defaultDuration, durationList, startIndex, endIndex}) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changes to allow passing in a list and a default duration. This allows us to use partsAndSegments, just parts, or just segments


let startIndex;

for (let i = 0; i < partsAndSegments.length; i++) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have to find the partAndSegment index for the specified segment/part

};
}
}

// We are out of possible candidates so load the last one...
return {
mediaIndex: partsAndSegments[partsAndSegments.length - 1].segmentIndex,
segmentIndex: partsAndSegments[partsAndSegments.length - 1].segmentIndex,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed to segmentIndex as that is actually what this is.

src/playlist.js Outdated Show resolved Hide resolved
src/playlist.js Show resolved Hide resolved
src/playlist.js Outdated
}

// skip this if part index does not match.
if (typeof partIndex === 'number' && typeof partAndSegment.partIndex === 'number' && partIndex !== partAndSegment.partIndex) {
Copy link
Member

Choose a reason for hiding this comment

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

question: why would partIndex not be a number? Is it if it isn't present?

Copy link
Member

Choose a reason for hiding this comment

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

I guess below I can see that it is default to null. Maybe we should default it to -1 instead, that way we would only need to check partIndex !== partAndSegment.partIndex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We ran into problems with mediaIndex being negative previously during live playlist updates. I wanted to avoid the whole issue with partIndex. The issue was something like: If we get a playlist update at just the right time we can end up downloading the same segment again because -1 + 1 = 0. Using null we prevent choosing an incorrect segment via incrementing.

src/playlist.js Outdated Show resolved Hide resolved
gkatsev
gkatsev previously approved these changes May 25, 2021
Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

Aside from my nitpick above, LGTM

src/playlist.js Outdated Show resolved Hide resolved
src/playlist.js Outdated Show resolved Hide resolved
src/playlist.js Outdated Show resolved Hide resolved
src/sync-controller.js Show resolved Hide resolved
src/sync-controller.js Outdated Show resolved Hide resolved
src/sync-controller.js Outdated Show resolved Hide resolved
test/playlist.test.js Outdated Show resolved Hide resolved
test/playlist.test.js Outdated Show resolved Hide resolved
test/sync-controller.test.js Outdated Show resolved Hide resolved
test/sync-controller.test.js Outdated Show resolved Hide resolved
brandonocasey and others added 7 commits May 26, 2021 12:30
Co-authored-by: Garrett Singer <[email protected]>
Co-authored-by: Garrett Singer <[email protected]>
Co-authored-by: Garrett Singer <[email protected]>
Co-authored-by: Garrett Singer <[email protected]>
Co-authored-by: Garrett Singer <[email protected]>
Co-authored-by: Garrett Singer <[email protected]>
Base automatically changed from feat/llhls-3 to main May 26, 2021 20:18
gesinger
gesinger previously approved these changes May 26, 2021
src/sync-controller.js Outdated Show resolved Hide resolved
src/sync-controller.js Outdated Show resolved Hide resolved
Co-authored-by: Garrett Singer <[email protected]>
Co-authored-by: Garrett Singer <[email protected]>
@brandonocasey brandonocasey merged commit ee5841d into main May 26, 2021
@brandonocasey brandonocasey deleted the fix/llhls-fixes branch May 26, 2021 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants