Switch AudioDevice classes from SyncSocket to CancelableSyncSocket.

Switch AudioDevice classes from SyncSocket to CancelableSyncSocket.
This fixes the double close issue and makes Stop() instant.
Also, this addresses some issues reported by tsan (see bug reports).

BUG=103711, 95509
TEST=content_unittests,media_unittests


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

[tommi (sloooow) - chröme, 2012-01-30 12:05:46]

switching reviewers from Eugene to Shijing since Shijing was working on this
initially.

[no longer working on chromium, 2012-01-30 13:35:17]

Great work! 
only two nits, LGTM if you take a look at them.

BR,
/SX

https://chromiumcodereview.appspot.com/9121045/diff/1/content/renderer/media/...
File content/renderer/media/audio_input_device.cc (right):

https://chromiumcodereview.appspot.com/9121045/diff/1/content/renderer/media/...
content/renderer/media/audio_input_device.cc:278: int pending_data;
nit: what about initializing the value to 0?

https://chromiumcodereview.appspot.com/9121045/diff/1/content/renderer/media/...
content/renderer/media/audio_input_device.cc:280: if (received !=
sizeof(pending_data) || pending_data < 0) {
nit: how is it possible for pending_data to be negative?
the relevant code in AudioInputController is
sync_writer_->UpdateRecordedBytes(hardware_delay_bytes); where
hardware_delay_bytes is uint32
Maybe we do not need checking pending_data.

[tommi (sloooow) - chröme, 2012-01-30 17:07:39]

https://chromiumcodereview.appspot.com/9121045/diff/1/content/renderer/media/...
File content/renderer/media/audio_input_device.cc (right):

https://chromiumcodereview.appspot.com/9121045/diff/1/content/renderer/media/...
content/renderer/media/audio_input_device.cc:278: int pending_data;
On 2012/01/30 13:35:18, xians1 wrote:
> nit: what about initializing the value to 0?

Done.

https://chromiumcodereview.appspot.com/9121045/diff/1/content/renderer/media/...
content/renderer/media/audio_input_device.cc:280: if (received !=
sizeof(pending_data) || pending_data < 0) {
On 2012/01/30 13:35:18, xians1 wrote:
> nit: how is it possible for pending_data to be negative?
> the relevant code in AudioInputController is
> sync_writer_->UpdateRecordedBytes(hardware_delay_bytes); where
> hardware_delay_bytes is uint32
> Maybe we do not need checking pending_data.

Changed the type to uint32 as discussed (to match the other end) and changed the
check.

[no longer working on chromium, 2012-01-30 17:29:32]

LGTM++

[commit-bot: I haz the power, 2012-01-30 17:39:12]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[scherkus (not reviewing), 2012-01-30 19:10:57]

LGTM w/ some nits

http://codereview.chromium.org/9121045/diff/8001/content/renderer/media/audio...
File content/renderer/media/audio_device.cc (right):

http://codereview.chromium.org/9121045/diff/8001/content/renderer/media/audio...
content/renderer/media/audio_device.cc:274: base::CancelableSyncSocket*
audio_socket = audio_socket_.get();
nit: do we need this anymore?

http://codereview.chromium.org/9121045/diff/8001/content/renderer/media/audio...
content/renderer/media/audio_device.cc:336: // TODO(tommi): We must not do this
from the IO thread.  Fix.
is this coming in a follow up CL or should we file a bug for M19?

http://codereview.chromium.org/9121045/diff/8001/content/renderer/media/audio...
File content/renderer/media/audio_input_device.cc (right):

http://codereview.chromium.org/9121045/diff/8001/content/renderer/media/audio...
content/renderer/media/audio_input_device.cc:270: base::CancelableSyncSocket*
socket = audio_socket_.get();
nit: do we need this line of code?

[tommi (sloooow) - chröme, 2012-01-30 21:53:13]

Thanks Andrew, replies below.

http://codereview.chromium.org/9121045/diff/8001/content/renderer/media/audio...
File content/renderer/media/audio_device.cc (right):

http://codereview.chromium.org/9121045/diff/8001/content/renderer/media/audio...
content/renderer/media/audio_device.cc:274: base::CancelableSyncSocket*
audio_socket = audio_socket_.get();
On 2012/01/30 19:10:57, scherkus wrote:
> nit: do we need this anymore?

yes, this is different than what we had before (scoped_refptr vs
CancelableSyncSocket*)

http://codereview.chromium.org/9121045/diff/8001/content/renderer/media/audio...
content/renderer/media/audio_device.cc:336: // TODO(tommi): We must not do this
from the IO thread.  Fix.
On 2012/01/30 19:10:57, scherkus wrote:
> is this coming in a follow up CL or should we file a bug for M19?

Yes, I'll start working on it tomorrow.  I'll file a bug.

http://codereview.chromium.org/9121045/diff/8001/content/renderer/media/audio...
File content/renderer/media/audio_input_device.cc (right):

http://codereview.chromium.org/9121045/diff/8001/content/renderer/media/audio...
content/renderer/media/audio_input_device.cc:270: base::CancelableSyncSocket*
socket = audio_socket_.get();
On 2012/01/30 19:10:57, scherkus wrote:
> nit: do we need this line of code?

yup (different than before)

[commit-bot: I haz the power, 2012-01-30 21:53:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommi@chromium.org/9121045/8001

[commit-bot: I haz the power, 2012-01-30 23:48:05]

Change committed as 119769