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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 6 additions & 18 deletions src/segment-loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import Config from './config';
import window from 'global/window';
import { initSegmentId, segmentKeyId } from './bin-utils';
import { mediaSegmentRequest, REQUEST_ERRORS } from './media-segment-request';
import TransmuxWorker from 'worker!./transmuxer-worker.js';
import segmentTransmuxer from './segment-transmuxer';
import { TIME_FUDGE_FACTOR, timeUntilRebuffer as timeUntilRebuffer_ } from './ranges';
import { minRebufferMaxBandwidthSelector } from './playlist-selectors';
Expand Down Expand Up @@ -530,7 +529,6 @@ export default class SegmentLoader extends videojs.EventTarget {
};

this.transmuxer_ = this.createTransmuxer_();

this.triggerSyncInfoUpdate_ = () => this.trigger('syncinfoupdate');
this.syncController_.on('syncinfoupdate', this.triggerSyncInfoUpdate_);

Expand Down Expand Up @@ -591,20 +589,13 @@ 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.

const transmuxer = new TransmuxWorker();

transmuxer.postMessage({
action: 'init',
options: {
parse708captions: this.parse708captions_,
remux: false,
alignGopsAtEnd: this.safeAppend_,
keepOriginalTimestamps: true,
handlePartialData: this.handlePartialData_
}
return segmentTransmuxer.createTransmuxer({
remux: false,
alignGopsAtEnd: this.safeAppend_,
keepOriginalTimestamps: true,
handlePartialData: this.handlePartialData_,
parse708captions: this.parse708captions_
});

return transmuxer;
}

/**
Expand Down Expand Up @@ -632,9 +623,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.

}
this.resetStats_();

Expand Down
94 changes: 53 additions & 41 deletions src/segment-transmuxer.js
Original file line number Diff line number Diff line change
@@ -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.

let currentTransmux;
import TransmuxWorker from 'worker!./transmuxer-worker.js';

export const handleData_ = (event, transmuxedData, callback) => {
const {
Expand Down Expand Up @@ -66,30 +65,31 @@ export const handleGopInfo_ = (event, transmuxedData) => {
transmuxedData.gopInfo = event.data.gopInfo;
};

export const processTransmux = ({
transmuxer,
bytes,
audioAppendStart,
gopsToAlignWith,
isPartial,
remux,
onData,
onTrackInfo,
onAudioTimingInfo,
onVideoTimingInfo,
onVideoSegmentTimingInfo,
onAudioSegmentTimingInfo,
onId3,
onCaptions,
onDone
}) => {
export const processTransmux = (options) => {
const {
transmuxer,
bytes,
audioAppendStart,
gopsToAlignWith,
isPartial,
remux,
onData,
onTrackInfo,
onAudioTimingInfo,
onVideoTimingInfo,
onVideoSegmentTimingInfo,
onAudioSegmentTimingInfo,
onId3,
onCaptions,
onDone
} = options;
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.

// disposed
return;
}
Expand Down Expand Up @@ -134,7 +134,7 @@ export const processTransmux = ({
});

/* eslint-disable no-use-before-define */
dequeue();
dequeue(transmuxer);
/* eslint-enable */
};

