Cleaned up the WebRtcAudioCapturer a bit.

Cleaned up the WebRtcAudioCapturer a bit.

This CL cleaned up the capturer a bit by moving the initialize() method to private and do NOT allow re-initializing the capturer when a new getUserMedia is called.
Instead, it creates a new capturer for each getUserMedia.

BUG=333285
TEST=content_unittests

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

[no longer working on chromium, 2014-01-10 16:45:06]

Guys, this is a CL to do some clean-up on the existing code. This should not
change the behaviour but make the code more readable and cleaner.

Please review it.

Thanks,
SX

[perkj_chrome, 2014-01-12 21:53:05]

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

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:308: continue;
indentation

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:458: // TODO(xians):
Check with perkj on if this comment is valid.
Yes. CreateNativeAudioMediaStreamTrack is called whenever a new track is created
from blink. It can be from a gum call or from cloning a track. 
The the track happen to be a remote track- there is no source object implemented
yet.

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:941: if (capturer)
Can capturer be null?

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:942:
GetWebRtcAudioDevice()->AddAudioCapturer(capturer);
Rename method to SetAudioCapturer if there can be only one?

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/m...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/m...
content/renderer/media/media_stream_impl.cc:736: WebRtcAudioDeviceImpl*
audio_device =
Can this be moved to AudioCapturer::Stop instead? - ie - let the capturer know
what WebRtcAudioDeviceImpl it belongs to. It seems wrong that MediaStreamImpl
should have anything to do with this.

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/m...
content/renderer/media/media_stream_impl.cc:815: for
(LocalStreamSources::const_iterator it = local_sources_.begin();
This is kind of backwards. We moved this info to the audio device in order to be
able to refactor render creation to a separate file and don't depend on
MediaStreamImpl. 
So, please don't depend on anything in MediaStreamImpl.

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/m...
content/renderer/media/media_stream_impl.cc:821:
!extra_data->GetAudioCapturer()->device_id().empty()) {
GetAudioCapturer()->device_id().empty() ? 
Can this be expressed in a different way if it is needed at all?

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/m...
File content/renderer/media/mock_media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/m...
content/renderer/media/mock_media_stream_dependency_factory.cc:458:
scoped_refptr<WebRtcAudioCapturer> audio_capturer = capturer ?
This  seem strange. Why don't you know if you must create a capturer or not?

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/r...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/r...
content/renderer/media/rtc_peer_connection_handler.cc:560:
DCHECK_EQ(source.type(), blink::WebMediaStreamSource::TypeAudio);
no need for dcheck

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/r...
content/renderer/media/rtc_peer_connection_handler.cc:562:
extra_data->GetAudioCapturer()->EnablePeerConnectionMode();
Is EnablePeerConnectionMode() needed?

[no longer working on chromium, 2014-01-13 16:59:59]

PTAL, thanks.

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

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:308: continue;
On 2014/01/12 21:53:06, perkj wrote:
> indentation

Done.

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:458: // TODO(xians):
Check with perkj on if this comment is valid.
On 2014/01/12 21:53:06, perkj wrote:
> Yes. CreateNativeAudioMediaStreamTrack is called whenever a new track is
created
> from blink. It can be from a gum call or from cloning a track. 
> The the track happen to be a remote track- there is no source object
implemented
> yet.

Done with removing the TODO. Good to know, thanks.

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:941: if (capturer)
On 2014/01/12 21:53:06, perkj wrote:
> Can capturer be null? 

Yes, CreateCapturer will return a NULL if it fails to create and initialize the
capturer using the passing |render_view_id| and |device_info|.

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:942:
GetWebRtcAudioDevice()->AddAudioCapturer(capturer);
On 2014/01/12 21:53:06, perkj wrote:
> Rename method to SetAudioCapturer if there can be only one? 

A couple of days ago I landed a workaround to fix a crash when there are
multiple input streams. That means we can actually support multiple capturers,
it is not recommended, but it works. so I don't stop the previous capturers when
a new capturer is created.
AddAudioCapturer() is correct from this sense.

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/m...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/m...
content/renderer/media/media_stream_impl.cc:815: for
(LocalStreamSources::const_iterator it = local_sources_.begin();
On 2014/01/12 21:53:06, perkj wrote:
> This is kind of backwards. We moved this info to the audio device in order to
be
> able to refactor render creation to a separate file and don't depend on
> MediaStreamImpl. 
> So, please don't depend on anything in MediaStreamImpl. 

Done.

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/m...
content/renderer/media/media_stream_impl.cc:821:
!extra_data->GetAudioCapturer()->device_id().empty()) {
On 2014/01/12 21:53:06, perkj wrote:
> GetAudioCapturer()->device_id().empty() ? 
> Can this be expressed in a different way if it is needed at all? 

Remove now.

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/m...
File content/renderer/media/mock_media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/m...
content/renderer/media/mock_media_stream_dependency_factory.cc:458:
scoped_refptr<WebRtcAudioCapturer> audio_capturer = capturer ?
On 2014/01/12 21:53:06, perkj wrote:
> This  seem strange. Why don't you know if you must create a capturer or not? 

Previously the capturer is got from WebRtcAudioDeviceImpl, and we know for sure
that in unittests WebRtcAudioDeviceImpl does not exist.
But this CL changes the code that the capturer will be got from the existing
source. So the capturer might exist or might not, depending on how the unittests
are written.

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/r...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/r...
content/renderer/media/rtc_peer_connection_handler.cc:560:
DCHECK_EQ(source.type(), blink::WebMediaStreamSource::TypeAudio);
On 2014/01/12 21:53:06, perkj wrote:
> no need for dcheck

Done.

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/r...
content/renderer/media/rtc_peer_connection_handler.cc:562:
extra_data->GetAudioCapturer()->EnablePeerConnectionMode();
On 2014/01/12 21:53:06, perkj wrote:
> Is EnablePeerConnectionMode() needed?

Yes. the capturer might be setup for WebAudio, in such case, it might run with a
packet size smaller than 10ms chunk. When the peer connnection is hooked up, we
need to make sure the capturer is running at 10ms packet size to have the peer
connection working properly.

[perkj_chrome, 2014-01-14 08:42:40]

https://codereview.chromium.org/133903004/diff/110001/content/renderer/media/...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/133903004/diff/110001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:736: WebRtcAudioDeviceImpl*
audio_device =
What is the lifetime of audio_device? How about letting this be internal to
AudioCapturer::Stop.

https://codereview.chromium.org/133903004/diff/110001/content/renderer/media/...
File content/renderer/media/webrtc_audio_capturer.h (right):

https://codereview.chromium.org/133903004/diff/110001/content/renderer/media/...
content/renderer/media/webrtc_audio_capturer.h:49: WebRtcAudioDeviceImpl*
audio_device);
Here you already have the audio_device. So why can not the capturer unregister
itself once it Stops?

https://codereview.chromium.org/133903004/diff/110001/content/renderer/media/...
content/renderer/media/webrtc_audio_capturer.h:194: // Raw pointer to the
WebRtcAudioDeviceImpl.
Explain how this can be safe. ie - what is the lifetime of audio_device.

https://codereview.chromium.org/133903004/diff/110001/content/renderer/media/...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/133903004/diff/110001/content/renderer/media/...
content/renderer/media/webrtc_audio_device_impl.cc:461: // capturer, the first
one or the last one.
And how many can you currently have? You create one per call to GetUSerMEdia? In
that case I would say you should use the last one.

[Jói, 2014-01-14 09:16:36]

Sorry to be late to this party, I started reviewing this on Saturday but wanted
to read it more carefully, then forgot about it yesterday.

For now I just have one small comment below, but will wait to see the responses
to Per's last comments and then take another look.

Cheers,
Jói

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer.h (right):

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.h:42: // Used to construct the
audio capturer. |render_view_id| specifies the
document the -1 case

[no longer working on chromium, 2014-01-14 11:10:20]

PTAL, thanks.

SX

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer.h (right):

https://codereview.chromium.org/133903004/diff/30001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.h:42: // Used to construct the
audio capturer. |render_view_id| specifies the
On 2014/01/14 09:16:37, Jói wrote:
> document the -1 case

Done.

https://codereview.chromium.org/133903004/diff/110001/content/renderer/media/...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/133903004/diff/110001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:736: WebRtcAudioDeviceImpl*
audio_device =
On 2014/01/14 08:42:40, perkj wrote:
> What is the lifetime of audio_device? How about letting this be internal to
> AudioCapturer::Stop.

Sorry, I just forgot the save the change in this file, these lines of code
should have been removed.

FYI, the audio_device is scoped_ptr owned by the dependency_factory, and it will
go away when the dependency_factory goes away.

https://codereview.chromium.org/133903004/diff/110001/content/renderer/media/...
File content/renderer/media/webrtc_audio_capturer.h (right):

https://codereview.chromium.org/133903004/diff/110001/content/renderer/media/...
content/renderer/media/webrtc_audio_capturer.h:49: WebRtcAudioDeviceImpl*
audio_device);
On 2014/01/14 08:42:40, perkj wrote:
> Here you already have the audio_device. So why can not the capturer unregister
> itself once it Stops?

