Make SegmentLoader great again #1044

imbcmdth · 2017-03-08T00:20:03Z

Description

Segment XHR-related state is strewn across large parts of SegmentLoader. The actual problem of fetching a segment and one or two other requests that the segment might depend on is fairly self-contained. This PR aims to refactor the xhr code almost completely out of SegmentLoader so that it can focus on deciding what to get, not how to get them.

Specific Changes proposed

All code related to xhr is completely removed from SegmentLoader into mediaSegmentRequest - now the only thing it has to do is handle a nicely packaged response that has either failed or is ready for transmuxing
All decryption code has also moved to mediaSegmentRequest including the callback management that was in MasterPlaylistController
- The only thing MPC does with the decrypter is life-cycle management
Added better tracking for mediaRequests* stats
- mediaRequests now properly tracks the count of ALL media requests even failed or aborted requests
- mediaRequestsErrored has the number of requests that resulted in a failure (40x, 50x, etc.)
- mediaRequestsTimedout has the number of requests that timed-out
- mediaRequestsAborted has the number of requests explicitly aborted by the SegmentLoader
Added mediaSegmentRequest unit-tests

Requirements Checklist

Feature implemented / Bug fixed
If necessary, more likely in a feature request than a bug fix
- Unit Tests updated or fixed
Reviewed by Two Core Contributors

mjneil · 2017-03-08T22:57:46Z

src/media-segment-request.js

