Switch LiveAudio to source provider solution.

Switch LiveAudio to a source provider solution.

The current LiveAudio has some problems since it creates its own input stream instead of using the stream from gUM.
This patch will make the WebAudio uses the data from gUM through a source provider.


BUG=257963
TEST=content_unittests && http://webrtc.googlecode.com/svn/trunk/samples/js/demos/html/webaudio-and-webrtc.html

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

[no longer working on chromium, 2013-09-05 08:53:14]

Tommi, I still need to tune some code in the source provider, but the overall
solution has been there. Could you please start reviewing it?

Thanks,
SX

[no longer working on chromium, 2013-09-05 16:27:30]

Hi Tommi,

I think I have already correctly rebased, and please take a look at the CL when
you have time.

Thanks,
SX

[tommi (sloooow) - chröme, 2013-09-06 11:20:30]

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.cc (left):

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:610:
MaybeCreateAudioCapturer(-1, StreamDeviceInfo()));
nice to get rid of this :)

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

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:64:
webrtc::MediaConstraintsInterface::kValueFalse},
please cc bemasc.  It does seem though that if all these are set to false we
might as well not set them and remove the ApplyFixedWebAudioConstraints
function.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:492: return
native_stream->RemoveTrack(native_stream->FindVideoTrack(track_id));
can you add a DCHECK_EQ(type, WebKit::WebMediaStreamSource::TypeVideo) first?

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:809: if
(render_view_id < 0)
Can we now simply DCHECK that render_view_id is >= 0 and skip this if()?

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.h (right):

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.h:176:
WebAudioCapturerSource* webaudio_source,
Please add documentation for this parameter.  It's confusing to see two sources
so I think a very clear explanation would be good.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.h:177:
webrtc::AudioSourceInterface* track_source,
nit: just keep the name as |source|

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/rt...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/rt...
content/renderer/media/rtc_peer_connection_handler.cc:536: // A media stream is
connected to a peer connection, enable the native mode
s/the native mode/peer connection mode

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
File content/renderer/media/webaudio_capturer_source.cc (right):

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webaudio_capturer_source.cc:18: static const int
kMaxNumberOfBuffer = 5;
kMaxNumberOfBuffers (plural).
It would also be good to document what buffers this refers to.  Before it hinted
at the fifo but now it doesn't.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webaudio_capturer_source.cc:42:
params_.Reset(media::AudioParameters::AUDIO_PCM_LOW_LATENCY,
does params_ not need to be behind the lock?  (if not that's fine but it might
be a good idea to document that somewhere)

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webaudio_capturer_source.cc:44: sample_rate / 100);
Can you document here that we always use a 10ms buffer and why we do that?

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webaudio_capturer_source.cc:80: size_t number_of_frames)
{
On which thread are we here?  If this is not the same thread as Start/Stop are
called on, then I think we should move param_ usage on that thread under a lock.
Also, it would be good to have a thread checker check the controller thread at
least and then document elsewhere what thread we are on in each place.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
File content/renderer/media/webaudio_capturer_source.h (right):

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webaudio_capturer_source.h:42: void
Start(WebRtcLocalAudioTrack* track,
can you add documentation about the parameters and how they are linked?

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webaudio_capturer_source.h:49: friend class
base::RefCountedThreadSafe<WebAudioCapturerSource>;
indent seems off

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webaudio_capturer_source.h:53: // a weak pointer to the
audio track this WebAudioCapturerSource is feeding
this isn't a weak pointer, it's a raw pointer. "weak pointer" refers to WeakPtr.
 How is ownership managed?  If the track object is reference counted, why aren't
we using scoped_refptr? etc (please add comments that explain all of this)

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webaudio_capturer_source.h:59:
WebRtcLocalAudioSourceProvider* source_provider_;
this is also a raw pointer.  please add comments that explain lifetime
management.

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

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:152: if (render_view_id == -1) {
this should never happen now that you've added the above dcheck, right?

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:283: DVLOG(1) << "New capture
source will now be utilized.";
s/utilized/used
suggest "Switching to a new capture source."

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:305: // Do nothing if the native
mode has been enabled.
s/the native mode/peer connection mode
(or just remove the comment)

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:309: DLOG(WARNING) <<
"EnablePeerConnectionMode";
Remove?

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:323:
SetCapturerSource(AudioDeviceFactory::NewInputDevice(render_view_id_),
Since you're no longer under the lock, is it possible that a source could have
been set between here and where the lock was released?
If you only ever change the value of source_ and render_view_id_ on this thread
(i.e. the same thread as you use the thread checker on), then this is ok, but if
not, then you need to hold the lock while you do the check as well as assign new
values.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:325:
static_cast<float>(params.sample_rate()));
why are we using float for the sample rate?

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:447: return
source_provider_.get();
since we never change the value of source_provider_, let's make it a const
member variable in the header.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:455: DLOG(WARNING) <<
"GetBufferSize " << peer_connection_mode_;
remove?

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:456: if (peer_connection_mode_)
{
before checking this, should we grab the lock?  (or dcheck that we're always on
the same thread when we touch this variable?)

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

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.h:81: void
EnablePeerConnectionMode();
documentation

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.h:105:
WebKit::WebAudioSourceProvider* AudioSourceProvider() const;
GetAudioSourceProvider

usually for inline getters this would be:
audio_source_provider(), but since this isn't inline we use Get + camel caps

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.h:138: // Helper function to get
the buffer size based on |native_mode_| and
s/native_mode_/peer_connection_mode_

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.h:155: // Cached values of utilized
audio parameters.
nit: utilized and used does not mean exactly the same thing.  Suggest simply
"Cached audio parameters for output."

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_source_provider.cc (right):

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.cc:37:
media::AudioHardwareConfig* hardware_config =
FYI this needs to change probably before you can land your cl.  Dependency on
AudioHardwareConfig is going away since we are getting closer to support
non-default output devices.  See here what you'll need to do:
https://codereview.chromium.org/23466008/
(basically, you'll need to feed the sample rate via the constructor and not use
AudioHardwareConfig)

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.cc:73: // Cache the
values for GetStreamInfo().
if you change the function name, please also update the comments

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.cc:83: if
(!last_fill_.is_null()) {
there's no locking here.  add a thread check?

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_source_provider.h (right):

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.h:31: //    
WebKit::WebAudioSourceProvider <---> WebRtcAudioCapturer
is the flow of data bidirectional or unidirectional?

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.h:40: :
NON_EXPORTED_BASE(public media::AudioConverter::InputCallback),
nit: public NON_EXPORTED_BASE(media::AudioConverter::InputCallback),

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.h:44:
~WebRtcLocalAudioSourceProvider();
virtual?

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.h:59: void
GetStreamInfo(int* delay_ms, int* volume, bool* key_pressed);
Can we call this something to indicate audio processing?  E.g.
GetAudioProcessingParams?

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.h:83: base::Lock
lock_;
document what the lock is used to protect?

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:16: static const size_t
kMaxNumberOfBufferInFifo = 2;
kMaxNumberOfBuffersInFifo (plural)

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:61: void Initialize(const
media::AudioParameters& params) {
Since Initialize() returns void and it's not a virtual method, can we just pass
the params to the constructor and do initialization there?

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:64: sink_buffer_size_ =
params.sample_rate() / 100;
document why 10ms

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:77: DCHECK(fifo_.get());
this isn't necessary since you dereference the pointer anyway

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:78: CHECK(fifo_->frames() +
audio_source->frames() <= fifo_->max_frames());
DCHECK?

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:83:
DCHECK(audio_wrapper_.get());
unnecessary

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:301: // capturer as its sink.
document why not.  The "what" can usually be understood from reading the code
but the "why" is often more important to explain in comments.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:321:
capturer_->RemoveTrack(this);
why do we not want to call RemoveTrack if webaudio_source is non-NULL?

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_track_unittest.cc (right):

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track_unittest.cc:137:
capturer_->EnablePeerConnectionMode();
why do we need this?  Should we run all tests twice, once with and once without
peerconnection mode enabled?  (it's possible to parameterize tests like that)

https://codereview.chromium.org/23691038/diff/17001/media/base/audio_hardware...
File media/base/audio_hardware_config.cc (right):

https://codereview.chromium.org/23691038/diff/17001/media/base/audio_hardware...
media/base/audio_hardware_config.cc:41: int
AudioHardwareConfig::GetInputBufferSize() const {
If we can avoid adding this that'd be great.  We're trying to avoid
AudioHardwareConfig at all and the sample rate + buffer sizes should already be
available for the input device.

[no longer working on chromium, 2013-09-10 12:43:15]

Tommi, could you please take another look?

Thanks,
SX

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

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:64:
webrtc::MediaConstraintsInterface::kValueFalse},
On 2013/09/06 11:20:30, tommi wrote:
> please cc bemasc.  It does seem though that if all these are set to false we
> might as well not set them and remove the ApplyFixedWebAudioConstraints
> function.

Removed the changed now, I want to completely remove the hard-coded constrains,
but lets do it in a separate CL.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:492: return
native_stream->RemoveTrack(native_stream->FindVideoTrack(track_id));
On 2013/09/06 11:20:30, tommi wrote:
> can you add a DCHECK_EQ(type, WebKit::WebMediaStreamSource::TypeVideo) first?

Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:809: if
(render_view_id < 0)
On 2013/09/06 11:20:30, tommi wrote:
> Can we now simply DCHECK that render_view_id is >= 0 and skip this if()?

I think the answer might depend on if we support gUM without a proper render
view, like extension.
I added a TODO here and changed to a DCHECK, let me know if you have another
thought.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.h (right):

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.h:177:
webrtc::AudioSourceInterface* track_source,
On 2013/09/06 11:20:30, tommi wrote:
> nit: just keep the name as |source|

Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/rt...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/rt...
content/renderer/media/rtc_peer_connection_handler.cc:536: // A media stream is
connected to a peer connection, enable the native mode
On 2013/09/06 11:20:30, tommi wrote:
> s/the native mode/peer connection mode

Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
File content/renderer/media/webaudio_capturer_source.cc (right):

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webaudio_capturer_source.cc:18: static const int
kMaxNumberOfBuffer = 5;
On 2013/09/06 11:20:30, tommi wrote:
> kMaxNumberOfBuffers (plural).
> It would also be good to document what buffers this refers to.  Before it
hinted
> at the fifo but now it doesn't.
> 

Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webaudio_capturer_source.cc:42:
params_.Reset(media::AudioParameters::AUDIO_PCM_LOW_LATENCY,
On 2013/09/06 11:20:30, tommi wrote:
> does params_ not need to be behind the lock?  (if not that's fine but it might
> be a good idea to document that somewhere)

Put it behind a lock as well.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webaudio_capturer_source.cc:44: sample_rate / 100);
On 2013/09/06 11:20:30, tommi wrote:
> Can you document here that we always use a 10ms buffer and why we do that?

Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webaudio_capturer_source.cc:80: size_t number_of_frames)
{
On 2013/09/06 11:20:30, tommi wrote:
> On which thread are we here?  If this is not the same thread as Start/Stop are
> called on, then I think we should move param_ usage on that thread under a
lock.
> Also, it would be good to have a thread checker check the controller thread at
> least and then document elsewhere what thread we are on in each place.

Added a thread check for Start/Stop/setFormat;
This is called on the webaudio audio thread, I have already move the params_
under the lock and also added a comment in the header file.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
File content/renderer/media/webaudio_capturer_source.h (right):

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webaudio_capturer_source.h:42: void
Start(WebRtcLocalAudioTrack* track,
On 2013/09/06 11:20:30, tommi wrote:
> can you add documentation about the parameters and how they are linked?

Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webaudio_capturer_source.h:49: friend class
base::RefCountedThreadSafe<WebAudioCapturerSource>;
On 2013/09/06 11:20:30, tommi wrote:
> indent seems off

Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webaudio_capturer_source.h:53: // a weak pointer to the
audio track this WebAudioCapturerSource is feeding
On 2013/09/06 11:20:30, tommi wrote:
> this isn't a weak pointer, it's a raw pointer. "weak pointer" refers to
WeakPtr.
>  How is ownership managed?  If the track object is reference counted, why
aren't
> we using scoped_refptr? etc (please add comments that explain all of this)

Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webaudio_capturer_source.h:59:
WebRtcLocalAudioSourceProvider* source_provider_;
On 2013/09/06 11:20:30, tommi wrote:
> this is also a raw pointer.  please add comments that explain lifetime
> management.

Done.

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

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:152: if (render_view_id == -1) {
On 2013/09/06 11:20:30, tommi wrote:
> this should never happen now that you've added the above dcheck, right?

This happens and is needed for testing cases. Also it might be a valid case for
extension without a proper render view.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:283: DVLOG(1) << "New capture
source will now be utilized.";
On 2013/09/06 11:20:30, tommi wrote:
> s/utilized/used
> suggest "Switching to a new capture source."

Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:305: // Do nothing if the native
mode has been enabled.
On 2013/09/06 11:20:30, tommi wrote:
> s/the native mode/peer connection mode
> (or just remove the comment)

Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:309: DLOG(WARNING) <<
"EnablePeerConnectionMode";
On 2013/09/06 11:20:30, tommi wrote:
> Remove?

Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:323:
SetCapturerSource(AudioDeviceFactory::NewInputDevice(render_view_id_),
On 2013/09/06 11:20:30, tommi wrote:
> Since you're no longer under the lock, is it possible that a source could have
> been set between here and where the lock was released?
> If you only ever change the value of source_ and render_view_id_ on this
thread
> (i.e. the same thread as you use the thread checker on), then this is ok, but
if
> not, then you need to hold the lock while you do the check as well as assign
new
> values.

SetCapturerSource() can't be called under a lock since it is using lock inside
inside the method.
The concern is not a problem here since SetCapturerSource() will only be called
in the one thread with thread check.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:325:
static_cast<float>(params.sample_rate()));
On 2013/09/06 11:20:30, tommi wrote:
> why are we using float for the sample rate?

Just chatted with Henrik on this, it is simply someone wants it to be float.
We should change it back to int in a separate CL.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:447: return
source_provider_.get();
On 2013/09/06 11:20:30, tommi wrote:
> since we never change the value of source_provider_, let's make it a const
> member variable in the header.

Done with changing the source_provider_ to be const
scoped_ptr<WebRtcLocalAudioSourceProvider>

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:455: DLOG(WARNING) <<
"GetBufferSize " << peer_connection_mode_;
On 2013/09/06 11:20:30, tommi wrote:
> remove?

Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:456: if (peer_connection_mode_)
{
On 2013/09/06 11:20:30, tommi wrote:
> before checking this, should we grab the lock?  (or dcheck that we're always
on
> the same thread when we touch this variable?)

Done with adding a thread check.

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

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.h:81: void
EnablePeerConnectionMode();
On 2013/09/06 11:20:30, tommi wrote:
> documentation

Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.h:105:
WebKit::WebAudioSourceProvider* AudioSourceProvider() const;
On 2013/09/06 11:20:30, tommi wrote:
> GetAudioSourceProvider
> 
> usually for inline getters this would be:
> audio_source_provider(), but since this isn't inline we use Get + camel caps

Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.h:138: // Helper function to get
the buffer size based on |native_mode_| and
On 2013/09/06 11:20:30, tommi wrote:
> s/native_mode_/peer_connection_mode_

Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.h:155: // Cached values of utilized
audio parameters.
On 2013/09/06 11:20:30, tommi wrote:
> nit: utilized and used does not mean exactly the same thing.  Suggest simply
> "Cached audio parameters for output."

Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_source_provider.cc (right):

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.cc:37:
media::AudioHardwareConfig* hardware_config =
On 2013/09/06 11:20:30, tommi wrote:
> FYI this needs to change probably before you can land your cl.  Dependency on
> AudioHardwareConfig is going away since we are getting closer to support
> non-default output devices.  See here what you'll need to do:
> https://codereview.chromium.org/23466008/
> (basically, you'll need to feed the sample rate via the constructor and not
use
> AudioHardwareConfig) 


As discussed offline, we have to keep this for now since WebAudio is running on
a native sample rate of the default device.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.cc:73: // Cache the
values for GetStreamInfo().
On 2013/09/06 11:20:30, tommi wrote:
> if you change the function name, please also update the comments

Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.cc:83: if
(!last_fill_.is_null()) {
On 2013/09/06 11:20:30, tommi wrote:
> there's no locking here.  add a thread check?

I have already added a thread check for the Initialize() which happens on the
render thread.
Added a comment to explain this method is called by webaudio audio thread,
following the provideInput(), hope it is fine.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_source_provider.h (right):

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.h:31: //    
WebKit::WebAudioSourceProvider <---> WebRtcAudioCapturer
On 2013/09/06 11:20:30, tommi wrote:
> is the flow of data bidirectional or unidirectional?

It is one way, from WebRtcAudioCapturer to WebKit::WebAudioSourceProvider.
I changed the <---> to WebRtcAudioCapturer ---> WebKit::WebAudioSourceProvider

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.h:40: :
NON_EXPORTED_BASE(public media::AudioConverter::InputCallback),
On 2013/09/06 11:20:30, tommi wrote:
> nit: public NON_EXPORTED_BASE(media::AudioConverter::InputCallback),

Ignore as discussed offline.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.h:44:
~WebRtcLocalAudioSourceProvider();
On 2013/09/06 11:20:30, tommi wrote:
> virtual?

:) why not? Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.h:59: void
GetStreamInfo(int* delay_ms, int* volume, bool* key_pressed);
On 2013/09/06 11:20:30, tommi wrote:
> Can we call this something to indicate audio processing?  E.g.
> GetAudioProcessingParams?

Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.h:83: base::Lock
lock_;
On 2013/09/06 11:20:30, tommi wrote:
> document what the lock is used to protect?

The lock is protecting all the member variables.
Done with adding a comment.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:16: static const size_t
kMaxNumberOfBufferInFifo = 2;
On 2013/09/06 11:20:30, tommi wrote:
> kMaxNumberOfBuffersInFifo (plural)

Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:61: void Initialize(const
media::AudioParameters& params) {
On 2013/09/06 11:20:30, tommi wrote:
> Since Initialize() returns void and it's not a virtual method, can we just
pass
> the params to the constructor and do initialization there?

The main reason for existing code is that we will grab a reference of of the
buffer_ in our real audio thread, and in this use case we don't want to
initialize the buffer. See code for details:
scoped_refptr<ConfiguredBuffer> current_buffer;
{
  base::AutoLock auto_lock(lock_);
  current_buffer = buffer_;
}
// then use the current_buffer

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:64: sink_buffer_size_ =
params.sample_rate() / 100;
On 2013/09/06 11:20:30, tommi wrote:
> document why 10ms

Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:78: CHECK(fifo_->frames() +
audio_source->frames() <= fifo_->max_frames());
On 2013/09/06 11:20:30, tommi wrote:
> DCHECK?

Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:83:
DCHECK(audio_wrapper_.get());
On 2013/09/06 11:20:30, tommi wrote:
> unnecessary

Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:301: // capturer as its sink.
On 2013/09/06 11:20:30, tommi wrote:
> document why not.  The "what" can usually be understood from reading the code
> but the "why" is often more important to explain in comments.

Done.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:321:
capturer_->RemoveTrack(this);
On 2013/09/06 11:20:30, tommi wrote:
> why do we not want to call RemoveTrack if webaudio_source is non-NULL?

Added a comment to explain the track is not a sink to the capturer when webaudio
source is non-NULL.

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_track_unittest.cc (right):

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track_unittest.cc:137:
capturer_->EnablePeerConnectionMode();
On 2013/09/06 11:20:30, tommi wrote:
> why do we need this?  Should we run all tests twice, once with and once
without
> peerconnection mode enabled?  (it's possible to parameterize tests like that)

Oh, we don't need this. Removed.

[tommi (sloooow) - chröme, 2013-09-10 16:00:38]

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

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:64:
webrtc::MediaConstraintsInterface::kValueFalse},
On 2013/09/10 12:43:15, xians1 wrote:
> On 2013/09/06 11:20:30, tommi wrote:
> > please cc bemasc.  It does seem though that if all these are set to false we
> > might as well not set them and remove the ApplyFixedWebAudioConstraints
> > function.
> 
> Removed the changed now, I want to completely remove the hard-coded
constrains,
> but lets do it in a separate CL.

sgtm

https://codereview.chromium.org/23691038/diff/17001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:809: if
(render_view_id < 0)
On 2013/09/10 12:43:15, xians1 wrote:
> On 2013/09/06 11:20:30, tommi wrote:
> > Can we now simply DCHECK that render_view_id is >= 0 and skip this if()?
> 
> I think the answer might depend on if we support gUM without a proper render
> view, like extension.
> I added a TODO here and changed to a DCHECK, let me know if you have another
> thought.

sgtm

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

https://codereview.chromium.org/23691038/diff/30001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:386: 
nit: one empty line is enough

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

https://codereview.chromium.org/23691038/diff/30001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:448: // TODO(leozwang): Tune and
adjust buffer size on Android.
can you assign this to henrika instead?

https://codereview.chromium.org/23691038/diff/30001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_source_provider.cc (right):

https://codereview.chromium.org/23691038/diff/30001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.cc:26:
use_sink_params_for_testing_(false) {
I don't like mixing test code and production code (we definitely should avoid
shipping test code).  Can't the test just pass the sink params via Initialize?

https://codereview.chromium.org/23691038/diff/30001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/23691038/diff/30001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:69:
media::AudioBus::Create(params.channels(),sink_buffer_size_);
space after ,

https://codereview.chromium.org/23691038/diff/30001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:177: bool
need_audio_processing = need_audio_processing_?
space before ?

https://codereview.chromium.org/23691038/diff/30001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:219: it != sinks.end(); ++it)
{}

[no longer working on chromium, 2013-09-11 10:22:07]

Tommi, please take another look.
Jochen, owner stamp for the gypi files.

Thanks,
SX

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

https://codereview.chromium.org/23691038/diff/30001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:386: 
On 2013/09/10 16:00:38, tommi wrote:
> nit: one empty line is enough

Done.

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

https://codereview.chromium.org/23691038/diff/30001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:448: // TODO(leozwang): Tune and
adjust buffer size on Android.
On 2013/09/10 16:00:38, tommi wrote:
> can you assign this to henrika instead?

Done.

https://codereview.chromium.org/23691038/diff/30001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_source_provider.cc (right):

https://codereview.chromium.org/23691038/diff/30001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.cc:26:
use_sink_params_for_testing_(false) {
On 2013/09/10 16:00:38, tommi wrote:
> I don't like mixing test code and production code (we definitely should avoid
> shipping test code).  Can't the test just pass the sink params via Initialize?

How about now?

https://codereview.chromium.org/23691038/diff/30001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/23691038/diff/30001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:69:
media::AudioBus::Create(params.channels(),sink_buffer_size_);
On 2013/09/10 16:00:38, tommi wrote:
> space after ,

Done.

https://codereview.chromium.org/23691038/diff/30001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:177: bool
need_audio_processing = need_audio_processing_?
On 2013/09/10 16:00:38, tommi wrote:
> space before ?

Done.

https://codereview.chromium.org/23691038/diff/30001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:219: it != sinks.end(); ++it)
On 2013/09/10 16:00:38, tommi wrote:
> {}

Done.

[jochen (gone - plz use gerrit), 2013-09-12 09:59:07]

gypi files lgtm

[tommi (sloooow) - chröme, 2013-09-12 11:02:15]

need to run to a meeting, here are a few comments, will continue later today.

https://chromiumcodereview.appspot.com/23691038/diff/47001/content/renderer/m...
File content/renderer/media/webrtc_local_audio_source_provider.cc (right):

https://chromiumcodereview.appspot.com/23691038/diff/47001/content/renderer/m...
content/renderer/media/webrtc_local_audio_source_provider.cc:38: if
(RenderThreadImpl::current()) {
what if this is NULL?  Won't the sink_params be invalid?  Seems to me that we
shouldn't be checking this or there is something seriously wrong. (we already
have the thread check above for being called on the correct thread).

https://chromiumcodereview.appspot.com/23691038/diff/47001/content/renderer/m...
content/renderer/media/webrtc_local_audio_source_provider.cc:103:
NOTIMPLEMENTED();
Let's talk about this.  It seems like the WebAudio implementation is using this
to tell webkit what sample rate to expect.  Can we do that?  If not, can you add
documentation here that explains why not?

https://chromiumcodereview.appspot.com/23691038/diff/47001/content/renderer/m...
File content/renderer/media/webrtc_local_audio_source_provider.h (right):

https://chromiumcodereview.appspot.com/23691038/diff/47001/content/renderer/m...
content/renderer/media/webrtc_local_audio_source_provider.h:47: // times if the
source format has changed.
Multiple times for the same source?  what would be examples of when that would
happen?

https://chromiumcodereview.appspot.com/23691038/diff/47001/content/renderer/m...
content/renderer/media/webrtc_local_audio_source_provider.h:75: void
SetSinkParamsForTesting(const media::AudioParameters& sink_params);
Can you add:
TODO(xians,tommi): Remove and instead offer a way to inject the sink parameters
so that the implementation doesn't rely on the global default hardware config
but instead gets the parameters directly from the sink (WebAudio in this case). 
Ideally the unit test should be able to use that same mechanism to inject the
sink parameters for testing.

[no longer working on chromium, 2013-09-12 19:14:55]

Hi Tommi, any look?

SX

https://codereview.chromium.org/23691038/diff/47001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_source_provider.cc (right):

https://codereview.chromium.org/23691038/diff/47001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.cc:38: if
(RenderThreadImpl::current()) {
On 2013/09/12 11:02:16, tommi wrote:
> what if this is NULL?  Won't the sink_params be invalid?  Seems to me that we
> shouldn't be checking this or there is something seriously wrong. (we already
> have the thread check above for being called on the correct thread).

We check this since it will be NULL for most of the unittests without mocking
the IPC message.

We are still exposing SetSinkParamsForTesting() method to allow the unittests to
inject their own sink params for testing.
If the unittests do not do it, it will crash when creating the audio_converter_.

https://codereview.chromium.org/23691038/diff/47001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.cc:103:
NOTIMPLEMENTED();
On 2013/09/12 11:02:16, tommi wrote:
> Let's talk about this.  It seems like the WebAudio implementation is using
this
> to tell webkit what sample rate to expect.  Can we do that?  If not, can you
add
> documentation here that explains why not?

Telling webkit about the sample rate will force moving the resampler to webkit,
we would like to do in Chrome instead.

https://codereview.chromium.org/23691038/diff/47001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_source_provider.h (right):

https://codereview.chromium.org/23691038/diff/47001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.h:47: // times if the
source format has changed.
On 2013/09/12 11:02:16, tommi wrote:
> Multiple times for the same source?  what would be examples of when that would
> happen?

For example, Initialize() can be called again when the capturer is setting to
peer connection mode, where the capturer will use 10ms as buffer size then the
source_param will be changed.

https://codereview.chromium.org/23691038/diff/47001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.h:75: void
SetSinkParamsForTesting(const media::AudioParameters& sink_params);
On 2013/09/12 11:02:16, tommi wrote:
> Can you add:
> TODO(xians,tommi): Remove and instead offer a way to inject the sink
parameters
> so that the implementation doesn't rely on the global default hardware config
> but instead gets the parameters directly from the sink (WebAudio in this
case). 
> Ideally the unit test should be able to use that same mechanism to inject the
> sink parameters for testing.

Done.
I would love to inject a new interface to webkit::audiosourceprovider to do
this. but let me do it in a separate CL.

[tommi (sloooow) - chröme, 2013-09-12 20:40:54]

lgtm with a few minor comments.  I'm fine with todos in places where addressing
it later is fine.

https://codereview.chromium.org/23691038/diff/53001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_source_provider.cc (right):

https://codereview.chromium.org/23691038/diff/53001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.cc:18: static const
size_t kMaxNumberOfBuffer = 10;
kMaxNumberOfBuffers (plural) or kMaxBufferCount

https://codereview.chromium.org/23691038/diff/53001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.cc:19: static const
size_t kWebAudioRenderBufferSize = 128;
Ideally this should be the same constant as used in media... I don't suppose
there's a way to share?  If not, please add a note saying where the constant is
defined that we use for webaudio.

https://codereview.chromium.org/23691038/diff/53001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.cc:49:
sink_params_.IsValid();
missing DCHECK()

https://codereview.chromium.org/23691038/diff/53001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_source_provider_unittest.cc
(right):

https://codereview.chromium.org/23691038/diff/53001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider_unittest.cc:19:
media::CHANNEL_LAYOUT_STEREO, 2, 0, 44100, 16, 128);
use the webaudio buffer size constant?

https://codereview.chromium.org/23691038/diff/53001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider_unittest.cc:57: // the
source provider FIFO into th e resampler.
the resampler

https://codereview.chromium.org/23691038/diff/53001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_track.h (right):

https://codereview.chromium.org/23691038/diff/53001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.h:25: }
}  // media

https://codereview.chromium.org/23691038/diff/53001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_track_unittest.cc (right):

https://codereview.chromium.org/23691038/diff/53001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track_unittest.cc:140: //   
WebRtcLocalAudioSourceProvider* source_provider =
remove this?

[no longer working on chromium, 2013-09-17 13:08:00]

Thanks Tommi for the review, I will commit the CL now.

https://codereview.chromium.org/23691038/diff/53001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_source_provider.cc (right):

https://codereview.chromium.org/23691038/diff/53001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.cc:18: static const
size_t kMaxNumberOfBuffer = 10;
On 2013/09/12 20:40:55, tommi wrote:
> kMaxNumberOfBuffers (plural) or kMaxBufferCount

Done.

https://codereview.chromium.org/23691038/diff/53001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.cc:19: static const
size_t kWebAudioRenderBufferSize = 128;
On 2013/09/12 20:40:55, tommi wrote:
> Ideally this should be the same constant as used in media... I don't suppose
> there's a way to share?  If not, please add a note saying where the constant
is
> defined that we use for webaudio.

Done.

https://codereview.chromium.org/23691038/diff/53001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider.cc:49:
sink_params_.IsValid();
On 2013/09/12 20:40:55, tommi wrote:
> missing DCHECK()

Done.

https://codereview.chromium.org/23691038/diff/53001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_source_provider_unittest.cc
(right):

https://codereview.chromium.org/23691038/diff/53001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_source_provider_unittest.cc:57: // the
source provider FIFO into th e resampler.
On 2013/09/12 20:40:55, tommi wrote:
> the resampler

Done.

https://codereview.chromium.org/23691038/diff/53001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_track.h (right):

https://codereview.chromium.org/23691038/diff/53001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.h:25: }
On 2013/09/12 20:40:55, tommi wrote:
> }  // media

Done.

https://codereview.chromium.org/23691038/diff/53001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_track_unittest.cc (right):

https://codereview.chromium.org/23691038/diff/53001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track_unittest.cc:140: //   
WebRtcLocalAudioSourceProvider* source_provider =
On 2013/09/12 20:40:55, tommi wrote:
> remove this?

Sorry, we need this, uncomment these lines.

[commit-bot: I haz the power, 2013-09-17 13:08:27]

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

[commit-bot: I haz the power, 2013-09-17 15:24:15]

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

[commit-bot: I haz the power, 2013-09-17 18:52:43]

Change committed as 223648