Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
221243002:
Move many calls of lazyInitialize() from AudioContext to AudioNode. (Closed)
|
Created:
6 years, 8 months ago by KhNo Modified:
6 years, 8 months ago CC:
blink-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionMove many calls of lazyInitialize() from AudioContext to AudioNode.
lazyInitialize() has been called on many places on AudioContext.
All can be replaced to construct of AudioNode once.
Remove redundant codes.
BUG=none
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170829
Patch Set 1 #
Total comments: 14
Patch Set 2 : Applying review comment #Patch Set 3 : Add comment #
Total comments: 4
Patch Set 4 : Fix comments #
Messages
Total messages: 12 (0 generated)
Please review this patch.
https://codereview.chromium.org/221243002/diff/1/Source/modules/webaudio/Audi... File Source/modules/webaudio/AudioContext.cpp (left): https://codereview.chromium.org/221243002/diff/1/Source/modules/webaudio/Audi... Source/modules/webaudio/AudioContext.cpp:190: Don't gratuitously delete (or add) blank lines. https://codereview.chromium.org/221243002/diff/1/Source/modules/webaudio/Audi... Source/modules/webaudio/AudioContext.cpp:201: m_isInitialized = true; Why is this moved up? https://codereview.chromium.org/221243002/diff/1/Source/modules/webaudio/Audi... Source/modules/webaudio/AudioContext.cpp:348: ASSERT(isMainThread()); Should the ASSERT be moved to lazyInitialize? https://codereview.chromium.org/221243002/diff/1/Source/modules/webaudio/Audi... Source/modules/webaudio/AudioContext.cpp:368: ASSERT(isMainThread()); This ASSERT should probably not have been removed. https://codereview.chromium.org/221243002/diff/1/Source/modules/webaudio/Audi... Source/modules/webaudio/AudioContext.cpp:396: ASSERT(isMainThread()); This ASSERT should not have been removed. https://codereview.chromium.org/221243002/diff/1/Source/modules/webaudio/Audi... Source/modules/webaudio/AudioContext.cpp:611: Don't gratuitously delete (or add) blank lines.
PTAL :) and check my answer too. https://codereview.chromium.org/221243002/diff/1/Source/modules/webaudio/Audi... File Source/modules/webaudio/AudioContext.cpp (left): https://codereview.chromium.org/221243002/diff/1/Source/modules/webaudio/Audi... Source/modules/webaudio/AudioContext.cpp:190: On 2014/04/01 17:49:49, Raymond Toy wrote: > Don't gratuitously delete (or add) blank lines. Done. https://codereview.chromium.org/221243002/diff/1/Source/modules/webaudio/Audi... Source/modules/webaudio/AudioContext.cpp:201: m_isInitialized = true; On 2014/04/01 17:49:49, Raymond Toy wrote: > Why is this moved up? It should be inside if (m_destinationNode.get()) {m_destinationNode->initialize(); ........ } Currently, if lazyInitialize() is called once without actual m_destinationNode->initialize(); next calling is just return. When I create this patch, I meet this issue on creating AudioDestinationNode. https://codereview.chromium.org/221243002/diff/1/Source/modules/webaudio/Audi... Source/modules/webaudio/AudioContext.cpp:348: ASSERT(isMainThread()); On 2014/04/01 17:49:49, Raymond Toy wrote: > Should the ASSERT be moved to lazyInitialize? It couldn't. Those function can be called internally with wrong use. ASSERT(isMainThread()); gives to developer that should be called on main thread. If we move it to lazyIntialize, readability might be less https://codereview.chromium.org/221243002/diff/1/Source/modules/webaudio/Audi... Source/modules/webaudio/AudioContext.cpp:368: ASSERT(isMainThread()); On 2014/04/01 17:49:49, Raymond Toy wrote: > This ASSERT should probably not have been removed. It doesn't removed, Just move to top of function same as others. When "if (!mediaElement) ......" was added, it should have added under ASSERT(thread checking) https://codereview.chromium.org/221243002/diff/1/Source/modules/webaudio/Audi... Source/modules/webaudio/AudioContext.cpp:396: ASSERT(isMainThread()); On 2014/04/01 17:49:49, Raymond Toy wrote: > This ASSERT should not have been removed. ditto https://codereview.chromium.org/221243002/diff/1/Source/modules/webaudio/Audi... Source/modules/webaudio/AudioContext.cpp:611: On 2014/04/01 17:49:49, Raymond Toy wrote: > Don't gratuitously delete (or add) blank lines. Done.
https://codereview.chromium.org/221243002/diff/1/Source/modules/webaudio/Audi... File Source/modules/webaudio/AudioContext.cpp (left): https://codereview.chromium.org/221243002/diff/1/Source/modules/webaudio/Audi... Source/modules/webaudio/AudioContext.cpp:201: m_isInitialized = true; On 2014/04/01 18:25:08, KhNo wrote: > On 2014/04/01 17:49:49, Raymond Toy wrote: > > Why is this moved up? > > It should be inside if (m_destinationNode.get()) > {m_destinationNode->initialize(); ........ } > Currently, if lazyInitialize() is called once without actual > m_destinationNode->initialize(); > next calling is just return. When I create this patch, I meet this issue on > creating AudioDestinationNode. Ah. So it's because the destination node is created but not initialized until some audio node is created which calls lazyInitialize. Adding a comment would be useful here or near the if (m_destinationNode.get()) line. https://codereview.chromium.org/221243002/diff/1/Source/modules/webaudio/Audi... Source/modules/webaudio/AudioContext.cpp:348: ASSERT(isMainThread()); On 2014/04/01 18:25:08, KhNo wrote: > On 2014/04/01 17:49:49, Raymond Toy wrote: > > Should the ASSERT be moved to lazyInitialize? > > It couldn't. Those function can be called internally with wrong use. > > ASSERT(isMainThread()); gives to developer that should be called on main thread. > If we move it to lazyIntialize, readability might be less Ok. Same could be said for lazyInitialize. Now some magic happens inside the AudioNode to initialize the context. The current code makes it clear. Your proposal hides that magic initialization. I'll have to think about this.
> Ok. Same could be said for lazyInitialize. Now some magic happens inside the > AudioNode to initialize the context. The current code makes it clear. Your > proposal hides that magic initialization. I'll have to think about this. Yes, it can be. But in my humble opinion, don't developers generally want to consider adding "lazyinitialize();" when they create the new type of node ? I thought developer prefered somewhere initialization then each creation of nodes. I have added comments in header as well.
Just a few fixes for the comments. I would also consider adding a comment to m_isInitialized in AudioContext.h to describe what initialized means. Maybe something like // Set to true when the destination node has been initialized and is ready to process data. https://codereview.chromium.org/221243002/diff/40001/Source/modules/webaudio/... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/221243002/diff/40001/Source/modules/webaudio/... Source/modules/webaudio/AudioContext.cpp:188: // When a destination node of context is created, initialize() should be skipped to initialize afterwards by creation of other node. I think this reads better. Adjust as needed. // Creation of a destination node should not start the audio HW. The // creation of any other AudioNode will initialize the audio HW and start processing. https://codereview.chromium.org/221243002/diff/40001/Source/modules/webaudio/... File Source/modules/webaudio/AudioContext.h (right): https://codereview.chromium.org/221243002/diff/40001/Source/modules/webaudio/... Source/modules/webaudio/AudioContext.h:88: // This is called on constructor of AudioNode when the first node of context is created except destination node. I think this comment is slightly wrong because the destination node is an AudioNode. When the destination node is created the AudioNode constructor is called, but since m_destinationNode isn't set yet, nothing happens. Maybe the following is better? // The constructor of an AudioNode must call this to initialize the context.
PTAL :) I have added and fixed some comment as review. https://codereview.chromium.org/221243002/diff/40001/Source/modules/webaudio/... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/221243002/diff/40001/Source/modules/webaudio/... Source/modules/webaudio/AudioContext.cpp:188: // When a destination node of context is created, initialize() should be skipped to initialize afterwards by creation of other node. On 2014/04/02 17:11:32, Raymond Toy wrote: > I think this reads better. Adjust as needed. > > // Creation of a destination node should not start the audio HW. The > // creation of any other AudioNode will initialize the audio HW and start > processing. > Done. https://codereview.chromium.org/221243002/diff/40001/Source/modules/webaudio/... File Source/modules/webaudio/AudioContext.h (right): https://codereview.chromium.org/221243002/diff/40001/Source/modules/webaudio/... Source/modules/webaudio/AudioContext.h:88: // This is called on constructor of AudioNode when the first node of context is created except destination node. On 2014/04/02 17:11:32, Raymond Toy wrote: > I think this comment is slightly wrong because the destination node is an > AudioNode. When the destination node is created the AudioNode constructor is > called, but since m_destinationNode isn't set yet, nothing happens. > > Maybe the following is better? > // The constructor of an AudioNode must call this to initialize the context. Done.
On 2014/04/03 13:21:02, KhNo wrote: > PTAL :) I have added and fixed some comment as review. > > https://codereview.chromium.org/221243002/diff/40001/Source/modules/webaudio/... > File Source/modules/webaudio/AudioContext.cpp (right): > > https://codereview.chromium.org/221243002/diff/40001/Source/modules/webaudio/... > Source/modules/webaudio/AudioContext.cpp:188: // When a destination node of > context is created, initialize() should be skipped to initialize afterwards by > creation of other node. > On 2014/04/02 17:11:32, Raymond Toy wrote: > > I think this reads better. Adjust as needed. > > > > // Creation of a destination node should not start the audio HW. The > > // creation of any other AudioNode will initialize the audio HW and start > > processing. > > > > Done. > > https://codereview.chromium.org/221243002/diff/40001/Source/modules/webaudio/... > File Source/modules/webaudio/AudioContext.h (right): > > https://codereview.chromium.org/221243002/diff/40001/Source/modules/webaudio/... > Source/modules/webaudio/AudioContext.h:88: // This is called on constructor of > AudioNode when the first node of context is created except destination node. > On 2014/04/02 17:11:32, Raymond Toy wrote: > > I think this comment is slightly wrong because the destination node is an > > AudioNode. When the destination node is created the AudioNode constructor is > > called, but since m_destinationNode isn't set yet, nothing happens. > > > > Maybe the following is better? > > // The constructor of an AudioNode must call this to initialize the context. > > Done. 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/221243002/60001
On 2014/04/03 17:45:57, Raymond Toy wrote: > On 2014/04/03 13:21:02, KhNo wrote: > > PTAL :) I have added and fixed some comment as review. > > > > > https://codereview.chromium.org/221243002/diff/40001/Source/modules/webaudio/... > > File Source/modules/webaudio/AudioContext.cpp (right): > > > > > https://codereview.chromium.org/221243002/diff/40001/Source/modules/webaudio/... > > Source/modules/webaudio/AudioContext.cpp:188: // When a destination node of > > context is created, initialize() should be skipped to initialize afterwards by > > creation of other node. > > On 2014/04/02 17:11:32, Raymond Toy wrote: > > > I think this reads better. Adjust as needed. > > > > > > // Creation of a destination node should not start the audio HW. The > > > // creation of any other AudioNode will initialize the audio HW and start > > > processing. > > > > > > > Done. > > > > > https://codereview.chromium.org/221243002/diff/40001/Source/modules/webaudio/... > > File Source/modules/webaudio/AudioContext.h (right): > > > > > https://codereview.chromium.org/221243002/diff/40001/Source/modules/webaudio/... > > Source/modules/webaudio/AudioContext.h:88: // This is called on constructor of > > AudioNode when the first node of context is created except destination node. > > On 2014/04/02 17:11:32, Raymond Toy wrote: > > > I think this comment is slightly wrong because the destination node is an > > > AudioNode. When the destination node is created the AudioNode constructor > is > > > called, but since m_destinationNode isn't set yet, nothing happens. > > > > > > Maybe the following is better? > > > // The constructor of an AudioNode must call this to initialize the context. > > > > Done. > > lgtm Thanks, Raymond.
Message was sent while issue was closed.
Change committed as 170829 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
