Refactor fragment loading to be promise-based & Typescript loaders #2123

johnBartos · 2019-02-09T21:59:45Z

This PR will...

Refactor fragment-loader.js:
- No longer listens or emits events
- load now takes a fragment and returns a promise
  - Resolves replaces the FRAG_LOADED event, and returns onSuccess
  - Reject replaces ERROR , and returns onError, and onTimeout
  - Removes loadprogress (for now)
  - Simplify fragment cancellation check (can check fragCurrent by reference)
Refactor *-stream-controller.js to use new promise based data flow
- Each controller has a local fragment-loader instance
- Calls fragmentLoader.load
- Emits FRAG_LOADING for API compatibility
- No longer listens to FRAG_LOADED
- Removes logic branches handling different fragment loading scenarios (playback, bitrate test, and init segment)
- Fires ERROR events on load error
_loadFragForPlayback:
- Handles loading a frag which will be buffered
- Calls the onFragLoaded function
- Promise resolves to call
- FiresFRAG_LOADED event for API compatibility
_loadBitrateTestFrag
- Handles loading a frag for testing client bandwidth
- Does not (!) emit FRAG_LOADED
_loadInitSegment
- Handles loading the init segment
- Does not (!) emit FRAG_LOADED
Typescript fragment-loader and xhr-loader
Add tests

This is a big one but each task is in its own commit:

Promisfying: 9815b3c
Typescriptifying: 0177836

The only catch: Hls.js now relies on Promises to be present. This is probably API breaking. We can merge this into a 1.0.0 branch if we decide not to ship this in a minor.

Why is this Pull Request needed?

Making fragment loading private and explicit to the controller which requested it allows us to simplify load callbacks. Instead of having each FRAG_LOADED callback check whether it should handle the event (by checking fragment type & controller state), promises allow each controller to handle only the fragment it requested. This allows us to remove a few checks at the top of each callback. Furthermore, we can split out specialized fragment loading scenarios (bitrate test, init segment) which are not demuxed into their own functions by attaching a different .then.

In addition to code cleanliness, this new pattern is the first step in enabling LHLS. LHLS requires progressive playback, which means that we're not going to have the same event flow that we have now; a fragment will be loading, parsing, and buffering at the same time, several times during its download. Our current architecture cannot handle this because it relies on events being fired in a specific sequence to propagate fragment data. By shifting to promises we eliminate the dependency on events for propagating fragment data.

Are there any points in the code the reviewer needs to double check?

Are there any places in which API compatibility has been broken
Any Typescript critiques? This is my first TS PR

Resolves issues:

N/A

Checklist

changes have been done against master branch, and PR does not conflict
new unit / functional tests have been added (whenever applicable)
API or design changes are documented in API.md

Refactor classes which load fragments in order to utilize new promise flow

johnBartos · 2019-02-09T22:21:57Z

@itsjamie

itsjamie · 2019-02-10T00:41:29Z

Weird behaviour with https://deploy-preview-2123--hls-js-pr.netlify.com/demo/?src=https%3A%2F%2Fdemo.unified-streaming.com%2Fvideo%2Ftears-of-steel%2Ftears-of-steel-fmp4.ism%2F.m3u8&demoConfig=eyJlbmFibGVTdHJlYW1pbmciOnRydWUsImF1dG9SZWNvdmVyRXJyb3IiOnRydWUsImR1bXBmTVA0IjpmYWxzZSwibGV2ZWxDYXBwaW5nIjotMSwibGltaXRNZXRyaWNzIjotMX0=

I went to go test it with the promise loading to see if it resolved some of the issues from #2099, and the initSegment loading cycles super fast cancelling itself.

johnBartos · 2019-02-10T02:19:57Z

Yeah that is weird, I’ll take a look tomorrow

…

On Sat, Feb 9, 2019 at 19:41 Jamie Stackhouse ***@***.***> wrote: Weird behaviour with https://deploy-preview-2123--hls-js-pr.netlify.com/demo/?src=https%3A%2F%2Fdemo.unified-streaming.com%2Fvideo%2Ftears-of-steel%2Ftears-of-steel-fmp4.ism%2F.m3u8&demoConfig=eyJlbmFibGVTdHJlYW1pbmciOnRydWUsImF1dG9SZWNvdmVyRXJyb3IiOnRydWUsImR1bXBmTVA0IjpmYWxzZSwibGV2ZWxDYXBwaW5nIjotMSwibGltaXRNZXRyaWNzIjotMX0= I went to go test it with the promise loading to see if it resolved some of the issues, and the initSegment loading cycles super fast cancelling itself. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2123 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEzwZCGT7NZdg0NqMGVL0iBfMXTeaClUks5vL2q8gaJpZM4aywzm> .

itsjamie

👍 on the typescript.

itsjamie · 2019-02-10T21:45:10Z

src/loader/fragment-loader.ts

+    this.config = config;
+  }
+
+  load (frag: Fragment): Promise<FragLoadSuccessResult | FragLoadFailResult> {


There doesn't seem to be a method to define the type for the rejection case of a promise. I'm not sure what the best experience here would be, going to look at other public facing APIs for TypeScript.

What were you're experiences with typing load this way?

I haven't tested this in another TS file, the controllers are still JS. I added this for TS completeness but I can remove it for now if it's too much of a hassle

itsjamie · 2019-02-10T21:47:05Z

src/loader/fragment-loader.ts

+
+    return new Promise((resolve, reject) => {
+      if (!loader) {
+        return Promise.reject(new LoadError(null, 'Loader was destroyed after fragment request'));


Instead of returning a new rejected promise, I think calling the reject callback here, or throwing would be better.

Ah right meant to do that

itsjamie · 2019-02-10T22:11:04Z

src/types/loader.ts

+    aborted: boolean
+}
+
+export interface LoaderCallbacks {


Here is some typing code that I had locally when working on the playlist loader. The only real change is that it makes the Loader interface able to take concrete implementations of the loading context, and that the callbacks are typed because of that.

This will allow us to share the same Loader interface between Frag and Playlist loaders without having a bunch of optional properties. For the Typescript side, it uses the generics part of the language to enable this.

export interface LoaderContext { // target URL url: string // loader response type (arraybuffer or default response type for playlist) responseType: XMLHttpRequestResponseType // start byte range offset rangeStart?: number // end byte range offset rangeEnd?: number // true if onProgress should report partial chunk of loaded content progressData?: boolean } export interface LoaderResponse { url: string, data: string | ArrayBuffer } export interface LoaderConfiguration { // Max number of load retries maxRetry: number // Timeout after which `onTimeOut` callback will be triggered // (if loading is still not finished after that delay) timeout: number // Delay between an I/O error and following connection retry (ms). // This to avoid spamming the server retryDelay: number // max connection retry delay (ms) maxRetryDelay: number } type LoaderOnSuccess < T extends LoaderContext > = ( response: LoaderResponse, stats: LoaderStats, context: T, networkDetails: any ) => void; type LoaderOnProgress < T extends LoaderContext > = ( stats: LoaderStats, context: T, data: string | ArrayBuffer, networkDetails: any, ) => void; type LoaderOnError < T extends LoaderContext > = ( error: { // error status code code: number, // error description text: string, }, context: T, networkDetails: any, ) => void; type LoaderOnTimeout < T extends LoaderContext > = ( stats: LoaderStats, context: T, ) => void; export interface LoaderCallbacks<T extends LoaderContext>{ onSuccess: LoaderOnSuccess<T>, onError: LoaderOnError<T>, onTimeout: LoaderOnTimeout<T>, onProgress?: LoaderOnProgress<T>, } export interface Loader<T extends LoaderContext> { destory(): void abort(): void load( context: LoaderContext, config: LoaderConfiguration, callbacks: LoaderCallbacks<T>, ): void context: T }

Nice I'll add these in

…or compatibility in base-stream-controller

johnBartos · 2019-02-11T04:02:53Z

I changed the base branch from master to 1.0.0 so we don't have to worry about maintaining API compatibility in this PR.

@itsjamie I noticed some weird results running tests locally with Istanbul enabled. Have you noticed that it's not always running all tests? On one run it'll say something like 276 assertions and on the next it'll be 300 something.

itsjamie · 2019-02-12T01:40:00Z

Yeah, I've noticed Istanbul doing weird things reporting different test numbers.

johnBartos · 2019-02-22T17:27:15Z

@itsjamie Addressed feedback & integrated your types from the playlist-loader TS pr

michaelcunningham19

LGTM 👍

✅ All demo page assets work as expected (with the exception of a couple streams which are unavailable)
✅ Can't reproduce the issue spotted by @itsjamie when checking out this branch locally and testing. But I can see it on the deploy preview (updates to this branch might not have been propagated to netlify?)

@johnBartos @itsjamie
I think it would be good to get this in the 1.0.0 branch soon as it's a pre-req for LHLS

michaelcunningham19 · 2019-04-04T12:15:52Z

src/hls.js

@@ -124,7 +124,7 @@ export default class Hls extends Observer {
    const capLevelController = new config.capLevelController(this);
    const fpsController = new config.fpsController(this);
    const playListLoader = new PlaylistLoader(this);
-    const fragmentLoader = new FragmentLoader(this);
+    // const fragmentLoader = new FragmentLoader(this);


Not sure if you wanna remove or keep this @johnBartos ?

johnBartos · 2019-05-08T17:31:05Z

No longer valid, I've made a bunch of changes since opening this PR. Would probably be easier to bundle this with all of the other refactoring work I've done for 1.0.0

John Bartos added 3 commits February 9, 2019 13:36

Refactor fragment-loader to directly return results via promise

9815b3c

Refactor classes which load fragments in order to utilize new promise flow

Typescript fragment loading classes

0177836

Consolidate fragment loader default export into class definition

2e80c6c

johnBartos added the Enhancement label Feb 9, 2019

johnBartos requested review from tjenkinson and michaelcunningham19 February 9, 2019 21:59

itsjamie reviewed Feb 10, 2019

View reviewed changes

johnBartos changed the base branch from master to 1.0.0 February 10, 2019 23:33

John Bartos added 2 commits February 10, 2019 18:54

Fix exceptions; rename tracks[] to levels[] in *-stream-controllers f…

a3f6f0b

…or compatibility in base-stream-controller

Fix unit test errors

082bb85

This was referenced Feb 12, 2019

Fix Istanbul reporting different data. #2125

Closed

Clear audio tick interval when we stopLoad. #2110

Closed

johnBartos mentioned this pull request Feb 13, 2019

hls.js loader memory leak fix #2105

Closed

3 tasks

itsjamie mentioned this pull request Feb 14, 2019

Fragment + Playlist loaders are not always destroyed before freeing #2104

Closed

5 tasks

itsjamie mentioned this pull request Feb 22, 2019

Playlist loader typescript #2144

Merged

1 task

address feedback

c220dde

johnBartos mentioned this pull request Feb 22, 2019

Feature/frag loading refactor jwplayer/hls.js#196

Merged

michaelcunningham19 approved these changes Apr 4, 2019

View reviewed changes

johnBartos closed this May 8, 2019

robwalch deleted the upstream/chore/frag-loading-refactor branch March 15, 2020 03:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor fragment loading to be promise-based & Typescript loaders #2123

Refactor fragment loading to be promise-based & Typescript loaders #2123

johnBartos commented Feb 9, 2019 •

edited

Loading

johnBartos commented Feb 9, 2019

itsjamie commented Feb 10, 2019 •

edited

Loading

johnBartos commented Feb 10, 2019 via email

itsjamie left a comment

itsjamie Feb 10, 2019

johnBartos Feb 11, 2019

itsjamie Feb 10, 2019

johnBartos Feb 11, 2019

itsjamie Feb 10, 2019

johnBartos Feb 11, 2019

johnBartos commented Feb 11, 2019

itsjamie commented Feb 12, 2019

johnBartos commented Feb 22, 2019

michaelcunningham19 left a comment

michaelcunningham19 Apr 4, 2019

johnBartos commented May 8, 2019

Refactor fragment loading to be promise-based & Typescript loaders #2123

Refactor fragment loading to be promise-based & Typescript loaders #2123

Conversation

johnBartos commented Feb 9, 2019 • edited Loading

This PR will...

Why is this Pull Request needed?

Are there any points in the code the reviewer needs to double check?

Resolves issues:

Checklist

johnBartos commented Feb 9, 2019

itsjamie commented Feb 10, 2019 • edited Loading

johnBartos commented Feb 10, 2019 via email

itsjamie left a comment

Choose a reason for hiding this comment

itsjamie Feb 10, 2019

Choose a reason for hiding this comment

johnBartos Feb 11, 2019

Choose a reason for hiding this comment

itsjamie Feb 10, 2019

Choose a reason for hiding this comment

johnBartos Feb 11, 2019

Choose a reason for hiding this comment

itsjamie Feb 10, 2019

Choose a reason for hiding this comment

johnBartos Feb 11, 2019

Choose a reason for hiding this comment

johnBartos commented Feb 11, 2019

itsjamie commented Feb 12, 2019

johnBartos commented Feb 22, 2019

michaelcunningham19 left a comment

Choose a reason for hiding this comment

michaelcunningham19 Apr 4, 2019

Choose a reason for hiding this comment

johnBartos commented May 8, 2019

johnBartos commented Feb 9, 2019 •

edited

Loading

itsjamie commented Feb 10, 2019 •

edited

Loading