Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
196993002:
Guarantee a thread safety when accessing source location info in pannerNode. (Closed)
|
Created:
6 years, 9 months ago by KhNo Modified:
6 years, 9 months ago CC:
blink-reviews, Raymond Toy Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionGuarantee a thread safety when accessing source location info and distance model in pannerNode.
ScriptExcution thread can set source location info such as postioin, orientation and velocity.
These locations also are read from audio thread.
Distance model also need to quarantee a thread safety.
FIXME :consider thread safety about m_position in audio thread.
FIXME :consider thread safety about m_orientation in audio thread.
FIXME :consider thread safety about m_velocity in audio thread.
BUG=350583
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169537
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 10
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #
Messages
Total messages: 21 (0 generated)
Please review this patch. Thank you. :) https://codereview.chromium.org/196993002/diff/40001/Source/modules/webaudio/... File Source/modules/webaudio/PannerNode.cpp (left): https://codereview.chromium.org/196993002/diff/40001/Source/modules/webaudio/... Source/modules/webaudio/PannerNode.cpp:61: , m_cachedDistanceConeGain(0) Generally, default gain value should be 1.0f, Not 0. It is too small nit to divide CL. So, it is included this patch.
https://codereview.chromium.org/196993002/diff/40001/Source/modules/webaudio/... File Source/modules/webaudio/PannerNode.cpp (right): https://codereview.chromium.org/196993002/diff/40001/Source/modules/webaudio/... Source/modules/webaudio/PannerNode.cpp:162: // We must be in the middle of changing the one of attributes which must be locked during process. This comment is not very helpful. Be more explicit about what might be changing. https://codereview.chromium.org/196993002/diff/40001/Source/modules/webaudio/... File Source/modules/webaudio/PannerNode.h (right): https://codereview.chromium.org/196993002/diff/40001/Source/modules/webaudio/... Source/modules/webaudio/PannerNode.h:179: // Synchronize process() and setters panningModel, distanceModel and source location info. Update comment: // Synchronize process() with setting of the panning model, distance model, and caching of the source location/orientation info. https://codereview.chromium.org/196993002/diff/40001/Source/modules/webaudio/... Source/modules/webaudio/PannerNode.h:180: mutable Mutex m_processLock; Why change the name?
Please review the patch set #4. Thanks for reviewing https://codereview.chromium.org/196993002/diff/40001/Source/modules/webaudio/... File Source/modules/webaudio/PannerNode.cpp (right): https://codereview.chromium.org/196993002/diff/40001/Source/modules/webaudio/... Source/modules/webaudio/PannerNode.cpp:162: // We must be in the middle of changing the one of attributes which must be locked during process. On 2014/03/13 21:37:46, Raymond Toy wrote: > This comment is not very helpful. Be more explicit about what might be changing. Done. https://codereview.chromium.org/196993002/diff/40001/Source/modules/webaudio/... File Source/modules/webaudio/PannerNode.h (right): https://codereview.chromium.org/196993002/diff/40001/Source/modules/webaudio/... Source/modules/webaudio/PannerNode.h:179: // Synchronize process() and setters panningModel, distanceModel and source location info. On 2014/03/13 21:37:46, Raymond Toy wrote: > Update comment: > > // Synchronize process() with setting of the panning model, distance model, and > caching of the source location/orientation info. Done. https://codereview.chromium.org/196993002/diff/40001/Source/modules/webaudio/... Source/modules/webaudio/PannerNode.h:180: mutable Mutex m_processLock; On 2014/03/13 21:37:46, Raymond Toy wrote: > Why change the name? Previous code only has locked for m_panner which is for panningModel. Now, there are additional things have to lock such as distanceModel, location info... In General, other source about web audio named m_processLock when there is something to lock during excute process().
Just some nits. https://codereview.chromium.org/196993002/diff/40001/Source/modules/webaudio/... File Source/modules/webaudio/PannerNode.h (right): https://codereview.chromium.org/196993002/diff/40001/Source/modules/webaudio/... Source/modules/webaudio/PannerNode.h:179: // Synchronize process() and setters panningModel, distanceModel and source location info. On 2014/03/13 21:37:46, Raymond Toy wrote: > Update comment: > > // Synchronize process() with setting of the panning model, distance model, and > caching of the source location/orientation info. Please use this comment. Your replacement is not quite the same and doesn't read quite right. https://codereview.chromium.org/196993002/diff/40001/Source/modules/webaudio/... Source/modules/webaudio/PannerNode.h:180: mutable Mutex m_processLock; On 2014/03/14 10:39:58, KhNo wrote: > On 2014/03/13 21:37:46, Raymond Toy wrote: > > Why change the name? > > Previous code only has locked for m_panner which is for panningModel. > Now, there are additional things have to lock such as distanceModel, location > info... > In General, other source about web audio named m_processLock when there is > something to lock during excute process(). Ok. https://codereview.chromium.org/196993002/diff/50001/Source/modules/webaudio/... File Source/modules/webaudio/PannerNode.cpp (right): https://codereview.chromium.org/196993002/diff/50001/Source/modules/webaudio/... Source/modules/webaudio/PannerNode.cpp:163: // such as panningModel, distanceModel and source's location info Replace this comment with the equivalent but briefer: // We must be in the middle of changing the panning model, the distance model, or the source's location information.
Could you review again? :) Thanks. https://codereview.chromium.org/196993002/diff/40001/Source/modules/webaudio/... File Source/modules/webaudio/PannerNode.h (right): https://codereview.chromium.org/196993002/diff/40001/Source/modules/webaudio/... Source/modules/webaudio/PannerNode.h:179: // Synchronize process() and setters panningModel, distanceModel and source location info. On 2014/03/14 22:30:36, Raymond Toy wrote: > On 2014/03/13 21:37:46, Raymond Toy wrote: > > Update comment: > > > > // Synchronize process() with setting of the panning model, distance model, > and > > caching of the source location/orientation info. > > Please use this comment. Your replacement is not quite the same and doesn't read > quite right. Done. https://codereview.chromium.org/196993002/diff/50001/Source/modules/webaudio/... File Source/modules/webaudio/PannerNode.cpp (right): https://codereview.chromium.org/196993002/diff/50001/Source/modules/webaudio/... Source/modules/webaudio/PannerNode.cpp:163: // such as panningModel, distanceModel and source's location info On 2014/03/14 22:30:36, Raymond Toy wrote: > Replace this comment with the equivalent but briefer: > > // We must be in the middle of changing the panning model, the distance model, > or the source's location information. Done.
lgtm. You'll need an lgtm from kbr, still.
On 2014/03/18 16:41:52, Raymond Toy wrote: > lgtm. > > You'll need an lgtm from kbr, still. @kbr Could you review? Thank you always.
LGTM with one comment. https://codereview.chromium.org/196993002/diff/70001/Source/modules/webaudio/... File Source/modules/webaudio/PannerNode.cpp (right): https://codereview.chromium.org/196993002/diff/70001/Source/modules/webaudio/... Source/modules/webaudio/PannerNode.cpp:457: ASSERT(context()->isAudioThread()); Since dopplerRate() is public in PannerNode.h please comment that it must be called on the audio thread. It looks like it only has one caller.
Thanks for reviewing. https://codereview.chromium.org/196993002/diff/70001/Source/modules/webaudio/... File Source/modules/webaudio/PannerNode.cpp (right): https://codereview.chromium.org/196993002/diff/70001/Source/modules/webaudio/... Source/modules/webaudio/PannerNode.cpp:457: ASSERT(context()->isAudioThread()); On 2014/03/19 00:37:41, Ken Russell wrote: > Since dopplerRate() is public in PannerNode.h please comment that it must be > called on the audio thread. It looks like it only has one caller. Thanks for sharing I missed. I have added. :)
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/196993002/90001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on win_blink_compile_dbg
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/196993002/110001
The CQ bit was unchecked by keonho07.kim@samsung.com
The CQ bit was checked by keonho07.kim@samsung.com
The CQ bit was unchecked by keonho07.kim@samsung.com
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/196993002/130001
Message was sent while issue was closed.
Change committed as 169537 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
