Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1485003002:
Make setTarget followed by linear or exponential ramp continuous. (Closed)
|
Created:
5 years ago by Raymond Toy Modified:
4 years, 7 months ago Reviewers:
hongchan CC:
chromium-reviews, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake setTarget followed by linear or exponential ramp continuous.
When setTarget is followed by a linear or exponential ramp, the
current behavior produces discontinuous curves. In this case, make
the curve continuous by inserting a setValue to freeze the curve at
the last value of the setTarget. This establishes the starting point
for the following linear and exponential ramp to make it continuous.
WebAudio issue: https://github.com/WebAudio/web-audio-api/issues/652
Spec proposal: https://github.com/WebAudio/web-audio-api/pull/665
BUG=564157
TEST=audioparam-setTargetAtTime-continuous.html
Committed: https://crrev.com/b1fd9cf76c2e80540100262b09911600936f2ae3
Cr-Commit-Position: refs/heads/master@{#391597}
Patch Set 1 : #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : Rebase #Patch Set 6 : Rebase #
Total comments: 8
Patch Set 7 : Add comments. #
Total comments: 3
Patch Set 8 : Rebase and fix test failures #
Messages
Total messages: 21 (8 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Make setTarget followed by linear or exponential ramp continuous. When setTarget is followed by a linear or exponential ramp, the current behavior produces discontinuous curves. In this case, make the curve continuous by inserting a setValue to freeze the curve at the last value of the setTarget. This establishes the starting point for the following linear and exponential ramp to make it continuous. BUG= TEST= ========== to ========== Make setTarget followed by linear or exponential ramp continuous. When setTarget is followed by a linear or exponential ramp, the current behavior produces discontinuous curves. In this case, make the curve continuous by inserting a setValue to freeze the curve at the last value of the setTarget. This establishes the starting point for the following linear and exponential ramp to make it continuous. BUG=564157 TEST= ==========
Description was changed from ========== Make setTarget followed by linear or exponential ramp continuous. When setTarget is followed by a linear or exponential ramp, the current behavior produces discontinuous curves. In this case, make the curve continuous by inserting a setValue to freeze the curve at the last value of the setTarget. This establishes the starting point for the following linear and exponential ramp to make it continuous. BUG=564157 TEST= ========== to ========== Make setTarget followed by linear or exponential ramp continuous. When setTarget is followed by a linear or exponential ramp, the current behavior produces discontinuous curves. In this case, make the curve continuous by inserting a setValue to freeze the curve at the last value of the setTarget. This establishes the starting point for the following linear and exponential ramp to make it continuous. BUG=564157 TEST=audioparam-setTargetAtTime-continuous.html ==========
Description was changed from ========== Make setTarget followed by linear or exponential ramp continuous. When setTarget is followed by a linear or exponential ramp, the current behavior produces discontinuous curves. In this case, make the curve continuous by inserting a setValue to freeze the curve at the last value of the setTarget. This establishes the starting point for the following linear and exponential ramp to make it continuous. BUG=564157 TEST=audioparam-setTargetAtTime-continuous.html ========== to ========== Make setTarget followed by linear or exponential ramp continuous. When setTarget is followed by a linear or exponential ramp, the current behavior produces discontinuous curves. In this case, make the curve continuous by inserting a setValue to freeze the curve at the last value of the setTarget. This establishes the starting point for the following linear and exponential ramp to make it continuous. WebAudio issue: https://github.com/WebAudio/web-audio-api/issues/652 Spec proposal: https://github.com/WebAudio/web-audio-api/pull/665 BUG=564157 TEST=audioparam-setTargetAtTime-continuous.html ==========
rtoy@chromium.org changed reviewers: + hongchan@chromium.org
PTAL.
https://codereview.chromium.org/1485003002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/audioparam-setTargetAtTime-continuous.html (right): https://codereview.chromium.org/1485003002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audioparam-setTargetAtTime-continuous.html:27: audit.defineTask("linear ramp replace", function (done) { Can we have a brief description for each test task? It is a bit hard to understand what the test is about. https://codereview.chromium.org/1485003002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audioparam-setTargetAtTime-continuous.html:39: audit.defineTask("delayed linear ramp", function (done) { Ditto. https://codereview.chromium.org/1485003002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audioparam-setTargetAtTime-continuous.html:51: audit.defineTask("expo ramp", function (done) { Ditto. https://codereview.chromium.org/1485003002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audioparam-setTargetAtTime-continuous.html:63: audit.defineTask("delayed expo ramp", function (done) { Ditto.
https://codereview.chromium.org/1485003002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/audioparam-setTargetAtTime-continuous.html (right): https://codereview.chromium.org/1485003002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audioparam-setTargetAtTime-continuous.html:27: audit.defineTask("linear ramp replace", function (done) { On 2016/01/29 18:53:58, hoch wrote: > Can we have a brief description for each test task? It is a bit hard to > understand what the test is about. Done. https://codereview.chromium.org/1485003002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audioparam-setTargetAtTime-continuous.html:39: audit.defineTask("delayed linear ramp", function (done) { On 2016/01/29 18:53:58, hoch wrote: > Ditto. Done. https://codereview.chromium.org/1485003002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audioparam-setTargetAtTime-continuous.html:51: audit.defineTask("expo ramp", function (done) { On 2016/01/29 18:53:58, hoch wrote: > Ditto. Done. https://codereview.chromium.org/1485003002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audioparam-setTargetAtTime-continuous.html:63: audit.defineTask("delayed expo ramp", function (done) { On 2016/01/29 18:53:58, hoch wrote: > Ditto. Done.
lgtm with nits. https://codereview.chromium.org/1485003002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp (right): https://codereview.chromium.org/1485003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:408: ParamEvent::Type nextEventType = nextEvent ? static_cast<ParamEvent::Type>(nextEvent->type()) : ParamEvent::LastType /* unknown */; Are we okay with the comment style here? I've never seen something like this before.
https://codereview.chromium.org/1485003002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp (right): https://codereview.chromium.org/1485003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:408: ParamEvent::Type nextEventType = nextEvent ? static_cast<ParamEvent::Type>(nextEvent->type()) : ParamEvent::LastType /* unknown */; On 2016/04/27 21:40:09, hoch wrote: > Are we okay with the comment style here? I've never seen something like this > before. Don't know. I just basically moved this line up from line 438 below, without changing anything.
On 2016/04/27 21:43:15, Raymond Toy wrote: > https://codereview.chromium.org/1485003002/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp (right): > > https://codereview.chromium.org/1485003002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:408: > ParamEvent::Type nextEventType = nextEvent ? > static_cast<ParamEvent::Type>(nextEvent->type()) : ParamEvent::LastType /* > unknown */; > On 2016/04/27 21:40:09, hoch wrote: > > Are we okay with the comment style here? I've never seen something like this > > before. > > Don't know. I just basically moved this line up from line 438 below, without > changing anything. I don't know what that means - and it doesn't look meaningful either. Should we just remove it?
https://codereview.chromium.org/1485003002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp (right): https://codereview.chromium.org/1485003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:408: ParamEvent::Type nextEventType = nextEvent ? static_cast<ParamEvent::Type>(nextEvent->type()) : ParamEvent::LastType /* unknown */; On 2016/04/27 21:43:15, Raymond Toy wrote: > On 2016/04/27 21:40:09, hoch wrote: > > Are we okay with the comment style here? I've never seen something like this > > before. > > Don't know. I just basically moved this line up from line 438 below, without > changing anything. It means a nextEventType equal to LastType indicates an unknown eventType. Because there is no nextEvent. I'll remove that and add a better comment.
PTAL. Due to various changes, rebasing this required a bit of rework to get the ramps to be continuous and pass the tests.
On 2016/04/27 23:23:52, Raymond Toy wrote: > PTAL. Due to various changes, rebasing this required a bit of rework to get the > ramps to be continuous and pass the tests. Ping
lgtm
The CQ bit was checked by rtoy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1485003002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1485003002/160001
Message was sent while issue was closed.
Description was changed from ========== Make setTarget followed by linear or exponential ramp continuous. When setTarget is followed by a linear or exponential ramp, the current behavior produces discontinuous curves. In this case, make the curve continuous by inserting a setValue to freeze the curve at the last value of the setTarget. This establishes the starting point for the following linear and exponential ramp to make it continuous. WebAudio issue: https://github.com/WebAudio/web-audio-api/issues/652 Spec proposal: https://github.com/WebAudio/web-audio-api/pull/665 BUG=564157 TEST=audioparam-setTargetAtTime-continuous.html ========== to ========== Make setTarget followed by linear or exponential ramp continuous. When setTarget is followed by a linear or exponential ramp, the current behavior produces discontinuous curves. In this case, make the curve continuous by inserting a setValue to freeze the curve at the last value of the setTarget. This establishes the starting point for the following linear and exponential ramp to make it continuous. WebAudio issue: https://github.com/WebAudio/web-audio-api/issues/652 Spec proposal: https://github.com/WebAudio/web-audio-api/pull/665 BUG=564157 TEST=audioparam-setTargetAtTime-continuous.html ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Make setTarget followed by linear or exponential ramp continuous. When setTarget is followed by a linear or exponential ramp, the current behavior produces discontinuous curves. In this case, make the curve continuous by inserting a setValue to freeze the curve at the last value of the setTarget. This establishes the starting point for the following linear and exponential ramp to make it continuous. WebAudio issue: https://github.com/WebAudio/web-audio-api/issues/652 Spec proposal: https://github.com/WebAudio/web-audio-api/pull/665 BUG=564157 TEST=audioparam-setTargetAtTime-continuous.html ========== to ========== Make setTarget followed by linear or exponential ramp continuous. When setTarget is followed by a linear or exponential ramp, the current behavior produces discontinuous curves. In this case, make the curve continuous by inserting a setValue to freeze the curve at the last value of the setTarget. This establishes the starting point for the following linear and exponential ramp to make it continuous. WebAudio issue: https://github.com/WebAudio/web-audio-api/issues/652 Spec proposal: https://github.com/WebAudio/web-audio-api/pull/665 BUG=564157 TEST=audioparam-setTargetAtTime-continuous.html Committed: https://crrev.com/b1fd9cf76c2e80540100262b09911600936f2ae3 Cr-Commit-Position: refs/heads/master@{#391597} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/b1fd9cf76c2e80540100262b09911600936f2ae3 Cr-Commit-Position: refs/heads/master@{#391597} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
