Rework definition of ABSN output behavior for rate-adjusted duration param #1681

joeberkovitz · 2018-06-21T18:41:05Z

Fixes #1660

Preview | Diff

…’s interaction with the start(…duration) argument.

karlt · 2018-06-21T21:03:00Z

LGTM, thank you.

rtoy

Can you not check in index.html? People doing this generally cause merge issues in index.html that are hard to resolve.

Algorithm looks good and I think it does what we want. The text part confuses me, though.

rtoy · 2018-06-22T17:18:47Z

index.bs

 			when: The <code>when</code> parameter describes at what time (in seconds) the sound should start playing. It is in the same time coordinate system as the {{AudioContext}}'s {{BaseAudioContext/currentTime}} attribute. If 0 is passed in for this value or if the value is less than <b>currentTime</b>, then the sound will start playing immediately. <span class="synchronous">A {{RangeError}} exception MUST be thrown if <code>when</code> is negative.</span>
 			offset: The <code>offset</code> parameter supplies a <a>playhead position</a> where playback will begin. If 0 is passed in for this value, then playback will start from the beginning of the buffer. <span class="synchronous">A {{RangeError}} exception MUST be thrown if <code>offset</code> is negative.</span> If <code>offset</code> is greater than {{AudioBufferSourceNode/loopEnd}}, playback will begin at {{AudioBufferSourceNode/loopEnd}} (and immediately loop to {{AudioBufferSourceNode/loopStart}}). <code>offset</code> is silently clamped to [0, <code>duration</code>], when <code>startTime</code> is reached, where <code>duration</code> is the value of the <code>duration</code> attribute of the {{AudioBuffer}} set to the {{AudioBufferSourceNode/buffer}} attribute of this <code>AudioBufferSourceNode</code>.
-			duration: The {{AudioBufferSourceNode/start(when, offset, duration)/duration}} parameter describes the duration of the sound (in seconds) to be played. If this parameter is passed, this method has exactly the same effect as the invocation of <code>start(when, offset)</code> followed by <code>stop(when + duration)</code>. <span class="synchronous">A {{RangeError}} exception MUST be thrown if <code>duration</code> is negative.</span>
+			duration: The {{AudioBufferSourceNode/start(when, offset, duration)/duration}} parameter describes the duration of sound to be played, expressed as seconds of total buffer content to be output, including any whole or partial loop iterations. The units of {{AudioBufferSourceNode/start(when, offset, duration)/duration}} are independent of the effects of {{AudioBufferSourceNode/playbackRate}}. This behavior contrasts with the absolute time units employed by {{AudioScheduledSourceNode/stop()}}. Thus, a {{AudioBufferSourceNode/start(when, offset, duration)/duration}} of 5 seconds with a playback rate of 0.5 will output 5 seconds of buffer content at half speed, producing 10 seconds of audible output. <span class="synchronous">A {{RangeError}} exception MUST be thrown if <code>duration</code> is negative.</span>


I don't think we need to say anything about stop().

I'm not really sure how much to say here actually about what happens. It's probably best to point to the algorithm. I think what you're saying here is correct and is understandable if we're not looping, but if we are, it's much harder to say clearly in words what happens.

rtoy · 2018-06-22T17:22:10Z

