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

fix: store queue and current transmux on transmuxer instead of global #1045

Merged
merged 4 commits into from
Feb 2, 2021

Conversation

brandonocasey
Copy link
Contributor

Problem

So this is somewhat of a bigger issue then I originally realized when I found an issue in tests for #1043 . The segmentTransmuxer stores a global cache of the currentTransmux and transmux queues. This causes a few issues:

  1. Separate audio/video transmuxes will bottleneck on another
  2. When running two or more videos on a page each video has a chance to bottleneck all the others with it's transmuxQueue and currentTransmux
  3. Tests/dispose is flakey because mediaSegmentRequest sets currentTransmux/transmuxQueue but only segmentLoader disposes it.

Changes

Store the transmuxQueue and currentTransmux on the transmuxer that is being used rather than keeping a global list.

@brandonocasey brandonocasey force-pushed the fix/segment-transmuxer-global branch from 0f18dec to a0d6775 Compare January 21, 2021 20:45
@@ -590,23 +595,6 @@ export default class SegmentLoader extends videojs.EventTarget {
}
}

createTransmuxer_() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This moved to segmentTransmuxer.createTransmuxer for a few reasons:

  1. We use it in tests in exactly the same manner
  2. We need to add some special properties to the transmuxer when we create it in segmentTransmuxer to prevent state from being kept in the segmentTransmuxer itself
  3. We need to clear those same properties on terminate, which we do by wrapping terminate in segmentTransmuxer
  4. segmentTransmuxer already handles all the other transmuxer message logic, so having logic here for it is weird.

@brandonocasey brandonocasey force-pushed the fix/segment-transmuxer-global branch 4 times, most recently from cf98826 to 6257202 Compare January 22, 2021 16:31
@@ -632,9 +620,6 @@ export default class SegmentLoader extends videojs.EventTarget {
this.abort_();
if (this.transmuxer_) {
this.transmuxer_.terminate();
// Although it isn't an instance of a class, the segment transmuxer must still be
// cleaned up.
segmentTransmuxer.dispose();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

dispose is gone, segmentTransmuxer stores no state.

@@ -1,5 +1,4 @@
const transmuxQueue = [];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now stored on transmuxer, not global.

const transmuxedData = {
isPartial,
buffer: []
};

const handleMessage = (event) => {
if (!currentTransmux) {
if (transmuxer.currentTransmux !== options) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need access to options above so that we can verify that we haven't been disposed.

src/segment-transmuxer.js Outdated Show resolved Hide resolved
gesinger
gesinger previously approved these changes Feb 2, 2021
@brandonocasey brandonocasey merged commit a34b4da into main Feb 2, 2021
@gkatsev gkatsev deleted the fix/segment-transmuxer-global branch May 17, 2021 21:43
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.

None yet

2 participants