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

feat: Temporarily disable the active variant from NETWORK HTTP_ERROR #4189

Merged
merged 10 commits into from
May 5, 2022
Merged

feat: Temporarily disable the active variant from NETWORK HTTP_ERROR #4189

merged 10 commits into from
May 5, 2022

Conversation

albertdaurell
Copy link
Contributor

@albertdaurell albertdaurell commented May 3, 2022 •
edited by joeyparrish
Loading

Description

When a NETWORK HTTP_ERROR is thrown we'll temporary disable the problematic variant during shaka.extern.Restrictions.maxDisabledTime seconds ( default will be 30 seconds )

Relates

#4121

BEGIN_COMMIT_OVERRIDE
feat: Temporarily disable the active variant from NETWORK HTTP_ERROR (#4189)

When a NETWORK HTTP_ERROR is thrown, we'll temporarily disable the variant for shaka.extern.Restrictions.maxDisabledTime seconds (default will be 30 seconds).

Closes #4121
Closes #1542
Closes #2541
END_COMMIT_OVERRIDE

@google-cla
Copy link

google-cla bot commented May 3, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@albertdaurell
Copy link
Contributor Author

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

CLA signed

@avelad
Copy link
Member

avelad commented May 3, 2022

Please execute ./build/all.py, I suspect that the code doesn't compile.

@albertdaurell
Copy link
Contributor Author

./build/all.py

@avelad I did it and it worked ok

@avelad avelad added the type: enhancement New feature or request label May 3, 2022
avelad
avelad requested changes May 3, 2022
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.

In general, I see it quite well.

Missed adding test to cover the new functionality that is being added.

Thanks for working on this feature.

externs/shaka/manifest.js Outdated Show resolved Hide resolved
externs/shaka/player.js Outdated Show resolved Hide resolved
lib/player.js Show resolved Hide resolved
externs/shaka/player.js Outdated Show resolved Hide resolved
theodab
theodab requested changes May 3, 2022
View reviewed changes
lib/player.js Outdated Show resolved Hide resolved
externs/shaka/manifest.js Outdated Show resolved Hide resolved
externs/shaka/player.js Outdated Show resolved Hide resolved
@albertdaurell albertdaurell force-pushed the feature/temporarily-disable-variants-on-error branch 2 times, most recently from 0b42552 to fdcc2ae Compare May 3, 2022 21:02
@albertdaurell albertdaurell force-pushed the feature/temporarily-disable-variants-on-error branch from fdcc2ae to 2c9e675 Compare May 3, 2022 21:03
@albertdaurell
Copy link
Contributor Author

@avelad @theodab once you agree with the changes I'll start checking / creating the unit tests.

@albertdaurell albertdaurell requested review from avelad and theodab May 4, 2022 10:40
@avelad
Copy link
Member

avelad commented May 4, 2022

Please check the errors, for example: https://github.com/shaka-project/shaka-player/runs/6288317308?check_suite_focus=true

avelad
avelad requested changes May 4, 2022
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.

LGTM, but I think it would be necessary to add a test with disabledUntilTime != 0

@avelad avelad mentioned this pull request May 4, 2022
Closed
@avelad
Copy link
Member

avelad commented May 4, 2022

Note: related to #2541

@avelad
Copy link
Member

avelad commented May 4, 2022

If we put a value other than 0, it would really be doing what is requested in #2541, @joeyparrish, what do you think?

theodab
theodab requested changes May 4, 2022
View reviewed changes
externs/shaka/manifest.js Outdated Show resolved Hide resolved
externs/shaka/player.js Outdated Show resolved Hide resolved
lib/player.js Outdated Show resolved Hide resolved
lib/player.js Outdated Show resolved Hide resolved
@albertdaurell albertdaurell force-pushed the feature/temporarily-disable-variants-on-error branch from e62b8c7 to b53535f Compare May 4, 2022 20:32
theodab
theodab previously approved these changes May 4, 2022
View reviewed changes
@albertdaurell albertdaurell requested a review from avelad May 4, 2022 21:50
@albertdaurell albertdaurell force-pushed the feature/temporarily-disable-variants-on-error branch from ab17dfe to 3acab8e Compare May 4, 2022 21:59
@albertdaurell albertdaurell requested a review from theodab May 4, 2022 22:00
avelad
avelad requested changes May 5, 2022
View reviewed changes
externs/shaka/manifest.js Outdated Show resolved Hide resolved
@albertdaurell albertdaurell requested a review from avelad May 5, 2022 09:59
@joeyparrish joeyparrish merged commit b57279d into shaka-project:main May 5, 2022
@github-actions github-actions bot mentioned this pull request May 5, 2022
Closed
@avelad avelad added this to the v4.1 milestone May 5, 2022
@albertdaurell albertdaurell deleted the feature/temporarily-disable-variants-on-error branch May 5, 2022 17:25
@joeyparrish joeyparrish mentioned this pull request May 10, 2022
Closed
@github-actions github-actions bot mentioned this pull request May 17, 2022
Merged
@joeyparrish joeyparrish mentioned this pull request Aug 18, 2022
Closed
joeyparrish added a commit to joeyparrish/shaka-player that referenced this pull request Aug 18, 2022
@joeyparrish
fix: Fix vanishing tracks while offline
4a43ea4 
Introduced in shaka-project#4189, as a side-effect of restricting tracks when a
network failure occurs.  We should not trigger such restrictions when
the browser is known to be offline.

Closes shaka-project#4408
@joeyparrish joeyparrish mentioned this pull request Aug 18, 2022
Merged
joeyparrish added a commit that referenced this pull request Aug 18, 2022
@joeyparrish
fix: Fix vanishing tracks while offline (#4426)
c935cc1 
Introduced in #4189, as a side-effect of restricting tracks when a
network failure occurs.  We should not trigger such restrictions when
the browser is known to be offline.

Closes #4408
avelad pushed a commit that referenced this pull request Aug 31, 2022
@joeyparrish
fix: Fix vanishing tracks while offline (#4426)
186d298 
Introduced in #4189, as a side-effect of restricting tracks when a
network failure occurs.  We should not trigger such restrictions when
the browser is known to be offline.

Closes #4408
avelad pushed a commit that referenced this pull request Aug 31, 2022
@joeyparrish
fix: Fix vanishing tracks while offline (#4426)
d668790 
Introduced in #4189, as a side-effect of restricting tracks when a
network failure occurs.  We should not trigger such restrictions when
the browser is known to be offline.

Closes #4408
@friday friday mentioned this pull request Nov 18, 2022
Closed
@friday friday mentioned this pull request Nov 29, 2022
Merged
avelad pushed a commit that referenced this pull request Dec 1, 2022
@friday
feat: Enable variant failover for BAD_HTTP_STATUS and TIMEOUT (#4769)
b46012d 
This enables #4189 for two more error types. As mentioned in #4728 404
status results in `BAD_HTTP_STATUS` rather than `HTTP_ERROR`. I also
think `TIMEOUT` should be included, and another issue was just created
for this (#4764), so I included that as well.

Closes #4728
Closes #4764
This was referenced Mar 6, 2023
Closed
Closed
Merged
avelad pushed a commit that referenced this pull request Apr 18, 2023
@zangue
fix: Fix temporarily disable streams on network error (#5057)
fdc5cb1 
Relates to #4189
Fixes #5054 
Fixes #5055
Fixes #5150
@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.
Reviewers

@avelad avelad avelad approved these changes

@joeyparrish joeyparrish joeyparrish approved these changes

@theodab theodab Awaiting requested review from theodab

Assignees
No one assigned
Labels
status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Milestone
v4.1
4 participants
@albertdaurell @avelad @joeyparrish @theodab