Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12220063:
Possible solution to synchronization problems in webrtc audio capturer. (Closed)
|
Created:
7 years, 10 months ago by phoglund_chromium Modified:
7 years, 10 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPossible solution to synchronization problems in webrtc audio capturer.
This will hold on to a reference to the buffer while the buffer is being used by WebRTC. I also tried to fix the places where synchronization was missing (mostly for the params_ instance).
Let me know if you want an offline explanation!
BUG=173987
TESTED=content_unittests on win and linux, chrome + apprtc on win and linux, release-built chrome with asan on original crashing test page.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181929
Patch Set 1 #
Total comments: 10
Patch Set 2 : Attempt with scoped_refptr #Patch Set 3 : Rebased #
Total comments: 21
Patch Set 4 : Moved responsibility to the buffer class; moved params and init code in #Patch Set 5 : Tweaking #Patch Set 6 : Fixed potential update issues #
Total comments: 14
Patch Set 7 : Made params accessor return as value, fixed refptr compilation error in release, cleaned up reconfi… #Patch Set 8 : Fixed comment #
Total comments: 8
Patch Set 9 : Nit fixes #
Messages
Total messages: 18 (0 generated)
I have a possible solution to https://code.google.com/p/chromium/issues/detail?id=173987. Let's discuss it and then move to main review. I have no idea how to test this though. What kind of tests should I run? https://codereview.chromium.org/12220063/diff/1/content/renderer/media/webrtc... File content/renderer/media/webrtc_audio_capturer.cc (left): https://codereview.chromium.org/12220063/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc_audio_capturer.cc:100: This code didn't check if GetBufferSize... returned 0 earlier, but I guess it should? It will now with the extracted method. https://codereview.chromium.org/12220063/diff/1/content/renderer/media/webrtc... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/12220063/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc_audio_capturer.cc:65: I found two places in the code which was doing roughly the same thing, hence this new method here. https://codereview.chromium.org/12220063/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc_audio_capturer.cc:83: So along with copying parameters, this is the addition. The buffer_in_use parameter will keep track of if the buffer is being used by webrtc. https://codereview.chromium.org/12220063/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc_audio_capturer.cc:86: Once the buffer is not used, we can move ahead with the reconfiguration. https://codereview.chromium.org/12220063/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc_audio_capturer.cc:201: source_ = source; We copy this here to avoid grabbing the lock again inside the if statement below. https://codereview.chromium.org/12220063/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc_audio_capturer.cc:381: Here we use buffer_in_use variable to protect against reconfigurations when we are calling into WebRTC below. I assumed this method is called by a single thread. We will get into trouble if this method is called concurrently. https://codereview.chromium.org/12220063/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc_audio_capturer.cc:415: If this note isn't true, we need WebRTC to somehow tell us when it's done with the buffer. Let's hope it's not that bad :)
Thanks for looking at this but we can't add 70+ lines of code for this. Also, I am unable to find any references to ConditionVariable in Chrome and I don't think we need it. On Thu, Feb 7, 2013 at 3:03 PM, <phoglund@chromium.org> wrote: > Reviewers: henrika, xians, > > Message: > I have a possible solution to > https://code.google.com/p/**chromium/issues/detail?id=**173987<https://code.g.... > Let's discuss it and > then move to main review. > > I have no idea how to test this though. What kind of tests should I run? > > > https://codereview.chromium.**org/12220063/diff/1/content/** > renderer/media/webrtc_audio_**capturer.cc<https://codereview.chromium.org/12220063/diff/1/content/renderer/media/webrtc_audio_capturer.cc> > File content/renderer/media/webrtc_**audio_capturer.cc (left): > > https://codereview.chromium.**org/12220063/diff/1/content/** > renderer/media/webrtc_audio_**capturer.cc#oldcode100<https://codereview.chromium.org/12220063/diff/1/content/renderer/media/webrtc_audio_capturer.cc#oldcode100> > content/renderer/media/webrtc_**audio_capturer.cc:100: > This code didn't check if GetBufferSize... returned 0 earlier, but I > guess it should? It will now with the extracted method. > > https://codereview.chromium.**org/12220063/diff/1/content/** > renderer/media/webrtc_audio_**capturer.cc<https://codereview.chromium.org/12220063/diff/1/content/renderer/media/webrtc_audio_capturer.cc> > File content/renderer/media/webrtc_**audio_capturer.cc (right): > > https://codereview.chromium.**org/12220063/diff/1/content/** > renderer/media/webrtc_audio_**capturer.cc#newcode65<https://codereview.chromium.org/12220063/diff/1/content/renderer/media/webrtc_audio_capturer.cc#newcode65> > content/renderer/media/webrtc_**audio_capturer.cc:65: > I found two places in the code which was doing roughly the same thing, > hence this new method here. > > https://codereview.chromium.**org/12220063/diff/1/content/** > renderer/media/webrtc_audio_**capturer.cc#newcode83<https://codereview.chromium.org/12220063/diff/1/content/renderer/media/webrtc_audio_capturer.cc#newcode83> > content/renderer/media/webrtc_**audio_capturer.cc:83: > So along with copying parameters, this is the addition. The > buffer_in_use parameter will keep track of if the buffer is being used > by webrtc. > > https://codereview.chromium.**org/12220063/diff/1/content/** > renderer/media/webrtc_audio_**capturer.cc#newcode86<https://codereview.chromium.org/12220063/diff/1/content/renderer/media/webrtc_audio_capturer.cc#newcode86> > content/renderer/media/webrtc_**audio_capturer.cc:86: > Once the buffer is not used, we can move ahead with the reconfiguration. > > https://codereview.chromium.**org/12220063/diff/1/content/** > renderer/media/webrtc_audio_**capturer.cc#newcode201<https://codereview.chromium.org/12220063/diff/1/content/renderer/media/webrtc_audio_capturer.cc#newcode201> > content/renderer/media/webrtc_**audio_capturer.cc:201: source_ = source; > We copy this here to avoid grabbing the lock again inside the if > statement below. > > https://codereview.chromium.**org/12220063/diff/1/content/** > renderer/media/webrtc_audio_**capturer.cc#newcode381<https://codereview.chromium.org/12220063/diff/1/content/renderer/media/webrtc_audio_capturer.cc#newcode381> > content/renderer/media/webrtc_**audio_capturer.cc:381: > Here we use buffer_in_use variable to protect against reconfigurations > when we are calling into WebRTC below. I assumed this method is called > by a single thread. We will get into trouble if this method is called > concurrently. > > https://codereview.chromium.**org/12220063/diff/1/content/** > renderer/media/webrtc_audio_**capturer.cc#newcode415<https://codereview.chromium.org/12220063/diff/1/content/renderer/media/webrtc_audio_capturer.cc#newcode415> > content/renderer/media/webrtc_**audio_capturer.cc:415: > If this note isn't true, we need WebRTC to somehow tell us when it's > done with the buffer. Let's hope it's not that bad :) > > Description: > Possible solution to synchronization problems in webrtc audio capturer. > > This will hold off buffer reconfigurations while the buffer is being used > by > WebRTC. I also tried to fix the places where synchronization was missing > (mostly > for the params_ instance). > > Let me know if you want an offline explanation! > > BUG=173987 > > Please review this at https://codereview.chromium.**org/12220063/<https://codereview.chromium.org/1... > > SVN Base: svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src> > > Affected files: > M content/renderer/media/webrtc_**audio_capturer.h > M content/renderer/media/webrtc_**audio_capturer.cc > > >
Adding Tommmi as well. Tommi: to me it just feels bad to add so much code "only" to fix a "user-after-free" issue. It must be possible fix in a more simple way, adding a new state or at least avoiding CondVars.
On 2013/02/07 14:36:06, henrika wrote: > Adding Tommmi as well. > > Tommi: to me it just feels bad to add so much code "only" to fix a > "user-after-free" issue. It must be possible fix in a more simple way, adding a > new state or at least avoiding CondVars. Well, it adds 75 lines of code and removes 37, so the net is +38 lines :) But yeah, it'd be good to find a simpler solution. Like I said, one could be to copy the buffer before giving it to WebRTC instead of trying to protect it.
https://codereview.chromium.org/12220063/diff/1/content/renderer/media/webrtc... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/12220063/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc_audio_capturer.cc:75: // Ignored since the audio stack uses float32. s/Ignored/bits_per_sample is always 16 https://codereview.chromium.org/12220063/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc_audio_capturer.cc:381: On 2013/02/07 14:03:18, phoglund wrote: > Here we use buffer_in_use variable to protect against reconfigurations when we > are calling into WebRTC below. I assumed this method is called by a single > thread. We will get into trouble if this method is called concurrently. There's no thread check that ensures that the method is called by a single thread. Since there's a lock here already it does suggest that we could get here from multiple threads. https://codereview.chromium.org/12220063/diff/1/content/renderer/media/webrtc... File content/renderer/media/webrtc_audio_capturer.h (right): https://codereview.chromium.org/12220063/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc_audio_capturer.h:178: // |loopback_fifo_|, |params| and |buffering_|. params_
https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:66: scoped_array<int16> buffer_; we're deprecating scoped_array and use scoped_ptr instead. https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:67: }; I don't see the audio parameters here. I think they need to go with the buffer. https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:77: media::ChannelLayout channel_layout) { I was actually thinking that we'd move much of this logic into the CaptureBuffer class. So, this function would look closer to: scoped_refptr<CaptureBuffer> new_buffer(new CaptureBuffer()); if (!new_buffer->Initialize()) // Initialize() checks the buffer size etc. return false; SinkList sinks; { base::AutoLock auto_lock(lock_); buffer_ = new_buffer; sinks = sinks_; } for (SinkList::const_iterator it = sinks.begin(); it != sinks.end(); ++it) (*it)->SetCaptureFormat(new_buffer->params()); return true; Also... is Reconfigure() always called on the same thread? If so, then it would be good to add this to the top of the function: DCHECK(thread_checker_.CalledOnValidThread()); https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:209: no_default_audio_source_exists = !buffer_.get()->buffer(); .get()-> should not be necessary. instead just use -> https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:375: // CaptureCallback. We generally keep comments like this inside the implementation (where it was). Otherwise the comment would be in the header file. https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:390: bytes_per_sample = params_.bits_per_sample() / 8; if params_ is owned by the CaptureBuffer, then you can do this outside of the lock, below. https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:396: if (loopback_fifo_ && buffering_) { should this block of code be moved out of the lock scope? https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... File content/renderer/media/webrtc_audio_capturer.h (right): https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.h:13: #include "base/synchronization/condition_variable.h" remove https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.h:30: class CaptureBuffer; can we make this a private class inside WebRtcAudioCapturer? https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.h:179: // |loopback_fifo_|, |params| and |buffering_|. params_
PTAL https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:66: scoped_array<int16> buffer_; On 2013/02/08 14:56:57, tommi wrote: > we're deprecating scoped_array and use scoped_ptr instead. Done. https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:67: }; On 2013/02/08 14:56:57, tommi wrote: > I don't see the audio parameters here. I think they need to go with the buffer. We could do that, but in that case you could argue to also put the sinks there. I guess it depends on how much we think the buffer is related to the parameters. https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:77: media::ChannelLayout channel_layout) { On 2013/02/08 14:56:57, tommi wrote: > I was actually thinking that we'd move much of this logic into the CaptureBuffer > class. So, this function would look closer to: > > scoped_refptr<CaptureBuffer> new_buffer(new CaptureBuffer()); > if (!new_buffer->Initialize()) // Initialize() checks the buffer size etc. > return false; > > SinkList sinks; > { > base::AutoLock auto_lock(lock_); > buffer_ = new_buffer; > sinks = sinks_; > } > > for (SinkList::const_iterator it = sinks.begin(); it != sinks.end(); ++it) > (*it)->SetCaptureFormat(new_buffer->params()); > > return true; > > > Also... is Reconfigure() always called on the same thread? > If so, then it would be good to add this to the top of the function: > DCHECK(thread_checker_.CalledOnValidThread()); Done. I think we can here from a different thread when resetting the capturer source. https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:209: no_default_audio_source_exists = !buffer_.get()->buffer(); On 2013/02/08 14:56:57, tommi wrote: > .get()-> should not be necessary. instead just use -> Done. https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:375: // CaptureCallback. On 2013/02/08 14:56:57, tommi wrote: > We generally keep comments like this inside the implementation (where it was). > Otherwise the comment would be in the header file. Done. https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:390: bytes_per_sample = params_.bits_per_sample() / 8; On 2013/02/08 14:56:57, tommi wrote: > if params_ is owned by the CaptureBuffer, then you can do this outside of the > lock, below. Done. https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:396: if (loopback_fifo_ && buffering_) { On 2013/02/08 14:56:57, tommi wrote: > should this block of code be moved out of the lock scope? I have no idea, so I'll just leave it for now. It probably can. https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... File content/renderer/media/webrtc_audio_capturer.h (right): https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.h:13: #include "base/synchronization/condition_variable.h" On 2013/02/08 14:56:57, tommi wrote: > remove Done. https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.h:30: class CaptureBuffer; On 2013/02/08 14:56:57, tommi wrote: > can we make this a private class inside WebRtcAudioCapturer? Done. https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.h:179: // |loopback_fifo_|, |params| and |buffering_|. On 2013/02/08 14:56:57, tommi wrote: > params_ Done. https://codereview.chromium.org/12220063/diff/4003/content/renderer/media/web... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/12220063/diff/4003/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:244: // Make sure to grab the new parameters in case they were reconfigured. This here was tricky. I initially passed in current_buffer->params(), but that is NOT what we want if the Reconfigure above happens. By using the accessor I'm sure to get the latest version of params, at the cost of one more lock grab. https://codereview.chromium.org/12220063/diff/4003/content/renderer/media/web... File content/renderer/media/webrtc_audio_capturer.h (right): https://codereview.chromium.org/12220063/diff/4003/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.h:138: // capturer. Removing const looks silly, but I actually have to now since this one grabs a lock.
https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/12220063/diff/8001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:67: }; On 2013/02/08 16:10:38, phoglund wrote: > On 2013/02/08 14:56:57, tommi wrote: > > I don't see the audio parameters here. I think they need to go with the > buffer. > We could do that, but in that case you could argue to also put the sinks there. > I guess it depends on how much we think the buffer is related to the parameters. The buffer is allocated based on specs from the parameters and any processing done on the buffer must be done in accordance with how the parameters specify the buffer. As soon as the parameters change, the buffer has to be discarded. The parameters themselves are then useless without a buffer to go with them, so I think they're practically married :) The sink list on the other hand can outlive multiple parameters just as long as they're notified of change, so I don't think they could live here. https://codereview.chromium.org/12220063/diff/4003/content/renderer/media/web... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/12220063/diff/4003/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:90: scoped_ptr<int16[]> buffer_; hmm... this doesn't look right. should just be scoped_ptr<int16> https://codereview.chromium.org/12220063/diff/4003/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:92: // Cached values of utilized audio parameters. Platform dependent. nit: skip the "platform dependent" part. https://codereview.chromium.org/12220063/diff/4003/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:239: channel_layout)) use {} https://codereview.chromium.org/12220063/diff/4003/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:244: // Make sure to grab the new parameters in case they were reconfigured. On 2013/02/08 16:10:38, phoglund wrote: > This here was tricky. I initially passed in current_buffer->params(), but that > is NOT what we want if the Reconfigure above happens. By using the accessor I'm > sure to get the latest version of params, at the cost of one more lock grab. audio_parameters() returns a reference to the parameters owned by buffer_. Once you've called that method, you're no longer holding the lock that protects |buffer_|. So, if buffer_ is changed during the call to Initialize(), we will crash. Instead I suggest this little change to the code above: if (!Reconfigure(sample_rate, current_buffer->params().format(), channel_layout)) { return; } else { // The buffer has been reconfigured. Update |current_buffer|. base::AutoLock auto_lock(lock_); current_buffer = buffer_; } It's true that this is yet one more lock and we could avoid that but I'll leave it up to you if you think it's worth it. But with the above then when you get here, you can use |current_buffer| as you did before. https://codereview.chromium.org/12220063/diff/4003/content/renderer/media/web... File content/renderer/media/webrtc_audio_capturer.h (right): https://codereview.chromium.org/12220063/diff/4003/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.h:138: // capturer. On 2013/02/08 16:10:38, phoglund wrote: > Removing const looks silly, but I actually have to now since this one grabs a > lock. You could make the lock mutable which is common in cases like this. However, since audio_parameters() returns a pointer to a variable owned by a reference counted object that we're using a lock to access, then the returned reference here cannot be trusted. So, I'm not sure this accessor is a good idea. https://codereview.chromium.org/12220063/diff/4003/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.h:139: const media::AudioParameters& audio_parameter(); nit: audio_parameters() (plural) or just params() as you do in the cc file.
PTAL https://codereview.chromium.org/12220063/diff/4003/content/renderer/media/web... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/12220063/diff/4003/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:90: scoped_ptr<int16[]> buffer_; On 2013/02/08 16:32:04, tommi wrote: > hmm... this doesn't look right. should just be scoped_ptr<int16> The deprecation comment for scoped_array says "replace scoped_array<int16> with scoped_ptr<int16[]>." https://codereview.chromium.org/12220063/diff/4003/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:92: // Cached values of utilized audio parameters. Platform dependent. On 2013/02/08 16:32:04, tommi wrote: > nit: skip the "platform dependent" part. Done. https://codereview.chromium.org/12220063/diff/4003/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:239: channel_layout)) On 2013/02/08 16:32:04, tommi wrote: > use {} Done. https://codereview.chromium.org/12220063/diff/4003/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:244: // Make sure to grab the new parameters in case they were reconfigured. On 2013/02/08 16:32:04, tommi wrote: > On 2013/02/08 16:10:38, phoglund wrote: > > This here was tricky. I initially passed in current_buffer->params(), but that > > is NOT what we want if the Reconfigure above happens. By using the accessor > I'm > > sure to get the latest version of params, at the cost of one more lock grab. > > audio_parameters() returns a reference to the parameters owned by buffer_. Once > you've called that method, you're no longer holding the lock that protects > |buffer_|. So, if buffer_ is changed during the call to Initialize(), we will > crash. > > Instead I suggest this little change to the code above: > > if (!Reconfigure(sample_rate, current_buffer->params().format(), > channel_layout)) { > return; > } else { > // The buffer has been reconfigured. Update |current_buffer|. > base::AutoLock auto_lock(lock_); > current_buffer = buffer_; > } > > It's true that this is yet one more lock and we could avoid that but I'll leave > it up to you if you think it's worth it. > > But with the above then when you get here, you can use |current_buffer| as you > did before. Done. https://codereview.chromium.org/12220063/diff/4003/content/renderer/media/web... File content/renderer/media/webrtc_audio_capturer.h (right): https://codereview.chromium.org/12220063/diff/4003/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.h:138: // capturer. On 2013/02/08 16:32:04, tommi wrote: > On 2013/02/08 16:10:38, phoglund wrote: > > Removing const looks silly, but I actually have to now since this one grabs a > > lock. > > You could make the lock mutable which is common in cases like this. > > However, since audio_parameters() returns a pointer to a variable owned by a > reference counted object that we're using a lock to access, then the returned > reference here cannot be trusted. So, I'm not sure this accessor is a good > idea. Yeah, I wrote a TODO on the accessor. https://codereview.chromium.org/12220063/diff/4003/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.h:139: const media::AudioParameters& audio_parameter(); On 2013/02/08 16:32:04, tommi wrote: > nit: audio_parameters() (plural) or just params() as you do in the cc file. Done.
lgtm https://codereview.chromium.org/12220063/diff/5004/content/renderer/media/web... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/12220063/diff/5004/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:93: scoped_ptr<int16[]> buffer_; ah, thanks. I didn't know about the DefaultDeleter specialization we have now. Makes sense. https://codereview.chromium.org/12220063/diff/5004/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:121: for (SinkList::const_iterator it = sinks.begin(); it != sinks.end(); ++it) No action needed in this CL since I think you've improved the code considerably and fixed the most serious issues. Thinking about this pattern some more and the fact that Reconfigure can be called on multiple threads, it looks like there's a design flaw in the way the WebRtcAudioCapturer and sinks work together. Firstly this puts a requirement on the sinks that they need to be thread safe, which I'm not sure all implementations are aware of. Secondly, this particular pattern is not guaranteed to give the sinks the correct audio parameters. To explain what I mean, take this example: Thread A enters Reconfigure, allocates a new ConfiguredBuffer, takes the lock, assigns the new buffer to buffer_ and releases the lock. Before entering this loop though, Thread A runs out of quantum. Thread B runs, enters Reconfigure() does all the steps that A previously did, but with different audio parameters. Thread B continues to run and runs through the loop, letting all the sinks know that buffer_->params() are the new params. This is correct now that buffer_ points to those new parameters. However, now Thread A wakes up and enters the loop. Its new_buffer however still holds a reference to the previous buffer and all the sinks will now be told that the old audio parameters are the current ones. Oops. I guess the morale of the story is that we need to make the threading story of the class better. Ideally the buffer and params should always be modifed on the same thread and then we won't have the above problem. I don't think we need to address this in this CL, but I figured I'd get someone else to think about it as well :) https://codereview.chromium.org/12220063/diff/5004/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:479: DCHECK(buffer_.get() != NULL); you can remove this. we avoid having (D)CHECKs followed by dereferencing the checked pointer anyway. https://codereview.chromium.org/12220063/diff/5004/content/renderer/media/web... File content/renderer/media/webrtc_audio_capturer.h (right): https://codereview.chromium.org/12220063/diff/5004/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.h:142: media::AudioParameters audio_parameters(); nit: I think it's still OK to keep the constness of the function as it was (which would mean to make the lock mutable).
https://codereview.chromium.org/12220063/diff/5004/content/renderer/media/web... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/12220063/diff/5004/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:93: scoped_ptr<int16[]> buffer_; On 2013/02/08 20:00:39, tommi wrote: > ah, thanks. I didn't know about the DefaultDeleter specialization we have now. > Makes sense. Done. https://codereview.chromium.org/12220063/diff/5004/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:121: for (SinkList::const_iterator it = sinks.begin(); it != sinks.end(); ++it) On 2013/02/08 20:00:39, tommi wrote: > No action needed in this CL since I think you've improved the code considerably > and fixed the most serious issues. Thinking about this pattern some more and > the fact that Reconfigure can be called on multiple threads, it looks like > there's a design flaw in the way the WebRtcAudioCapturer and sinks work > together. > > Firstly this puts a requirement on the sinks that they need to be thread safe, > which I'm not sure all implementations are aware of. > > Secondly, this particular pattern is not guaranteed to give the sinks the > correct audio parameters. To explain what I mean, take this example: > > Thread A enters Reconfigure, allocates a new ConfiguredBuffer, takes the lock, > assigns the new buffer to buffer_ and releases the lock. Before entering this > loop though, Thread A runs out of quantum. > > Thread B runs, enters Reconfigure() does all the steps that A previously did, > but with different audio parameters. Thread B continues to run and runs through > the loop, letting all the sinks know that buffer_->params() are the new params. > This is correct now that buffer_ points to those new parameters. > > However, now Thread A wakes up and enters the loop. Its new_buffer however > still holds a reference to the previous buffer and all the sinks will now be > told that the old audio parameters are the current ones. Oops. > > I guess the morale of the story is that we need to make the threading story of > the class better. Ideally the buffer and params should always be modifed on the > same thread and then we won't have the above problem. > > I don't think we need to address this in this CL, but I figured I'd get someone > else to think about it as well :) Yeah, the buffer and parameters really need to be delivered together to the sinks if we are to preserve that invariant. Now it's like we send updates on the capture format to our sinks, and they expect the buffer in this class to correspond to the parameters they last saw, but as you demonstrate that isn't true. Yeah, we can either require that the sinks are thread-safe OR that they don't call us back from within a handler (e.g we call them under our lock). Before we do this though we need to be really sure that this can indeed be called from different threads. I'm pretty sure but I could be wrong. https://codereview.chromium.org/12220063/diff/5004/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.cc:479: DCHECK(buffer_.get() != NULL); On 2013/02/08 20:00:39, tommi wrote: > you can remove this. we avoid having (D)CHECKs followed by dereferencing the > checked pointer anyway. Done. https://codereview.chromium.org/12220063/diff/5004/content/renderer/media/web... File content/renderer/media/webrtc_audio_capturer.h (right): https://codereview.chromium.org/12220063/diff/5004/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.h:142: media::AudioParameters audio_parameters(); On 2013/02/08 20:00:39, tommi wrote: > nit: I think it's still OK to keep the constness of the function as it was > (which would mean to make the lock mutable). Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/phoglund@chromium.org/12220063/12004
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/phoglund@chromium.org/12220063/12004
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is android_dbg, revision is HEAD
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/phoglund@chromium.org/12220063/12004
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is android_dbg, revision is HEAD
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/phoglund@chromium.org/12220063/12004
Hmm... the android_dbg_triggered_tests bot keeps preventing the change from going in. Annoyingly it doesn't seem to always fail the same way. I'm checking the box one more time but if it fails yet again for an unrelated reason, I think we need to land this manually. On Tue, Feb 12, 2013 at 12:52 AM, <commit-bot@chromium.org> wrote: > The commit queue went berserk retrying too often for a > seemingly flaky test. Builder is android_dbg, revision is HEAD > > https://chromiumcodereview.**appspot.com/12220063/<https://chromiumcodereview... > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
