Skip to content
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

Feature/frag loading refactor #196

Merged
merged 12 commits into from
Feb 26, 2019
Merged

Conversation

johnBartos
Copy link

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:

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

Downstream clone of video-dev#2123

@@ -35,12 +35,12 @@
}
},
"@babel/generator": {
"version": "7.2.0",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need this?

John Bartos and others added 5 commits February 25, 2019 17:04
* Convert fetch loader to TypeScript, implement simple abort and improve error handling

* Use fetch loader by default in browsers that support Readable Stream

* Implement abort and timeout in fetch loader
fetch and xhr loaders abort on timeout
fetch and xhr loaders call onProgress before success
Improve typing in fetch and xhr loaders

* Implement progress callback in fetch loader

* Only listen to xhr progress if onProgress callback is defined

* Handle loader abort callbacks
@johnBartos johnBartos merged commit 2b885d9 into LHLS Feb 26, 2019
@robwalch robwalch deleted the feature/frag-loading-refactor branch January 27, 2020 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants