921675 - use maximum instead of current delay for DelayNode tail time

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Karl Tomlinson (:karlt)]

Delay can increase faster than time passes.

"When the delay time is changed, the implementation must make the transition
smoothly, without introducing noticeable clicks or glitches to the audio
stream", so I wonder whether we shouldn't let delay increase faster than time passes.

However, the a-rate delayTime parameter "must be sampled for each sample-frame of the block", and we give most control to the client if we take time-varying parameters exactly, as in the current implementation.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: patch

Keeping the current delay implementation here, which seems consistent with Blink's, but making the lifetime match up with the implementation.

This patch is also necessary for the one I'm about to attach to bug 910174.

Comment 2

[(no longer active)]

Comment on attachment 811439 [details] [diff] [review]
patch

Review of attachment 811439 [details] [diff] [review]:
-----------------------------------------------------------------

This test may be a bit flaky, I guess we'll know soon when this lands.

Comment 3

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a8400387d03

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> This test may be a bit flaky, I guess we'll know soon when this lands.

While the test relied on some timing to fail, I expect it to always pass if the code is correct.  If you see invalid assumptions in the test, please let me know and I'll see if I can correct them.

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 811439 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new feature: Web Audio DelayNode
User impact if declined: 
This is needed to fix Bug 921457 and Bug 910174.
Please see Bug 910174 comment 5 for reasons, and the approval request in bug 910174 for impact.

Testing completed (on m-c, etc.): on m-i, in testsuite
Risk to taking this patch (and alternatives if risky):
The patch only modifies behavior of Web Audio's DelayNode so risk is confined to that.

String or IDL/UUID changes made by this patch: none

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3a8400387d03

Comment 6

[(no longer active)]

(In reply to comment #3)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3a8400387d03
> 
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> > This test may be a bit flaky, I guess we'll know soon when this lands.
> 
> While the test relied on some timing to fail, I expect it to always pass if the
> code is correct.  If you see invalid assumptions in the test, please let me
> know and I'll see if I can correct them.

No, I don't have any specific concerns, and you're right, it is the failure path that may be timing-flaky, so hopefully that's alright!

Comment 7

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1405a08b51e2
https://hg.mozilla.org/releases/mozilla-beta/rev/12f66793d3c1