Closed
Bug 1257718
Opened 9 years ago
Closed 9 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•9 years ago
|
Whiteboard: Web Audio API
Assignee | ||
Comment 1•9 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•9 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•9 years ago
|
Rank: 10
Priority: -- → P1
Reporter | ||
Comment 3•9 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•9 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•9 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•9 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•9 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•9 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•9 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•9 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•9 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•9 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•9 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•9 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•9 years ago
|
Assignee: amarchesini → karlt
Flags: needinfo?(amarchesini)
Comment 16•9 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•9 years ago
|
Attachment #8764111 -
Flags: review?(padenot) → review+
Comment 17•9 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•9 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•9 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•9 years ago
|
Attachment #8764114 -
Flags: review?(padenot) → review+
Comment 20•9 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•9 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•9 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•9 years ago
|
Attachment #8764116 -
Flags: review?(padenot) → review+
Comment 23•9 years ago
|
||
Comment on attachment 8764116 [details]
test for bug 1257718
https://reviewboard.mozilla.org/r/60170/#review57248
Comment 24•9 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•9 years ago
|
||
Comment 26•9 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•9 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: 9 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 28•9 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•9 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•9 years ago
|
Flags: needinfo?(padenot)
You need to log in
before you can comment on or make changes to this bug.
Description
•