Skip to content

Commit

Permalink
🐛 Fix audio destination reconnection logic
Browse files Browse the repository at this point in the history
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
  • Loading branch information
clabe45 committed Jul 20, 2023
1 parent 26d253c commit fd8964d
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 32 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
46 changes: 46 additions & 0 deletions spec/integration/movie.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<void>(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 () {
Expand Down
31 changes: 23 additions & 8 deletions spec/unit/layer/audio-source.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,89 +8,104 @@ describe('Unit Tests ->', function () {
const CustomMedia = etro.layer.AudioSourceMixin(etro.layer.Base)

let source
let layer
let movie

beforeEach(async function () {
source = jasmine.createSpyObj('source', ['addEventListener', 'play'])
source.readyState = 4
source.duration = 4
source.currentTime = 0
layer = new CustomMedia({ startTime: 0, source })

movie = mockMovie()
movie.currentTime = 2
movie.duration = 4
})

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()

expect(layer.audioNode.disconnect).toHaveBeenCalled()
})

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()
Expand Down
30 changes: 6 additions & 24 deletions src/layer/audio-source.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ function AudioSourceMixin<OptionsSuperclass extends BaseOptions> (superclass: Co
private _unstretchedDuration: number
private _playbackRate: number
private _initialized: boolean
private _connectedToDestination: boolean
private _lastAudioDestination: AudioNode

/**
* @param options
Expand Down Expand Up @@ -123,36 +123,18 @@ function AudioSourceMixin<OptionsSuperclass extends BaseOptions> (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 = <T extends AudioDestinationNode>(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 = <T extends AudioDestinationNode>(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 () {
Expand Down

0 comments on commit fd8964d

Please sign in to comment.