Closed Bug 1257718 Opened 9 years ago Closed 9 years ago

linearRampToValueAtTime not ramping to correct value

Categories

(Core :: Web Audio, defect, P1)

44 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox45 --- affected
firefox50 --- fixed

People

(Reporter: yotammann, Assigned: karlt)

References

()

Details

(Keywords: regression, testcase)

Attachments

(9 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.116 Safari/537.36 Steps to reproduce: 1. schedule a setValueAtTime(0, 0) on the gain value of a GainNode to set an anchor 2. schedule a linearRampToValueAtTime(1, 0.1) Actual results: The value of the gain node went above 1 and continued ramping beyond 0.1 seconds Here is a jsfiddle which illustrates: https://jsfiddle.net/yotammann/o8f24qjh/1/ Works correctly on Chrome and Safari. Expected results: The gain AudioParam should ramp until 1 and stop ramping and should arrive at 1 at exactly 0.1 seconds.
Whiteboard: Web Audio API
Component: Untriaged → Web Audio
Keywords: testcase
Product: Firefox → Core
Whiteboard: Web Audio API
Blocks: 1140448
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Hi Andrea, Karl suspects this is a regression caused by bug 1140448, can you take a look? If it is caused by that bug, can you take this bug and put up a fix? Thanks!
Flags: needinfo?(amarchesini)
Rank: 10
Priority: -- → P1
Maybe this is helpful in debugging: I noticed that if the time argument was block-aligned (128 samples), it would work correctly. You can test this in the jsfiddle if you swap out `attackTime` for `blockAlignedTime` as the argument to linearRampToValueAtTime.
Just a quick update: baku has a patch to revert part of bug 1140448 with a small test. There's a reasonable chance that landing that patch will fix this bug.
baku -- I'm assigning this to you under the assumption that your existing patch (ref: comment 4) will fix this bug. If it doesn't, I'll then find a different owner (unless you want to continue owning this). Thanks!
Assignee: nobody → amarchesini
The comment was not necessarily true where it was previously positioned. Review commit: https://reviewboard.mozilla.org/r/60158/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60158/
Attachment #8764110 - Flags: review?(padenot)
Attachment #8764111 - Flags: review?(padenot)
Attachment #8764112 - Flags: review?(padenot)
Attachment #8764113 - Flags: review?(padenot)
Attachment #8764114 - Flags: review?(padenot)
Attachment #8764115 - Flags: review?(james)
Attachment #8764116 - Flags: review?(padenot)
Attachment #8764117 - Flags: review?(padenot)
Attachment #8764118 - Flags: review?(padenot)
This limits recompilation required when modifying the methods, and makes the public interface easier to read. Review commit: https://reviewboard.mozilla.org/r/60160/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60160/
This is not necessarily related to the last event and it is not the previous event. Review commit: https://reviewboard.mozilla.org/r/60162/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60162/
|bailout| is reset for each aTime, so that the appropriate events for that time can be found. The |eventIndex| loop is adjusted so that, when it is re-entered, it keeps the current set of events if they are appropriate (instead of advancing every time it is entered). |previous| and |next| are now advanced even when passing the last event, removing the special case when past all events. Review commit: https://reviewboard.mozilla.org/r/60172/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60172/
Assignee: amarchesini → karlt
Flags: needinfo?(amarchesini)
Blocks: 1213313
Comment on attachment 8764110 [details] bug 1257718 move comment to within the code path it describes https://reviewboard.mozilla.org/r/60158/#review57232
Attachment #8764110 - Flags: review?(padenot) → review+
Attachment #8764111 - Flags: review?(padenot) → review+
Comment on attachment 8764111 [details] bug 1257718 don't export implementation of complex timeline methods https://reviewboard.mozilla.org/r/60160/#review57234
Attachment #8764112 - Flags: review?(padenot) → review+
Comment on attachment 8764113 [details] bug 1257718 introduce function-scope TimeOf() to simplify templated event time getter calls https://reviewboard.mozilla.org/r/60164/#review57240
Attachment #8764113 - Flags: review?(padenot) → review+
Attachment #8764114 - Flags: review?(padenot) → review+
Comment on attachment 8764114 [details] bug 1257718 use is() for comparison with more info on failure than ok() https://reviewboard.mozilla.org/r/60166/#review57242
Attachment #8764117 - Flags: review?(padenot) → review+
Comment on attachment 8764118 [details] bug 1257718 replace bailOut variable with more descriptive timeMatchesEventIndex https://reviewboard.mozilla.org/r/60174/#review57246
Attachment #8764118 - Flags: review?(padenot) → review+
Attachment #8764116 - Flags: review?(padenot) → review+
Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d49eededc392 move comment to within the code path it describes r=padenot https://hg.mozilla.org/integration/mozilla-inbound/rev/b8990d2e2c4b don't export implementation of complex timeline methods r=padenot https://hg.mozilla.org/integration/mozilla-inbound/rev/ace1a6bc5af4 rename lastEventId to eventIndex r=padenot https://hg.mozilla.org/integration/mozilla-inbound/rev/185595dfe3d3 introduce function-scope TimeOf() to simplify templated event time getter calls r=padenot https://hg.mozilla.org/integration/mozilla-inbound/rev/623150d380e0 use is() for comparison with more info on failure than ok() r=padenot https://hg.mozilla.org/integration/mozilla-inbound/rev/57029fe80210 test for bug 1257718 r=padenot https://hg.mozilla.org/integration/mozilla-inbound/rev/48d91469d4bc look for new events as time advances r=padenot https://hg.mozilla.org/integration/mozilla-inbound/rev/96600441f797 replace bailOut variable with more descriptive timeMatchesEventIndex r=padenot
Karl, Paul -- Does it make any sense to uplift these patches(or pieces of them) to Fx 49 (our current Dev Edition)? Or is it best for them to ride the Fx 50 train to release?
Flags: needinfo?(padenot)
Flags: needinfo?(karlt)
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.
Flags: needinfo?(karlt)
Flags: needinfo?(padenot)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: