use maximum instead of current delay for DelayNode tail time

RESOLVED FIXED in Firefox 25

Status

()

Core
Web Audio
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: karlt, Assigned: karlt)

Tracking

Trunk
mozilla27
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox24 unaffected, firefox25 fixed, firefox26 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 811439 [details] [diff] [review]
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.
Attachment #811439 - Flags: review?(ehsan)

Comment 2

5 years ago
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+
(Assignee)

Comment 3

5 years ago
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+
(Assignee)

Comment 4

5 years ago
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?

Comment 5

5 years ago
https://hg.mozilla.org/mozilla-central/rev/3a8400387d03
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27

Updated

5 years ago
Attachment #811439 - Flags: approval-mozilla-beta?
Attachment #811439 - Flags: approval-mozilla-beta+
Attachment #811439 - Flags: approval-mozilla-aurora?
Attachment #811439 - Flags: approval-mozilla-aurora+

Comment 6

5 years ago
(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!
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/1405a08b51e2
https://hg.mozilla.org/releases/mozilla-beta/rev/12f66793d3c1
status-firefox24: --- → unaffected
status-firefox25: --- → fixed
status-firefox26: --- → fixed
You need to log in before you can comment on or make changes to this bug.