linearRampToValueAtTime not ramping to correct value

RESOLVED FIXED in Firefox 50

Status

()

Core
Web Audio
P1
normal
Rank:
10
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Yotam Mann, Assigned: karlt)

Tracking

({regression, testcase})

44 Branch
mozilla50
regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 affected, firefox50 fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(9 attachments)

58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
58 bytes, text/x-review-board-request
jgraham
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
(Reporter)

Description

2 years ago
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

2 years ago
Whiteboard: Web Audio API

Updated

2 years ago
Component: Untriaged → Web Audio
Keywords: testcase
Product: Firefox → Core
Whiteboard: Web Audio API
(Assignee)

Comment 1

2 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
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
(Reporter)

Comment 3

2 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.
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
(Assignee)

Comment 6

2 years ago
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/
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

2 years ago
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/
(Assignee)

Comment 8

2 years ago
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/
(Assignee)

Comment 9

2 years ago
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/
(Assignee)

Comment 10

2 years ago
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/
(Assignee)

Comment 13

2 years ago
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/
(Assignee)

Comment 14

2 years ago
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/
(Assignee)

Updated

2 years ago
Assignee: amarchesini → karlt
Flags: needinfo?(amarchesini)
(Assignee)

Updated

2 years ago
Blocks: 1213313
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1265435
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

2 years ago
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
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 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

2 years ago
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
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 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

2 years ago
Attachment #8764116 - Flags: review?(padenot) → review+
Comment on attachment 8764115 [details]
bug 1257718 wpt manifest update noise

https://reviewboard.mozilla.org/r/60168/#review57058
Attachment #8764115 - Flags: review?(james) → review+

Comment 26

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

2 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

2 years ago
Flags: needinfo?(padenot)
You need to log in before you can comment on or make changes to this bug.