Expand Down Expand Up @@ -187,30 +187,30 @@ export const processTransmux = ({
transmuxer.postMessage({ action: isPartial ? 'partialFlush' : 'flush' });
};

export const dequeue = () => {
currentTransmux = null;
if (transmuxQueue.length) {
currentTransmux = transmuxQueue.shift();
if (typeof currentTransmux === 'function') {
currentTransmux();
export const dequeue = (transmuxer) => {
transmuxer.currentTransmux = null;
if (transmuxer.transmuxQueue.length) {
transmuxer.currentTransmux = transmuxer.transmuxQueue.shift();
if (typeof transmuxer.currentTransmux === 'function') {
transmuxer.currentTransmux();
} else {
processTransmux(currentTransmux);
processTransmux(transmuxer.currentTransmux);
}
}
};

export const processAction = (transmuxer, action) => {
transmuxer.postMessage({ action });
dequeue();
dequeue(transmuxer);
};

export const enqueueAction = (action, transmuxer) => {
if (!currentTransmux) {
currentTransmux = action;
if (!transmuxer.currentTransmux) {
transmuxer.currentTransmux = action;
processAction(transmuxer, action);
return;
}
transmuxQueue.push(processAction.bind(null, transmuxer, action));
transmuxer.transmuxQueue.push(processAction.bind(null, transmuxer, action));
};

export const reset = (transmuxer) => {
Expand All @@ -222,23 +222,35 @@ export const endTimeline = (transmuxer) => {
};

export const transmux = (options) => {
if (!currentTransmux) {
currentTransmux = options;
if (!options.transmuxer.currentTransmux) {
options.transmuxer.currentTransmux = options;
processTransmux(options);
return;
}
transmuxQueue.push(options);
options.transmuxer.transmuxQueue.push(options);
};

export const dispose = () => {
// clear out module-level references
currentTransmux = null;
transmuxQueue.length = 0;
export const createTransmuxer = (options) => {
const transmuxer = new TransmuxWorker();

transmuxer.currentTransmux = null;
transmuxer.transmuxQueue = [];
const term = transmuxer.terminate;

transmuxer.terminate = () => {
transmuxer.currentTransmux = null;
transmuxer.transmuxQueue.length = 0;
return term.call(transmuxer);
};

transmuxer.postMessage({action: 'init', options});

return transmuxer;
};

export default {
reset,
dispose,
endTimeline,
transmux
transmux,
createTransmuxer
};
21 changes: 5 additions & 16 deletions test/media-segment-request.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@ import {
standardXHRResponse,
downloadProgress
} from './test-helpers';
import TransmuxWorker from 'worker!../src/transmuxer-worker.js';
import {createTransmuxer as createTransmuxer_} from '../src/segment-transmuxer.js';
import Decrypter from 'worker!../src/decrypter-worker.js';
import {dispose as segmentTransmuxerDispose} from '../src/segment-transmuxer.js';
import {
aacWithoutId3 as aacWithoutId3Segment,
aacWithId3 as aacWithId3Segment,
Expand Down Expand Up @@ -71,18 +70,11 @@ const sharedHooks = {
};

this.createTransmuxer = (isPartial) => {
const transmuxer = new TransmuxWorker();

transmuxer.postMessage({
action: 'init',
options: {
remux: false,
keepOriginalTimestamps: true,
handlePartialData: isPartial
}
return createTransmuxer_({
remux: false,
keepOriginalTimestamps: true,
handlePartialData: isPartial
});

return transmuxer;
};
},
afterEach(assert) {
Expand All @@ -92,9 +84,6 @@ const sharedHooks = {
if (this.transmuxer) {
this.transmuxer.terminate();
}

// clear current transmux on segment transmuxer
segmentTransmuxerDispose();
}

};
Expand Down
17 changes: 5 additions & 12 deletions test/segment-loader.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import {
mediaDuration,
getTroublesomeSegmentDurationMessage
} from '../src/segment-loader';
import segmentTransmuxer from '../src/segment-transmuxer';
import videojs from 'video.js';
import mp4probe from 'mux.js/lib/mp4/probe';
import {
Expand Down Expand Up @@ -2381,29 +2380,23 @@ QUnit.module('SegmentLoader', function(hooks) {
QUnit.test('dispose cleans up transmuxer', function(assert) {
return setupMediaSource(loader.mediaSource_, loader.sourceUpdater_).then(() => {
loader.playlist(playlistWithDuration(20));
const transmuxer = loader.transmuxer_;

const origTransmuxerTerminate =
loader.transmuxer_.terminate.bind(loader.transmuxer_);
const origTransmuxerTerminate = transmuxer.terminate.bind(transmuxer);
let transmuxerTerminateCount = 0;
const origSegmentTransmuxerDispose =
segmentTransmuxer.dispose.bind(segmentTransmuxer);
let segmentTransmuxerDisposeCalls = 0;

loader.transmuxer_.terminate = () => {
transmuxer.terminate = () => {
transmuxerTerminateCount++;
origTransmuxerTerminate();
};
segmentTransmuxer.dispose = () => {
origSegmentTransmuxerDispose();
segmentTransmuxerDisposeCalls++;
};

loader.load();
this.clock.tick(1);
loader.dispose();

assert.equal(transmuxerTerminateCount, 1, 'terminated transmuxer');
assert.equal(segmentTransmuxerDisposeCalls, 1, 'disposed segment transmuxer');
assert.ok(!transmuxer.currentTransmux, 'no current transmux');
assert.equal(transmuxer.transmuxQueue.length, 0, 'no queue');
});
});

Expand Down
25 changes: 9 additions & 16 deletions test/segment-transmuxer.test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import QUnit from 'qunit';
import sinon from 'sinon';
import TransmuxWorker from 'worker!../src/transmuxer-worker.js';
import {
muxed as muxedSegment,
caption as captionSegment,
Expand All @@ -17,32 +16,27 @@ import {
handleGopInfo_,
handleDone_,
handleData_,
dispose
createTransmuxer as createTransmuxer_
} from '../src/segment-transmuxer';
// needed for plugin registration
import '../src/videojs-http-streaming';

const noop = () => {};

const createTransmuxer = (isPartial) => {
const transmuxer = new TransmuxWorker();

transmuxer.postMessage({
action: 'init',
options: {
remux: false,
keepOriginalTimestamps: true,
handlePartialData: isPartial
}
return createTransmuxer_({
remux: false,
keepOriginalTimestamps: true,
handlePartialData: isPartial
});

return transmuxer;
};

const mockTransmuxer = (isPartial) => {
const transmuxer = {
postMessage(event) {},
terminate() {}
terminate() {},
currentTransmux: null,
transmuxQueue: []
};

return transmuxer;
Expand All @@ -54,7 +48,6 @@ QUnit.module('Segment Transmuxer', {
assert.timeout(5000);
},
afterEach(assert) {
dispose();
if (this.transmuxer) {
this.transmuxer.terminate();
}
Expand Down Expand Up @@ -240,7 +233,7 @@ QUnit.test('dequeues and processes action on dequeue()', function(assert) {
'the transmux() posted `flush` to the transmuxer'
);

dequeue();
dequeue(this.transmuxer);
assert.deepEqual(this.transmuxer.postMessage.callCount, 2, 'two actions processed');
assert.deepEqual(
this.transmuxer.postMessage.args[1][0],
Expand Down
17 changes: 5 additions & 12 deletions test/transmuxer-worker.test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import QUnit from 'qunit';
import TransmuxWorker from 'worker!../src/transmuxer-worker.js';
import {createTransmuxer as createTransmuxer_} from '../src/segment-transmuxer.js';
import {
mp4Captions as mp4CaptionsSegment,
muxed as muxedSegment,
Expand All @@ -10,18 +10,11 @@ import {
import '../src/videojs-http-streaming';

const createTransmuxer = (isPartial) => {
const transmuxer = new TransmuxWorker();

transmuxer.postMessage({
action: 'init',
options: {
remux: false,
keepOriginalTimestamps: true,
handlePartialData: isPartial
}
return createTransmuxer_({
remux: false,
keepOriginalTimestamps: true,
handlePartialData: isPartial
});

return transmuxer;
};

// The final done message from the Transmux worker
Expand Down