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(DASH): segments being fetched out of the range of the timeline #5889

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

dave-nicholas
Copy link
Contributor

fixes: #3952

There was an issue that would occur that caused segments to be fetched that are out of the range of the manifest.

@dave-nicholas dave-nicholas changed the title fix: segments being fetched out of the range of the time fix: segments being fetched out of the range of the timeline Nov 15, 2023
@avelad avelad changed the title fix: segments being fetched out of the range of the timeline fix(DASH): segments being fetched out of the range of the timeline Nov 15, 2023
@avelad avelad added type: bug Something isn't working correctly component: DASH The issue involves the MPEG DASH manifest format priority: P1 Big impact or workaround impractical; resolve before feature release labels Nov 15, 2023
@avelad avelad added this to the v4.6 milestone Nov 15, 2023
Copy link
Member

@avelad avelad left a comment

Choose a reason for hiding this comment

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

@littlespex For me the change is fine, can you check if it does not introduce any regression in your streams? Thanks!

@shaka-bot
Copy link
Collaborator

Incremental code coverage: 100.00%

@littlespex
Copy link
Contributor

I've tested a few streams and do not see any issues with this PR. I did notice that we lost captions on some of our streams, but this issue occurs in main as well. Unfortunately I don't have a lot of time to investigate right now.

@nrcrast
Copy link
Contributor

nrcrast commented Nov 15, 2023

@avelad @littlespex For some more info on this fix (I'm on a team with @dave-nicholas :) )

Using numEvicted is not ideal for the segmentReplacement calculation, as that value is not reset or consistent when you close and reopen segment indexes. If you let the stream play on one variant with ABR disabled, it will work fine. It's when variant switches come into the picture that numEvicted becomes problematic. You can reproduce fairly easily by turning ABR off, playing a linear stream for a couple minutes, and then switching to a different variant and immediately switching back.

Furthermore, architecturally numEvicted has one single purpose, and that's to keep consistent positioning while you're playing a stream. It's not there to be used for absolute URI calculations or to be consistent between closing and re-opening of a Segment Index. This dual purpose of using numEvicted to calculate the Segment Replacement is both architecturally confusing and causes an actual behavioral bug.

I think, architecturally, it makes sense that all info required to build the Segment URI should be contained within the TemplateInfo structure. Mixing in a variable internal to the Segment Index just makes it all the more confusing.

@avelad avelad merged commit d8aa24f into shaka-project:main Nov 16, 2023
23 of 24 checks passed
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Jan 15, 2024
@shaka-project shaka-project locked as resolved and limited conversation to collaborators Jan 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component: DASH The issue involves the MPEG DASH manifest format priority: P1 Big impact or workaround impractical; resolve before feature release status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loading future DASH segments in v4.x
5 participants