User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/45.0.2454.101 Safari/537.36 Steps to reproduce: Run http://jsfiddle.net/rsqbh0qc/2/ Script reproduced here: ================== var ctx = new AudioContext(); // Create a sound source var osc = ctx.createOscillator(); osc.frequency.value = 440; osc.start(ctx.currentTime + 1); osc.stop(ctx.currentTime + 3); // Create a gain node whose gain AudioParam will be automated var gain = ctx.createGain(); // UNCOMMENT THIS LINE TO WORK AROUND FF 41 BUG // gain.gain.value = 0.1; // Set a starting value into the automation right now gain.gain.setValueAtTime(0.1, ctx.currentTime); // ramp towards a zero target value 1.5s from now w/ time constant // In FF 41+, this causes an immediate spike to gain=1 at start of ramp, instead of 0.1 gain.gain.setTargetAtTime(0, ctx.currentTime + 1.5, 0.2); osc.connect(gain); gain.connect(ctx.destination); Actual results: A loud audible spike occurs at the point (currentTime + 1.5) when the oscillator tone should begin to decay in volume. Expected results: The oscillator tone should decay gradually with no spike, because the setTargetAtTime() call should use the previously established automation value set by the prior call to setValueAtTime().

This is likely to be fixed by the rewrite of bug 1184057, but I'm leaving this one open to make sure, and turn Joe's code into a test case.

The bug does not occur when the event time aligns with a block boundary.

Sorry, there is a race in the testcase, so that sometimes the document does not yet have a body when the canvas is added. Reloading seems work around the problem.

P1 because the spec recommends setTargetAtTime to perform smooth transitions but it is not doing that.

2015-04-22 946ac85af8f4 good 2015-04-23 0b202671c9e2 bad % hg log -r 'ancestors(0b202671c9e2)-ancestors(946ac85af8f4)' dom/media/webaudio changeset: 240463:29bf41172acd user: Andrea Marchesini <amarchesini@mozilla.com> date: Wed Apr 22 08:29:17 2015 +0200 summary: Bug 1156632 - Remove unused forward class declarations - patch 2 - dom/media, dom/indexedDB, dom/svg, r=ehsan changeset: 240470:65050168e0b7 user: Andrea Marchesini <amarchesini@mozilla.com> date: Wed Apr 22 08:53:33 2015 +0200 summary: Bug 1140448 - Improving the performances of how AudioEventTimeline calculates values, r=padenot

Any ideas what has happened here?

This patch mainly reverts what I pushed for bug 1140448.

IIRC, bug 1140448 gave us a significant performance improvement. Do you remember otherwise? If we back out that, then we'll just have another regression to fix. Is there an intrinsic reason why that approach cannot work, or do you think there is just a small logic error somewhere that we haven't found? It may be better use of time to find the logic error, than to review the backout.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3adaf610a92 I'm testing this bug with this patch.

Comment on attachment 8728487 [details] [diff] [review] revert.patch (In reply to Andrea Marchesini (:baku) from comment #8) > This patch mainly reverts what I pushed for bug 1140448. It's helpful if authors can explain to reviewers what is changing in the patch instead of expecting the reviewer to work our whether, for example, this is a partial revert or does something in addition to reverting the patch. The "mainly" here implies at least one of these but I don't know which. Similarly, commit messages should describe what the patch changes. There was more than one change that landed for bug 1140448, but none are documented in the commit message. Preferably separate changes would land separately, but if they need to land in a single patch, then they should all be documented in the commit message. Without documentation, there is little clue re how "Improving the performances of how AudioEventTimeline calculates values" may have changed the behavior. http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html This regression highlights that the existing test coverage is poor. If it were better, then 65050168e0b7 wouldn't have landed with the broken event selection logic. However, multiple things landed in one patch in bug 1140448, and I don't want to back them all out. Moving to a model of getting a block of values at a time, and discarding old events were both nice improvements. I think it is worth fixing the code for these improvements, and I'd prefer to fix the event selection logic than to review a revert to previous code (which was already very buggy) and try to work out what other changes may be included in this patch.

Review commit: https://reviewboard.mozilla.org/r/60178/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60178/

from mLastComputedValue. Review commit: https://reviewboard.mozilla.org/r/60180/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60180/

The start of the curve should actually be obtained from the zeroth tick in the setTarget curve, but the patch here starts the curve one tick prior, as was done before this regression from bug 1140448. Fixing that is related to fixing bug 1281378, bug 1281382 and bug 1228207.

Comment on attachment 8764120 [details] test for bug 1213313 https://reviewboard.mozilla.org/r/60178/#review57250

Comment on attachment 8764121 [details] bug 1213313 set mComputedValue for each tick so that SetTarget values are calculated correctly https://reviewboard.mozilla.org/r/60180/#review57252

Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0a9fbd145aeb test for bug 1213313 r=padenot https://hg.mozilla.org/integration/mozilla-inbound/rev/c12837f42f7e set mComputedValue for each tick so that SetTarget values are calculated correctly r=padenot

https://hg.mozilla.org/mozilla-central/rev/0a9fbd145aeb https://hg.mozilla.org/mozilla-central/rev/c12837f42f7e

Karl, Paul -- Does it make sense to ask for uplift on this to Fx 49 (Aurora)?

I'm not aware of an urgent need for this with large scale impact, and so my feeling is that this is not the kind of bug that needs an uplift at the end of the aurora track, beginning of beta track.