+
+  if (progressEvent.lengthComputable) {
+    stats.bytesReceived = progressEvent.loaded;
+    stats.bandwidth = (stats.bytesReceived / roundTripTime) * 8 * 1000;


I think this should use stats.roundTripTime or use Math.max when initializing roundTripTime to avoid division by 0 (I assume the use of Math.max above was to not report a round trip of 0)

mjneil · 2017-03-08T23:34:03Z

src/media-segment-request.js

+    if (error) {
+      // If there are errors, we have to abort any outstanding requests
+      abortAll(activeXhrs);
+      errors.unshift(error);


Wouldn't unshifting cause the original error that caused all the aborts to be at the end of the array? You abort all the other requests, unshift the error, and then when the aborted requests get handled, they would have an error here and unshift themselves in front of the original?

You might be right. It looks like I was seeing sinon's mock xhr results (which are synchronous) and was confusing it with actual behavior.

mjneil · 2017-03-08T23:37:17Z

src/media-segment-request.js

+    const xhr = activeXhrs[xhrKey];
+
+    // only abort xhrs that haven't had a response
+    if (!xhr.responseTime) {


Does it hurt to call abort on a request that has already been aborted? If so should probably add a check for that here as well. If one of the requests causes an error, the rest of the requests are aborted which in turn would cause an error on the aborted request, which would loop through all the requests again, even the aborted one

I haven't seen any issues. Aborting an already aborted xhr seems to be a no-op and doesn't trigger any sort of readystatechange.

mjneil · 2017-03-08T23:54:37Z

src/media-segment-request.js

+
+  segment.map.bytes = new Uint8Array(request.response);
+
+  if (!Array.isArray(segment.bandwidth)) {


This bandwidth array isn't used anywhere. What is it for?

mjneil · 2017-03-09T00:00:39Z

src/media-segment-request.js

+ * Handle all error conditions in one place and return an object
+ * with all the information
+ *
+ * @param {Anything} error - if non-null signals an error occured with the XHR


I think {Object|null} or {Error|null} would be acceptable here

mjneil · 2017-03-09T00:02:16Z

src/media-segment-request.js

    }
  };
 };

-const handleProgress = (segment, callback) => (event) => {
+/**
+ * Simple progress event callback handler that gathers some states before


gathers some ~~states~~ stats

mjneil · 2017-03-09T20:16:47Z

src/segment-loader.js

+    // the request was aborted and the SegmentLoader has already started
+    // another request. this can happen when the timeout for an aborted
+    // request triggers due to a limitation in the XHR library
+    // do not count this as any sort of request or we risk double-counting


This is a bit confusing. Can you explain the limitation? (Not requesting you add more detail to the actual code comment)

mjneil · 2017-03-09T20:57:07Z

I'm not sure what detail I'm missing here since Travis obviously seems to think the tests should pass, but I notice that all the mediaSegmentRequest unit tests have assert.expect(n) but only have n - 2 asserts, which would lead me to believe that the tests should fail.

EDIT: Ran the tests locally and see that is because of no unexpected logs at level "warn" and no unexpected logs at level "error": assertions run on every test

mjneil · 2017-03-09T21:03:53Z

I think it would be good to have a test that makes sure doneFn isn't called until segment, key, and init segment requests have responded

mjneil · 2017-03-09T23:31:31Z

test/segment-loader.test.js

@@ -249,7 +249,6 @@ QUnit.test('calculates bandwidth after downloading a segment', function(assert)
  // verify stats
  assert.equal(loader.mediaBytesTransferred, 10, '10 bytes');
  assert.equal(loader.mediaTransferDuration, 100, '100 ms (clock above)');
-  assert.equal(loader.mediaRequests, 1, '1 request');


why remove this?

Because we have this same test dozens of times and it's very redundant. I actually tried to remove more of them but lost hope.

mjneil · 2017-03-14T14:07:17Z

src/segment-loader.js

@@ -669,6 +644,7 @@ export default class SegmentLoader extends videojs.EventTarget {
    let segment = playlist.segments[mediaIndex];

    return {
+      requestId: 'segment-loader-' + Math.random(),


I think it would be better to do Math.random().toString(36) like you do with decryptionId in mediaSegmentRequest.

mjneil · 2017-03-14T14:08:38Z

src/media-segment-request.js

+ * @param {Function} doneFn - a callback that is executed after decryption has completed
+ */
+const decryptSegment = (decrypter, segment, doneFn) => {
+  const decryptionId = 'segment-request-' + Math.random().toString(36);


Why not use segment.requestId here instead of generating another random string?

mjneil

LGTM

…gmentRequest function

…king for aborted, timeout, and errored requets

…al meaning of the event

…g convention in the hls spec

…or mediaRequests* stats tracking

…vent asynchronously to updating the updating property on a SourceBuffer

mjneil

LGTM PART 2

imbcmdth force-pushed the cleanup-segment-xhr branch from 110875b to 8da13c6 Compare March 8, 2017 00:23

imbcmdth added enhancement needs:testing labels Mar 8, 2017

mjneil reviewed Mar 8, 2017

View reviewed changes

mjneil reviewed Mar 9, 2017

View reviewed changes

imbcmdth force-pushed the cleanup-segment-xhr branch 2 times, most recently from bee0e37 to ed2de70 Compare March 9, 2017 01:40

mjneil self-assigned this Mar 9, 2017

mjneil reviewed Mar 9, 2017

View reviewed changes

imbcmdth force-pushed the cleanup-segment-xhr branch 2 times, most recently from a14dbfd to e356bb9 Compare March 14, 2017 01:14

mjneil reviewed Mar 14, 2017

View reviewed changes

mjneil approved these changes Mar 14, 2017

View reviewed changes

imbcmdth removed the needs:testing label Mar 14, 2017

jrivera added 8 commits March 15, 2017 13:35

Refactor xhr handling code and state out of SegmentLoader into the Se…

f026a11

…gmentRequest function

Fixed breaking tests with minimal changes and added better stats trac…

ca9f360

…king for aborted, timeout, and errored requets

Move the segmentRequest done callback out of the loadSegment function

29868e6

Rename processingcomplete to bandwidthupdate to better reflect the re…

8ed401d

…al meaning of the event

Renamed segmentRequest to mediaSegmentRequest to align with the namin…

9a287f5

…g convention in the hls spec

Made control flow order more explicit

026e78c

Added a few more comments to each function explaining the parameters

14258e2

Fixed some small issues

d80b9fc

jrivera added 3 commits March 15, 2017 13:36

mediaSegmentRequest now only returns a single error and added tests f…

3dbbbea

…or mediaRequests* stats tracking

Response is an empty arraybuffer on aborted requests in Firefox

66938a3

Use the same work-tracking value for decryption and segment request

a79b6a7

imbcmdth force-pushed the cleanup-segment-xhr branch from ce94669 to a79b6a7 Compare March 15, 2017 17:37

Fixed a bug that resulted from Firefox emitting the handleUpdateEnd e…

470cf4b

…vent asynchronously to updating the updating property on a SourceBuffer

imbcmdth force-pushed the cleanup-segment-xhr branch from 68c3073 to 470cf4b Compare March 15, 2017 21:57

mjneil approved these changes Mar 15, 2017

View reviewed changes

imbcmdth merged commit cd7cd9b into master Mar 15, 2017

imbcmdth deleted the cleanup-segment-xhr branch March 15, 2017 22:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make SegmentLoader great again #1044

Make SegmentLoader great again #1044

imbcmdth commented Mar 8, 2017 •

edited

Loading

mjneil Mar 8, 2017

imbcmdth Mar 9, 2017

mjneil Mar 8, 2017

imbcmdth Mar 9, 2017

mjneil Mar 8, 2017

imbcmdth Mar 9, 2017

mjneil Mar 8, 2017

imbcmdth Mar 8, 2017

mjneil Mar 9, 2017

imbcmdth Mar 9, 2017

mjneil Mar 9, 2017

imbcmdth Mar 9, 2017

mjneil Mar 9, 2017

mjneil commented Mar 9, 2017 •

edited

Loading

mjneil commented Mar 9, 2017

mjneil Mar 9, 2017

imbcmdth Mar 10, 2017

mjneil Mar 14, 2017

mjneil Mar 14, 2017

mjneil left a comment

mjneil left a comment


		segment.map.bytes = new Uint8Array(request.response);

		if (!Array.isArray(segment.bandwidth)) {

Make SegmentLoader great again #1044

Make SegmentLoader great again #1044

Conversation

imbcmdth commented Mar 8, 2017 • edited Loading

Description

Specific Changes proposed

Requirements Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjneil commented Mar 9, 2017 • edited Loading

mjneil commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjneil left a comment

Choose a reason for hiding this comment

mjneil left a comment

Choose a reason for hiding this comment

imbcmdth commented Mar 8, 2017 •

edited

Loading

mjneil commented Mar 9, 2017 •

edited

Loading