Ensures that WebRTC works for device selection using a different sample rate than default

Ensures that WebRTC works for device selections using a different sample rate than default.

See https://docs.google.com/a/google.com/document/d/1Gi5Hmtfz_d6WSoSXwhgK4q7yBgjL7cTcgez-URFdZV4/edit for more details regarding this CL.

In addition, I have introduced base::ThreadChecker in WebRtcAudioCapturer to make it less flexible in how it can be called. It feels better to add restrictions given the existing usage than to "allow almost anything" as is the case now.

BUG=164058
TEST=A long list of WebRTC demos plus content and media unit tests. Also did browser_tests locally.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178287

[henrika (OOO until Aug 14), 2013-01-14 14:12:19]

Tommi, I have tried to explain the nature of this CL in a design doc to make it
more clear. Hope that helps.

Per, hope you can check the parts in MediaStreamImpl and
MediaStreamDependencyFactory.

I will probably need some more owners as well to land this one but my plan was
to finalize the bulk work first.

[henrika (OOO until Aug 14), 2013-01-14 14:25:07]

Will add joi@ for content/public later.

[perkj_chrome, 2013-01-15 09:00:19]

Please update and add unit test for creating an audio source in
MediaStreamDependencyFactoryTest.

https://codereview.chromium.org/11783059/diff/13001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/11783059/diff/13001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:235:
static_cast<MediaStreamSourceExtraData*>(source.extraData());
Will WebAudio also end up here? Fix comment in that case.

https://codereview.chromium.org/11783059/diff/13001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:252: // The capturer
does not support all possible sample rates.
nit: all possible ? I don't understand the comment- does it add any value here?
All we care about is if the source was successfully initialized.

https://codereview.chromium.org/11783059/diff/13001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:268: DLOG(WARNING) <<
"Unsupported audio source";
nit: NOTREACHED ?

https://codereview.chromium.org/11783059/diff/13001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.h (left):

https://codereview.chromium.org/11783059/diff/13001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.h:149: virtual void
SetAudioDeviceSessionId(int session_id);
Please fix the MockDependencyFactory after removing this. See all failing
unittests on the bots.

