Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2567183002:
k-rate playbackRate and detune reaches final value (Closed)
|
Created:
4 years ago by Raymond Toy Modified:
3 years, 11 months ago Reviewers:
hongchan CC:
chromium-reviews, blink-reviews, haraken, Raymond Toy, hongchan Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionk-rate playbackRate and detune reaches final value
The optimization for the case when the last event is in the past does
not work for k-rate AudioParams because the final value returned is
the last value requested.
This happens because for k-rate parameters we only ask for one sample
from the automation at the beginning of the rendering quantum. This
also computes a default value for future use. At the next render, the
event is in the past so we were just using the default value, which
is actually now a render quantum behind.
Change the criteria so that we wait for 1.5 render quantum before
applying the optimization. This allows the timeline to compute one
more value, updating the default value too, which will be the correct
ending value of the event.
Additional test added that illustrates the issue is fixed and also
test for the other k-rate parameters.
BUG=672857
TEST=audioparam-k-rate.html
Review-Url: https://codereview.chromium.org/2567183002
Cr-Commit-Position: refs/heads/master@{#444455}
Committed: https://chromium.googlesource.com/chromium/src/+/01154e998c7a37c47cf83aabe44331ff4503ec3c
Patch Set 1 #Patch Set 2 : Let a render quantum go by before optimizing #Patch Set 3 : Wait 1.5 quanta instead of 1 for roundoff. #
Total comments: 8
Patch Set 4 : Address review comments. #
Messages
Total messages: 19 (12 generated)
The CQ bit was checked by rtoy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== k-rate playbackRate and detune reaches final value The optimization for the case when the last event is in the past does not work for k-rate AudioParams because the final value returned is the last value requested because the event is now in the past. Remove the optimization. Additional test added that illustrates the issue is fixed and also test for the other k-rate parameters. BUG=672857 TEST=audioparam-k-rate.html ========== to ========== k-rate playbackRate and detune reaches final value The optimization for the case when the last event is in the past does not work for k-rate AudioParams because the final value returned is the last value requested. This happens because for k-rate parameters we only ask for one sample from the automation, which computes a default value for future use. At the next render, the event is in the past so we were just using the default value, which is actually now a render quantum behind. Change the criteria so that we wait for a render quantum before applying the optimization. Additional test added that illustrates the issue is fixed and also test for the other k-rate parameters. BUG=672857 TEST=audioparam-k-rate.html ==========
Description was changed from ========== k-rate playbackRate and detune reaches final value The optimization for the case when the last event is in the past does not work for k-rate AudioParams because the final value returned is the last value requested. This happens because for k-rate parameters we only ask for one sample from the automation, which computes a default value for future use. At the next render, the event is in the past so we were just using the default value, which is actually now a render quantum behind. Change the criteria so that we wait for a render quantum before applying the optimization. Additional test added that illustrates the issue is fixed and also test for the other k-rate parameters. BUG=672857 TEST=audioparam-k-rate.html ========== to ========== k-rate playbackRate and detune reaches final value The optimization for the case when the last event is in the past does not work for k-rate AudioParams because the final value returned is the last value requested. This happens because for k-rate parameters we only ask for one sample from the automation, which computes a default value for future use. At the next render, the event is in the past so we were just using the default value, which is actually now a render quantum behind. Change the criteria so that we wait for a render quantum before applying the optimization. Additional test added that illustrates the issue is fixed and also test for the other k-rate parameters. BUG=672857 TEST=audioparam-k-rate.html ==========
rtoy@chromium.org changed reviewers: + hongchan@chromium.org
PTAL
https://codereview.chromium.org/2567183002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/AudioParam/audioparam-k-rate.html (right): https://codereview.chromium.org/2567183002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/AudioParam/audioparam-k-rate.html:13: var sampleRate = 48000; Use 'let' since this is a new test. https://codereview.chromium.org/2567183002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/AudioParam/audioparam-k-rate.html:61: task.describe("Test k-rate PannerNode and AudioListenerNode AudioParams"); Let's wrap this line. https://codereview.chromium.org/2567183002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/AudioParam/audioparam-k-rate.html:73: Remove a redundant empty line. https://codereview.chromium.org/2567183002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/AudioParam/audioparam-k-rate.html:123: should(panner.orientationX.value, "Final panner.orientationX.value") Wrap lines at 80 cols from here. https://codereview.chromium.org/2567183002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp (right): https://codereview.chromium.org/2567183002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:864: // for 1.5 render quanta instead of 1. What is the possible side effect of waiting 1.5 RQ? https://codereview.chromium.org/2567183002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:868: lastEventType != ParamEvent::SetTarget) { Wow. Is this what 'git cl format' offers? Could you please confirm it?
https://codereview.chromium.org/2567183002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp (right): https://codereview.chromium.org/2567183002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:868: lastEventType != ParamEvent::SetTarget) { On 2017/01/12 23:32:15, hongchan wrote: > Wow. Is this what 'git cl format' offers? Could you please confirm it? Yep. This isn't how I had originally indented it. I tried again and put everything on one line, and git cl format produces this again.
https://codereview.chromium.org/2567183002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp (right): https://codereview.chromium.org/2567183002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:864: // for 1.5 render quanta instead of 1. On 2017/01/12 23:32:16, hongchan wrote: > What is the possible side effect of waiting 1.5 RQ? I think the only side-effect would be that we now end up clearing the event list after 2 RQ after the last event instead of immediately. I think it's useful to be able to empty the event list so that we can do the fast path if no events are added again.
Description was changed from ========== k-rate playbackRate and detune reaches final value The optimization for the case when the last event is in the past does not work for k-rate AudioParams because the final value returned is the last value requested. This happens because for k-rate parameters we only ask for one sample from the automation, which computes a default value for future use. At the next render, the event is in the past so we were just using the default value, which is actually now a render quantum behind. Change the criteria so that we wait for a render quantum before applying the optimization. Additional test added that illustrates the issue is fixed and also test for the other k-rate parameters. BUG=672857 TEST=audioparam-k-rate.html ========== to ========== k-rate playbackRate and detune reaches final value The optimization for the case when the last event is in the past does not work for k-rate AudioParams because the final value returned is the last value requested. This happens because for k-rate parameters we only ask for one sample from the automation at the beginning of the rendering quantum. This also computes a default value for future use. At the next render, the event is in the past so we were just using the default value, which is actually now a render quantum behind. Change the criteria so that we wait for a render quantum before applying the optimization. This allows the timeline to compute one more value, updating the default value too, which will be the correct ending value of the event. Additional test added that illustrates the issue is fixed and also test for the other k-rate parameters. BUG=672857 TEST=audioparam-k-rate.html ==========
lgtm
Description was changed from ========== k-rate playbackRate and detune reaches final value The optimization for the case when the last event is in the past does not work for k-rate AudioParams because the final value returned is the last value requested. This happens because for k-rate parameters we only ask for one sample from the automation at the beginning of the rendering quantum. This also computes a default value for future use. At the next render, the event is in the past so we were just using the default value, which is actually now a render quantum behind. Change the criteria so that we wait for a render quantum before applying the optimization. This allows the timeline to compute one more value, updating the default value too, which will be the correct ending value of the event. Additional test added that illustrates the issue is fixed and also test for the other k-rate parameters. BUG=672857 TEST=audioparam-k-rate.html ========== to ========== k-rate playbackRate and detune reaches final value The optimization for the case when the last event is in the past does not work for k-rate AudioParams because the final value returned is the last value requested. This happens because for k-rate parameters we only ask for one sample from the automation at the beginning of the rendering quantum. This also computes a default value for future use. At the next render, the event is in the past so we were just using the default value, which is actually now a render quantum behind. Change the criteria so that we wait for 1.5 render quantum before applying the optimization. This allows the timeline to compute one more value, updating the default value too, which will be the correct ending value of the event. Additional test added that illustrates the issue is fixed and also test for the other k-rate parameters. BUG=672857 TEST=audioparam-k-rate.html ==========
The CQ bit was checked by rtoy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484763484164450,
"parent_rev": "bfee4857f9bdb7efdc7e5cb5d39edfeb39c5adbd", "commit_rev":
"01154e998c7a37c47cf83aabe44331ff4503ec3c"}
Message was sent while issue was closed.
Description was changed from ========== k-rate playbackRate and detune reaches final value The optimization for the case when the last event is in the past does not work for k-rate AudioParams because the final value returned is the last value requested. This happens because for k-rate parameters we only ask for one sample from the automation at the beginning of the rendering quantum. This also computes a default value for future use. At the next render, the event is in the past so we were just using the default value, which is actually now a render quantum behind. Change the criteria so that we wait for 1.5 render quantum before applying the optimization. This allows the timeline to compute one more value, updating the default value too, which will be the correct ending value of the event. Additional test added that illustrates the issue is fixed and also test for the other k-rate parameters. BUG=672857 TEST=audioparam-k-rate.html ========== to ========== k-rate playbackRate and detune reaches final value The optimization for the case when the last event is in the past does not work for k-rate AudioParams because the final value returned is the last value requested. This happens because for k-rate parameters we only ask for one sample from the automation at the beginning of the rendering quantum. This also computes a default value for future use. At the next render, the event is in the past so we were just using the default value, which is actually now a render quantum behind. Change the criteria so that we wait for 1.5 render quantum before applying the optimization. This allows the timeline to compute one more value, updating the default value too, which will be the correct ending value of the event. Additional test added that illustrates the issue is fixed and also test for the other k-rate parameters. BUG=672857 TEST=audioparam-k-rate.html Review-Url: https://codereview.chromium.org/2567183002 Cr-Commit-Position: refs/heads/master@{#444455} Committed: https://chromium.googlesource.com/chromium/src/+/01154e998c7a37c47cf83aabe443... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/01154e998c7a37c47cf83aabe443... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
