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.

Sign up for GitHub

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

Jump to bottom

fix: Defer closeSegmentIndex() for old streams during ABR switches when segment fetches are ongoing #7157

Merged
merged 3 commits into from
Aug 17, 2024
Merged

fix: Defer closeSegmentIndex() for old streams during ABR switches when segment fetches are ongoing #7157

merged 3 commits into from
Aug 17, 2024

Conversation

JulianDomingo
Copy link
Contributor

@JulianDomingo JulianDomingo commented Aug 15, 2024 •
edited
Loading

Resolves #7156.

@JulianDomingo JulianDomingo added type: enhancement New feature or request priority: P1 Big impact or workaround impractical; resolve before feature release component: DASH The issue involves the MPEG DASH manifest format platform: Cast Issues affecting Cast devices labels Aug 15, 2024
@JulianDomingo JulianDomingo self-assigned this Aug 15, 2024
avelad
avelad reviewed Aug 15, 2024
View reviewed changes
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.

Why do a config for that?

@shaka-bot
Copy link
Collaborator

shaka-bot commented Aug 15, 2024 •
edited
Loading

Incremental code coverage: 95.92%

joeyparrish
joeyparrish requested changes Aug 15, 2024
View reviewed changes
Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

Looks great otherwise.

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
@JulianDomingo JulianDomingo changed the title feat: Add option to defer closeSegmentIndex() for old streams during ABR switches fix: Defer closeSegmentIndex() for old streams during ABR switches when segment fetches are ongoing Aug 16, 2024
@JulianDomingo JulianDomingo force-pushed the defer-old-stream-clean-on-abr-switch branch from c45be71 to 50ab376 Compare August 16, 2024 10:31
joeyparrish
joeyparrish approved these changes Aug 16, 2024
View reviewed changes
test/media/streaming_engine_unit.js
@@ -1188,7 +1244,6 @@ describe('StreamingEngine', () => {
netEngine.expectRequest('text-20-0.mp4', segmentType, segmentContext);
netEngine.expectNoRequest('text-20-init', segmentType, segmentContext);
netEngine.expectNoRequest('text-21-init', segmentType, segmentContext);
// TODO: huh?
Copy link
Member

Choose a reason for hiding this comment

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

🤣

@JulianDomingo JulianDomingo marked this pull request as ready for review August 16, 2024 23:28
@JulianDomingo JulianDomingo merged commit 4cff18d into shaka-project:main Aug 17, 2024
23 of 29 checks passed
@shaka-bot shaka-bot mentioned this pull request Aug 17, 2024
Merged
avelad pushed a commit that referenced this pull request Aug 19, 2024
@JulianDomingo @avelad
fix: Defer closeSegmentIndex() for old streams during ABR switches …
403574f 
…when segment fetches are ongoing (#7157)

Resolves #7156.
@shaka-bot shaka-bot mentioned this pull request Aug 19, 2024
Merged
avelad pushed a commit that referenced this pull request Aug 19, 2024
@JulianDomingo @avelad
fix: Defer closeSegmentIndex() for old streams during ABR switches …
cd76d3f 
…when segment fetches are ongoing (#7157)

Resolves #7156.
@shaka-bot shaka-bot mentioned this pull request Aug 19, 2024
Merged
@avelad avelad mentioned this pull request Aug 26, 2024
Closed
@joeyparrish
Copy link
Member

A regression was reported in video-dev.org Slack, and they narrowed it down to this PR. Playing https://d24rwxnt7vw9qb.cloudfront.net/v1/dash/e6d234965645b411ad572802b6c9d5a10799c9c1/All_Reference_Streams/4577dca5f8a44756875ab5cc913cd1f1/index.mpd with this PR, reportedly:

playback stops rendering with a warning "cannot find segment endTime" just a few seconds prior to the rendering alt.

This was initially observed on Samsung TVs, but then later on LG and even in Chrome.

I'm reverting this PR and reopening the related issue.

@joeyparrish joeyparrish mentioned this pull request Aug 26, 2024
Merged
joeyparrish added a commit that referenced this pull request Aug 26, 2024
@joeyparrish
fix: Revert change that caused stalls with "cannot find endTime" (#7213)
2d2bddd 
Reverts #7157 ("fix: Defer
`closeSegmentIndex()` for old streams during ABR switches when segment
fetches are ongoing")

A regression was reported in video-dev.org Slack, and they narrowed it
down to this PR. Playing
https://d24rwxnt7vw9qb.cloudfront.net/v1/dash/e6d234965645b411ad572802b6c9d5a10799c9c1/All_Reference_Streams/4577dca5f8a44756875ab5cc913cd1f1/index.mpd
with this PR, reportedly:

> playback stops rendering with a warning "cannot find segment endTime"
just a few seconds prior to the rendering alt.

This was initially observed on Samsung TVs, but then later on LG and
even in Chrome.
joeyparrish added a commit that referenced this pull request Aug 27, 2024
@joeyparrish
fix: Revert change that caused stalls with "cannot find endTime" (#7213)
c690e4e 
Reverts #7157 ("fix: Defer
`closeSegmentIndex()` for old streams during ABR switches when segment
fetches are ongoing")

A regression was reported in video-dev.org Slack, and they narrowed it
down to this PR. Playing
https://d24rwxnt7vw9qb.cloudfront.net/v1/dash/e6d234965645b411ad572802b6c9d5a10799c9c1/All_Reference_Streams/4577dca5f8a44756875ab5cc913cd1f1/index.mpd
with this PR, reportedly:

> playback stops rendering with a warning "cannot find segment endTime"
just a few seconds prior to the rendering alt.

This was initially observed on Samsung TVs, but then later on LG and
even in Chrome.
joeyparrish added a commit that referenced this pull request Aug 27, 2024
@joeyparrish
fix: Revert change that caused stalls with "cannot find endTime" (#7213)
e69ade2 
Reverts #7157 ("fix: Defer
`closeSegmentIndex()` for old streams during ABR switches when segment
fetches are ongoing")

A regression was reported in video-dev.org Slack, and they narrowed it
down to this PR. Playing
https://d24rwxnt7vw9qb.cloudfront.net/v1/dash/e6d234965645b411ad572802b6c9d5a10799c9c1/All_Reference_Streams/4577dca5f8a44756875ab5cc913cd1f1/index.mpd
with this PR, reportedly:

> playback stops rendering with a warning "cannot find segment endTime"
just a few seconds prior to the rendering alt.

This was initially observed on Samsung TVs, but then later on LG and
even in Chrome.
JulianDomingo pushed a commit to JulianDomingo/shaka-player that referenced this pull request Aug 28, 2024
@joeyparrish @JulianDomingo
fix: Revert change that caused stalls with "cannot find endTime" (sha…
c32bb0b 
…ka-project#7213)

Reverts shaka-project#7157 ("fix: Defer
`closeSegmentIndex()` for old streams during ABR switches when segment
fetches are ongoing")

A regression was reported in video-dev.org Slack, and they narrowed it
down to this PR. Playing
https://d24rwxnt7vw9qb.cloudfront.net/v1/dash/e6d234965645b411ad572802b6c9d5a10799c9c1/All_Reference_Streams/4577dca5f8a44756875ab5cc913cd1f1/index.mpd
with this PR, reportedly:

> playback stops rendering with a warning "cannot find segment endTime"
just a few seconds prior to the rendering alt.

This was initially observed on Samsung TVs, but then later on LG and
even in Chrome.
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Oct 16, 2024
@shaka-project shaka-project locked as resolved and limited conversation to collaborators Oct 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@avelad avelad avelad left review comments

@joeyparrish joeyparrish joeyparrish approved these changes

Assignees

@JulianDomingo JulianDomingo

Labels
component: DASH The issue involves the MPEG DASH manifest format platform: Cast Issues affecting Cast devices priority: P1 Big impact or workaround impractical; resolve before feature release status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Milestone
No milestone
4 participants
@JulianDomingo @shaka-bot @joeyparrish @avelad