Closed
Bug 1257718
Opened 8 years ago
Closed 8 years ago
linearRampToValueAtTime not ramping to correct value
Categories
(Core :: Web Audio, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla50
People
(Reporter: yotammann, Assigned: karlt)
References
()
Details
(Keywords: regression, testcase)
Attachments
(9 files)
58 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jgraham
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
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.
Reporter | ||
Updated•8 years ago
|
Whiteboard: Web Audio API
Assignee | ||
Comment 1•8 years ago
|
||
Thanks for the nice testcase. https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=946ac85af8f4&tochange=0b202671c9e2 Suspect bug 1140448.
Blocks: 1140448
Status: UNCONFIRMED → NEW
status-firefox45:
--- → affected
Ever confirmed: true
Keywords: regression
Comment 2•8 years ago
|
||
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)
Updated•8 years ago
|
Rank: 10
Priority: -- → P1
Reporter | ||
Comment 3•8 years ago
|
||
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.
Comment 4•8 years ago
|
||
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.
Comment 5•8 years ago
|
||
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
Assignee | ||
Comment 6•8 years ago
|
||
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)
Assignee | ||
Comment 7•8 years ago
|
||
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/
Assignee | ||
Comment 8•8 years ago
|
||
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/
Assignee | ||
Comment 9•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/60164/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60164/
Assignee | ||
Comment 10•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/60166/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60166/
Assignee | ||
Comment 11•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/60168/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60168/
Assignee | ||
Comment 12•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/60170/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60170/
Assignee | ||
Comment 13•8 years ago
|
||
|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 | ||
Comment 14•8 years ago
|
||
with the new variable matching the loop exit status of interest. Review commit: https://reviewboard.mozilla.org/r/60174/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60174/
Assignee | ||
Updated•8 years ago
|
Assignee: amarchesini → karlt
Flags: needinfo?(amarchesini)
Comment 16•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8764111 -
Flags: review?(padenot) → review+
Comment 17•8 years ago
|
||
Comment on attachment 8764111 [details] bug 1257718 don't export implementation of complex timeline methods https://reviewboard.mozilla.org/r/60160/#review57234
Comment 18•8 years ago
|
||
Comment on attachment 8764112 [details] bug 1257718 rename lastEventId to eventIndex https://reviewboard.mozilla.org/r/60162/#review57236
Attachment #8764112 -
Flags: review?(padenot) → review+
Comment 19•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8764114 -
Flags: review?(padenot) → review+
Comment 20•8 years ago
|
||
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
Comment 21•8 years ago
|
||
Comment on attachment 8764117 [details] bug 1257718 look for new events as time advances https://reviewboard.mozilla.org/r/60172/#review57244
Attachment #8764117 -
Flags: review?(padenot) → review+
Comment 22•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8764116 -
Flags: review?(padenot) → review+
Comment 23•8 years ago
|
||
Comment on attachment 8764116 [details] test for bug 1257718 https://reviewboard.mozilla.org/r/60170/#review57248
Comment 24•8 years ago
|
||
Comment on attachment 8764115 [details] bug 1257718 wpt manifest update noise https://reviewboard.mozilla.org/r/60168/#review57058
Attachment #8764115 -
Flags: review?(james) → review+
Assignee | ||
Comment 25•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bf3b8acf341
Comment 26•8 years ago
|
||
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
Comment 27•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d49eededc392 https://hg.mozilla.org/mozilla-central/rev/b8990d2e2c4b https://hg.mozilla.org/mozilla-central/rev/ace1a6bc5af4 https://hg.mozilla.org/mozilla-central/rev/185595dfe3d3 https://hg.mozilla.org/mozilla-central/rev/623150d380e0 https://hg.mozilla.org/mozilla-central/rev/57029fe80210 https://hg.mozilla.org/mozilla-central/rev/48d91469d4bc https://hg.mozilla.org/mozilla-central/rev/96600441f797
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 28•8 years ago
|
||
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)
Assignee | ||
Comment 29•8 years ago
|
||
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)
Updated•8 years ago
|
Flags: needinfo?(padenot)
You need to log in
before you can comment on or make changes to this bug.
Description
•