-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Update redirect detection in dash parser #2763
Conversation
// request, it doesn't contaminate future requests. | ||
request.method = request.method || 'GET'; | ||
request.headers = request.headers || {}; | ||
request.retryParameters = request.retryParameters ? | ||
ObjectUtils.cloneObject(request.retryParameters) : | ||
shaka.net.NetworkingEngine.defaultRetryParameters(); | ||
request.uris = ObjectUtils.cloneObject(request.uris); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need the final original URI of the request in the maifestUris_ to replace it with the redirected one. I think these changes are safe as with the current approach, every time the manifest URI changes (for example in request filters) it will be added to the manifestUris_ array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think app-level filters should be able to modify a private array inside the library. That could break all kinds of expectations. And it looks like you don't even need this, since you have the dash parser updating the array when it gets a response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that we need to have the exact request URL in the manifest URI array in order to be able to find the index of it and replace this URI with the new one.
In case when the request URI was changed in request filters, the manifestUris_ array will not include the exact uri of the request, and this.manifestUris.indexOf(response.originalUri) will be -1 and we will not be able to understand what url needs to be replaced.
For example scenario:
1.this.manifestUris_ = ['url']
2. in request filter we change the uris[0] it to 'url-after-filter'
3. this url redirected with 302 code to 'url-after-redirect'
4. so without array modification, we will receive the next after awaiting request
this.manifestUris_ = ['url']
response.uri = 'url-after-redirect'
response.originalUri = 'url-after-filter'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good otherwise. Sorry for the massive delay in review!
// request, it doesn't contaminate future requests. | ||
request.method = request.method || 'GET'; | ||
request.headers = request.headers || {}; | ||
request.retryParameters = request.retryParameters ? | ||
ObjectUtils.cloneObject(request.retryParameters) : | ||
shaka.net.NetworkingEngine.defaultRetryParameters(); | ||
request.uris = ObjectUtils.cloneObject(request.uris); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think app-level filters should be able to modify a private array inside the library. That could break all kinds of expectations. And it looks like you don't even need this, since you have the dash parser updating the array when it gets a response.
@glhvta Can you review Joey's comments and rebase? Thanks! |
Hi @avelad! It looks like this PR was created quite a long time ago and the current shaka player code has changed since then. I'll try to check this week if the changes introduced in this PR are still needed and reply with the updates. Thanks |
Closing due to inactivity. If you need to reopen this issue, just put @shaka-bot reopen in a comment. Thanks! |
Fixes false redirect detection in Dash parser.
Problem: with the current approach each time the manifest URL has changed it will be added to the list of manifestUris_. In the scenarios when the application changes the manifest URL in request filters manifestUris_ will contain all the previous URL too.
This behavior may not affect the playback, but the retry mechanism will be not working correctly. When the request for the manifest will be failed, the manifest with the previous URL will be requested during the retry flow.
According to the DASH IF IOP 4.3 chapter 3.2.15.3. Client Requirements and Recommendations:
https://dashif.org/docs/DASH-IF-IOP-v4.3.pdf
So new manifest should not be 'added' to the list of manifestUris_, but replace the redirected URL.
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. The problem is described in #2216.
Note: Application that uses custom HTTP plugin should return expected response - <shaka.extern.Response>
https://shaka-player-demo.appspot.com/docs/api/shaka.extern.html#.Response
And return originalUri in order to have a sticky redirect..
These changes are needed in order to have correct detection for the redirect.
Closes #2216
Thanks.