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

Refactor transmuxing data flow #199

Merged
merged 23 commits into from
Mar 26, 2019
Merged

Refactor transmuxing data flow #199

merged 23 commits into from
Mar 26, 2019

Conversation

johnBartos
Copy link

@johnBartos johnBartos commented Feb 25, 2019

This PR will...

  • Remove all events from the transmuxing pipeline, aside from ERROR and FRAG_DECRYPTED
  • Typescripts the following:
    • tsdemuxer
    • mp4-remuxer
    • mp4demuxer
    • passthrough-remuxer
    • demuxer (renamed to transmuxer-interface)
    • demuxer-inline (renamed to transmuxer)
    • demuxer-worker (renamed to transmuxer-worker)
  • Refactor the demuxer's append method to a new method: demux:
    • Accepts binary data
    • Synchronously returns the demuxed data as a DemuxerResult object when unencrypted
    • Asynchronously returns a DemuxerResult promise when data is SAMPLE-AES encrypted
  • Refactor the remuxer's remux method:
    • Accepts values from a DemuxerResult object
    • Synchronously returns a RemuxerResult object
  • Refactor demuxer-inline to transmuxer
    • Owns both a demuxer and remuxer
      • The demuxer no longer owns a remuxer
    • push now returns the following:
      • Synchronously returns a TransmuxerResult object when the segment data is unencrypted
      • Asynchronously returns a TransmuxerResult promise when the segment data is encrypted
  • Refactor demuxer-worker to transmuxer-worker
    • Posts a transmuxComplete message when transmuxing is complete
  • Refactor demuxer to transmuxer-interface
    • Accepts a new onTransmuxComplete handler
      • Triggered when transmuxComplete is received from a worker
      • Waits for the transmux promise to resolve for non-worker contexts; or triggers the handle immediately if synchronous
      • No longer attaches frag and id to each event; instead, the interface passes along a TransmuxIdentifier object which is used post-transmux to regain the fragment reference
  • Refactor mp4demuxer and passthrough-remuxer
    • startDTS offsetting has been shifted from the demuxer to the remuxer
    • InitSegment and InitPTS generation has been shifted from the demuxer to the remuxer
  • Refactor stream-controller and audio-stream-controller:
    • No longer listens for FRAG_PARSING_DATA, FRAG_PARSED, FRAG_PARSING_DATA events
      • But still emits them for compatibility
    • Each event is handled privately within the controller
    • Shifts fragment load abort logic to the top of the handleTransmuxComplete handler
      • This means that a fragment either no data is buffered, or the entire fragment is buffered; previously, each event handler had it's own abort check logic. This could lead to cases such as audio being buffered, but not video.
    • Cleans up handlers by relying on new assumptions (e.g. the only data received is of its expected type)
    • handleTransmuxComplete emits the FRAG_PARSING_USERDATA and FRAG_PARSING_METADATA events
  • Refactors transmuxing flow to allow for progressive transmuxing:
    • Demuxers implement a flush method which is called when progressive downloading completes
      • Exposed to controllers via flush on the transmuxer-worker
      • Clears any remaining data from the demuxer & calls remux once more
    • Progressive loading is in the project but not yet implemented in controllers; flush is called after
      the load promise resolves
    • Demuxers which do not support progressive parsing extend the new NonProgressiveDemuxer class, which buffers data until flush is called for the final remux
      • Currently all demuxers extend this class

Why is this Pull Request needed?

The event-driven architecture of the tranmuxing pipeline is not compatibly with a clean progressive segment parsing pipeline. By shifting to a return value based architecture, we're able to scope transmuxing to the controllers and segments to which the operation belongs. With this new architecture we will be able to safely transmux segments as a series of chunks, a core requirement for LHLS.

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

Yes

Resolves issues:

JW8-2638

@johnBartos johnBartos force-pushed the feature/demuxing-refactor branch 2 times, most recently from fa04412 to d725cd0 Compare February 27, 2019 21:36
@johnBartos johnBartos changed the base branch from feature/frag-loading-refactor to LHLS February 27, 2019 21:38
@johnBartos johnBartos force-pushed the feature/demuxing-refactor branch 3 times, most recently from b80d6a7 to 988f82a Compare February 27, 2019 22:54
src/demux/mp4demuxer.ts Outdated Show resolved Hide resolved
src/demux/transmuxer.ts Outdated Show resolved Hide resolved
@robwalch
Copy link

