Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
232453003:
Add logic to cache elements of panner node from calculation with own attributes and AudioListener (Closed)
|
Created:
6 years, 8 months ago by KhNo Modified:
6 years, 8 months ago CC:
blink-reviews, Raymond Toy Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionAdd logic to cache elements of panner node from calculation with own attributes and AudioListener.
In a panner node, there are several elements, azimuth, elevation, distanceConeGain and dopplerRate
which are calculatedby attributes such as the panning model, source's location information, listener,
distance parameters and sound cones.
AudioListener lock and pannerNode lock are required to quarantee a thread safety when cached elements is queued.
In addition, listener will have a list that contain all pannerNode instance in the context to be used update dirtys of each panner nodes.
BUG=350583
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172135
Patch Set 1 : #
Total comments: 20
Patch Set 2 : #
Total comments: 24
Patch Set 3 : Fix some comments and function names #
Total comments: 6
Patch Set 4 : Unrelated fix has been separated. #
Messages
Total messages: 15 (0 generated)
Could you review ? I have left some comments. https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... File Source/modules/webaudio/AudioListener.h (left): https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... Source/modules/webaudio/AudioListener.h:50: void setPosition(const FloatPoint3D &position) { m_position = position; } Moved to private, no need to public. https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... Source/modules/webaudio/AudioListener.h:59: void setOrientation(const FloatPoint3D &orientation) { m_orientation = orientation; } Moved to private, no need to public. https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... Source/modules/webaudio/AudioListener.h:62: // Up-vector Orientation and up-vector should be categorized togather. https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... Source/modules/webaudio/AudioListener.h:63: void setUpVector(const FloatPoint3D &upVector) { m_upVector = upVector; } Moved to private, no need to public. https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... Source/modules/webaudio/AudioListener.h:68: void setVelocity(const FloatPoint3D &velocity) { m_velocity = velocity; } Moved to private, no need to public. https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... Source/modules/webaudio/AudioListener.h:86: blank is not necessary for categorization. https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... Source/modules/webaudio/AudioListener.h:88: blank is not necessary for categorization. https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... File Source/modules/webaudio/AudioListener.h (right): https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... Source/modules/webaudio/AudioListener.h:50: virtual ~AudioListener(); This destructor was added for clear m_panners which contains all panner nodes in entire context. https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... Source/modules/webaudio/AudioListener.h:56: // Orientation and Up-vector Orientation and up-vector should be categorized togather. https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... Source/modules/webaudio/AudioListener.h:78: Mutex& listenerLock() { return m_listenerLock; } listener lock will be used for pannerNode's process() to prevent changing while calculation. https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... Source/modules/webaudio/AudioListener.h:90: void updatePannersDirty(unsigned); Common private method for change dirties of all panners. https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... Source/modules/webaudio/AudioListener.h:104: PannerList m_panners; audio listener should have all pannernode's instance in context, to change those are dirty when one of listener attributes is changed. https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... File Source/modules/webaudio/PannerNode.cpp (right): https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... Source/modules/webaudio/PannerNode.cpp:123: MutexTryLocker tryListenerLocker(listener()->listenerLock()); It is required to lock context's listener. https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... File Source/modules/webaudio/PannerNode.h (left): https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... Source/modules/webaudio/PannerNode.h:77: // AudioContext's listener moved to private https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... Source/modules/webaudio/PannerNode.h:156: FloatPoint3D m_cachedPosition; All cached attributes are removed. That will be checked with boolean. https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... Source/modules/webaudio/PannerNode.h:171: RefPtr<AudioListener> m_cachedListener; AudioListener is checking dirty itself. no required to comparing with previous listener. https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... File Source/modules/webaudio/PannerNode.h (right): https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... Source/modules/webaudio/PannerNode.h:28: #include "modules/webaudio/AudioContext.h" webkit style checker error. https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... Source/modules/webaudio/PannerNode.h:53: EqualPower = 0, webkit style checker error. https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... Source/modules/webaudio/PannerNode.h:60: LinearDistance = 0, webkit style checker error. https://codereview.chromium.org/232453003/diff/80002/Source/modules/webaudio/... Source/modules/webaudio/PannerNode.h:132: It seems to be needed one blank.
Please review this patch. I left some comment to explain my approaching.
https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... File Source/modules/webaudio/AudioListener.cpp (right): https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/AudioListener.cpp:35: #include "modules/webaudio/PannerNode.h" Is this necessary? https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/AudioListener.cpp:74: void AudioListener::updatePannersDirty(unsigned type) I think updateDirtyPanners is a better name. https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/AudioListener.cpp:88: updatePannersDirty(PannerNode::AzimuthElevationDirty | PannerNode::DistanceConeGainDirty | PannerNode::DopplerRateDirty); udpatePannersDirty is protected by m_processLock (panner) and m_listenerLock. Any possibility of deadlock? Would it be possible to use the PannerNode's process lock for this instead of the separate m_listenerLock? https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... File Source/modules/webaudio/AudioListener.h (right): https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/AudioListener.h:77: // Provide for pannerNodes. Provide what? Clarify this comment. https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/AudioListener.h:90: void updatePannersDirty(unsigned); I think this is better: void updateDirtyPanners(unsigned dirtyFlags); or maybe markPannersAsDirty. https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/AudioListener.h:103: typedef Vector<PannerNode*> PannerList; Is the typedef necessary? This seems a bad style. https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... File Source/modules/webaudio/PannerNode.cpp (right): https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/PannerNode.cpp:155: // We must be in the middle of changing the panning model, source's location information, listener, distance parameters and sound cones. Maybe simplify comment to just: // We must be in the middle of changing the properties of the panner or the listener. https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/PannerNode.cpp:166: listener()->addPanner(this); This happens in the main thread, but the processing of the panner list is in the audio thread, right? Any possible race conditions because of this? https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/PannerNode.cpp:177: listener()->removePanner(this); Is there a race condition here? You've cleared the panner and are removing it from the list, but could the audio thread be processing the list now? https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/PannerNode.cpp:534: void PannerNode::updatePannerDirty(unsigned dirty) I think the name markPannerAsDirty is more explicit about what this function is doing. https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... File Source/modules/webaudio/PannerNode.h (right): https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/PannerNode.h:52: EqualPower = 0, Why is the case changed for this enum and the enum below. https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/PannerNode.h:64: enum { Add comment on what these flags are for.
PTAL :) https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... File Source/modules/webaudio/AudioListener.cpp (right): https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/AudioListener.cpp:35: #include "modules/webaudio/PannerNode.h" On 2014/04/16 17:36:15, Raymond Toy wrote: > Is this necessary? Yes, it is for m_panners[i]->updatePannerDirty(type); https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/AudioListener.cpp:74: void AudioListener::updatePannersDirty(unsigned type) On 2014/04/16 17:36:15, Raymond Toy wrote: > I think updateDirtyPanners is a better name. Changed to markPannersAsDirty https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/AudioListener.cpp:88: updatePannersDirty(PannerNode::AzimuthElevationDirty | PannerNode::DistanceConeGainDirty | PannerNode::DopplerRateDirty); On 2014/04/16 17:36:15, Raymond Toy wrote: > udpatePannersDirty is protected by m_processLock (panner) and m_listenerLock. > Any possibility of deadlock? > > Would it be possible to use the PannerNode's process lock for this instead of > the separate m_listenerLock? Actually, I have tried to using pannerNode's process lock. In case of multiple panners, when one of parameter has changed in AudioListener, all pannerNodes should be locked since what panner's process() is running in audio thread. In addition, it should marked dirty of all panners. There is no deadlock. AudioListener's main thread is lock only listenerlock to set true. Panner's main thread is lock only processlock to set true. Audiothread is trylock both locks to set false. https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... File Source/modules/webaudio/AudioListener.h (right): https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/AudioListener.h:77: // Provide for pannerNodes. On 2014/04/16 17:36:15, Raymond Toy wrote: > Provide what? Clarify this comment. Removed, this comment is duplicated with comment of variables. https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/AudioListener.h:90: void updatePannersDirty(unsigned); On 2014/04/16 17:36:15, Raymond Toy wrote: > I think this is better: > > void updateDirtyPanners(unsigned dirtyFlags); > > or maybe markPannersAsDirty. Changed to markPannersAsDirty https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/AudioListener.h:103: typedef Vector<PannerNode*> PannerList; On 2014/04/16 17:36:15, Raymond Toy wrote: > Is the typedef necessary? This seems a bad style. Done. https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... File Source/modules/webaudio/PannerNode.cpp (right): https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/PannerNode.cpp:155: // We must be in the middle of changing the panning model, source's location information, listener, distance parameters and sound cones. On 2014/04/16 17:36:15, Raymond Toy wrote: > Maybe simplify comment to just: > > // We must be in the middle of changing the properties of the panner or the > listener. Done. https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/PannerNode.cpp:166: listener()->addPanner(this); On 2014/04/16 17:36:15, Raymond Toy wrote: > This happens in the main thread, but the processing of the panner list is in the > audio thread, right? Any possible race conditions because of this? No, It doesn't happen. panners list in AudioListener is using only in main thread by setters to update boolean dirty. https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/PannerNode.cpp:177: listener()->removePanner(this); On 2014/04/16 17:36:15, Raymond Toy wrote: > Is there a race condition here? You've cleared the panner and are removing it > from the list, but could the audio thread be processing the list now? No, It doesn't happen. panners list in AudioListener is using only in main thread by setters to update boolean dirty. https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/PannerNode.cpp:534: void PannerNode::updatePannerDirty(unsigned dirty) On 2014/04/16 17:36:15, Raymond Toy wrote: > I think the name markPannerAsDirty is more explicit about what this function is > doing. Done. https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... File Source/modules/webaudio/PannerNode.h (right): https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/PannerNode.h:52: EqualPower = 0, On 2014/04/16 17:36:15, Raymond Toy wrote: > Why is the case changed for this enum and the enum below. When I added dirty enums, webkit style checker of presubmit returns error about all capitals. https://codereview.chromium.org/232453003/diff/120001/Source/modules/webaudio... Source/modules/webaudio/PannerNode.h:64: enum { On 2014/04/16 17:36:15, Raymond Toy wrote: > Add comment on what these flags are for. Done.
lgtm with a few nits. https://codereview.chromium.org/232453003/diff/140001/Source/modules/webaudio... File Source/modules/webaudio/PannerNode.cpp (left): https://codereview.chromium.org/232453003/diff/140001/Source/modules/webaudio... Source/modules/webaudio/PannerNode.cpp:218: Question: If you didn't delete this line (and you shouldn't), can you keep the original uppercase panning model enums? https://codereview.chromium.org/232453003/diff/140001/Source/modules/webaudio... File Source/modules/webaudio/PannerNode.h (right): https://codereview.chromium.org/232453003/diff/140001/Source/modules/webaudio... Source/modules/webaudio/PannerNode.h:68: DopplerRateDirty = 0x00000004, Nit: Don't add all the zeroes. Just 0x1, 0x2, 0x4 is enough.
Please refer my comments. Thanks https://codereview.chromium.org/232453003/diff/140001/Source/modules/webaudio... File Source/modules/webaudio/PannerNode.cpp (left): https://codereview.chromium.org/232453003/diff/140001/Source/modules/webaudio... Source/modules/webaudio/PannerNode.cpp:218: On 2014/04/21 16:48:26, Raymond Toy wrote: > Question: If you didn't delete this line (and you shouldn't), can you keep the > original uppercase panning model enums? That was for making code consensus before calling mutex lock with others since only here has empty line. If you want let empty line, I will. As you know, I added new enum, but there is webkit style checker error. So it also changed for code consensus. It can be changed if it is fine that new added enum only has different style enum in header. If you want it, I will change back to original uppercase style for two enum type, panningmodel and distancemodel. https://codereview.chromium.org/232453003/diff/140001/Source/modules/webaudio... File Source/modules/webaudio/PannerNode.h (right): https://codereview.chromium.org/232453003/diff/140001/Source/modules/webaudio... Source/modules/webaudio/PannerNode.h:68: DopplerRateDirty = 0x00000004, On 2014/04/21 16:48:26, Raymond Toy wrote: > Nit: Don't add all the zeroes. Just 0x1, 0x2, 0x4 is enough. Ok. Thank for guide. I will change it.
https://codereview.chromium.org/232453003/diff/140001/Source/modules/webaudio... File Source/modules/webaudio/PannerNode.cpp (left): https://codereview.chromium.org/232453003/diff/140001/Source/modules/webaudio... Source/modules/webaudio/PannerNode.cpp:218: On 2014/04/21 17:29:01, KhNo wrote: > On 2014/04/21 16:48:26, Raymond Toy wrote: > > Question: If you didn't delete this line (and you shouldn't), can you keep > the > > original uppercase panning model enums? > > That was for making code consensus before calling mutex lock with others since > only here has empty line. If you want let empty line, I will. > > As you know, I added new enum, but there is webkit style checker error. So it > also changed for code consensus. It can be changed if it is fine that new added > enum only has different style enum in header. If you want it, I will change back > to original uppercase style for two enum type, panningmodel and distancemodel. I leave it up to you. It's just that I don't like CLs that change parts of the code that aren't really related to the CL itself.
https://codereview.chromium.org/232453003/diff/140001/Source/modules/webaudio... File Source/modules/webaudio/PannerNode.cpp (left): https://codereview.chromium.org/232453003/diff/140001/Source/modules/webaudio... Source/modules/webaudio/PannerNode.cpp:218: On 2014/04/22 00:57:49, Raymond Toy wrote: > On 2014/04/21 17:29:01, KhNo wrote: > > On 2014/04/21 16:48:26, Raymond Toy wrote: > > > Question: If you didn't delete this line (and you shouldn't), can you keep > > the > > > original uppercase panning model enums? > > > > That was for making code consensus before calling mutex lock with others since > > only here has empty line. If you want let empty line, I will. > > > > As you know, I added new enum, but there is webkit style checker error. So it > > also changed for code consensus. It can be changed if it is fine that new > added > > enum only has different style enum in header. If you want it, I will change > back > > to original uppercase style for two enum type, panningmodel and distancemodel. > > I leave it up to you. It's just that I don't like CLs that change parts of the > code that aren't really related to the CL itself. Definitely, you're right. It is good for others. I will seperate it.
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/232453003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_rel
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/232453003/160001
Message was sent while issue was closed.
Change committed as 172135 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
