From 9087b5c0e20bc5ab896ea68e5598225a16cf7090 Mon Sep 17 00:00:00 2001 From: Justin D'Arcangelo Date: Fri, 22 Nov 2019 14:16:00 -0500 Subject: [PATCH] Fix #3 - Safari crashes when remote video tiles are added or toggled --- .../NoOpVideoElementFactory.ts | 1 + src/videotile/DefaultVideoTile.ts | 21 ++++++- test/dommock/DOMMockBuilder.ts | 5 ++ test/videotile/DefaultVideoTile.test.ts | 60 +++++++++++-------- .../DefaultVideoTileController.test.ts | 11 +++- 5 files changed, 68 insertions(+), 30 deletions(-) diff --git a/src/videoelementfactory/NoOpVideoElementFactory.ts b/src/videoelementfactory/NoOpVideoElementFactory.ts index ea89865673..91bcd8bd9d 100644 --- a/src/videoelementfactory/NoOpVideoElementFactory.ts +++ b/src/videoelementfactory/NoOpVideoElementFactory.ts @@ -21,6 +21,7 @@ export default class NoOpVideoElementFactory implements VideoElementFactory { removeAttribute: (): void => {}, setAttribute: (): void => {}, srcObject: false, + pause: (): void => {}, }; // @ts-ignore return element; diff --git a/src/videotile/DefaultVideoTile.ts b/src/videotile/DefaultVideoTile.ts index 9bb89fb113..f3cf14d896 100644 --- a/src/videotile/DefaultVideoTile.ts +++ b/src/videotile/DefaultVideoTile.ts @@ -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( diff --git a/test/dommock/DOMMockBuilder.ts b/test/dommock/DOMMockBuilder.ts index 51d5272fe5..4673207367 100644 --- a/test/dommock/DOMMockBuilder.ts +++ b/test/dommock/DOMMockBuilder.ts @@ -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 { @@ -825,5 +829,6 @@ export default class DOMMockBuilder { delete GlobalAny.MediaQueryList; delete GlobalAny.matchMedia; delete GlobalAny.document; + delete GlobalAny.requestAnimationFrame; } } diff --git a/test/videotile/DefaultVideoTile.test.ts b/test/videotile/DefaultVideoTile.test.ts index 6d5ee5c081..5667a83fe6 100644 --- a/test/videotile/DefaultVideoTile.test.ts +++ b/test/videotile/DefaultVideoTile.test.ts @@ -13,6 +13,16 @@ import DefaultVideoTile from '../../src/videotile/DefaultVideoTile'; import VideoTileController from '../../src/videotilecontroller/VideoTileController'; import DOMMockBuilder from '../dommock/DOMMockBuilder'; +// @ts-ignore +const mockMediaStream: MediaStream = { + getTracks: (): MediaStreamTrack[] => { + // @ts-ignore + const track: MediaStreamTrack = {}; + return [track]; + }, + removeTrack: () => {}, +}; + class InvokingDevicePixelRatioMonitor implements DevicePixelRatioMonitor { private observerQueue: Set; @@ -74,12 +84,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); @@ -121,7 +129,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; @@ -147,8 +159,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); @@ -167,18 +178,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); }); }); @@ -191,8 +204,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); @@ -241,8 +253,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); @@ -256,8 +267,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 @@ -366,8 +376,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); @@ -384,8 +393,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); diff --git a/test/videotilecontroller/DefaultVideoTileController.test.ts b/test/videotilecontroller/DefaultVideoTileController.test.ts index f26028be5a..313258dd28 100644 --- a/test/videotilecontroller/DefaultVideoTileController.test.ts +++ b/test/videotilecontroller/DefaultVideoTileController.test.ts @@ -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; @@ -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); }); }); @@ -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); }); }); });