Closed Bug 921675 Opened 6 years ago Closed 6 years ago

use maximum instead of current delay for DelayNode tail time

Categories

(Core :: Web Audio, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox24 --- unaffected
firefox25 --- fixed
firefox26 --- fixed

People

(Reporter: karlt, Assigned: karlt)

References

Details

Attachments

(1 file)

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.
Attached patch patchSplinter Review
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.
Attachment #811439 - Flags: review?(ehsan)
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.
Attachment #811439 - Flags: review?(ehsan) → review+
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.
Flags: in-testsuite+
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
Attachment #811439 - Flags: approval-mozilla-beta?
Attachment #811439 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/3a8400387d03
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Attachment #811439 - Flags: approval-mozilla-beta?
Attachment #811439 - Flags: approval-mozilla-beta+
Attachment #811439 - Flags: approval-mozilla-aurora?
Attachment #811439 - Flags: approval-mozilla-aurora+
(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!
You need to log in before you can comment on or make changes to this bug.