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

False redirect detection in dash parser #2216

Closed
pcruiksh opened this issue Oct 25, 2019 · 1 comment · Fixed by #5910
Closed

False redirect detection in dash parser #2216

pcruiksh opened this issue Oct 25, 2019 · 1 comment · Fixed by #5910
Labels
component: DASH The issue involves the MPEG DASH manifest format flag: good first issue This might be a relatively easy issue; good for new contributors priority: P2 Smaller impact or easy workaround status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Milestone

Comments

@pcruiksh
Copy link
Contributor

pcruiksh commented Oct 25, 2019

Have you read the FAQ and checked for duplicate open issues?
Yes

What version of Shaka Player are you using?
2.5.5

Can you reproduce the issue with our latest release version?
Yes

Can you reproduce the issue with the latest code from master?
Yes

Are you using the demo app or your own custom app?
Custom

If custom app, can you reproduce the issue using our demo app?
Yes

What browser and OS are you using?
Chrome/MacOS Catalina

For embedded devices (smart TVs, etc.), what model and firmware version are you using?

What are the manifest and license server URIs?
N/A

What did you do?
Play content with an asset URI supplied by our content auth backend.

What did you expect to happen?
Primary manifest request URL does not change if it is not an actual redirect. The networking engine observes the proper configured number of retries for a manifest request.

What actually happened?
This code in dash_parser does not account for case insensitivity of the hostname in the manifest URL, and can cause false redirect detection (since the host in response.uri will be lowercase):

// For redirections add the response uri to the first entry in the
// Manifest Uris array.
if (response.uri && !this.manifestUris_.includes(response.uri)) {
   this.manifestUris_.unshift(response.uri);
}

Therefore, a request for [Manifest.com/manifest.mpd] becomes [manifest.com/manifest.mpd, Manifest.com/manifest.mpd] on subsequent updates.

This does not necessarily have consequences for playback but may result in extra unexpected networking retries as the networking engine is now given an array of multiple urls, and furthermore, application layer networking filters or proxies that depend on url detection may behave unexpectedly or fail.

@TheModMaker TheModMaker added flag: good first issue This might be a relatively easy issue; good for new contributors type: enhancement New feature or request and removed needs triage labels Oct 25, 2019
@TheModMaker TheModMaker added this to the Backlog milestone Oct 25, 2019
@joeyparrish joeyparrish modified the milestones: Backlog, Backlog 2 Jan 28, 2020
@glhvta
Copy link
Contributor

glhvta commented Jul 30, 2020

Hello!

We faced the same issue. We are trying to play a stream that includes tokens that can change during the playback. In request preprocessors we inject this token into the manifest URL, so each time the manifest URL has changed the new URL will be included to the manifestUris_ array. This behavior will not affect the playback, but the retry mechanism will be not working correctly.

So if the request for the manifest will be failed, the manifest with the previous token will be requested during the retry flow. The token can already be not valid, so the retry mechanism will not work correctly.

This will also fix the behavior when manifest with the original URI will be requested in the retry flow, when the manifest request would be failed.

Using Charles the 302 response was emulated on the demo page for dynamic manifest and then error for the manifest URL. Current approach: the original URI is requested in the retry flow. It is not correct because it would definitely cause redirect one more time:

image

Expected approach: redirected URL is used in the retry flow.

image

PR that fixes the problem: #2763

Best,
Tatsiana

@joeyparrish joeyparrish added priority: P2 Smaller impact or easy workaround type: bug Something isn't working correctly and removed type: enhancement New feature or request labels Sep 29, 2021
@joeyparrish joeyparrish modified the milestones: Backlog, v3.3 Oct 4, 2021
@avelad avelad added the component: DASH The issue involves the MPEG DASH manifest format label May 2, 2022
@avelad avelad modified the milestones: v3.3, v4.1 May 4, 2022
@avelad avelad modified the milestones: v4.1, v4.2 Jun 3, 2022
@avelad avelad modified the milestones: v4.2, v4.3 Aug 17, 2022
@avelad avelad modified the milestones: v4.3, v4.4 Nov 11, 2022
@avelad avelad modified the milestones: v4.4, v4.5 Aug 31, 2023
@avelad avelad modified the milestones: v4.5, v4.6 Oct 5, 2023
@avelad avelad modified the milestones: v4.6, v5.0 Nov 16, 2023
avelad added a commit that referenced this issue Nov 21, 2023
avelad added a commit that referenced this issue Nov 22, 2023
Robloche pushed a commit to Robloche/shaka-player that referenced this issue Nov 30, 2023
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Jan 20, 2024
@shaka-project shaka-project locked as resolved and limited conversation to collaborators Jan 20, 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 flag: good first issue This might be a relatively easy issue; good for new contributors priority: P2 Smaller impact or easy workaround status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
6 participants