Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
189143010:
Cache values for panning and spatialization effects to enhance performance on pannerNode. (Closed)
|
Created:
6 years, 9 months ago by KhNo Modified:
6 years, 9 months ago CC:
blink-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionCache values for panning and spatialization effects to enhance performance on pannerNode.
DopplerShif, azimuth, elevation, distanceGain and ConeGain are
calculated always to give panning and spatialization effects.
These values are valid before changing parameters in AudioListener and PannerNode.
When there is no changing of parameters, cached values will be returned.
There were 3 FIXMEs on current implementation.
FIXME: we should cache azimuth and elevation (if possible), so we only
re-calculate if a change has been made.
FIXME: optimize for case when neither source nor listener has changed...
FIXME: could optimize by caching coneGain
BUG=347456
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168976
Patch Set 1 #
Total comments: 2
Patch Set 2 : #Patch Set 3 : #
Messages
Total messages: 25 (0 generated)
Please review the patch. I fixed nits as well. There is my misunderstanding of style. Thanks for helping me. :) You were correct.
On 2014/03/10 17:32:42, KhNo wrote: > Please review the patch. I fixed nits as well. > There is my misunderstanding of style. > Thanks for helping me. :) You were correct. What happened to https://codereview.chromium.org/182313005/? That was the one we were reviewing. How is this CL related to that?
On 2014/03/10 17:36:52, Raymond Toy wrote: > On 2014/03/10 17:32:42, KhNo wrote: > > Please review the patch. I fixed nits as well. > > There is my misunderstanding of style. > > Thanks for helping me. :) You were correct. > > What happened to https://codereview.chromium.org/182313005/? That was the one we > were reviewing. How is this CL related to that? OMG. I'm really sorry about it. I understood you want new CL with fix nits and remove previous one. It's my fault. I have only changed separating line according to your review from 182313005. Do we need to restart review again? :( I'm too much bother you with this patch, sorry.
On 2014/03/10 17:40:51, KhNo wrote: > On 2014/03/10 17:36:52, Raymond Toy wrote: > > On 2014/03/10 17:32:42, KhNo wrote: > > > Please review the patch. I fixed nits as well. > > > There is my misunderstanding of style. > > > Thanks for helping me. :) You were correct. > > > > What happened to https://codereview.chromium.org/182313005/? That was the one > we > > were reviewing. How is this CL related to that? > > OMG. I'm really sorry about it. I understood you want new CL with fix nits and > remove previous one. > It's my fault. I have only changed separating line according to your review from > 182313005. > Do we need to restart review again? :( I'm too much bother you with this patch, > sorry. The original CL was fine. You just needed to fix the nits and you could have committed it. Now I need to review this again.
lgtm. Unfortunate that we lost the original review CL. Please don't do that. https://codereview.chromium.org/189143010/diff/1/Source/modules/webaudio/Pann... File Source/modules/webaudio/PannerNode.cpp (right): https://codereview.chromium.org/189143010/diff/1/Source/modules/webaudio/Pann... Source/modules/webaudio/PannerNode.cpp:464: if (m_cachedPosition != m_position) Nit: Add comment: // Do a quick test and return if possible. Add similar comment to isDistanceConeGainDirty and isDopplerDirty. Since the original discussion of this is gone, a comment here is useful so future developers don't combine them into one. Recall that boolean operations shortcut, so you could combine the two if statements into one big if statement and there's no loss in speed.
On 2014/03/10 20:47:19, Raymond Toy wrote: > lgtm. > > Unfortunate that we lost the original review CL. Please don't do that. > > https://codereview.chromium.org/189143010/diff/1/Source/modules/webaudio/Pann... > File Source/modules/webaudio/PannerNode.cpp (right): > > https://codereview.chromium.org/189143010/diff/1/Source/modules/webaudio/Pann... > Source/modules/webaudio/PannerNode.cpp:464: if (m_cachedPosition != m_position) > Nit: Add comment: > > // Do a quick test and return if possible. > > Add similar comment to isDistanceConeGainDirty and isDopplerDirty. Since the > original discussion of this is gone, a comment here is useful so future > developers don't combine them into one. > > Recall that boolean operations shortcut, so you could combine the two if > statements into one big if statement and there's no loss in speed. Thank you.
The CQ bit was checked by keonho07.kim@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keonho07.kim@samsung.com/189143010/40001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/189143010/diff/1/Source/modules/webaudio/Pann... File Source/modules/webaudio/PannerNode.cpp (right): https://codereview.chromium.org/189143010/diff/1/Source/modules/webaudio/Pann... Source/modules/webaudio/PannerNode.cpp:464: if (m_cachedPosition != m_position) On 2014/03/10 20:47:20, Raymond Toy wrote: > Nit: Add comment: > > // Do a quick test and return if possible. > > Add similar comment to isDistanceConeGainDirty and isDopplerDirty. Since the > original discussion of this is gone, a comment here is useful so future > developers don't combine them into one. > > Recall that boolean operations shortcut, so you could combine the two if > statements into one big if statement and there's no loss in speed. Done.
On 2014/03/11 05:32:11, KhNo wrote: > https://codereview.chromium.org/189143010/diff/1/Source/modules/webaudio/Pann... > File Source/modules/webaudio/PannerNode.cpp (right): > > https://codereview.chromium.org/189143010/diff/1/Source/modules/webaudio/Pann... > Source/modules/webaudio/PannerNode.cpp:464: if (m_cachedPosition != m_position) > On 2014/03/10 20:47:20, Raymond Toy wrote: > > Nit: Add comment: > > > > // Do a quick test and return if possible. > > > > Add similar comment to isDistanceConeGainDirty and isDopplerDirty. Since the > > original discussion of this is gone, a comment here is useful so future > > developers don't combine them into one. > > > > Recall that boolean operations shortcut, so you could combine the two if > > statements into one big if statement and there's no loss in speed. > > Done. Raymond, You mentioned becoming a owner. Do I need to LGTM from kbr? There was commit-bot comment.
Raymond's an OWNER in modules/webaudio/ and platform/audio/ now. His review is sufficient.
On 2014/03/11 19:03:21, Ken Russell wrote: > Raymond's an OWNER in modules/webaudio/ and platform/audio/ now. His review is > sufficient. The message in #10 says OWNERS is not involved. lgtm again from rtoy@chromium. I don't know if the original one was from rtoy@google or @chromium.
On 2014/03/11 19:32:11, Raymond Toy wrote: > On 2014/03/11 19:03:21, Ken Russell wrote: > > Raymond's an OWNER in modules/webaudio/ and platform/audio/ now. His review is > > sufficient. > > The message in #10 says OWNERS is not involved. > > lgtm > > again from rtoy@chromium. I don't know if the original one was from rtoy@google > or @chromium. Sorry, there was no way to check previous codereview. From now, I will include your account with @chromium. Thanks for detailed reivew for this patch again.
The CQ bit was checked by keonho07.kim@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keonho07.kim@samsung.com/189143010/40001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Raymond, could you look into whether you're a full Blink committer? LGTM based on Raymond's review.
On 2014/03/12 01:20:20, Ken Russell wrote: > Raymond, could you look into whether you're a full Blink committer? > > LGTM based on Raymond's review. Thank Ken Russell for "LGTM"
The CQ bit was checked by keonho07.kim@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keonho07.kim@samsung.com/189143010/40001
Message was sent while issue was closed.
Change committed as 168976 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