robwalch commented Mar 19, 2019

@johnBartos I got a TypeScript error:

$ yarn type-check                                          
yarn run v1.13.0
$ tsc --noEmit
src/controller/buffer-controller.ts:30:7 - error TS2415: Class 'BufferController' incorrectly extends base class 'EventHandler'.
  Property 'hls' is private in type 'BufferController' but not in type 'EventHandler'.

30 class BufferController extends EventHandler {
         ~~~~~~~~~~~~~~~~


Found 1 error.

@jwplayer-robot
Copy link

@jwplayer-robot
Copy link

@jwplayer-robot
Copy link

Copy link

@robwalch robwalch left a comment

Choose a reason for hiding this comment

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

destroy () {}
}

const dummyTrack = () => ({ type: '', id: -1, pid: -1, inputTimeScale: 90000, sequenceNumber: -1, len: 0, samples: [] });

Choose a reason for hiding this comment

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

Maybe this should just be exported/imported from non-progressive-demuxer.ts or defined as it's own module/type?

Copy link
Author

Choose a reason for hiding this comment

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

Agreed

@jwplayer-robot
Copy link

@johnBartos
Copy link
Author

johnBartos commented Mar 21, 2019

  1. Player does not resume from live pause http://player-pr-test-jenkins.longtailvideo.com/builds/20555/archive/test/public/events/?config=hlsjs/hls-live-bloomberg.json

  2. this.fragCurrent is null http://player-pr-test-jenkins.longtailvideo.com/builds/20555/archive/test/public/events/?config=captions/captions-hls-embedded-vtt-korean.json

  3. data is undefined http://player-pr-test-jenkins.longtailvideo.com/builds/20555/archive/test/harness.html?automated=true&edition=ads&config=hlsjs%2Fhls-cbc-ima-midroll.json&setup=1

  4. Exceptions thrown from handleTransmuxComplete: http://player-pr-test-jenkins.longtailvideo.com/builds/20555/archive/test/harness.html?automated=true&edition=enterprise&config=hlsjs%2Fhls-archived-fox-dvr-force-hlsjs.json&setup=1 & http://player-pr-test-jenkins.longtailvideo.com/builds/20555/archive/test/public/events/?config=hlsjs/hls-custom-labels-force-hlsjs.json & http://player-pr-test-jenkins.longtailvideo.com/builds/20555/archive/test/harness.html?automated=true&edition=enterprise&config=hlsjs%2Fhls-encoding-uplynk-force-hlsjs.json&setup=1

  5. Perhaps a stream issue, but this DVR stream is not starting: http://player-pr-test-jenkins.longtailvideo.com/builds/20555/archive/test/public/events/?config=hlsjs/hls-dvr-single-bitrate.json

  6. Stream plays but gap seeking behavior at the end of the stream has changed: http://player-pr-test-jenkins.longtailvideo.com/builds/20555/archive/test/harness.html?automated=true&edition=ads&config=hlsjs%2Fhls-stream-with-large-gap-at-start-of-stream-autostart.json&setup=1

  7. CORS error code is different: http://player-pr-test-jenkins.longtailvideo.com/builds/20555/archive/test/harness.html?automated=true&edition=ads&config=hlsjs%2Fhls-cors-bunny.json&setup=1

  8. Stream does not start; possible backtracking issue: http://player-pr-test-jenkins.longtailvideo.com/builds/20555/archive/test/harness.html?automated=true&edition=ads&config=hlsjs%2Fhls-dvr-hockey-archive.json&setup=1

  9. Possible ID3 exceptions, but haven't been able to repro locally: http://player-pr-test-jenkins.longtailvideo.com/builds/20555/archive/test/harness.html?setup&edition=ads&language=&config=hlsjs/hls-id3-timed-metadata&feature=hlsjs/id3_timed_metadata_and_program_date_time_in_manifest_using_hlsjs_provider

Dead stream link causing failures: https://bob.jwplayer.com/~alex/121598/new_master.m3u8

@jwplayer-robot
Copy link

@jwplayer-robot
Copy link

@jwplayer-robot
Copy link

@johnBartos
Copy link
Author

@jwplayer-robot
Copy link

@jwplayer-robot
Copy link

@jwplayer-robot
Copy link

@johnBartos
Copy link
Author

TODO: Pass AES token into provider fetch config

@jwplayer-robot
Copy link

@jwplayer-robot
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants