Don't wait for requests to finish when encountering an error in media… #286
Conversation
| * For instance if a timeout and two aborts occur, then the aborts were | ||
| * likely triggered by the timeout so return that error object. | ||
| */ | ||
| const getMostImportantError = (errors) => { |
There was a problem hiding this comment.
- Does this change mean that we might not get the most "pertinent" error on the player?
- Or does it actually end up returning the most relevant error by bubbling up the first error?
- Is there a case where we would want other requests in the group to be made even if one failed?
There was a problem hiding this comment.
Technically, before this change I don't think it was possible for there to be more than just aborted/the original error. So this should have no change.
| // never get an aborted event. For instance, if the network connection is lost and | ||
| // there were two requests, the first may have triggered an error immediately, while | ||
| // the second request remains unsent. In that case, the aborted algorithm will not | ||
| // trigger an abort: see https://xhr.spec.whatwg.org/#the-abort()-method |
| // trigger an abort: see https://xhr.spec.whatwg.org/#the-abort()-method | ||
| // | ||
| // We also can't rely on the ready state of the XHR, since the request that | ||
| // triggered the connection error may also show as a ready state of 0 (unsent). |
| // the request will never "finish." In order to mimic this behavior, override the | ||
| // default abort function so that it doesn't finish. | ||
| keyReq.abort = () => { | ||
| abortedKeyReq = true; |
There was a problem hiding this comment.
might want to rename abortedKeyReq so it's clear that we're overriding default behavior
There was a problem hiding this comment.
Alternatively, can we mimic this scenario by setting the XHR state to unsent?
There was a problem hiding this comment.
I just tried that, and we can, the only thing I was wary of after I wrote it up is I still wanted to have a way to verify that we called abort on the key request, otherwise, since we mock other aspects of the XHR, I'd worry that the state would always be in unsent for some reason and we didn't actually call abort properly. Though maybe that would cause problems elsewhere. What do you think?
There was a problem hiding this comment.
I think this makes sense for the purpose of verifying we called abort. Can we also verify that the XHR state changed in the test or does the mocking make that difficult/not useful?
There was a problem hiding this comment.
The XHR state for the key request optimally shouldn't actually change if we want to reflect the browser behavior (that was the cause of the bug, that it remains in its original, unsent, state), but with the mocking and setup, the state does change (why we override the abort in this case).
…-segment-request
Description
When playing content where two requests may be made for a segment (i.e., there's a map tag for a segment, so both the segment and init segment are requested), if there's a request error on one of the requests before the other has started, then it's possible media-segment-request will be stuck forever, and never finish its group of requests.
To see the behavior:
Go to https://videojs.github.io/http-streaming/
Enter the source as: https://dash.akamaized.net/akamai/bbb_30fps/bbb_30fps.mpd and the mime type as: application/dash+xml
Click play
Open the console and go to the network tab
Click "Offline"
Wait until the network tab starts showing only audio segment requests (m4a)
Uncheck "Offline"
Only the m4a requests will continue loading. Content may continue playback with audio only, and a stuck video frame.
The fix makes it so that both audio segment (m4a) requests, and init and video segment (m4v) requests are continued, and content resumes as expected.
Requirements Checklist