From fd8964d23327aa8f35e263740c7074e7e167555f Mon Sep 17 00:00:00 2001 From: Caleb Sacks <16855387+clabe45@users.noreply.github.com> Date: Thu, 20 Jul 2023 15:32:27 -0500 Subject: [PATCH] :bug: Fix audio destination reconnection logic Before this commit, `Video` and `Audio` layers are silent after recording once. Playing, streaming or recording again does not result in any sound. This fix introduces a new error message when updating examples/introduction/audio.html to record instead of play, but Etro's support for users directly modifying the audio graph is already very buggy. This buggy behavior can be illustrated when updating the same audio example to record instead of play and setting `muter.gain.value` to a positive value. When making these changes before this commit, the `layer1` is still muted. It probably is not routed to the destination node properly. Closes: #106 --- CHANGELOG.md | 5 +++ spec/integration/movie.spec.ts | 46 ++++++++++++++++++++++++++++ spec/unit/layer/audio-source.spec.ts | 31 ++++++++++++++----- src/layer/audio-source.ts | 30 ++++-------------- 4 files changed, 80 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b1983f8c..242ff9c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,10 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/) and this project adheres to [Semantic Versioning](http://semver.org/). +## [Unreleased] +### Fixed +- Audio and video layers going silent after the first time recording the movie ([#106](https://github.com/etro-js/etro/issues/106)). + ## [0.10.1] - 2023-07-16 ### Security - Bump engine.io and socket.io. @@ -280,6 +284,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Gaussian blur - Transform +[Unreleased]: https://github.com/etro-js/etro/compare/v0.10.1...HEAD [0.10.1]: https://github.com/etro-js/etro/compare/v0.10.0...v0.10.1 [0.10.0]: https://github.com/etro-js/etro/compare/v0.9.1...v0.10.0 [0.9.1]: https://github.com/etro-js/etro/compare/v0.9.0...v0.9.1 diff --git a/spec/integration/movie.spec.ts b/spec/integration/movie.spec.ts index b14cf785..54345b65 100644 --- a/spec/integration/movie.spec.ts +++ b/spec/integration/movie.spec.ts @@ -268,6 +268,52 @@ describe('Integration Tests ->', function () { // Clean up URL.revokeObjectURL(audioElement.src) }) + + it('should produce audio when recording twice', async function () { + // Remove all existing layers (optional) + movie.layers.length = 0 + + // Add an audio layer + const audio = new Audio('/base/spec/integration/assets/layer/audio.wav') + await new Promise(resolve => { + audio.onloadeddata = resolve + }) + const layer = new etro.layer.Audio({ + source: audio, + startTime: 0 + }) + movie.layers.push(layer) + + // Record audio + await movie.record({ + frameRate: 30, + video: false, + type: 'audio/ogg' + }) + const blob = await movie.record({ + frameRate: 30, + video: false, + type: 'audio/ogg' + }) + + // Make sure the audio blob is not empty + expect(blob.size).toBeGreaterThan(0) + + // Load blob into html audio element + const audioElement = document.createElement('audio') + audioElement.src = URL.createObjectURL(blob) + await new Promise(resolve => { + audioElement.addEventListener('canplaythrough', () => { + resolve() + }) + }) + + // Make sure the audio is not completely silent + expect(await isAudioSilent(audioElement)).toBe(false) + + // Clean up + URL.revokeObjectURL(audioElement.src) + }) }) describe('events ->', function () { diff --git a/spec/unit/layer/audio-source.spec.ts b/spec/unit/layer/audio-source.spec.ts index e141a1e5..cd519a04 100644 --- a/spec/unit/layer/audio-source.spec.ts +++ b/spec/unit/layer/audio-source.spec.ts @@ -8,7 +8,6 @@ describe('Unit Tests ->', function () { const CustomMedia = etro.layer.AudioSourceMixin(etro.layer.Base) let source - let layer let movie beforeEach(async function () { @@ -16,7 +15,6 @@ describe('Unit Tests ->', function () { source.readyState = 4 source.duration = 4 source.currentTime = 0 - layer = new CustomMedia({ startTime: 0, source }) movie = mockMovie() movie.currentTime = 2 @@ -24,66 +22,81 @@ describe('Unit Tests ->', function () { }) it('should be ready when source is ready', async function () { + const layer = new CustomMedia({ startTime: 0, source }) + expect(layer.ready).toBe(true) await layer.whenReady() }) it('should not be ready when source is not ready', function () { + const layer = new CustomMedia({ startTime: 0, source }) + source.readyState = 3 expect(layer.ready).toBe(false) }) it('should update its currentTime when seeking', function () { + const layer = new CustomMedia({ startTime: 0, source }) + layer.seek(2) expect(layer.currentTime).toBe(2) }) it('should update source.currentTime when seeking', function () { + const layer = new CustomMedia({ startTime: 0, source }) + layer.seek(2) expect(layer.source.currentTime).toBe(layer.currentTime) }) it('should update source.currentTime when seeking with sourceStartTime set', function () { + const layer = new CustomMedia({ startTime: 0, source }) + layer.sourceStartTime = 0.02 layer.seek(2) expect(layer.source.currentTime).toBe(layer.currentTime + layer.sourceStartTime) }) it('should have its duration depend on its playbackRate', function () { + const layer = new CustomMedia({ startTime: 0, source }) + const oldDuration = layer.duration layer.playbackRate = 2 expect(layer.duration).toBe(oldDuration / 2) }) it('should have no audioNode set on creation', function () { + const layer = new CustomMedia({ startTime: 0, source }) + expect(layer.audioNode).toBeFalsy() }) it('should have an audioNode set when attached', function () { + const layer = new CustomMedia({ startTime: 0, source }) + layer.tryAttach(movie) expect(layer.audioNode).toBeTruthy() }) it('should connect audioNode when attached', function () { + const layer = new CustomMedia({ startTime: 0, source }) + // Create audio node and connect it to movie.actx destination layer.tryAttach(movie) // Disconnect audio node (but don't destroy it) layer.tryDetach() - spyOn(layer.audioNode, 'connect') - // `attach` replaces the `audioNode.connect` method in-place, so store the - // spied method here. - const connectCache = layer.audioNode.connect // Now, connect to movie destination again layer.tryAttach(movie) // `connect` should have been called after we attached the second time. - expect(connectCache).toHaveBeenCalled() + expect(layer.audioNode.connect).toHaveBeenCalled() }) it('should disconnect audioNode when detached', function () { + const layer = new CustomMedia({ startTime: 0, source }) + layer.tryAttach(movie) - spyOn(layer.audioNode, 'disconnect') layer.tryDetach() @@ -91,6 +104,8 @@ describe('Unit Tests ->', function () { }) it('should keep the same audioNode when detached and re-attached', function () { + const layer = new CustomMedia({ startTime: 0, source }) + layer.tryAttach(movie) const original = layer.audioNode layer.tryDetach() diff --git a/src/layer/audio-source.ts b/src/layer/audio-source.ts index 21be2473..24cfd475 100644 --- a/src/layer/audio-source.ts +++ b/src/layer/audio-source.ts @@ -50,7 +50,7 @@ function AudioSourceMixin (superclass: Co private _unstretchedDuration: number private _playbackRate: number private _initialized: boolean - private _connectedToDestination: boolean + private _lastAudioDestination: AudioNode /** * @param options @@ -123,36 +123,18 @@ function AudioSourceMixin (superclass: Co // TODO: on unattach? subscribe(movie, 'audiodestinationupdate', event => { - // Connect to new destination if immediately connected to the existing - // destination. - if (this._connectedToDestination) { - this.audioNode.disconnect(movie.actx.destination) - this.audioNode.connect(event.destination) - } + this.audioNode.disconnect(this._lastAudioDestination) + this.audioNode.connect(event.destination) + + this._lastAudioDestination = event.destination }) // connect to audiocontext this._audioNode = this.audioNode || movie.actx.createMediaElementSource(this.source) - // Spy on connect and disconnect to remember if it connected to - // actx.destination (for Movie#record). - const oldConnect = this._audioNode.connect.bind(this.audioNode) - this._audioNode.connect = (destination: T, outputIndex?: number, inputIndex?: number): AudioNode => { - this._connectedToDestination = destination === movie.actx.destination - return oldConnect(destination, outputIndex, inputIndex) - } - const oldDisconnect = this._audioNode.disconnect.bind(this.audioNode) - this._audioNode.disconnect = (destination?: T | number, output?: number, input?: number): AudioNode => { - if (this._connectedToDestination && - destination === movie.actx.destination) { - this._connectedToDestination = false - } - - return oldDisconnect(destination, output, input) - } - // Connect to actx.destination by default (can be rewired by user) this.audioNode.connect(movie.actx.destination) + this._lastAudioDestination = movie.actx.destination } detach () {