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

Don't wait for requests to finish when encountering an error in media… #286

Merged
merged 1 commit into from Nov 8, 2018

Conversation

Projects
None yet
2 participants
@gesinger
Copy link
Contributor

gesinger commented Nov 7, 2018

…-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

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors
* 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) => {

This comment has been minimized.

@ldayananda

ldayananda Nov 7, 2018

Contributor
  • 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?

This comment has been minimized.

@gesinger

gesinger Nov 7, 2018

Contributor

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

This comment has been minimized.

@ldayananda

ldayananda Nov 7, 2018

Contributor

😬

// 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).

This comment has been minimized.

@ldayananda

ldayananda Nov 7, 2018

Contributor

😬

// 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;

This comment has been minimized.

@ldayananda

ldayananda Nov 7, 2018

Contributor

might want to rename abortedKeyReq so it's clear that we're overriding default behavior

This comment has been minimized.

@ldayananda

ldayananda Nov 7, 2018

Contributor

Alternatively, can we mimic this scenario by setting the XHR state to unsent?

This comment has been minimized.

@gesinger

gesinger Nov 8, 2018

Contributor

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?

This comment has been minimized.

@ldayananda

ldayananda Nov 8, 2018

Contributor

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?

This comment has been minimized.

@gesinger

gesinger Nov 8, 2018

Contributor

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).

@ldayananda
Copy link
Contributor

ldayananda left a comment

lgtm and tested

@gesinger gesinger merged commit 970e3ce into videojs:master Nov 8, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/videojs-http-streaming/deploy-preview Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment