Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7661017:
Refactor AudioInputDevice to remove race conditions and allow more flexible calling sequences. (Closed)
|
Created:
9 years, 4 months ago by henrika (OOO until Aug 14) Modified:
9 years, 4 months ago Reviewers:
tommi (sloooow) - chröme, Chris Rogers, henrika_dont_use, no longer working on chromium, wjia(left Chromium), scherkus (not reviewing) 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://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionRefactor AudioInputDevice to remove race conditions and allow more flexible calling sequences.
It also ensures that the AudioInputDevice can be destroyed during an active audio session
without crashing.
This CL is a summary of http://codereview.chromium.org/7497025, i.e., it removes potential
race conditions but in a more "lightweight" manner.
BUG=none
TEST=trybots
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98198
Patch Set 1 #
Total comments: 10
Patch Set 2 : Adds wait timeout #Patch Set 3 : Fixes spelling error #Patch Set 4 : Fixes potential resource leak due to removed delegate before LowLatencyStreamCreated #Patch Set 5 : Added TODO comment in AudioInputDevice #Patch Set 6 : Removed Stop call in destructor. Added DCHECK instead. #
Total comments: 2
Patch Set 7 : Improved comments and added CHECK() macro in destructor #
Total comments: 17
Patch Set 8 : Minor changes based on review by Andrew #
Messages
Total messages: 22 (0 generated)
This CL is an attempt to summarize all the proposed modifications of the AudioInputDevice. As we discussed, the initial CL became a bit too complex and we decided to take a few steps back. This CL combines input from Wei, Tommi, Shijing and Andrew. I hope that it captures the essence of what has been discussed before. I will improve the comments once I get some initial feedback.
What's the likelihood to handle asynchronous event coming from browser process, such as "error"? On 2011/08/16 15:34:00, henrika1 wrote: > This CL is an attempt to summarize all the proposed modifications of the > AudioInputDevice. As we discussed, the initial CL became a bit too complex and > we decided to take a few steps back. This CL combines input from Wei, Tommi, > Shijing and Andrew. I hope that it captures the essence of what has been > discussed before. > > I will improve the comments once I get some initial feedback.
great! lgtm http://codereview.chromium.org/7661017/diff/1/content/renderer/media/audio_in... File content/renderer/media/audio_input_device.cc (right): http://codereview.chromium.org/7661017/diff/1/content/renderer/media/audio_in... content/renderer/media/audio_input_device.cc:92: void AudioInputDevice::InitializeOnIOThread(const AudioParameters& params) { DCHECK(MessageLoop::current() == ChildProcess::current()->io_message_loop()) http://codereview.chromium.org/7661017/diff/1/content/renderer/media/audio_in... content/renderer/media/audio_input_device.cc:103: if (stream_id_) DCHECK(MessageLoop::current() == ChildProcess::current()->io_message_loop()) http://codereview.chromium.org/7661017/diff/1/content/renderer/media/audio_in... content/renderer/media/audio_input_device.cc:108: // Make sure we don't call shutdown more than once. DCHECK(MessageLoop::current() == ChildProcess::current()->io_message_loop()) http://codereview.chromium.org/7661017/diff/1/content/renderer/media/audio_in... content/renderer/media/audio_input_device.cc:122: if (stream_id_) DCHECK(MessageLoop::current() == ChildProcess::current()->io_message_loop()) http://codereview.chromium.org/7661017/diff/1/content/renderer/media/audio_in... content/renderer/media/audio_input_device.cc:143: base::SyncSocket socket(socket_handle); is this the normal way of closing a socket? i.e. is there no static function?
On 2011/08/17 00:30:46, wjia wrote: > What's the likelihood to handle asynchronous event coming from browser process, > such as "error"? > Hey Wei, I think this change is a step in that direction. Start is now void and we will need to add a way to notify the callers about failures in the future. We'll likely have to do the same for Stop(). I think we should get this CL in and fix the race issue. That'll give us time to design a callback mechanism without the pressure of having the race condition in the current code.
First the question from Wei. Yes, we might have to receive messages like OnStateChanged etc. from the AudioInputMessageFilter on the IO thread. I guess you want to ensure that we can receive and perhaps pass this on to the ADM, right? Set 3 adds modifications based on initial input from Tommi. I also added a timeout in the wait operation in Stop; just to ensure that we don't block the calling thread forever. Please note that this CL only attempts to do what it says in the description: fix the potential race conditions.
http://codereview.chromium.org/7661017/diff/1/content/renderer/media/audio_in... File content/renderer/media/audio_input_device.cc (right): http://codereview.chromium.org/7661017/diff/1/content/renderer/media/audio_in... content/renderer/media/audio_input_device.cc:92: void AudioInputDevice::InitializeOnIOThread(const AudioParameters& params) { On 2011/08/17 08:35:18, tommi wrote: > DCHECK(MessageLoop::current() == ChildProcess::current()->io_message_loop()) Done. http://codereview.chromium.org/7661017/diff/1/content/renderer/media/audio_in... content/renderer/media/audio_input_device.cc:103: if (stream_id_) On 2011/08/17 08:35:18, tommi wrote: > DCHECK(MessageLoop::current() == ChildProcess::current()->io_message_loop()) Done. http://codereview.chromium.org/7661017/diff/1/content/renderer/media/audio_in... content/renderer/media/audio_input_device.cc:108: // Make sure we don't call shutdown more than once. On 2011/08/17 08:35:18, tommi wrote: > DCHECK(MessageLoop::current() == ChildProcess::current()->io_message_loop()) Done. http://codereview.chromium.org/7661017/diff/1/content/renderer/media/audio_in... content/renderer/media/audio_input_device.cc:122: if (stream_id_) On 2011/08/17 08:35:18, tommi wrote: > DCHECK(MessageLoop::current() == ChildProcess::current()->io_message_loop()) Done. http://codereview.chromium.org/7661017/diff/1/content/renderer/media/audio_in... content/renderer/media/audio_input_device.cc:143: base::SyncSocket socket(socket_handle); Not sure if it is normal but it works ;-) And, it seems to be used on other places in Chrome as well. Please note that Close() is called during destruction.
LGTM On 2011/08/17 15:34:13, henrika1 wrote: > http://codereview.chromium.org/7661017/diff/1/content/renderer/media/audio_in... > File content/renderer/media/audio_input_device.cc (right): > > http://codereview.chromium.org/7661017/diff/1/content/renderer/media/audio_in... > content/renderer/media/audio_input_device.cc:92: void > AudioInputDevice::InitializeOnIOThread(const AudioParameters& params) { > On 2011/08/17 08:35:18, tommi wrote: > > DCHECK(MessageLoop::current() == ChildProcess::current()->io_message_loop()) > > Done. > > http://codereview.chromium.org/7661017/diff/1/content/renderer/media/audio_in... > content/renderer/media/audio_input_device.cc:103: if (stream_id_) > On 2011/08/17 08:35:18, tommi wrote: > > DCHECK(MessageLoop::current() == ChildProcess::current()->io_message_loop()) > > Done. > > http://codereview.chromium.org/7661017/diff/1/content/renderer/media/audio_in... > content/renderer/media/audio_input_device.cc:108: // Make sure we don't call > shutdown more than once. > On 2011/08/17 08:35:18, tommi wrote: > > DCHECK(MessageLoop::current() == ChildProcess::current()->io_message_loop()) > > Done. > > http://codereview.chromium.org/7661017/diff/1/content/renderer/media/audio_in... > content/renderer/media/audio_input_device.cc:122: if (stream_id_) > On 2011/08/17 08:35:18, tommi wrote: > > DCHECK(MessageLoop::current() == ChildProcess::current()->io_message_loop()) > > Done. > > http://codereview.chromium.org/7661017/diff/1/content/renderer/media/audio_in... > content/renderer/media/audio_input_device.cc:143: base::SyncSocket > socket(socket_handle); > Not sure if it is normal but it works ;-) And, it seems to be used on other > places in Chrome as well. Please note that Close() is called during destruction.
On 2011/08/17 15:12:46, henrika wrote: > First the question from Wei. Yes, we might have to receive messages like > OnStateChanged etc. from the AudioInputMessageFilter on the IO thread. I guess > you want to ensure that we can receive and perhaps pass this on to the ADM, > right? > > Set 3 adds modifications based on initial input from Tommi. I also added a > timeout in the wait operation in Stop; just to ensure that we don't block the > calling thread forever. > > Please note that this CL only attempts to do what it says in the description: > fix the potential race conditions. It sounds that this is a temporary solution and will be refactored in the future, please add some comments in the code so that it won't be forgot. Please also fix the racing condition in AudioInputMessageFilter.
lgtm++ imho this fix is only temporary if the existing code was meant to be temporary since it just fixes bugs in the existing implementation. Is the race condition in AudioInputMessageFilter related to this one? If not, then I suggest we fix that in a separate change so that it won't block this change. There's nothing stopping us from refactoring/redesigning the code btw. We can start working on that right away if we want. On Thu, Aug 18, 2011 at 2:28 PM, <wjia@chromium.org> wrote: > On 2011/08/17 15:12:46, henrika wrote: > >> First the question from Wei. Yes, we might have to receive messages like >> OnStateChanged etc. from the AudioInputMessageFilter on the IO thread. I >> guess >> you want to ensure that we can receive and perhaps pass this on to the >> ADM, >> right? >> > > Set 3 adds modifications based on initial input from Tommi. I also added a >> timeout in the wait operation in Stop; just to ensure that we don't block >> the >> calling thread forever. >> > > Please note that this CL only attempts to do what it says in the >> description: >> fix the potential race conditions. >> > > It sounds that this is a temporary solution and will be refactored in the > future, please add some comments in the code so that it won't be forgot. > > Please also fix the racing condition in AudioInputMessageFilter. > > > http://codereview.chromium.**org/7661017/<http://codereview.chromium.org/7661... >
Now also fixes race condition (resource leak) in AudioInputMessageFilter. I am not sure what to comment about "temporary fix". Hope this is OK for now.
On 2011/08/18 15:21:00, henrika1 wrote: > Now also fixes race condition (resource leak) in AudioInputMessageFilter. > > I am not sure what to comment about "temporary fix". Hope this is OK for now. Thanks for adding the fix in message filter. Please correct me if I am wrong. The AudioInputDevice will be used by other clients as well, e.g., Pepper. Right now, webrtc ADM is the only client. When more clients design audio input API based on this synchronous mode, that will make it harder to refactor code with more clients. If we do need asynchronous API to handle some events, then it's better to do it in the first place. Furthermore, some comments in audio_input_device.h could keep other modules well notified about the status of this API so that they can have corresponding plan. Hopefully, this is indeed temporary. The sooner it will be refactored, the better.
I assume (and hope) that e.g. Pepper will use this class, yes. However, I fail to understand what we/I have done here to prevent that this is possible. If you compare with e.g. AudioDevice or AudioRenderImpl, what do they contain that we are missing? My point is, if you are OK with the current approach as base but want to add more, then why not stop here and stick to the title of the CL? Then I can start working on a new CL but in the mean time we have a better version without any race conditions. If you feel that there is something fundamentally wrong with the current CL (that will prevent us from improving it), then please provide more details. I honestly don't see the problem. Is it that Stop is now sync or that we have no method to provide async callbacks/notifications, or both?
Thanks Wei, I believe this is going to be a really good discussion for the follow up CLs. Yes, I agree that we need a solution to pass some events from AudioInputDevice to ADM. At least for me, a OnCaptureStopped is needed. But I am a bit hesitant on setting up a pattern for the clients of AudioInputDevice to run on asynchronous mode. Think about what happens if every client of AudioInputDevice and AudioDevice needs to add a thread, the complexity will boom and may finally bite us. I do understand that asynchronous mode might be the solution to some certain cases, but what about leaving it to the clients to decide if they should run on async mode or sync mode. Take our ADM as example, AudioInputDevice passes input_delay_ms, out_put_delay_ms, and maybe some events like errors, OnCaptureStopped. For the delay values we may use lock to be thread safe, and for the events, AudioInputDevice can post the events to ADM on the IO thread, and on that event it can post a task to the thread which runns ADM, or we can still lock in the worst case. BR, - SX On Thu, Aug 18, 2011 at 6:06 PM, <wjia@chromium.org> wrote: > On 2011/08/18 15:21:00, henrika1 wrote: > >> Now also fixes race condition (resource leak) in AudioInputMessageFilter. >> > > I am not sure what to comment about "temporary fix". Hope this is OK for >> now. >> > > Thanks for adding the fix in message filter. > > Please correct me if I am wrong. The AudioInputDevice will be used by other > clients as well, e.g., Pepper. Right now, webrtc ADM is the only client. > When > more clients design audio input API based on this synchronous mode, that > will > make it harder to refactor code with more clients. If we do need > asynchronous > API to handle some events, then it's better to do it in the first place. > > Furthermore, some comments in audio_input_device.h could keep other modules > well > notified about the status of this API so that they can have corresponding > plan. > > Hopefully, this is indeed temporary. The sooner it will be refactored, the > better. > > > http://codereview.chromium.**org/7661017/<http://codereview.chromium.org/7661... >
Wei, I have added the TODO comment. Waiting for LGTM from you as an owner. Andrew: it would be great with a review from you as well. Not sure if it is required to commit though. Please advise. Thanks.
lgtm for now. Please do follow-up refactoring sooner than later. http://codereview.chromium.org/7661017/diff/18001/content/renderer/media/audi... File content/renderer/media/audio_input_device.cc (right): http://codereview.chromium.org/7661017/diff/18001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:43: DCHECK_EQ(0, stream_id_); Does this mean client has to call Stop() before destructing this class?
Added reply on why Stop was removed from dtor. http://codereview.chromium.org/7661017/diff/18001/content/renderer/media/audi... File content/renderer/media/audio_input_device.cc (right): http://codereview.chromium.org/7661017/diff/18001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:43: DCHECK_EQ(0, stream_id_); Yes, that is the current design criteria. If we add Stop here, we can't just create and then directly destruct, then we will crash when we try to post ShutDownOnIOThread using NewRunnable in Stop since we are ref counted. I found no better solution to this than to force the client to always Stop before deleting. Please comment if you have other/better ideas here. Thanks.
Hi Henrik, this looks good just looking over this quickly. I agree that it would be good to have something similar in AudioDevice as well...
LGTM w/ nits and a few questions what's the ETA on refactoring code? http://codereview.chromium.org/7661017/diff/22001/content/renderer/media/audi... File content/renderer/media/audio_input_device.cc (right): http://codereview.chromium.org/7661017/diff/22001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:16: // we will stop waiting and return false. It ensures that Stop can't block Stop -> Stop() http://codereview.chromium.org/7661017/diff/22001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:18: static const base::TimeDelta kMaxTimeOutMs = nit: drop Ms suffix since TimeDelats are unitless http://codereview.chromium.org/7661017/diff/22001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:74: // with OnLowLatencyCreated and to ensure that Stop acts as a synchronous don't forget () suffix on function call names in comments http://codereview.chromium.org/7661017/diff/22001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:76: if (completion.TimedWait(kMaxTimeOutMs)) { out of curiosity who is usually the calling thread? do we know how fast this executes under normal circumstances? http://codereview.chromium.org/7661017/diff/22001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:80: // Wait for the audio thread to exit. nit: most of these comments are useless as you have a block-level comment above describing what's going on http://codereview.chromium.org/7661017/diff/22001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:82: // Ensures that we can call Stop multiple times. who's calling Stop() multiple times? http://codereview.chromium.org/7661017/diff/22001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:86: DLOG(ERROR) << "Failed to shut down audio input on IO thread"; how often do we expect to see this in practice? might want to bump it up to LOG() if it's a serious enough issue http://codereview.chromium.org/7661017/diff/22001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:155: VLOG(1) << __FUNCTION__ << "[stream_id=" << stream_id_ << "]"; use OnLowLatencyCreated ? http://codereview.chromium.org/7661017/diff/22001/content/renderer/media/audi... File content/renderer/media/audio_input_device.h (right): http://codereview.chromium.org/7661017/diff/22001/content/renderer/media/audi... content/renderer/media/audio_input_device.h:49: // - Start is asynchronous/non-blocking. () by function names
I've got a question about the Mock*ResourceContext classes but rest is LGTM
Andrew, I really don't understand your last comment. Does it apply to this patch?
On 2011/08/23 19:32:18, henrika1 wrote: > Andrew, I really don't understand your last comment. Does it apply to this > patch? Whoops that was intended for mflodman's CL :)
Updated the patch based on input from Andrew. http://codereview.chromium.org/7661017/diff/22001/content/renderer/media/audi... File content/renderer/media/audio_input_device.cc (right): http://codereview.chromium.org/7661017/diff/22001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:16: // we will stop waiting and return false. It ensures that Stop can't block On 2011/08/23 16:54:19, scherkus wrote: > Stop -> Stop() Done. http://codereview.chromium.org/7661017/diff/22001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:18: static const base::TimeDelta kMaxTimeOutMs = Thanks. Done. http://codereview.chromium.org/7661017/diff/22001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:74: // with OnLowLatencyCreated and to ensure that Stop acts as a synchronous On 2011/08/23 16:54:19, scherkus wrote: > don't forget () suffix on function call names in comments Done. http://codereview.chromium.org/7661017/diff/22001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:76: if (completion.TimedWait(kMaxTimeOutMs)) { The calling client will be calling from the main render thread . Would work with any worker thread actually. The timeout is probably overkill since the main thread loop should be done when Join is called, hence the delay should be very low. I added the timeout just in case. http://codereview.chromium.org/7661017/diff/22001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:82: // Ensures that we can call Stop multiple times. Well, we just want to ensure that the design can deal with it if a client (that we can't control) calls Stop, Stop. This is one way of ensuring that the second call "does nothing" and then returns true. Without the audio_thread_.reset(NULL) we can't call Start()....Stop(),Stop() without crashing since we will try to join a thread multiple times. http://codereview.chromium.org/7661017/diff/22001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:86: DLOG(ERROR) << "Failed to shut down audio input on IO thread"; Very good question. Classic comment: "should never happen". Will add to LOG(). http://codereview.chromium.org/7661017/diff/22001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:155: VLOG(1) << __FUNCTION__ << "[stream_id=" << stream_id_ << "]"; On 2011/08/23 16:54:19, scherkus wrote: > use OnLowLatencyCreated ? Done. http://codereview.chromium.org/7661017/diff/22001/content/renderer/media/audi... File content/renderer/media/audio_input_device.h (right): http://codereview.chromium.org/7661017/diff/22001/content/renderer/media/audi... content/renderer/media/audio_input_device.h:49: // - Start is asynchronous/non-blocking. On 2011/08/23 16:54:19, scherkus wrote: > () by function names Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
