-
Notifications
You must be signed in to change notification settings - Fork 2k
Correct sound duration and playback timing #9249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -216,7 +216,11 @@ class SoundInstance extends EventHandler { | |
| */ | ||
| this._currentTime = 0; | ||
|
|
||
| /** @private */ | ||
| /** | ||
| * The playback position relative to startTime when _startedAt was recorded. | ||
| * | ||
| * @private | ||
| */ | ||
| this._currentOffset = 0; | ||
|
|
||
| /** | ||
|
|
@@ -302,14 +306,18 @@ class SoundInstance extends EventHandler { | |
| } | ||
|
|
||
| /** | ||
| * Sets the current time of the sound that is playing. If the value provided is bigger than the | ||
| * duration of the instance it will wrap from the beginning. | ||
| * Sets the current time of the sound that is playing, relative to {@link startTime}. If the | ||
| * value provided is bigger than the duration of the instance it will wrap from the beginning. | ||
| * | ||
| * @type {number} | ||
| */ | ||
| set currentTime(value) { | ||
| value = Number(value) || 0; | ||
| if (value < 0) return; | ||
|
|
||
| const duration = this.duration; | ||
| const currentTime = duration ? capTime(value, duration) : value; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the eager normalization worth calling out in the PR's API Changes section. Before this commit the setter stored the raw value and Matching the documented "it will wrap from the beginning" immediately is the better behavior, and the new test pins it — this is purely about the description not mentioning a changed property round-trip. One ordering consequence, if you think it's worth caring about: because the wrap is applied at assignment time using the duration as of that moment, the caller's original intent is discarded. Setting |
||
|
|
||
| if (this._state === STATE_PLAYING) { | ||
| const suspend = this._suspendInstanceEvents; | ||
| this._suspendInstanceEvents = true; | ||
|
|
@@ -318,19 +326,19 @@ class SoundInstance extends EventHandler { | |
| this.stop(); | ||
|
|
||
| // set _startOffset and play | ||
| this._startOffset = value; | ||
| this._startOffset = currentTime; | ||
| this.play(); | ||
| this._suspendInstanceEvents = suspend; | ||
| } else { | ||
| // set _startOffset which will be used when the instance will start playing | ||
| this._startOffset = value; | ||
| this._startOffset = currentTime; | ||
| // set _currentTime | ||
| this._currentTime = value; | ||
| this._currentTime = currentTime; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Gets the current time of the sound that is playing. | ||
| * Gets the current time of the sound that is playing, relative to {@link startTime}. | ||
| * | ||
| * @type {number} | ||
| */ | ||
|
|
@@ -375,7 +383,8 @@ class SoundInstance extends EventHandler { | |
| } | ||
|
|
||
| /** | ||
| * Gets the duration of the sound that the instance will play starting from startTime. | ||
| * Gets the duration of the sound that the instance will play starting from {@link startTime}. | ||
| * The returned value is clamped to the time available after the normalized start time. | ||
| * | ||
| * @type {number} | ||
| */ | ||
|
|
@@ -384,7 +393,9 @@ class SoundInstance extends EventHandler { | |
| return 0; | ||
| } | ||
| if (this._duration) { | ||
| return capTime(this._duration, this._sound.duration); | ||
| const soundDuration = this._sound.duration; | ||
| const startTime = capTime(this._startTime, soundDuration); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "the normalized start time" in the new doc wording is carrying more meaning than a reader will unpack.
Both match what |
||
| return Math.min(this._duration, soundDuration - startTime); | ||
| } | ||
| return this._sound.duration; | ||
| } | ||
|
|
@@ -724,19 +735,19 @@ class SoundInstance extends EventHandler { | |
| this._createSource(); | ||
| } | ||
|
|
||
| // calculate start offset | ||
| let offset = capTime(this._startOffset, this.duration); | ||
| offset = capTime(this._startTime + offset, this._sound.duration); | ||
| // calculate the current offset relative to startTime and its matching buffer offset | ||
| const currentOffset = capTime(this._startOffset, this.duration); | ||
| const offset = capTime(this._startTime + currentOffset, this._sound.duration); | ||
| // reset start offset now that we started the sound | ||
| this._startOffset = null; | ||
|
|
||
| // start source with specified offset | ||
| this._startSource(offset); | ||
| this._startSource(offset, currentOffset); | ||
|
|
||
| // reset times | ||
| this._startedAt = this._manager.context.currentTime; | ||
| this._currentTime = 0; | ||
| this._currentOffset = offset; | ||
| this._currentOffset = currentOffset; | ||
|
|
||
| // Initialize volume and loop - note moved to be after start() because of Chrome bug | ||
| this.volume = this._volume; | ||
|
|
@@ -762,11 +773,12 @@ class SoundInstance extends EventHandler { | |
| * playback at the end of the first iteration instead of looping. | ||
| * | ||
| * @param {number} offset - The offset into the buffer, in seconds, to start playing from. | ||
| * @param {number} currentOffset - The offset relative to startTime, in seconds. | ||
| * @private | ||
| */ | ||
| _startSource(offset) { | ||
| _startSource(offset, currentOffset) { | ||
| if (this._duration && !this._loop) { | ||
| this.source.start(0, offset, this._duration); | ||
| this.source.start(0, offset, this.duration - currentOffset); | ||
| } else { | ||
| this.source.start(0, offset); | ||
| } | ||
|
|
@@ -884,8 +896,8 @@ class SoundInstance extends EventHandler { | |
| return false; | ||
| } | ||
|
|
||
| // start at point where sound was paused | ||
| let offset = this.currentTime; | ||
| // start at the point relative to startTime where the sound was paused | ||
| let currentOffset = this.currentTime; | ||
|
|
||
| // set state back to playing | ||
| this._state = STATE_PLAYING; | ||
|
|
@@ -902,18 +914,19 @@ class SoundInstance extends EventHandler { | |
| // if the user set the 'currentTime' property while the sound | ||
| // was paused then use that as the offset instead | ||
| if (this._startOffset !== null) { | ||
| offset = capTime(this._startOffset, this.duration); | ||
| offset = capTime(this._startTime + offset, this._sound.duration); | ||
| currentOffset = capTime(this._startOffset, this.duration); | ||
|
|
||
| // reset offset | ||
| this._startOffset = null; | ||
| } | ||
|
|
||
| const offset = capTime(this._startTime + currentOffset, this._sound.duration); | ||
|
|
||
| // start source | ||
| this._startSource(offset); | ||
| this._startSource(offset, currentOffset); | ||
|
|
||
| this._startedAt = this._manager.context.currentTime; | ||
| this._currentOffset = offset; | ||
| this._currentOffset = currentOffset; | ||
|
|
||
| // Initialize parameters | ||
| this.volume = this._volume; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| import { expect } from 'chai'; | ||
|
|
||
| import { SoundSlot } from '../../../../src/framework/components/sound/slot.js'; | ||
| import { Sound } from '../../../../src/platform/sound/sound.js'; | ||
|
|
||
| function createSlot(options = {}) { | ||
| const asset = { resource: new Sound({ duration: 4 }) }; | ||
| const component = { | ||
| system: { | ||
| app: { | ||
| assets: { | ||
| get: () => asset | ||
| } | ||
| }, | ||
| manager: {} | ||
| } | ||
| }; | ||
|
|
||
| return new SoundSlot(component, 'Test', { asset: 1, ...options }); | ||
| } | ||
|
|
||
| describe('SoundSlot', function () { | ||
| describe('#duration', function () { | ||
| it('returns the asset duration when no duration is specified', function () { | ||
| expect(createSlot().duration).to.equal(4); | ||
| }); | ||
|
|
||
| it('returns a duration shorter than the asset', function () { | ||
| expect(createSlot({ duration: 2 }).duration).to.equal(2); | ||
| }); | ||
|
|
||
| it('returns the asset duration when the durations match', function () { | ||
| expect(createSlot({ duration: 4 }).duration).to.equal(4); | ||
| }); | ||
|
|
||
| it('clamps a duration longer than the asset', function () { | ||
| expect(createSlot({ duration: 6 }).duration).to.equal(4); | ||
| }); | ||
|
|
||
| it('clamps the duration to the time remaining after startTime', function () { | ||
| expect(createSlot({ startTime: 2, duration: 6 }).duration).to.equal(2); | ||
| expect(createSlot({ startTime: 2, duration: 3 }).duration).to.equal(2); | ||
| }); | ||
|
|
||
| it('normalizes startTime before clamping the duration', function () { | ||
| expect(createSlot({ startTime: 6, duration: 6 }).duration).to.equal(2); | ||
| expect(createSlot({ startTime: 4, duration: 6 }).duration).to.equal(4); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,163 @@ | ||
| import { expect } from 'chai'; | ||
|
|
||
| import { EventHandler } from '../../../src/core/event-handler.js'; | ||
| import { SoundInstance } from '../../../src/platform/sound/instance.js'; | ||
| import { Sound } from '../../../src/platform/sound/sound.js'; | ||
|
|
||
| function createInstance(options = {}, manager = { context: null }) { | ||
| const sound = new Sound({ duration: 4 }); | ||
|
|
||
| return new SoundInstance(manager, sound, options); | ||
| } | ||
|
|
||
| function createManager() { | ||
| const manager = new EventHandler(); | ||
| const sources = []; | ||
|
|
||
| manager.volume = 1; | ||
| manager.suspended = false; | ||
| manager.sources = sources; | ||
| manager.context = { | ||
| currentTime: 0, | ||
| destination: {}, | ||
| createGain: () => ({ | ||
| gain: { value: 1 }, | ||
| connect: () => {}, | ||
| disconnect: () => {} | ||
| }), | ||
| createBufferSource: () => { | ||
| const source = { | ||
| playbackRate: { value: 1 }, | ||
| loopStart: 0, | ||
| loopEnd: 0, | ||
| connect: () => {}, | ||
| start: (...args) => { | ||
| source.startArgs = args; | ||
| }, | ||
| stop: () => {} | ||
| }; | ||
| sources.push(source); | ||
| return source; | ||
| } | ||
| }; | ||
|
|
||
| return manager; | ||
| } | ||
|
|
||
| describe('SoundInstance', function () { | ||
| describe('#duration', function () { | ||
| it('returns the sound duration when no duration is specified', function () { | ||
| expect(createInstance().duration).to.equal(4); | ||
| }); | ||
|
|
||
| it('returns a duration shorter than the sound', function () { | ||
| expect(createInstance({ duration: 2 }).duration).to.equal(2); | ||
| }); | ||
|
|
||
| it('returns the sound duration when the durations match', function () { | ||
| expect(createInstance({ duration: 4 }).duration).to.equal(4); | ||
| }); | ||
|
|
||
| it('clamps a duration longer than the sound', function () { | ||
| expect(createInstance({ duration: 6 }).duration).to.equal(4); | ||
| }); | ||
|
|
||
| it('clamps the duration to the time remaining after startTime', function () { | ||
| expect(createInstance({ startTime: 2, duration: 6 }).duration).to.equal(2); | ||
| expect(createInstance({ startTime: 2, duration: 3 }).duration).to.equal(2); | ||
| }); | ||
|
|
||
| it('normalizes startTime before clamping the duration', function () { | ||
| expect(createInstance({ startTime: 6, duration: 6 }).duration).to.equal(2); | ||
| expect(createInstance({ startTime: 4, duration: 6 }).duration).to.equal(4); | ||
| }); | ||
| }); | ||
|
|
||
| describe('#currentTime', function () { | ||
| it('wraps an assigned time immediately', function () { | ||
| const instance = createInstance({ duration: 3 }); | ||
|
|
||
| instance.currentTime = 5; | ||
|
|
||
| expect(instance.currentTime).to.equal(2); | ||
| }); | ||
|
|
||
| it('uses the clamped duration when tracking playback', function () { | ||
| const manager = createManager(); | ||
| const instance = createInstance({ duration: 6 }, manager); | ||
|
|
||
| instance.play(); | ||
| manager.context.currentTime = 3; | ||
|
|
||
| expect(instance.currentTime).to.equal(3); | ||
|
|
||
| instance.stop(); | ||
| }); | ||
|
|
||
| it('tracks playback relative to startTime', function () { | ||
| const manager = createManager(); | ||
| const instance = createInstance({ startTime: 1, duration: 3, loop: true }, manager); | ||
|
|
||
| instance.play(); | ||
| expect(manager.sources[0].startArgs).to.deep.equal([0, 1]); | ||
| expect(instance.currentTime).to.equal(0); | ||
|
|
||
| manager.context.currentTime = 2; | ||
| expect(instance.currentTime).to.equal(2); | ||
|
|
||
| manager.context.currentTime = 3.5; | ||
| expect(instance.currentTime).to.equal(0.5); | ||
|
|
||
| instance.stop(); | ||
| }); | ||
|
|
||
| it('seeks relative to startTime and limits the remaining playback duration', function () { | ||
| const manager = createManager(); | ||
| const instance = createInstance({ startTime: 1, duration: 3 }, manager); | ||
|
|
||
| instance.currentTime = 2; | ||
| instance.play(); | ||
|
|
||
| expect(manager.sources[0].startArgs).to.deep.equal([0, 3, 1]); | ||
| expect(instance.currentTime).to.equal(2); | ||
|
|
||
| instance.stop(); | ||
| }); | ||
|
|
||
| it('resumes the paused position relative to startTime', function () { | ||
| const manager = createManager(); | ||
| const instance = createInstance({ startTime: 1, duration: 2 }, manager); | ||
|
|
||
| instance.play(); | ||
| expect(manager.sources[0].startArgs).to.deep.equal([0, 1, 2]); | ||
|
|
||
| manager.context.currentTime = 0.75; | ||
| instance.pause(); | ||
| expect(instance.currentTime).to.equal(0.75); | ||
|
|
||
| instance.resume(); | ||
|
|
||
| expect(manager.sources[1].startArgs).to.deep.equal([0, 1.75, 1.25]); | ||
| expect(instance.currentTime).to.equal(0.75); | ||
|
|
||
| instance.stop(); | ||
| }); | ||
|
|
||
| it('resumes an assigned time relative to startTime', function () { | ||
| const manager = createManager(); | ||
| const instance = createInstance({ startTime: 1, duration: 2 }, manager); | ||
|
|
||
| instance.play(); | ||
| manager.context.currentTime = 0.75; | ||
| instance.pause(); | ||
|
|
||
| instance.currentTime = 1.5; | ||
| instance.resume(); | ||
|
|
||
| expect(manager.sources[1].startArgs).to.deep.equal([0, 2.5, 0.5]); | ||
| expect(instance.currentTime).to.equal(1.5); | ||
|
|
||
| instance.stop(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This inlines the same rule
SoundInstance#durationexpresses ascapTime(this._startTime, soundDuration)— presumably becausecapTimeis module-private toinstance.js. Two encodings of one rule, and the|| 0is load-bearing in both: it converts theNaNfromx % 0when the asset isn't loaded, and collapses thestartTime === durationcase to0.The two getters are now required to agree (that's the invariant this PR establishes), so the duplication is the kind that drifts quietly — a future tweak to one wouldn't fail any test that compares them, since none does. Exporting
capTimefrominstance.js(or lifting it somewhere shared) and calling it from both would make them structurally impossible to diverge.