remove the race related to output_delay_ms_ in ADM and races in the unittests.

remove the race related to output_delay_ms_ in ADM and races in the unittests.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114002

[no longer working on chromium, 2011-12-05 13:31:29]

This patch fixes the races in the ADM and webrtc unittest.

Please review. 

Thanks,

BR,
/SX

[tommi (sloooow) - chröme, 2011-12-05 13:41:04]

lgtm

http://codereview.chromium.org/8799011/diff/3002/content/renderer/media/webrt...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

http://codereview.chromium.org/8799011/diff/3002/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_impl.cc:663: {
nit: no need for this scope

[henrika (OOO until Aug 14), 2011-12-05 13:56:17]

LGTM.

[henrika (OOO until Aug 14), 2011-12-05 13:56:24]

http://codereview.chromium.org/8799011/diff/3002/content/renderer/media/webrt...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

http://codereview.chromium.org/8799011/diff/3002/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_impl.cc:915: *delay_ms =
static_cast<uint16_t>(input_delay_ms_);
Why no lock here?

[wjia(left Chromium), 2011-12-07 01:10:01]

lgtm

[scherkus (not reviewing), 2011-12-08 23:49:41]

tiny style drive by

as a general comment... could the race get solved by fixing webrtc code
upstream? slapping locks around variables is one way of fixing things but it's
not clear to me why no other variables (such as input_delay_ms_) requires
locking

http://codereview.chromium.org/8799011/diff/3002/content/renderer/media/webrt...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

http://codereview.chromium.org/8799011/diff/3002/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_unittest.cc:124: return channel_id_;
}
drive by nit: drop } to next line here + below

[no longer working on chromium, 2011-12-09 09:48:18]

Changed the code based on the comments.

Andrew,
Thanks, it is a indeed really good idea to fix the race in webrtc instead. I
think it is just legacy to store the delay values in ADM, we can absolutely pass
the values up to webrtc so it doesn't need to query the values.

I will investigate on the possibility of such a patch, but changing the webrtc
ADMs is usually a pain and needs some time. So I want to land this CL first and
will refactor the ADM when webrtc is ready to handle the delay values.
Hope it is fine.

BR,
-SX

http://codereview.chromium.org/8799011/diff/3002/content/renderer/media/webrt...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

http://codereview.chromium.org/8799011/diff/3002/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_impl.cc:663: {
On 2011/12/05 13:41:04, tommi wrote:
> nit: no need for this scope

Done.

http://codereview.chromium.org/8799011/diff/3002/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_impl.cc:915: *delay_ms =
static_cast<uint16_t>(input_delay_ms_);
On 2011/12/05 13:56:24, henrika1 wrote:
> Why no lock here?

We don't use RecordingDelay() now, but I can add it anyway.

http://codereview.chromium.org/8799011/diff/3002/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_impl.cc:915: *delay_ms =
static_cast<uint16_t>(input_delay_ms_);
On 2011/12/05 13:56:24, henrika1 wrote:
> Why no lock here?

Done.

http://codereview.chromium.org/8799011/diff/3002/content/renderer/media/webrt...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

http://codereview.chromium.org/8799011/diff/3002/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_unittest.cc:124: return channel_id_;
}
On 2011/12/08 23:49:42, scherkus wrote:
> drive by nit: drop } to next line here + below

Done.

[tommi (sloooow) - chröme, 2011-12-09 10:16:42]

lgtm++

+1 on fixing things on the webrtc side whenever we can.  FYI the team has
already made several fixes there already, both for valgrind+tsan issues as
well as reducing the number of static initializers (getting close to zero
in webrtc, but we still have some in other dependent libraries such as
json).  We'll roll those in shortly.

On Fri, Dec 9, 2011 at 10:48 AM, <xians@chromium.org> wrote:

> Changed the code based on the comments.
>
> Andrew,
> Thanks, it is a indeed really good idea to fix the race in webrtc instead.
> I
> think it is just legacy to store the delay values in ADM, we can
> absolutely pass
> the values up to webrtc so it doesn't need to query the values.
>
> I will investigate on the possibility of such a patch, but changing the
> webrtc
> ADMs is usually a pain and needs some time. So I want to land this CL
> first and
> will refactor the ADM when webrtc is ready to handle the delay values.
> Hope it is fine.
>
> BR,
> -SX
>
>
>
> http://codereview.chromium.**org/8799011/diff/3002/content/**
>
renderer/media/webrtc_audio_**device_impl.cc<http://codereview.chromium.org/8799011/diff/3002/content/renderer/media/webrtc_audio_device_impl.cc>
> File content/renderer/media/webrtc_**audio_device_impl.cc (right):
>
> http://codereview.chromium.**org/8799011/diff/3002/content/**
>
renderer/media/webrtc_audio_**device_impl.cc#newcode663<http://codereview.chromium.org/8799011/diff/3002/content/renderer/media/webrtc_audio_device_impl.cc#newcode663>
> content/renderer/media/webrtc_**audio_device_impl.cc:663: {
>
> On 2011/12/05 13:41:04, tommi wrote:
>
>> nit: no need for this scope
>>
>
> Done.
>
>
> http://codereview.chromium.**org/8799011/diff/3002/content/**
>
renderer/media/webrtc_audio_**device_impl.cc#newcode915<http://codereview.chromium.org/8799011/diff/3002/content/renderer/media/webrtc_audio_device_impl.cc#newcode915>
> content/renderer/media/webrtc_**audio_device_impl.cc:915: *delay_ms =
> static_cast<uint16_t>(input_**delay_ms_);
> On 2011/12/05 13:56:24, henrika1 wrote:
>
>> Why no lock here?
>>
>
> We don't use RecordingDelay() now, but I can add it anyway.
>
>
> http://codereview.chromium.**org/8799011/diff/3002/content/**
>
renderer/media/webrtc_audio_**device_impl.cc#newcode915<http://codereview.chromium.org/8799011/diff/3002/content/renderer/media/webrtc_audio_device_impl.cc#newcode915>
> content/renderer/media/webrtc_**audio_device_impl.cc:915: *delay_ms =
> static_cast<uint16_t>(input_**delay_ms_);
> On 2011/12/05 13:56:24, henrika1 wrote:
>
>> Why no lock here?
>>
>
> Done.
>
>
> http://codereview.chromium.**org/8799011/diff/3002/content/**
>
renderer/media/webrtc_audio_**device_unittest.cc<http://codereview.chromium.org/8799011/diff/3002/content/renderer/media/webrtc_audio_device_unittest.cc>
> File content/renderer/media/webrtc_**audio_device_unittest.cc (right):
>
> http://codereview.chromium.**org/8799011/diff/3002/content/**
>
renderer/media/webrtc_audio_**device_unittest.cc#newcode124<http://codereview.chromium.org/8799011/diff/3002/content/renderer/media/webrtc_audio_device_unittest.cc#newcode124>
> content/renderer/media/webrtc_**audio_device_unittest.cc:124: return
> channel_id_; }
> On 2011/12/08 23:49:42, scherkus wrote:
>
>> drive by nit: drop } to next line here + below
>>
>
> Done.
>
>
http://codereview.chromium.**org/8799011/<http://codereview.chromium.org/8799...
>

[scherkus (not reviewing), 2011-12-09 18:29:35]

On 2011/12/09 09:48:18, xians1 wrote:
> Changed the code based on the comments.
> 
> Andrew,
> Thanks, it is a indeed really good idea to fix the race in webrtc instead. I
> think it is just legacy to store the delay values in ADM, we can absolutely
pass
> the values up to webrtc so it doesn't need to query the values.
> 
> I will investigate on the possibility of such a patch, but changing the webrtc
> ADMs is usually a pain and needs some time. So I want to land this CL first
and
> will refactor the ADM when webrtc is ready to handle the delay values.
> Hope it is fine.

Thanks for the explanation -- carry on :)
 
>
http://codereview.chromium.org/8799011/diff/3002/content/renderer/media/webrt...
> content/renderer/media/webrtc_audio_device_impl.cc:663: {
> On 2011/12/05 13:41:04, tommi wrote:
> > nit: no need for this scope
> 
> Done.
> 
>
http://codereview.chromium.org/8799011/diff/3002/content/renderer/media/webrt...
> content/renderer/media/webrtc_audio_device_impl.cc:915: *delay_ms =
> static_cast<uint16_t>(input_delay_ms_);
> On 2011/12/05 13:56:24, henrika1 wrote:
> > Why no lock here?
> 
> We don't use RecordingDelay() now, but I can add it anyway.
> 
>
http://codereview.chromium.org/8799011/diff/3002/content/renderer/media/webrt...
> content/renderer/media/webrtc_audio_device_impl.cc:915: *delay_ms =
> static_cast<uint16_t>(input_delay_ms_);
> On 2011/12/05 13:56:24, henrika1 wrote:
> > Why no lock here?
> 
> Done.
> 
>
http://codereview.chromium.org/8799011/diff/3002/content/renderer/media/webrt...
> File content/renderer/media/webrtc_audio_device_unittest.cc (right):
> 
>
http://codereview.chromium.org/8799011/diff/3002/content/renderer/media/webrt...
> content/renderer/media/webrtc_audio_device_unittest.cc:124: return
channel_id_;
> }
> On 2011/12/08 23:49:42, scherkus wrote:
> > drive by nit: drop } to next line here + below
> 
> Done.

[commit-bot: I haz the power, 2011-12-12 09:45:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/8799011/19001

[commit-bot: I haz the power, 2011-12-12 11:03:27]

Change committed as 114002