Yes, we are doing that.

https://codereview.chromium.org/133903004/diff/110001/content/renderer/media/...
content/renderer/media/webrtc_audio_capturer.h:194: // Raw pointer to the
WebRtcAudioDeviceImpl.
On 2014/01/14 08:42:40, perkj wrote:
> Explain how this can be safe. ie - what is the lifetime of audio_device.

Done.

https://codereview.chromium.org/133903004/diff/110001/content/renderer/media/...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/133903004/diff/110001/content/renderer/media/...
content/renderer/media/webrtc_audio_device_impl.cc:461: // capturer, the first
one or the last one.
On 2014/01/14 08:42:40, perkj wrote:
> And how many can you currently have? You create one per call to GetUSerMEdia?
In
> that case I would say you should use the last one.

OK, then lets switch over to the last one.

[Jói, 2014-01-14 12:29:25]

LGTM, please wait for Per's sign-off.

[perkj_chrome, 2014-01-14 12:53:54]

I think this looks good. But please fix the tests that chrash due to this
change.

Also - consider writing a test that do getusermedia twice and make sure that
audio works.

https://codereview.chromium.org/133903004/diff/280001/content/renderer/media/...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/133903004/diff/280001/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:466:
CreateLocalAudioTrack(base::UTF16ToUTF8(track.id()),
you can also do track.id().utf8() if you want.I am not sure what is the
preferred method to use.

https://codereview.chromium.org/133903004/diff/280001/content/renderer/media/...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/133903004/diff/280001/content/renderer/media/...
content/renderer/media/webrtc_audio_capturer.cc:376: if (audio_device_)
Can audio_device_ be NULL? Otherwise skip the check.

https://codereview.chromium.org/133903004/diff/280001/content/renderer/media/...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/133903004/diff/280001/content/renderer/media/...
content/renderer/media/webrtc_audio_device_impl.cc:460: for
(CapturerList::const_reverse_iterator iter = capturers_.rbegin();
return capturers_.empty() ? NULL : capturers_[capturers.size() -1];

[no longer working on chromium, 2014-01-14 14:10:56]

Per, PTAL, thanks.

SX

https://codereview.chromium.org/133903004/diff/280001/content/renderer/media/...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/133903004/diff/280001/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:466:
CreateLocalAudioTrack(base::UTF16ToUTF8(track.id()),
On 2014/01/14 12:53:55, perkj wrote:
> you can also do track.id().utf8() if you want.I am not sure what is the
> preferred method to use.

Good to know. id().utf8() looks cleaner to me.

https://codereview.chromium.org/133903004/diff/280001/content/renderer/media/...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/133903004/diff/280001/content/renderer/media/...
content/renderer/media/webrtc_audio_capturer.cc:376: if (audio_device_)
On 2014/01/14 12:53:55, perkj wrote:
> Can audio_device_ be NULL? Otherwise skip the check.

Yes, for example, for most of the unittests they are NULL.

https://codereview.chromium.org/133903004/diff/280001/content/renderer/media/...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/133903004/diff/280001/content/renderer/media/...
content/renderer/media/webrtc_audio_device_impl.cc:460: for
(CapturerList::const_reverse_iterator iter = capturers_.rbegin();
On 2014/01/14 12:53:55, perkj wrote:
> return capturers_.empty() ? NULL : capturers_[capturers.size() -1];  

Done.

https://codereview.chromium.org/133903004/diff/280001/content/renderer/media/...
content/renderer/media/webrtc_audio_device_impl.cc:460: for
(CapturerList::const_reverse_iterator iter = capturers_.rbegin();
On 2014/01/14 12:53:55, perkj wrote:
> return capturers_.empty() ? NULL : capturers_[capturers.size() -1];  

capturers_ is a list whose element can't be access via indexing.
Is it fine to keep this? otherwise I need to switch it to a vector.

[perkj_chrome, 2014-01-14 20:27:31]

lgtm

https://codereview.chromium.org/133903004/diff/280001/content/renderer/media/...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/133903004/diff/280001/content/renderer/media/...
content/renderer/media/webrtc_audio_device_impl.cc:460: for
(CapturerList::const_reverse_iterator iter = capturers_.rbegin();
On 2014/01/14 14:10:56, xians1 wrote:
> On 2014/01/14 12:53:55, perkj wrote:
> > return capturers_.empty() ? NULL : capturers_[capturers.size() -1];  
> 
> capturers_ is a list whose element can't be access via indexing.
> Is it fine to keep this? otherwise I need to switch it to a vector.

sure. You can also use *(capturers_.end()--) instead of an index.

https://codereview.chromium.org/133903004/diff/630001/content/renderer/media/...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/133903004/diff/630001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:560: if (extra_data &&
extra_data->GetAudioCapturer())
Humm. Can you add a note when extra data can be null?
I guess it can currently happen the track is a remote track.

[no longer working on chromium, 2014-01-15 18:31:03]

Thanks Per.

Patrik, could you please take a look at change in the browser test code and
getUserMedia.html?

Thanks,
SX

https://codereview.chromium.org/133903004/diff/280001/content/renderer/media/...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/133903004/diff/280001/content/renderer/media/...
content/renderer/media/webrtc_audio_device_impl.cc:460: for
(CapturerList::const_reverse_iterator iter = capturers_.rbegin();
On 2014/01/14 20:27:32, perkj wrote:
> On 2014/01/14 14:10:56, xians1 wrote:
> > On 2014/01/14 12:53:55, perkj wrote:
> > > return capturers_.empty() ? NULL : capturers_[capturers.size() -1];  
> > 
> > capturers_ is a list whose element can't be access via indexing.
> > Is it fine to keep this? otherwise I need to switch it to a vector.
> 
> sure. You can also use *(capturers_.end()--) instead of an index.

I would like to use the existing approach since it is a bit more readable. Hope
it is fine.

https://codereview.chromium.org/133903004/diff/630001/content/renderer/media/...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/133903004/diff/630001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:560: if (extra_data &&
extra_data->GetAudioCapturer())
On 2014/01/14 20:27:32, perkj wrote:
> Humm. Can you add a note when extra data can be null?
> I guess it can currently happen the track is a remote track.

Done.

[phoglund_chromium, 2014-01-16 08:22:06]

lgtm, one nit

https://codereview.chromium.org/133903004/diff/540002/content/test/data/media...
File content/test/data/media/getusermedia.html (right):

https://codereview.chromium.org/133903004/diff/540002/content/test/data/media...
content/test/data/media/getusermedia.html:68: // streams are detected to be
rolling, the test succeeds.
... we stop the local stream. Since both streams have the same origin, both
video streams should stop. If they do, the test succeeds.

[no longer working on chromium, 2014-01-16 13:08:55]

Thanks guys.

landing soon.

SX

https://codereview.chromium.org/133903004/diff/540002/content/test/data/media...
File content/test/data/media/getusermedia.html (right):

https://codereview.chromium.org/133903004/diff/540002/content/test/data/media...
content/test/data/media/getusermedia.html:68: // streams are detected to be
rolling, the test succeeds.
On 2014/01/16 08:22:07, phoglund wrote:
> ... we stop the local stream. Since both streams have the same origin, both
> video streams should stop. If they do, the test succeeds.

Fixed the comment, but actually it is because both streams have the same source
(not same origin).

[commit-bot: I haz the power, 2014-01-16 13:08:57]

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

[commit-bot: I haz the power, 2014-01-16 16:03:08]

Change committed as 245205