https://codereview.chromium.org/11783059/diff/13001/content/renderer/media/me...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/11783059/diff/13001/content/renderer/media/me...
content/renderer/media/media_stream_impl.cc:295: return local_renderer;
nit: return CreateLocalAudioRenderer(extra... ?

[tommi (sloooow) - chröme, 2013-01-15 17:43:48]

lgtm w/ some minor requests

https://codereview.chromium.org/11783059/diff/11017/content/browser/renderer_...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

https://codereview.chromium.org/11783059/diff/11017/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_device_manager.cc:157: int
sample_rate = 0;
define these two variables where they are used

https://codereview.chromium.org/11783059/diff/11017/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_device_manager.cc:223: if
(listener_) {
nit: revert this change (it's not consistent with the rest of the file)

https://codereview.chromium.org/11783059/diff/11017/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_ui_controller.cc (right):

https://codereview.chromium.org/11783059/diff/11017/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_ui_controller.cc:202: if
(dev->type == content::MEDIA_DEVICE_AUDIO_CAPTURE)
{}

https://codereview.chromium.org/11783059/diff/11017/content/public/common/med...
File content/public/common/media_stream_request.cc (right):

https://codereview.chromium.org/11783059/diff/11017/content/public/common/med...
content/public/common/media_stream_request.cc:46:
DCHECK(IsAudioMediaType(type));
these dchecks are a nice touch.

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:250:
static_cast<media::ChannelLayout>(device.channel_layout),
indent

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/me...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/me...
content/renderer/media/media_stream_impl.cc:378: <<
UTF16ToUTF8(description->label());
doesn't DVLOG support utf16?

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/we...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:102: for
(SinkList::const_iterator it = sinks_.begin();
is it possible that other methods can be called on this object before
Inititalize completes? (if not, then the lock isn't needed, right?)

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:103: it != sinks_.end(); ++it) {
fix indent

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:135:
DCHECK(std::find(sinks_.begin(), sinks_.end(), sink) == sinks_.end());
move back under lock

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:220:
DCHECK(thread_checker_.CalledOnValidThread());
thanks for adding all these thread dchecks!

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:288: void
WebRtcAudioCapturer::SetVolume(double volume) {
can SetVolume be called from multiple threads but not the other methods?

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/we...
File content/renderer/media/webrtc_audio_capturer.h (right):

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.h:47: NON_EXPORTED_BASE(public
content::WebRtcLocalAudioRenderer::LocalRenderCallback) {
80+

[Jói, 2013-01-15 22:30:20]

https://codereview.chromium.org/11783059/diff/11017/content/public/common/med...
File content/public/common/media_stream_request.h (right):

https://codereview.chromium.org/11783059/diff/11017/content/public/common/med...
content/public/common/media_stream_request.h:60: int sample_rate,
Document what the defaults for sample_rate and channel_layout are if you use the
3-param constructor.

https://codereview.chromium.org/11783059/diff/11017/content/public/common/med...
content/public/common/media_stream_request.h:76: int sample_rate;
Are the new struct members (sample_rate, channel_layout) going to be accessed
from outside of content?  If not, could they be added in a subclass of this
(after first changing it to a class)?

[henrika (OOO until Aug 14), 2013-01-16 16:37:17]

https://codereview.chromium.org/11783059/diff/11017/content/browser/renderer_...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

https://codereview.chromium.org/11783059/diff/11017/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_device_manager.cc:157: int
sample_rate = 0;
On 2013/01/15 17:43:49, tommi wrote:
> define these two variables where they are used

Done.

https://codereview.chromium.org/11783059/diff/11017/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_device_manager.cc:223: if
(listener_) {
On 2013/01/15 17:43:49, tommi wrote:
> nit: revert this change (it's not consistent with the rest of the file)

Done.

https://codereview.chromium.org/11783059/diff/11017/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_ui_controller.cc (right):

https://codereview.chromium.org/11783059/diff/11017/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_ui_controller.cc:202: if
(dev->type == content::MEDIA_DEVICE_AUDIO_CAPTURE)
On 2013/01/15 17:43:49, tommi wrote:
> {}

Done.

https://codereview.chromium.org/11783059/diff/11017/content/public/common/med...
File content/public/common/media_stream_request.cc (right):

https://codereview.chromium.org/11783059/diff/11017/content/public/common/med...
content/public/common/media_stream_request.cc:46:
DCHECK(IsAudioMediaType(type));
I know ;-)

https://codereview.chromium.org/11783059/diff/11017/content/public/common/med...
File content/public/common/media_stream_request.h (right):

https://codereview.chromium.org/11783059/diff/11017/content/public/common/med...
content/public/common/media_stream_request.h:60: int sample_rate,
On 2013/01/15 22:30:20, Jói wrote:
> Document what the defaults for sample_rate and channel_layout are if you use
the
> 3-param constructor.

Done.

https://codereview.chromium.org/11783059/diff/11017/content/public/common/med...
content/public/common/media_stream_request.h:76: int sample_rate;
No they are not (accessed outside content that is).

You mean that I should change the MediasStreamDevice to a class (and add a
subclass with only two members); right? Not sure how that would allow me to use
media::ChannelLayout. Also, what happens to the IPC part then
(https://codereview.chromium.org/11783059/diff/11017/content/common/media/medi...).
Still possible to send over IPC?

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:250:
static_cast<media::ChannelLayout>(device.channel_layout),
On 2013/01/15 17:43:49, tommi wrote:
> indent

Done.

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/me...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/me...
content/renderer/media/media_stream_impl.cc:378: <<
UTF16ToUTF8(description->label());
Correct. Done.

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/we...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:102: for
(SinkList::const_iterator it = sinks_.begin();
Really good point, thanks. Will make it more clear in the comments as well.

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:103: it != sinks_.end(); ++it) {
On 2013/01/15 17:43:49, tommi wrote:
> fix indent

Done.

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:135:
DCHECK(std::find(sinks_.begin(), sinks_.end(), sink) == sinks_.end());
Feels much better. Done.

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:220:
DCHECK(thread_checker_.CalledOnValidThread());
Thanks. I have done lots of manual testing here with all possible sorts of
clients to ensure that it holds.

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:288: void
WebRtcAudioCapturer::SetVolume(double volume) {
Correct. This one is special since it is called by the AudioInputDevice thread
when it is used in the AGC scheme.
Also, SetAutomaticGainControl is called from a Libjingle thread.

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/we...
File content/renderer/media/webrtc_audio_capturer.h (right):

https://codereview.chromium.org/11783059/diff/11017/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.h:47: NON_EXPORTED_BASE(public
content::WebRtcLocalAudioRenderer::LocalRenderCallback) {
On 2013/01/15 17:43:49, tommi wrote:
> 80+

Done.

[henrika (OOO until Aug 14), 2013-01-16 16:49:00]

Per,

think I've covered your parts. Did update MediaStreamDependencyFactoryTest but I
have not written any new test. Will see if I can come up with that as well.

https://codereview.chromium.org/11783059/diff/13001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/11783059/diff/13001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:235:
static_cast<MediaStreamSourceExtraData*>(source.extraData());
Do you mean for the MediaStreamDestination case (not yet in Chrome)?

https://codereview.chromium.org/11783059/diff/13001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:252: // The capturer
does not support all possible sample rates.
Refactored + fixed.

https://codereview.chromium.org/11783059/diff/13001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:268: DLOG(WARNING) <<
"Unsupported audio source";
See if you like new proposal better.

https://codereview.chromium.org/11783059/diff/13001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.h (left):

https://codereview.chromium.org/11783059/diff/13001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.h:149: virtual void
SetAudioDeviceSessionId(int session_id);
On 2013/01/15 09:00:19, perkj wrote:
> Please fix the MockDependencyFactory after removing this. See all failing
> unittests on the bots.

Done.

https://codereview.chromium.org/11783059/diff/13001/content/renderer/media/me...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/11783059/diff/13001/content/renderer/media/me...
content/renderer/media/media_stream_impl.cc:295: return local_renderer;
On 2013/01/15 09:00:19, perkj wrote:
> nit: return CreateLocalAudioRenderer(extra... ?

Done.

[henrika (OOO until Aug 14), 2013-01-16 16:53:25]

perkj: I can show you a proposal of a new unit test tomorrow.

joi: sorry but I had to reply with a question since I didn't really get the gist
in your comments.

[Jói, 2013-01-16 17:12:00]

https://codereview.chromium.org/11783059/diff/11017/content/public/common/med...
File content/public/common/media_stream_request.h (right):

https://codereview.chromium.org/11783059/diff/11017/content/public/common/med...
content/public/common/media_stream_request.h:76: int sample_rate;
On 2013/01/16 16:37:17, henrika wrote:
> No they are not (accessed outside content that is).
> 
> You mean that I should change the MediasStreamDevice to a class (and add a
> subclass with only two members); right? Not sure how that would allow me to
use
> media::ChannelLayout. Also, what happens to the IPC part then
>
(https://codereview.chromium.org/11783059/diff/11017/content/common/media/medi...).
> Still possible to send over IPC?

The IPC definitions are inside content (i.e. not in content/public) so I would
think they can reference the subclass?

If that doesn't work then I guess it's OK this way, but it is unfortunate to
expose stuff in content/public that isn't meant for consumption outside of
content.

[Jói, 2013-01-16 17:42:21]

+jam

John, can you take a look at content/public/common/media_stream_request.h?

The two new fields being added to MediaStreamDevice are not used by Chrome, so I
was asking Henrik if we could hide those parts in a subclass that wouldn't be in
content/public.  This is difficult because Chrome does actually constructs
MediaStreamDevice structs (see
chrome/browser/media/media_stream_devices_controller.cc) so hiding these fields
would imply not just a subclass but also adding a factory method to the Content
API for creating these objects.

What do you think, do we leave as is, with two fields currently unused by
embedders, or go for a more involved solution?

Cheers,
Jói

[jam, 2013-01-16 17:57:24]

this is fine with me, lgtm

https://codereview.chromium.org/11783059/diff/32003/content/public/common/med...
File content/public/common/media_stream_request.h (right):

https://codereview.chromium.org/11783059/diff/32003/content/public/common/med...
content/public/common/media_stream_request.h:76: // with three parameters
(intended for video) is used.
nit: here and below, I would skip this comment since it's needless. if someone
wants to see the default values, they can find it easily

[Jói, 2013-01-16 18:07:14]

LGTM

On Wed, Jan 16, 2013 at 9:57 AM,  <jam@chromium.org> wrote:
> this is fine with me, lgtm
>
>
>
https://codereview.chromium.org/11783059/diff/32003/content/public/common/med...
> File content/public/common/media_stream_request.h (right):
>
>
https://codereview.chromium.org/11783059/diff/32003/content/public/common/med...
> content/public/common/media_stream_request.h:76: // with three
> parameters (intended for video) is used.
> nit: here and below, I would skip this comment since it's needless. if
> someone wants to see the default values, they can find it easily
>
> https://codereview.chromium.org/11783059/

[henrika (OOO until Aug 14), 2013-01-17 12:26:40]

Thanks guys.

This guy (https://codereview.chromium.org/11369171/) just landed and it hits me
pretty hard so I must spend some time on merging these two. Will upload a new
version before landing.

[henrika (OOO until Aug 14), 2013-01-21 07:49:32]

cevans@: could you please double-check the changes in \content\public\common.
Many thanks.

[henrika (OOO until Aug 14), 2013-01-22 12:38:11]

cevans@: to clarify why I am asking for a late OK. Is still see this when trying
to land:

** Presubmit Messages **
Missing OWNER reviewers for files in these directories:
    content\common\media\media_stream_messages.h

Suggested OWNERS:
    acolwell@chromium.org
    joi@chromium.org
    inferno@chromium.org

[Chris Evans, 2013-01-23 09:39:04]

LGTM provided you action the comment if appropriate.

https://codereview.chromium.org/11783059/diff/62004/content/renderer/media/we...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/11783059/diff/62004/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:91: return false;
I see that sample_rate is carefully validated in this function, but
channel_layout is not? Seems inconsistent.

[henrika (OOO until Aug 14), 2013-01-23 10:25:40]

https://codereview.chromium.org/11783059/diff/62004/content/renderer/media/we...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/11783059/diff/62004/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:91: return false;
Good point. Will improve before committing. Thanks.

[commit-bot: I haz the power, 2013-01-23 10:45:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/henrika@chromium.org/11783059/68007

[commit-bot: I haz the power, 2013-01-23 12:55:08]

Change committed as 178287