Enable using Audio.setBuffer() after having played once #10060

Merged
merged 2 commits into from Nov 21, 2016

Projects

None yet

3 participants

@sttz
Contributor
sttz commented Nov 8, 2016

AudioBufferSourceNode can only be used once, the current Audio implementation already works around this by creating a new source whenever play() is called.

Since setBuffer() assigns the buffer directly to the source, calls to setBuffer() will fail after having played the audio once, making it impossible to use another buffer with the Audio instance (at least on Chrome, resulting in "Cannot set buffer after it has been already been set", Safari and Firefox don't mind).

This pull requests stores the buffer as an instance variable, which makes it possible to reuse it with a different buffer.

I removed creating a source in the constructor, avoiding to create a source that is never used because a new one is created later in in play(). On the other hand, bind() is now called on each invocation of play(). Not sure if you would prefer saving the bound callback in an instance variable, I wasn't able to find a precedent in three's source.

Since loop also directly forwards to the source, it now also gets saved as an instance variable to avoid errors when calling setLoop() before play(). This makes the implementations of setLoop() and setPlaybackRate() consistent, also fixing setLoop() not returning this.

@sttz sttz Don't rely on an existing buffer source node in Audio,
fixes calling setBuffer after Audio has been played
and makes get/setLoop consistent with get/setPlaybackRate.
2cf2878
@mrdoob
Owner
mrdoob commented Nov 20, 2016

@hoch how come Chrome errors here but not Firefox nor Safari?

src/audio/Audio.js
- source.onended = this.source.onended;
+ source.buffer = this.buffer;
+ source.loop = this.loop;
+ source.onended = this.onEnded.bind( this );
source.start( 0, this.startTime );
source.playbackRate.value = this.playbackRate;
@hoch
hoch Nov 20, 2016

No related to this change directly, but I believe it's slightly better to call .start() method at the end. It's also better to use playbackRate.setValueAtTime(value, this.startTime).

@mrdoob
mrdoob Nov 21, 2016 Owner

@sttz Do you mind doing these changes too?

@hoch
hoch commented Nov 20, 2016

Because Chrome does what the spec says:
https://webaudio.github.io/web-audio-api/#widl-AudioBufferSourceNode-buffer

Setting ASBN.buffer multiple times MUST throw an exception.

@sttz
Contributor
sttz commented Nov 21, 2016

Thanks for the feedback, I've added the proposed changes.

I also had to use setValueAtTime( this.playbackRate, this.context.currentTime ) in the setPlaybackRate() method, so that changing the pitch while the audio is playing still works.

@mrdoob mrdoob merged commit 1336d04 into mrdoob:dev Nov 21, 2016
@mrdoob
Owner
mrdoob commented Nov 21, 2016

Thanks!

@takahirox takahirox referenced this pull request Nov 22, 2016
Merged

Fix MMD audio example #10195

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment