Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7497025:
refactor AudioInputDevice to remove race condition. (Closed)
|
Created:
9 years, 5 months ago by wjia(left Chromium) Modified:
9 years, 4 months ago CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, jam, acolwell+watch_chromium.org, annacc+watch_chromium.org, joi+watch-content_chromium.org, ajwong+watch_chromium.org, vrk (LEFT CHROMIUM) Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
Descriptionrefactor AudioInputDevice to remove race condition.
fix resource loss.
refactor API between AudioInputDevice and WebRtcAudioDeviceImpl.
The racing condition is: |stream_id_| is accessed from different thread. More specifically, client could call AudioInputDevice::Stop before AudioInputDevice::InitializeOnIOThread, AudioInputDevice::StartOnIOThread or AudioInputDevice::OnLowLatencyCreated is executed.
BUG=none
TEST=trybots
Patch Set 1 #Patch Set 2 : add a new thread and keep joinable thread #Patch Set 3 : support repeating start/stop and fix resource loss #Patch Set 4 : add comments #
Total comments: 57
Patch Set 5 : code review #
Total comments: 2
Patch Set 6 : code review #Patch Set 7 : async API #Patch Set 8 : rebase + minor update #
Total comments: 37
Patch Set 9 : code review #
Messages
Total messages: 15 (0 generated)
Thanks Wei! Added some comments (mainly questions actually). Feels great but there are some areas where I feel that clarifications can be made. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... File content/renderer/media/audio_input_device.cc (right): http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:25: capture_thread_("AudioInputDevice"), Do we use camel-back style for thread names? http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:31: message_loop_proxy_ = capture_thread_.message_loop_proxy(); Sorry, again, why not message_loop()? Also, why is is called such a generic name? It might be more clear to give it a name so we know that tasks on this thread runs on the main/capture thread. Just and idea; perhaps your style is the preferred in Chrome. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:41: capture_thread_.Stop(); You did not like DCHECK_EQ(0, stream_id_)? http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:47: message_loop_proxy_->PostTask(FROM_HERE, Why bool if false is not an option? http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:53: message_loop_proxy_->PostTask(FROM_HERE, Why bool? http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:70: // Make sure we don't call Start() more than once. Must we check stream_id on the capture thread? Could it not be checked one level above this method? http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:82: ChildProcess::current()->io_message_loop_proxy()->PostTask(FROM_HERE, why proxy (again) ;-) ? It was jam who initially suggested ->io_message_loop()-> http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:84: delegate_event_.Wait(); I have only done a quick check so far but it feels like the new Start design is "not as clean" as the old version. The fact that this event is needed is one indication. Was there actually a potential race in the old version? Or is the new event a requirement now when the new worker thread has been added? My point is, it looks OK but was the current change really required for Start? I am not saying that we should change. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:86: Send(new AudioInputHostMsg_CreateStream(stream_id_, params, true)); Add comment about that true<=>low-latency audio stream. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:102: if (!stream_id_) { How could this happen? If so, can't we detect it earlier? http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:114: new base::DelegateSimpleThread(this, "renderer_audio_input_thread")); Note style for thread name and compare with AudioInputDevice thread. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:130: Send(new AudioInputHostMsg_CloseStream(stream_id_)); We wait here on Start. Is it not suitable to do so on Stop as well? If not, perhaps a comment to explain why not. Or, perhaps the comment should be in Start to explain why we must wait for the stream_id. Hope my comment makes sense. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:136: audio_thread_.reset(NULL); Is NULL really needed here? http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:139: delegate_event_.Wait(); OK, you wait here instead. Hmm. Not sure why. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:163: ChildProcess::current()->io_message_loop_proxy()->PostTask(FROM_HERE, It feels more clear to name this SendOnIOThread() now when it is called differently (not only on the IO as before)? http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:220: stream_id_ = filter_->AddDelegate(this); Would be nice with a VLOG(1) here. Feels great to be able to see what stream IDs we create. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:225: filter_->RemoveDelegate(stream_id); VLOG(1) please. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... File content/renderer/media/audio_input_device.h (right): http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.h:109: // be executed on that thread. Perhaps mention that this thread is the main working thread. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.h:126: // Add/Remove delegate on IO thread. Use same comment style as above, i.e., --------------------? http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.h:151: scoped_refptr<base::MessageLoopProxy> message_loop_proxy_; Sorry, but I actually don't understand why a proxy is needed. What is the advantage? Why not access the thread message loop directly? http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.h:153: // These 2 WaitableEvents are added to translate async operation (between two http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.h:155: // AudioInputDevice and ADM). The main reason is ADM can delete that the http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.h:156: // AudioInputDevice anytime after Stop() is called. It's highly recommended Hmm, we state that events are added to emulate sync operation and then we say that async is highly recommended. Can this comment/section be more clear? Also, today Start is async and Stop is sync. Does this section deal with Stop only?Feels like the complete comment section can be even more clear. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... File content/renderer/media/audio_input_message_filter.cc (right): http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_message_filter.cc:75: base::SyncSocket::Handle socket_handle = socket_descriptor.fd; Is this part required for this CL or is it "only", "nice to add while we are fixing things"?
Almost forgot. What happened to the thread-priority? We should set a higher prio, right?
http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... File content/renderer/media/audio_input_device.cc (right): http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:41: capture_thread_.Stop(); It is for thread safety, stream_id_ is only accessed on the capture thread. On 2011/07/30 15:50:59, henrika wrote: > You did not like DCHECK_EQ(0, stream_id_)? http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:70: // Make sure we don't call Start() more than once. ditto, thread safety. On 2011/07/30 15:50:59, henrika wrote: > Must we check stream_id on the capture thread? Could it not be checked one level > above this method? http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:84: delegate_event_.Wait(); My understanding is that this delegate_event.Wait() in the new Start() is used to sync with the IO thread on waiting for stream_id_ to be updated. Maybe Wei have something to add here. Yes, there was a potential race in the old version. Calling Start/Stop in sequence is an example. On 2011/07/30 15:50:59, henrika wrote: > I have only done a quick check so far but it feels like the new Start design is > "not as clean" as the old version. The fact that this event is needed is one > indication. Was there actually a potential race in the old version? Or is the > new event a requirement now when the new worker thread has been added? > > My point is, it looks OK but was the current change really required for Start? I > am not saying that we should change. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:104: base::SyncSocket socket(socket_handle); Sorry, the same question as Henrik has, + why do we need: base::SharedMemory::CloseHandle(handle); base::SyncSocket socket(socket_handle); http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:133: if (audio_thread_.get()) { shall we have a state here instead? Think about the following scenario: audio_thread_ was joined, then user call start/stop in sequence, then we may get into this ShutDownOnCaptureThread() before OnLowLatencyCreated(), then we will call join() twice, which may crash the browser. Or OnLowLatencyCreated() never comes, and user calls Stop, it will also join the audio_thread_. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... File content/renderer/media/audio_input_device.h (right): http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.h:155: // AudioInputDevice and ADM). The main reason is ADM can delete AudioInputDevice and client). The main reason is client can .. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.h:157: // to use async API between ADM and AudioInputDevice. client?
I will refactor ADM as well. After that, the WaitableEvents might be gone. The code to create "audio_thread_" and set its priority has not been changed. It's just a matter that it's not implemented to change DelegateSimpleThread's priority on Linux. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... File content/renderer/media/audio_input_device.cc (right): http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:25: capture_thread_("AudioInputDevice"), On 2011/07/30 15:50:59, henrika wrote: > Do we use camel-back style for thread names? Yes. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:31: message_loop_proxy_ = capture_thread_.message_loop_proxy(); On 2011/07/30 15:50:59, henrika wrote: > Sorry, again, why not message_loop()? > Also, why is is called such a generic name? It might be more clear to give it a > name so we know that tasks on this thread runs on the main/capture thread. Just > and idea; perhaps your style is the preferred in Chrome. In audio_input_device.h, the difference between MessageLoop and MessageLoopProxy has been explained. message_loop_proxy_ is renamed to capture_message_loop_proxy_. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:41: capture_thread_.Stop(); On 2011/07/30 15:50:59, henrika wrote: > You did not like DCHECK_EQ(0, stream_id_)? Shijing has the answer. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:47: message_loop_proxy_->PostTask(FROM_HERE, On 2011/07/30 15:50:59, henrika wrote: > Why bool if false is not an option? I didn't change ADM. ADM should be refactored as well. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:53: message_loop_proxy_->PostTask(FROM_HERE, On 2011/07/30 15:50:59, henrika wrote: > Why bool? Same reason for Start(). http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:82: ChildProcess::current()->io_message_loop_proxy()->PostTask(FROM_HERE, On 2011/07/30 15:50:59, henrika wrote: > why proxy (again) ;-) ? It was jam who initially suggested ->io_message_loop()-> MessageLoopProxy is preferred over MessageLoop. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:84: delegate_event_.Wait(); On 2011/07/30 15:50:59, henrika wrote: > I have only done a quick check so far but it feels like the new Start design is > "not as clean" as the old version. The fact that this event is needed is one > indication. Was there actually a potential race in the old version? Or is the > new event a requirement now when the new worker thread has been added? > > My point is, it looks OK but was the current change really required for Start? I > am not saying that we should change. Shijing has the answer. The delegate has to be added on IO thread (without lock). Without delegate_event_, it's needed to have some message posting back and forth between capture_thread_ and io thread, just like what's done in VideoCaptureImpl. That will result in a much bigger change. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:86: Send(new AudioInputHostMsg_CreateStream(stream_id_, params, true)); On 2011/07/30 15:50:59, henrika wrote: > Add comment about that true<=>low-latency audio stream. ?? There was no comment for this. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:102: if (!stream_id_) { On 2011/07/30 15:50:59, henrika wrote: > How could this happen? If so, can't we detect it earlier? When Stop is called before OnLowLatencyCreated is called. This is rare, but still a corner case. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:104: base::SyncSocket socket(socket_handle); On 2011/08/01 11:41:23, xians wrote: > Sorry, the same question as Henrik has, + why do we need: > base::SharedMemory::CloseHandle(handle); > base::SyncSocket socket(socket_handle); both handles should be closed. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:114: new base::DelegateSimpleThread(this, "renderer_audio_input_thread")); On 2011/07/30 15:50:59, henrika wrote: > Note style for thread name and compare with AudioInputDevice thread. I missed this one copied from previous code. Changed it to camelCase. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:130: Send(new AudioInputHostMsg_CloseStream(stream_id_)); On 2011/07/30 15:50:59, henrika wrote: > We wait here on Start. Is it not suitable to do so on Stop as well? If not, > perhaps a comment to explain why not. > > Or, perhaps the comment should be in Start to explain why we must wait for the > stream_id. Hope my comment makes sense. I moved Wait to a little bit later to allow IO thread and audio_thread_ execute something in parallel. Just a matter of optimization to save some time. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:133: if (audio_thread_.get()) { On 2011/08/01 11:41:23, xians wrote: > shall we have a state here instead? > Think about the following scenario: > audio_thread_ was joined, then user call start/stop in sequence, then we may get > into this ShutDownOnCaptureThread() before OnLowLatencyCreated(), then we will > call join() twice, which may crash the browser. > Or OnLowLatencyCreated() never comes, and user calls Stop, it will also join the > audio_thread_. Immediately after audio_thread_ is joined, it's set to NULL and will not be joined again. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:136: audio_thread_.reset(NULL); On 2011/07/30 15:50:59, henrika wrote: > Is NULL really needed here? yes, in Shijing's exemplary use case, 2nd Shutdown can happen before 2nd OnLowLatencyCreated is called. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:139: delegate_event_.Wait(); On 2011/07/30 15:50:59, henrika wrote: > OK, you wait here instead. Hmm. Not sure why. Explained as above. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:163: ChildProcess::current()->io_message_loop_proxy()->PostTask(FROM_HERE, On 2011/07/30 15:50:59, henrika wrote: > It feels more clear to name this SendOnIOThread() now when it is called > differently (not only on the IO as before)? Done. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:220: stream_id_ = filter_->AddDelegate(this); On 2011/07/30 15:50:59, henrika wrote: > Would be nice with a VLOG(1) here. Feels great to be able to see what stream IDs > we create. Done. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:225: filter_->RemoveDelegate(stream_id); On 2011/07/30 15:50:59, henrika wrote: > VLOG(1) please. Done. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... File content/renderer/media/audio_input_device.h (right): http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.h:109: // be executed on that thread. On 2011/07/30 15:50:59, henrika wrote: > Perhaps mention that this thread is the main working thread. Done. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.h:126: // Add/Remove delegate on IO thread. On 2011/07/30 15:50:59, henrika wrote: > Use same comment style as above, i.e., --------------------? Done. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.h:151: scoped_refptr<base::MessageLoopProxy> message_loop_proxy_; On 2011/07/30 15:50:59, henrika wrote: > Sorry, but I actually don't understand why a proxy is needed. What is the > advantage? Why not access the thread message loop directly? MessageLoop and MessageLoopProxy are very similar, except that MessageLoopProxy can outlive the target thread and is a safe way to hold on to message loop pointers and leads to far less brittle code. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.h:153: // These 2 WaitableEvents are added to translate async operation (between On 2011/07/30 15:50:59, henrika wrote: > two Done. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.h:155: // AudioInputDevice and ADM). The main reason is ADM can delete On 2011/07/30 15:50:59, henrika wrote: > that the Done. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.h:156: // AudioInputDevice anytime after Stop() is called. It's highly recommended On 2011/07/30 15:50:59, henrika wrote: > Hmm, we state that events are added to emulate sync operation and then we say > that async is highly recommended. Can this comment/section be more clear? Also, > today Start is async and Stop is sync. Does this section deal with Stop > only?Feels like the complete comment section can be even more clear. The recommendation is to get rid of emulating sync operation. Actually, Start always returns "true" now, which is not quite right. This is just an intermediate stage before changing API between AudioInputDevice and ADM. Eventually, ADM will be an EventHanlder of AudioInputDevice which receives status update events from AudioInputDevice. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_device.h:157: // to use async API between ADM and AudioInputDevice. On 2011/08/01 11:41:23, xians wrote: > client? Done. http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... File content/renderer/media/audio_input_message_filter.cc (right): http://codereview.chromium.org/7497025/diff/11001/content/renderer/media/audi... content/renderer/media/audio_input_message_filter.cc:75: base::SyncSocket::Handle socket_handle = socket_descriptor.fd; On 2011/07/30 15:50:59, henrika wrote: > Is this part required for this CL or is it "only", "nice to add while we are > fixing things"? It's necessary to fix possible resource leakage. In case delegate has been removed from filter before LowLatencyStreamCreated message arrives, both handles will be lost.
Thanks. Added some minor comments. http://codereview.chromium.org/7497025/diff/15001/content/renderer/media/audi... File content/renderer/media/audio_input_device.h (right): http://codereview.chromium.org/7497025/diff/15001/content/renderer/media/audi... content/renderer/media/audio_input_device.h:108: // The following methods are tasks posted on capture_thread_ that needs to Don't think we should use the exact name here. Please use capture thread instead. http://codereview.chromium.org/7497025/diff/15001/content/renderer/media/audi... content/renderer/media/audio_input_device.h:117: void SendOnIOThread(IPC::Message* message); Move to other IO-methods.
The refactoring of AudioInputDevice and ADM is done as of Patch Set 7. Now the AudioInputDevice API's are async. I didn't change audio output part of ADM. The patch is also supporting repetitive start/stop sequence with a same AudioInputDevice.
Thanks for doing this work Wei, looks nice so far. Added some comments and questions. I am still on vacation, hence might miss some details. You mention: "Now the AudioInputDevice API's are async.". Think I've asked same question before but StopRecording is still "emulated sync". Can you elaborate? It is essential that we comment all new parts carefully since similar changes are required in the AudioDevice. http://codereview.chromium.org/7497025/diff/21003/content/content_renderer.gypi File content/content_renderer.gypi (right): http://codereview.chromium.org/7497025/diff/21003/content/content_renderer.gy... content/content_renderer.gypi:221: '../third_party/webrtc/modules/audio_coding/main/source/audio_coding_module.gyp:audio_coding_module', Not required by this CL, right? http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/audi... File content/renderer/media/audio_input_device.cc (right): http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:18: AudioInputDeviceEventHandler* handler, Just an idea; could it be useful to allow NULL here? Gives user the option to enable/disable event handling. http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:84: event_handler_->OnRecordingStarted(); Why do we want to signal RecordingStarted here? http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:120: event_handler_->OnRecordingStarted(); Why? http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:183: DCHECK(ChildProcess::current()->io_message_loop_proxy()-> Why is this test needed? http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/audi... File content/renderer/media/audio_input_device.h (right): http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/audi... content/renderer/media/audio_input_device.h:29: // AudioInputMsg_NotifyLowLatencyStreamCreated <-- ? http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/audi... content/renderer/media/audio_input_device.h:126: // Methods called on main working thread for AudioInputDevice---------------- e - (missing space) http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/audi... content/renderer/media/audio_input_device.h:127: // The following methods are tasks posted on capturethread that needs to capture thread http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/audi... File content/renderer/media/audio_input_device_event_handler.h (right): http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/audi... content/renderer/media/audio_input_device_event_handler.h:10: // Audio capture has been started. Just thinking; we mix "input", "capture" and "record" in this file... http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/webr... File content/renderer/media/webrtc_audio_device_impl.cc (right): http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/webr... content/renderer/media/webrtc_audio_device_impl.cc:50: adm_message_loop_.get(), Perhaps add a comment on what we want to achieve by feeding this thread as input to the AudioInputDevice. http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/webr... content/renderer/media/webrtc_audio_device_impl.cc:457: adm_message_loop_->PostTask(FROM_HERE, Can't we set state in this method directly? It is called from the ADM thread, right? http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/webr... content/renderer/media/webrtc_audio_device_impl.cc:469: adm_message_loop_->PostTask(FROM_HERE, Same comment as for Start. http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/webr... File content/renderer/media/webrtc_audio_device_impl.h (right): http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/webr... content/renderer/media/webrtc_audio_device_impl.h:82: // This class must be created on the main render thread since it creates Would be great with some extended comments about the new ADM thread (only used on input side so far). http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/webr... content/renderer/media/webrtc_audio_device_impl.h:90: public AudioInputDeviceEventHandler { Could have been defines using same style as AudioInputDevice::CaptureCallback, i.e, not in a separate file. You choose not to because...? http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/webr... content/renderer/media/webrtc_audio_device_impl.h:103: size_t audio_delay_milliseconds) OVERRIDE; General question; what is the benefit of using this macro? http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/webr... content/renderer/media/webrtc_audio_device_impl.h:260: // Helpers running on Adm thread, corresponding to API functions. ADM=Audio Device Module (should be ADM, right?) But hey, I did not call this class WebRTCAudioDeviceImpl either... Compromise proposal, use ADM in comment, Adm in method signatures. http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/webr... content/renderer/media/webrtc_audio_device_impl.h:297: base::Lock lock_; Comment on what it protects. http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/webr... content/renderer/media/webrtc_audio_device_impl.h:320: DISABLE_RUNNABLE_METHOD_REFCOUNT(WebRtcAudioDeviceImpl); Why are these macros required now?
Could you describe in the CL comment what the race condition we're solving is? It's not clear anywhere in this patch as to what we're actually fixing. I'm also concerned that the solution is to add yet another thread and some locks. Is there seriously not an existing thread we can be using? Why does AudioInputDevice need to get involved? We're increasing the # of threads running through AudioInputDevice and WebRtcAudioDeviceImpl by one (Capture thread/ADM thread). Can we localize this change somehow?
http://codereview.chromium.org/7497025/diff/21003/content/content_renderer.gypi File content/content_renderer.gypi (right): http://codereview.chromium.org/7497025/diff/21003/content/content_renderer.gy... content/content_renderer.gypi:221: '../third_party/webrtc/modules/audio_coding/main/source/audio_coding_module.gyp:audio_coding_module', On 2011/08/07 16:52:27, henrika wrote: > Not required by this CL, right? This is leftover from test code. I will remove this one once the patch is finalized. http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/audi... File content/renderer/media/audio_input_device.cc (right): http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:18: AudioInputDeviceEventHandler* handler, On 2011/08/07 16:52:27, henrika wrote: > Just an idea; could it be useful to allow NULL here? Gives user the option to > enable/disable event handling. The client is required to wait for "stopped" signal before it can destruct AudioInputDevice. Therefore, non-NULL handler is required. http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:84: event_handler_->OnRecordingStarted(); On 2011/08/07 16:52:27, henrika wrote: > Why do we want to signal RecordingStarted here? The interface is: client calls Start() and expects "started", calls Stop() and expects "stopped". http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:120: event_handler_->OnRecordingStarted(); On 2011/08/07 16:52:27, henrika wrote: > Why? Same as above. http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:183: DCHECK(ChildProcess::current()->io_message_loop_proxy()-> On 2011/08/07 16:52:27, henrika wrote: > Why is this test needed? This is a public function. http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/audi... File content/renderer/media/audio_input_device.h (right): http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/audi... content/renderer/media/audio_input_device.h:29: // AudioInputMsg_NotifyLowLatencyStreamCreated On 2011/08/07 16:52:27, henrika wrote: > <-- ? This is the message coming from browser process and triggers a call to OnLowLatencyCreated. All these happen on IO thread. http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/audi... content/renderer/media/audio_input_device.h:126: // Methods called on main working thread for AudioInputDevice---------------- On 2011/08/07 16:52:27, henrika wrote: > e - (missing space) Done. http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/audi... content/renderer/media/audio_input_device.h:127: // The following methods are tasks posted on capturethread that needs to On 2011/08/07 16:52:27, henrika wrote: > capture thread Done. http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/audi... File content/renderer/media/audio_input_device_event_handler.h (right): http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/audi... content/renderer/media/audio_input_device_event_handler.h:10: // Audio capture has been started. On 2011/08/07 16:52:27, henrika wrote: > Just thinking; we mix "input", "capture" and "record" in this file... replace "capture" with "record". I will keep "record" since it's in one of the messages. http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/webr... File content/renderer/media/webrtc_audio_device_impl.cc (right): http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/webr... content/renderer/media/webrtc_audio_device_impl.cc:50: adm_message_loop_.get(), On 2011/08/07 16:52:27, henrika wrote: > Perhaps add a comment on what we want to achieve by feeding this thread as input > to the AudioInputDevice. Added some comments in AudioInputDevice constructor. http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/webr... content/renderer/media/webrtc_audio_device_impl.cc:457: adm_message_loop_->PostTask(FROM_HERE, On 2011/08/07 16:52:27, henrika wrote: > Can't we set state in this method directly? It is called from the ADM thread, > right? Setting state here put some assumption on AudioInputDevice implementation. http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/webr... content/renderer/media/webrtc_audio_device_impl.cc:469: adm_message_loop_->PostTask(FROM_HERE, On 2011/08/07 16:52:27, henrika wrote: > Same comment as for Start. See comments above. http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/webr... File content/renderer/media/webrtc_audio_device_impl.h (right): http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/webr... content/renderer/media/webrtc_audio_device_impl.h:82: // This class must be created on the main render thread since it creates On 2011/08/07 16:52:27, henrika wrote: > Would be great with some extended comments about the new ADM thread (only used > on input side so far). Done. http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/webr... content/renderer/media/webrtc_audio_device_impl.h:90: public AudioInputDeviceEventHandler { On 2011/08/07 16:52:27, henrika wrote: > Could have been defines using same style as AudioInputDevice::CaptureCallback, > i.e, not in a separate file. You choose not to because...? see Nested Classes in http://www.chromium.org/developers/coding-style http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/webr... content/renderer/media/webrtc_audio_device_impl.h:103: size_t audio_delay_milliseconds) OVERRIDE; On 2011/08/07 16:52:27, henrika wrote: > General question; what is the benefit of using this macro? The compiler can detect inconsistent change when the function is changed in base class. http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/webr... content/renderer/media/webrtc_audio_device_impl.h:260: // Helpers running on Adm thread, corresponding to API functions. On 2011/08/07 16:52:27, henrika wrote: > ADM=Audio Device Module (should be ADM, right?) But hey, I did not call this > class WebRTCAudioDeviceImpl either... > Compromise proposal, use ADM in comment, Adm in method signatures. Done. http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/webr... content/renderer/media/webrtc_audio_device_impl.h:297: base::Lock lock_; On 2011/08/07 16:52:27, henrika wrote: > Comment on what it protects. Done. http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/webr... content/renderer/media/webrtc_audio_device_impl.h:320: DISABLE_RUNNABLE_METHOD_REFCOUNT(WebRtcAudioDeviceImpl); On 2011/08/07 16:52:27, henrika wrote: > Why are these macros required now? These are needed for NewRunnableMethod. http://codereview.chromium.org/7497025/diff/21003/content/renderer/media/webr... content/renderer/media/webrtc_audio_device_impl.h:320: DISABLE_RUNNABLE_METHOD_REFCOUNT(WebRtcAudioDeviceImpl); On 2011/08/07 16:52:27, henrika wrote: > Why are these macros required now? These are needed for NewRunnableMethod.
On 2011/08/08 21:45:59, scherkus wrote: > Could you describe in the CL comment what the race condition we're solving is? > It's not clear anywhere in this patch as to what we're actually fixing. > CL description has been updated. > I'm also concerned that the solution is to add yet another thread and some > locks. Is there seriously not an existing thread we can be using? Why does > AudioInputDevice need to get involved? > The lock is added for one sync writer thread (input) to access variable (output_delay_ms_) modified on another sync writer (output) in WebRtcAudioDeviceImpl. The thread is added to remove racing condition in AudioInputDevice. They are separate issues. If a new thread is not preferred, we need add some sync mechanism (such WaitableEvent) when client of AudioInputDevice calls Stop. That's the Patch Set 3 which keeps synchronous API for AudioInputDevice. > We're increasing the # of threads running through AudioInputDevice and > WebRtcAudioDeviceImpl by one (Capture thread/ADM thread). Can we localize this > change somehow?
Random idea... it's definitely more refactoring but I think it should help clean up the code quite a bit I think this code would benefit greatly from having a proxy for webrtc::AudioDeviceModule and the actual implementation for example currently most methods currently return -1 or do nothing. this patch starts adding in sync-to-async method call conversion. you could likely isolate all of this code into a separate class as a proxy, then delegate/forward any necessary calls to the existing WebRtcAudioDeviceImpl. the idea being that if such locking/threading/etc.. code is unavoidable, it's better to at least isolate it inside of a class any thoughts?
I suggest that we start by fixing the race condition without changing the
current contract.
Currently the Start and Stop methods check (at least) two variables that are
modified on
the IO thread, stream_id and the audio thread (I forget the exact variable
name).
Basically we do something like (from memory):
bool Start() {
if (stream_id_)
return false;
PostIoWorkItem(&ActualStartFn);
return true;
}
bool Stop() {
if (!stream_id)
return false;
PostIoWorkItem(&ActualStopFn);
audio_thread_.Join();
return true;
}
Assignment to stream_id_ and audio_thread_ is done from the IO thread.
Also note that Start() isn't synchronous although it does return bool.
So, instead, I suggest that we start by changing this along these lines:
bool Start() {
PostIoWorkItem(&ActualStartFn);
return true;
}
bool Stop() {
WaitableEvent event;
PostIoWorkItem(&ActualStopFn, &event);
event.Wait();
return true;
}
Then in the implementation that runs on the IO thread, we'll actually touch
the member variables:
void ActualStartFn() {
// Runs on IO thread.
if (stream_id_)
return;
// rest as current
}
void ActualStopFn(..., WaitableEvent* event) {
// Runs on IO thread.
if (!stream_id_) {
event->Signal();
return;
}
// current implementation
// Here, we could do one of:
//
// 1) Join on the IO thread. (two threads will effectively wait).
audio_thread.Join();
audio_thread.reset();
event->Signal();
// 2) Signal from the audio thread. No extra wait.
audio_thread_.StopAndSignal(event, audio_thread_.get());
audio_thread_.release(); // Object will be closed/deleted from the audio
thread itself.
// 3) Spawn a temporary worker thread that does nothing but
// wait for the audiothread and then signals the event.
// Again, there are two threads that wait, but one is short lived.
ThreadHandle handle = audio_thread_.release(); // must remember to clear
audio_thread_ from the IO thread.
SpawnThreadToWaitAndSignal(handle, event);
}
Tommi
On Tue, Aug 9, 2011 at 11:39 PM, <scherkus@chromium.org> wrote:
> Random idea... it's definitely more refactoring but I think it should help
> clean
> up the code quite a bit
>
> I think this code would benefit greatly from having a proxy for
> webrtc::AudioDeviceModule and the actual implementation
>
> for example currently most methods currently return -1 or do nothing. this
> patch
> starts adding in sync-to-async method call conversion.
>
> you could likely isolate all of this code into a separate class as a proxy,
> then
> delegate/forward any necessary calls to the existing WebRtcAudioDeviceImpl.
>
> the idea being that if such locking/threading/etc.. code is unavoidable,
> it's
> better to at least isolate it inside of a class
>
> any thoughts?
>
>
>
http://codereview.chromium.**org/7497025/<http://codereview.chromium.org/7497...
>
Still on vacation; don't want to make too detailed comments. Some general feedback: I like Tommi's idea of fixing the main issues under the current contract. We have more issues in our list and I suggest that we discuss the solution for them one by one to ensure that the consequences are OK. Moving "unique" stuff up the chain (to the ADM or to a proxy) feels like a good idea for these parts. It is important that we don't forget that the changes we make in AudioInputDevice should most likely be adopted by AudioDevice as well. Hence, they must be clear and well defined and should not force AudioDevice clients to make too large changes. In the end, we must have good explainaitions why each change is actually required.
Hi, I took some time to try out Tommi's proposals today, and I got two questions and would like to know your opinions on them. 1, how bad is it to join the audio_thread_ on the IO thread. Since the stop command comes from the ADM, we can have the audio_thread_ Capture callback returns immediately when it detects the call has been stopped. In this case, the cost on joining the audio_thread_ on IO thread should not be much. In case joining on IO thread is not acceptable. 2, can we just use locks on Stop() and OnLowLatencyCreated()? Currently we join the audio_thread_ in Stop(), but audio_thread_ is also accessed in OnLowLatencyCreated() on IO thread. If we can consider using locks on them, we can keep the existing design. And since OnLowLatencyCreated() and Stop() are normally called only once during a call, so the lock should not matter much. BR, /SX On Wed, Aug 10, 2011 at 10:18 AM, <henrika@google.com> wrote: > Still on vacation; don't want to make too detailed comments. > > Some general feedback: > > I like Tommi's idea of fixing the main issues under the current contract. > > We have more issues in our list and I suggest that we discuss the solution > for > them one by one to ensure that the consequences are OK. Moving "unique" > stuff up > the chain (to the ADM or to a proxy) feels like a good idea for these > parts. > > It is important that we don't forget that the changes we make in > AudioInputDevice should most likely be adopted by AudioDevice as well. > Hence, > they must be clear and well defined and should not force AudioDevice > clients to > make too large changes. In the end, we must have good explainaitions why > each > change is actually required. > > > http://codereview.chromium.**org/7497025/<http://codereview.chromium.org/7497... >
IF it's "really" desired to have a synchronous API (between AudioInputDevice and ADM), there are several ways to fix the racing condition. But note that sync API might not work well when AudioInputDevice grows full-fledged, e.g., adding error handling code to process errors from browser process. Anyway, here are the possible fixes for sync API: fix #1: Change AudioInputDevice::Start() to real sync mode. This is the easiest, but client has to wait a bit longer during starting. The current Start() returns after it posts a task to IO thread. But AudioInputDevice is still in the middle of starting recording by IPC back and forth with browser process. If Start() can wait till Record message is sent to browser process, then racing condition is gone since there is no other async message coming from browser process for now. fix #2: This will be a bit complicated. Make all member variables accessed on IO thread. AudioInputDevice::ShutDownOnIOThread also closes socket_ as its last operation. When audio_thread_ exits, it signals a WaitableEvent which Stop() is waiting on. Then Stop() joins audio_thread_. Furthermore, AudioInputDevice needs to handle the timing when Stop() is called. Stop() could be called before AudioInputDevice::OnLowLatencyCreated is called, or before AudioInputDevice::StartOnIOThread is called. Wei On Wed, Aug 10, 2011 at 12:28 PM, Shijing Xian <xians@google.com> wrote: > > Hi, > > I took some time to try out Tommi's proposals today, and I got two > questions and would like to know your opinions on them. > > 1, how bad is it to join the audio_thread_ on the IO thread. > Since the stop command comes from the ADM, we can have the audio_thread_ > Capture callback returns immediately when it detects the call has been > stopped. > In this case, the cost on joining the audio_thread_ on IO thread should not > be much. > > In case joining on IO thread is not acceptable. > 2, can we just use locks on Stop() and OnLowLatencyCreated()? > Currently we join the audio_thread_ in Stop(), but audio_thread_ is also > accessed in OnLowLatencyCreated() on IO thread. > If we can consider using locks on them, we can keep the existing design. > And since OnLowLatencyCreated() and Stop() are normally called only once > during a call, so the lock should not matter much. > > > BR, > /SX > > > On Wed, Aug 10, 2011 at 10:18 AM, <henrika@google.com> wrote: > >> Still on vacation; don't want to make too detailed comments. >> >> Some general feedback: >> >> I like Tommi's idea of fixing the main issues under the current contract. >> >> We have more issues in our list and I suggest that we discuss the solution >> for >> them one by one to ensure that the consequences are OK. Moving "unique" >> stuff up >> the chain (to the ADM or to a proxy) feels like a good idea for these >> parts. >> >> It is important that we don't forget that the changes we make in >> AudioInputDevice should most likely be adopted by AudioDevice as well. >> Hence, >> they must be clear and well defined and should not force AudioDevice >> clients to >> make too large changes. In the end, we must have good explainaitions why >> each >> change is actually required. >> >> >> http://codereview.chromium.**org/7497025/<http://codereview.chromium.org/7497... >> > > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