index.bs

 	offset = pos;
 	if (arguments.length >= 3) {
-		stop = when + duration;
+		duration = dur;


Need to define duration?

The duration variable is already defined, on line 4819 (same place as offset).

rtoy · 2018-06-27T22:09:08Z

index.bs

 			when: The <code>when</code> parameter describes at what time (in seconds) the sound should start playing. It is in the same time coordinate system as the {{AudioContext}}'s {{BaseAudioContext/currentTime}} attribute. If 0 is passed in for this value or if the value is less than <b>currentTime</b>, then the sound will start playing immediately. <span class="synchronous">A {{RangeError}} exception MUST be thrown if <code>when</code> is negative.</span>
 			offset: The <code>offset</code> parameter supplies a <a>playhead position</a> where playback will begin. If 0 is passed in for this value, then playback will start from the beginning of the buffer. <span class="synchronous">A {{RangeError}} exception MUST be thrown if <code>offset</code> is negative.</span> If <code>offset</code> is greater than {{AudioBufferSourceNode/loopEnd}}, playback will begin at {{AudioBufferSourceNode/loopEnd}} (and immediately loop to {{AudioBufferSourceNode/loopStart}}). <code>offset</code> is silently clamped to [0, <code>duration</code>], when <code>startTime</code> is reached, where <code>duration</code> is the value of the <code>duration</code> attribute of the {{AudioBuffer}} set to the {{AudioBufferSourceNode/buffer}} attribute of this <code>AudioBufferSourceNode</code>.
-			duration: The {{AudioBufferSourceNode/start(when, offset, duration)/duration}} parameter describes the duration of the sound (in seconds) to be played. If this parameter is passed, this method has exactly the same effect as the invocation of <code>start(when, offset)</code> followed by <code>stop(when + duration)</code>. <span class="synchronous">A {{RangeError}} exception MUST be thrown if <code>duration</code> is negative.</span>
+			duration: The {{AudioBufferSourceNode/start(when, offset, duration)/duration}} parameter describes the duration of sound to be played, expressed as seconds of total buffer content to be output, including any whole or partial loop iterations. The units of {{AudioBufferSourceNode/start(when, offset, duration)/duration}} are independent of the effects of {{AudioBufferSourceNode/playbackRate}}. For example, a {{AudioBufferSourceNode/start(when, offset, duration)/duration}} of 5 seconds with a playback rate of 0.5 will output 5 seconds of buffer content at half speed, producing 10 seconds of audible output. <span class="synchronous">A {{RangeError}} exception MUST be thrown if <code>duration</code> is negative.</span>


I think (as mentioned previously), we shouldn't really try to say in words what supposed to happen. Can we just refer to the algorithm?

I feel strongly that here (as in many other places in the spec) it's valuable to have a short textual description of what a thing is, as a counterpoint to the algorithm.

I'm only concerned if it's inconsistent with the fine details of the algorithm. A non-normative note is acceptable to me.

I think it's consistent -- if you would like to rephrase it somehow, please propose (or commit) some alternate wording.

As stated, I think a non-normative note would be fine. And I think the duration here also applies to looping. The looping playback will play for duration secs, taking into account the playback rate.

rtoy · 2018-06-27T22:12:58Z

index.bs

-		// Check that currentTime is within allowable range for playback
-		if (currentTime &lt; start || currentTime >= stop) {
+		// Check that currentTime and bufferDuration are within allowable range for playback
+		if (currentTime &lt; start || currentTime >= stop || bufferDuration >= duration) {


I think this is wrong? Let duration = 129/sampleRate and let the playbackRate = 1. Since bufferDuration is initialized to 0 in process(), and framesToProcess is 128, bufferDuration is at most 128/sampleRate. So bufferDuration >= duration is never true. This means the following lines produce an output. I think that's wrong. Maybe this should be bufferTime instead?

Perhaps bufferDuration was intended to be declared outside the function?

Thanks @karlt for catching my error here. Now fixed.

rtoy

Pretty close; just a few more comments.

rtoy · 2018-07-10T18:08:31Z

index.bs

 	offset = pos;
 	if (arguments.length >= 3) {
-		stop = when + duration;
+		duration = dur;


rtoy · 2018-07-10T18:11:15Z

index.bs

 		if (!started) {
 			// Take note that buffer has started playing and get initial playhead position.
-			bufferTime = offset + ((currentTime - start) * computedPlaybackRate);
+			bufferTime = offset + bufferDuration;


Is this right? The initial value for bufferTime is now different between this version and the old version. I think the original is the right value.

You're correct about the inconsistency. But another way to go would be to initialize bufferDuration to (currentTime - start) * computedPlaybackRate in this clause. Wouldn't that be an even better fix? It would cause bufferDuration to respect fractional-frame start times.

I noticed this change too, but thought it was fixing a bug.

For the when parameter, "If 0 is passed in for this value or if the value is
less than currentTime, then the sound will start playing immediately." Also,
"The offset parameter supplies a playhead position where playback will begin."
I expect playback to begin with the head at offset regardless of when.

I couldn't make sense of ((currentTime - start) * computedPlaybackRate).
It seems to be aiming to make playback start with the head at some point
after offset if when is less than currentTime.

(The spec text is helpful to indicate the intention of the algorithm.)

Yeah, I didn't look to see what the original is doing, but I agree with @karlt: playback should start at offset independent of when. I think bufferDuration should actually start at 0. If you choose any other value, then the actual playback duration will be less then the duration arg, even if playback rate is 1.

bufferDuration starts at 0. Simply initializing bufferTime to offset would be more clear.

@rtoy could you please change this as per @karlt's last suggestion? I will not be able to work on this for a couple of weeks and this PR is close to done.

rtoy · 2018-07-10T18:21:47Z

index.bs


 // Variables for tracking node's playback state
 let bufferTime = 0, started = false, enteredLoop = false;
+let bufferDuration = 0;


Can we rename this? Maybe playbackDuration or something? I find it confusing because there's duration, buffer.duration, and now bufferDuration.

Actually, I am now remembering that I called it bufferDuration because it is duration in terms of the buffer frame rate, not the audible playback duration.

So, what should it be? I think that it's now really the audible playback duration, right?

This PR is changing duration from a measurement in the audible timeline to a measurement across the buffer. i.e. the same units as offset.

Now I'm thoroughly confused. I thought if start(when, offset,1) were called and the playback rate is 1/10, the audio would be heard for 10 sec and not 1 sec. (Assuming there's at least 1 sec of audio in the buffer, or we're looping.)

Yes, for hearing the 10s. playback would end when bufferDuration is 1.

Perhaps bufferTimeElapsed?

@rtoy we agree on the meaning. I'll let you modify the branch as heading out of touch for 2 weeks.

@joeberkovitz I'll take over this PR. Thanks for your work on this and see in a few weeks.

I think there's just a couple of minor things that need to be fixed:

Rename bufferDuration

Set bufferTime probably to just offset.

Renamed bufferDuration and initialized bufferTime to offset.

rtoy · 2018-07-10T18:23:42Z

index.bs

 			when: The <code>when</code> parameter describes at what time (in seconds) the sound should start playing. It is in the same time coordinate system as the {{AudioContext}}'s {{BaseAudioContext/currentTime}} attribute. If 0 is passed in for this value or if the value is less than <b>currentTime</b>, then the sound will start playing immediately. <span class="synchronous">A {{RangeError}} exception MUST be thrown if <code>when</code> is negative.</span>
 			offset: The <code>offset</code> parameter supplies a <a>playhead position</a> where playback will begin. If 0 is passed in for this value, then playback will start from the beginning of the buffer. <span class="synchronous">A {{RangeError}} exception MUST be thrown if <code>offset</code> is negative.</span> If <code>offset</code> is greater than {{AudioBufferSourceNode/loopEnd}}, playback will begin at {{AudioBufferSourceNode/loopEnd}} (and immediately loop to {{AudioBufferSourceNode/loopStart}}). <code>offset</code> is silently clamped to [0, <code>duration</code>], when <code>startTime</code> is reached, where <code>duration</code> is the value of the <code>duration</code> attribute of the {{AudioBuffer}} set to the {{AudioBufferSourceNode/buffer}} attribute of this <code>AudioBufferSourceNode</code>.
-			duration: The {{AudioBufferSourceNode/start(when, offset, duration)/duration}} parameter describes the duration of the sound (in seconds) to be played. If this parameter is passed, this method has exactly the same effect as the invocation of <code>start(when, offset)</code> followed by <code>stop(when + duration)</code>. <span class="synchronous">A {{RangeError}} exception MUST be thrown if <code>duration</code> is negative.</span>
+			duration: The {{AudioBufferSourceNode/start(when, offset, duration)/duration}} parameter describes the duration of sound to be played, expressed as seconds of total buffer content to be output, including any whole or partial loop iterations. The units of {{AudioBufferSourceNode/start(when, offset, duration)/duration}} are independent of the effects of {{AudioBufferSourceNode/playbackRate}}. For example, a {{AudioBufferSourceNode/start(when, offset, duration)/duration}} of 5 seconds with a playback rate of 0.5 will output 5 seconds of buffer content at half speed, producing 10 seconds of audible output. <span class="synchronous">A {{RangeError}} exception MUST be thrown if <code>duration</code> is negative.</span>


As stated, I think a non-normative note would be fine. And I think the duration here also applies to looping. The looping playback will play for duration secs, taking into account the playback rate.

joeberkovitz · 2018-07-10T18:42:26Z

What was the "Ack" comment about above - can you clarify? We need a temporary variable name for the value that gets stored in duration. How would you like this to look?

rtoy · 2018-07-10T21:47:49Z

By "Ack", I meant that you're right and duration is already defined; I just missed it.

* Rename `bufferDuration` to `bufferTimeElapsed` * Initialize `bufferTime` to `offset`.

rtoy · 2018-07-12T19:52:28Z

Marked as non substantive for IPR from ash-nazg.

rtoy · 2018-07-12T19:54:15Z

@joeberkovitz is an invited expert and former chair of the working group. The changes here are minor tweaks to the existing algorithm that @joeberkovitz actually wrote.

Since WebAudio/web-audio-api#1681 (in 2018) the `duration` argument is specced [1] as being the duration of the buffer's content to be played, and not necessarily the one of the AudioContext's clock. So if the source node has a `playbackRate` that is not `1`, using this parameter will not be the same as using `stop(duration)`. I based the wordings on the specs text, but I don't mind another formulation altogether. Might also be noted that there is an interop issue here, where on Firefox does follow the specs. I'll open issues to get this sorted out though. [1]: https://webaudio.github.io/web-audio-api/#dom-audiobuffersourcenode-start-when-offset-duration-duration

Rework definition of ABSN output behavior to account for playbackRate…

8411937

…’s interaction with the start(…duration) argument.

rtoy reviewed Jun 22, 2018

View reviewed changes

joeberkovitz added 3 commits June 27, 2018 16:15

Merge branch 'gh-pages' into 1660-duration-definition

77670a7

Remove mention of stop() in definition of duration parameter.

ff4a0fe

Regenerate index.html

5bee8b4

rtoy reviewed Jun 27, 2018

View reviewed changes

Initialize bufferDuration outside of process() in ABSN algorithm.

992eda2

rtoy reviewed Jul 10, 2018

View reviewed changes

Address review comments

a57fe08

* Rename `bufferDuration` to `bufferTimeElapsed` * Initialize `bufferTime` to `offset`.

rtoy approved these changes Jul 12, 2018

View reviewed changes

rtoy merged commit 827f8ba into gh-pages Jul 12, 2018

Kaiido mentioned this pull request Dec 8, 2025

Fix ABSN.start()'s 'duration' parameter description mdn/content#42282

Open

Rework definition of ABSN output behavior for rate-adjusted duration param #1681

Rework definition of ABSN output behavior for rate-adjusted duration param #1681

Uh oh!

Conversation

joeberkovitz commented Jun 21, 2018 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karlt commented Jun 21, 2018

Uh oh!

rtoy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtoy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joeberkovitz commented Jul 10, 2018

Uh oh!

rtoy commented Jul 10, 2018

Uh oh!

rtoy commented Jul 12, 2018

Uh oh!

rtoy commented Jul 12, 2018

joeberkovitz commented Jun 21, 2018 •

edited by pr-preview bot

Loading