1257718 - linearRampToValueAtTime not ramping to correct value

Status: RESOLVED FIXED

Description

[Yotam Mann]

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.

Comment 1

[Karl Tomlinson (back Jan 29 :karlt)]

Thanks for the nice testcase.

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=946ac85af8f4&tochange=0b202671c9e2
Suspect bug 1140448.

Comment 2

[Maire Reavy [:mreavy] Plz needinfo]

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!

Comment 3

[Yotam Mann]

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

[Maire Reavy [:mreavy] Plz needinfo]

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

[Maire Reavy [:mreavy] Plz needinfo]

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!

Comment 6

[Karl Tomlinson (back Jan 29 :karlt)]

Created attachment 8764110 [details]
bug 1257718 move comment to within the code path it describes

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/

Comment 7

[Karl Tomlinson (back Jan 29 :karlt)]

Created attachment 8764111 [details]
bug 1257718 don't export implementation of complex timeline methods

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/

Comment 8

[Karl Tomlinson (back Jan 29 :karlt)]

Created attachment 8764112 [details]
bug 1257718 rename lastEventId to eventIndex

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/

Comment 9

[Karl Tomlinson (back Jan 29 :karlt)]

Created attachment 8764113 [details]
bug 1257718 introduce function-scope TimeOf() to simplify templated event time getter calls

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

Comment 10

[Karl Tomlinson (back Jan 29 :karlt)]

Created attachment 8764114 [details]
bug 1257718 use is() for comparison with more info on failure than ok()

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

Comment 11

[Karl Tomlinson (back Jan 29 :karlt)]

Created attachment 8764115 [details]
bug 1257718 wpt manifest update noise

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

Comment 12

[Karl Tomlinson (back Jan 29 :karlt)]

Created attachment 8764116 [details]
test for bug 1257718

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

Comment 13

[Karl Tomlinson (back Jan 29 :karlt)]

Created attachment 8764117 [details]
bug 1257718 look for new events as time advances

|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/

Comment 14

[Karl Tomlinson (back Jan 29 :karlt)]

Created attachment 8764118 [details]
bug 1257718 replace bailOut variable with more descriptive timeMatchesEventIndex

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/

Comment 16

[Paul Adenot (:padenot)]

Comment on attachment 8764110 [details]
bug 1257718 move comment to within the code path it describes

https://reviewboard.mozilla.org/r/60158/#review57232

Comment 17

[Paul Adenot (:padenot)]

Comment on attachment 8764111 [details]
bug 1257718 don't export implementation of complex timeline methods

https://reviewboard.mozilla.org/r/60160/#review57234

Comment 18

[Paul Adenot (:padenot)]

Comment on attachment 8764112 [details]
bug 1257718 rename lastEventId to eventIndex

https://reviewboard.mozilla.org/r/60162/#review57236

Comment 19

[Paul Adenot (:padenot)]

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

Comment 20

[Paul Adenot (:padenot)]

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

[Paul Adenot (:padenot)]

Comment on attachment 8764117 [details]
bug 1257718 look for new events as time advances

https://reviewboard.mozilla.org/r/60172/#review57244

Comment 22

[Paul Adenot (:padenot)]

Comment on attachment 8764118 [details]
bug 1257718 replace bailOut variable with more descriptive timeMatchesEventIndex

https://reviewboard.mozilla.org/r/60174/#review57246

Comment 23

[Paul Adenot (:padenot)]

Comment on attachment 8764116 [details]
test for bug 1257718

https://reviewboard.mozilla.org/r/60170/#review57248

Comment 24

[James Graham [:jgraham]]

Comment on attachment 8764115 [details]
bug 1257718 wpt manifest update noise

https://reviewboard.mozilla.org/r/60168/#review57058

Comment 25

[Karl Tomlinson (back Jan 29 :karlt)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bf3b8acf341

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

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

Comment 28

[Maire Reavy [:mreavy] Plz needinfo]

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?

Comment 29

[Karl Tomlinson (back Jan 29 :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.