Skip to content

Commit

Permalink
Fix #3 - Safari crashes when remote video tiles are added or toggled
Browse files Browse the repository at this point in the history
  • Loading branch information
@justindarc
justindarc committed Nov 25, 2019
1 parent d24c5d0 commit 8e511bc
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 30 deletions.
1 change: 1 addition & 0 deletions src/videoelementfactory/NoOpVideoElementFactory.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export default class NoOpVideoElementFactory implements VideoElementFactory {
removeAttribute: (): void => {},
setAttribute: (): void => {},
srcObject: false,
pause: (): void => {},
};
// @ts-ignore
return element;
Expand Down
21 changes: 19 additions & 2 deletions src/videotile/DefaultVideoTile.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,36 @@ export default class DefaultVideoTile implements DevicePixelRatioObserver, Video
if (!videoElement.hasAttribute('muted')) {
videoElement.setAttribute('muted', 'true');
}

if (videoElement.srcObject !== videoStream) {
videoElement.srcObject = videoStream;
}
}

static disconnectVideoStreamFromVideoElement(videoElement: HTMLVideoElement | null): void {
if (!videoElement) {
if (!videoElement || !videoElement.srcObject) {
return;
}
videoElement.srcObject = null;

videoElement.pause();
videoElement.style.transform = '';

DefaultVideoTile.setVideoElementFlag(videoElement, 'disablePictureInPicture', false);
DefaultVideoTile.setVideoElementFlag(videoElement, 'disableRemotePlayback', false);

// We must remove all the tracks from the MediaStream before
// clearing the `srcObject` to prevent Safari from crashing.
const mediaStream = videoElement.srcObject as MediaStream;
const tracks = mediaStream.getTracks();
for (const track of tracks) {
mediaStream.removeTrack(track);
}

// Need to wait one frame before clearing `srcObject` to
// prevent Safari from crashing.
requestAnimationFrame(() => {
videoElement.srcObject = null;
});
}

constructor(
Expand Down
5 changes: 5 additions & 0 deletions test/dommock/DOMMockBuilder.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -803,6 +803,10 @@ export default class DOMMockBuilder {
GlobalAny.ImageData = class MockImageData {
constructor(public data: Uint8ClampedArray, public width: number, public height: number) {}
};

GlobalAny.requestAnimationFrame = function mockRequestAnimationFrame(callback: () => void) {
setTimeout(callback);
};
}

cleanup(): void {
Expand All @@ -825,5 +829,6 @@ export default class DOMMockBuilder {
delete GlobalAny.MediaQueryList;
delete GlobalAny.matchMedia;
delete GlobalAny.document;
delete GlobalAny.requestAnimationFrame;
}
}
57 changes: 31 additions & 26 deletions test/videotile/DefaultVideoTile.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,13 @@ import DefaultVideoTile from '../../src/videotile/DefaultVideoTile';
import VideoTileController from '../../src/videotilecontroller/VideoTileController';
import DOMMockBuilder from '../dommock/DOMMockBuilder';

// @ts-ignore
const mockMediaStream: MediaStream = {
getTracks: (): MediaStreamTrack[] => {
return [];
},
};

class InvokingDevicePixelRatioMonitor implements DevicePixelRatioMonitor {
private observerQueue: Set<DevicePixelRatioObserver>;

Expand Down Expand Up @@ -74,12 +81,10 @@ describe('DefaultVideoTile', () => {
const videoElement = videoElementFactory.create();
tile.bindVideoElement(videoElement);

// @ts-ignore
const mediaStream: MediaStream = { fake: 'stream' };
tile.bindVideoStream('attendee', true, mediaStream, 1, 1, 1);
tile.bindVideoStream('attendee', true, mockMediaStream, 1, 1, 1);

expect(tile.state().tileId).to.equal(tileId);
expect(tile.state().boundVideoElement.srcObject).to.equal(mediaStream);
expect(tile.state().boundVideoElement.srcObject).to.equal(mockMediaStream);

tile.destroy();
expect(tile.state().tileId).to.equal(null);
Expand Down Expand Up @@ -121,7 +126,11 @@ describe('DefaultVideoTile', () => {
const boundAttendeeId = 'attendee';
const localTile = true;
// @ts-ignore
const boundVideoStream: MediaStream = { fake: 'stream' };
const boundVideoStream: MediaStream = {
getTracks: (): MediaStreamTrack[] => {
return [];
},
};
const videoStreamContentWidth = 1;
const videoStreamContentHeight = 1;
const streamId = 1;
Expand All @@ -147,8 +156,7 @@ describe('DefaultVideoTile', () => {

it('unbinds a video stream', () => {
tile = new DefaultVideoTile(tileId, true, tileController, monitor);
// @ts-ignore
tile.bindVideoStream('attendee', true, { fake: 'stream' }, 1, 1, 1);
tile.bindVideoStream('attendee', true, mockMediaStream, 1, 1, 1);
tile.bindVideoStream(null, true, null, null, null, null);

expect(tile.state().boundAttendeeId).to.equal(null);
Expand All @@ -167,18 +175,20 @@ describe('DefaultVideoTile', () => {
const videoElement = videoElementFactory.create();
tile.bindVideoElement(videoElement);

// @ts-ignore
const mediaStream: MediaStream = { fake: 'stream' };
tile.bindVideoStream('attendee', true, mediaStream, 1, 1, 1);
expect(videoElement.srcObject).to.equal(mediaStream);
tile.bindVideoStream('attendee', true, mockMediaStream, 1, 1, 1);
expect(videoElement.srcObject).to.equal(mockMediaStream);

tile.bindVideoStream('attendee', true, mediaStream, 2, 2, 1);
expect(videoElement.srcObject).to.equal(mediaStream);
tile.bindVideoStream('attendee', true, mockMediaStream, 2, 2, 1);
expect(videoElement.srcObject).to.equal(mockMediaStream);

// @ts-ignore
const mediaStream2: MediaStream = { fake: 'stream' };
tile.bindVideoStream('attendee', true, mediaStream2, 2, 2, 1);
expect(videoElement.srcObject).to.equal(mediaStream2);
const mockMediaStream2: MediaStream = {
getTracks: (): MediaStreamTrack[] => {
return [];
},
};
tile.bindVideoStream('attendee', true, mockMediaStream2, 2, 2, 1);
expect(videoElement.srcObject).to.equal(mockMediaStream2);
});
});

Expand All @@ -191,8 +201,7 @@ describe('DefaultVideoTile', () => {
const setAttributeSpy = sinon.spy(videoElement, 'setAttribute');

tile.bindVideoElement(videoElement);
// @ts-ignore
tile.bindVideoStream('attendee', false, { fake: 'stream' }, 1, 1, 1);
tile.bindVideoStream('attendee', false, mockMediaStream, 1, 1, 1);

expect(tile.state().boundVideoElement).to.equal(videoElement);
expect(tile.state().videoElementCSSWidthPixels).to.equal(videoElement.clientWidth);
Expand Down Expand Up @@ -241,8 +250,7 @@ describe('DefaultVideoTile', () => {
const setAttributeSpy = sinon.spy(videoElement, 'setAttribute');

tile.bindVideoElement(videoElement);
// @ts-ignore
tile.bindVideoStream('attendee', false, { fake: 'stream' }, 1, 1, 1);
tile.bindVideoStream('attendee', false, mockMediaStream, 1, 1, 1);

expect(removeAttributeSpy.calledWith('controls')).to.be.true;
expect(setAttributeSpy.callCount).to.equal(0);
Expand All @@ -256,8 +264,7 @@ describe('DefaultVideoTile', () => {
// @ts-ignore
videoElement.disableRemotePlayback = false;
tile.bindVideoElement(videoElement);
// @ts-ignore
tile.bindVideoStream('attendee', true, { fake: 'stream' }, 1, 1, 1);
tile.bindVideoStream('attendee', true, mockMediaStream, 1, 1, 1);
// @ts-ignore
expect(videoElement.disablePictureInPicture).to.be.true;
// @ts-ignore
Expand Down Expand Up @@ -366,8 +373,7 @@ describe('DefaultVideoTile', () => {

const videoElement = videoElementFactory.create();
tile.bindVideoElement(videoElement);
// @ts-ignore
tile.bindVideoStream('attendee', true, { fake: 'stream' }, 1, 1, 1);
tile.bindVideoStream('attendee', true, mockMediaStream, 1, 1, 1);

const image = tile.capture();
expect(image.width).to.equal(videoElement.videoWidth);
Expand All @@ -384,8 +390,7 @@ describe('DefaultVideoTile', () => {
delete videoElement.videoHeight;

tile.bindVideoElement(videoElement);
// @ts-ignore
tile.bindVideoStream('attendee', true, { fake: 'stream' }, 1, 1, 1);
tile.bindVideoStream('attendee', true, mockMediaStream, 1, 1, 1);

const image = tile.capture();
expect(image.width).to.equal(videoElement.width);
Expand Down
11 changes: 9 additions & 2 deletions test/videotilecontroller/DefaultVideoTileController.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@ import VideoTileState from '../../src/videotile/VideoTileState';
import VideoTileController from '../../src/videotilecontroller/VideoTileController';
import DOMMockBuilder from '../dommock/DOMMockBuilder';

// @ts-ignore
const mockMediaStream: MediaStream = {
getTracks: (): MediaStreamTrack[] => {
return [];
},
};

describe('DefaultVideoTileController', () => {
const assert: Chai.AssertStatic = chai.assert;
const expect: Chai.ExpectStatic = chai.expect;
Expand Down Expand Up @@ -154,7 +161,7 @@ describe('DefaultVideoTileController', () => {
tileController
.getLocalVideoTile()
// @ts-ignore
.bindVideoStream('attendee', true, { fake: 'stream' }, 1, 1, 1);
.bindVideoStream('attendee', true, mockMediaStream, 1, 1, 1);
});
});

Expand Down Expand Up @@ -383,7 +390,7 @@ describe('DefaultVideoTileController', () => {
tileController
.getLocalVideoTile()
// @ts-ignore
.bindVideoStream('attendee', true, { fake: 'stream' }, 1, 1, 1);
.bindVideoStream('attendee', true, mockMediaStream, 1, 1, 1);
});
});
});
Expand Down

0 comments on commit 8e511bc

Please sign in to comment.