start/stop the source of the capturer when 1st audiotrack/last audiotrack is added/removed

This patch starts the source of the capturer when the first audiotrack is added to the capturer; and stops the source of the capturer when the last audiotrack is removed from the capturer.


BUG=242520
TEST=content_unittests --gtest_filter=="*WebRtcLocalAudioTrackTest*"
open chrome, go to https://apprtc.appspot.com/?debug=loopback
after the call setup, open javascript console, run:
navigator.webkitGetUserMedia({audio:true},function(s){s.stop();})
It should not stop the audio.

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

[henrika (OOO until Aug 14), 2013-06-05 09:09:20]

Looks good Shijing; a bit tricky to follow all the new details and I have a few
comments and questions.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/med...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:411: //
TODO(henrika,xians): Refactor how an audio track is created to harmonize
Is this TODO still vaild?

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:416: if
(GetWebRtcAudioDevice() && GetWebRtcAudioDevice()->capturer().get())
It is now a bit unclear why this if-statement is needed. It was clear in the old
revision but not now.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:417:
static_cast<WebRtcLocalAudioTrack*>(audio_track.get())->Start();
Can you add some comments here to explain what actually happens. I assume you
start the source (if not already started) which starts feeding the tracks.
Right?

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:692:
GetWebRtcAudioDevice() &&
Same comment as above. It is not clear why you must check for an existing ADM
here. Can't say by looking at the following code why this is needed.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:694:
static_cast<WebRtcLocalAudioTrack*>(
You call start when a track is added and stop when the source is stopped. What
happens when a track is removed then? Feels like there is an asymmetry here.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
File content/renderer/media/webaudio_capturer_source.cc (left):

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webaudio_capturer_source.cc:40: capturer_->Start();
Why was this line removed?

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_capturer.cc:200: // Start the source if a
real audio track is connected to the capturer.
Can you please extend the comment and give an example on what is not a "real"
audio track.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_capturer.cc:205: // Verify that |track| is
not already added to the list.
What happens if it is (in release mode)?

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_capturer.cc:246: if (tracks_.size() == 1 &&
default_sink_ &&
You must really add clear comments here explaining exactly what has to be
fulfilled for Stop to be called. Can't tracks_.size() be 0 if only one track had
been added and you have called erase above? OK, covered in else.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
File content/renderer/media/webrtc_audio_capturer.h (right):

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_capturer.h:55: // Called by the
WebRtcAudioDeviceImpl to add the ADM as the default sink to
Please add some comments about the effects are by calling this method. Are unit
tests modifications needed?

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_capturer.h:140: WebRtcAudioCapturerSink*
default_sink_;
Add comment.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_impl.cc:229: // TODO(xians): Register
the ADM as the sink of the audio track if WebRTC
Any existing crbug that you could add this this TODO?

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_impl.cc:231:
capturer_->SetDefaultSink(this);
What happens with the capturer if only non-default sinks are added? Is default
really a good name BTW?

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_unittest.cc:587: // Adds a local
audio track to trigger starting the capturer.
Could you make this comment a bit more clear. "..to trigger starting..." is a
bit vague. Do you mean, "create local audio track and connect it to the source
and then start the source by...."

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_unittest.cc:592:
local_audio_track->Start();
No EXPECT?

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_unittest.cc:755: // Adds a local
audio track to trigger starting the capturer.
dito

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_unittest.cc:830: // Adds a local
audio track to trigger starting the capturer.
same here

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_local_audio_track.cc:38: DCHECK(!capturer_.get());
What ensures that RemoveSink i called now?

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
File content/renderer/media/webrtc_local_audio_track.h (right):

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_local_audio_track.h:47: // Start the local audio
track. Called on the main render thread and
nit, Starts

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_local_audio_track.h:52: // should be called only
once when audio track going away.
Who is responsible for ensuring that it is only called once? What happens if it
is called more?

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
File content/renderer/media/webrtc_local_audio_track_unittest.cc (right):

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_local_audio_track_unittest.cc:159: track->Stop();
What happens if you call Stop before RemoveSink?

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_local_audio_track_unittest.cc:204:
EXPECT_CALL(*capturer_source_.get(), Start()).WillOnce(Return());
Who is default here and what is the difference when no default is used?

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_local_audio_track_unittest.cc:276:
track_1->Start();
The sequence is track calls AddSink on default sink and it therefore calls
Start, right?

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_local_audio_track_unittest.cc:281:
EXPECT_CALL(*capturer_source_.get(), Start()).Times(0);
Should you also verify that default start is not called?

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_local_audio_track_unittest.cc:288:
track_1->Stop();
Any difference if you stop #2 first?

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_local_audio_track_unittest.cc:299:
capturer_->SetDefaultSink(NULL);
Is this required?

[no longer working on chromium, 2013-06-05 16:29:45]

Henrik, I think I have already addressed your comments, PTAL.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/med...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:411: //
TODO(henrika,xians): Refactor how an audio track is created to harmonize
On 2013/06/05 09:09:20, henrika wrote:
> Is this TODO still vaild?

I think we can remove this TODO here since it does not tell much.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:416: if
(GetWebRtcAudioDevice() && GetWebRtcAudioDevice()->capturer().get())
On 2013/06/05 09:09:20, henrika wrote:
> It is now a bit unclear why this if-statement is needed. It was clear in the
old
> revision but not now.

Good catch, it is because we were using MockLocalAudioTrack on our unittest, and
it will crash when we cast it to WebRtcLocalAudioTrack.

I changed the unittest to create real WebRtcLocalAudioTrack instead, now the
checks are removed.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:417:
static_cast<WebRtcLocalAudioTrack*>(audio_track.get())->Start();
On 2013/06/05 09:09:20, henrika wrote:
> Can you add some comments here to explain what actually happens. I assume you
> start the source (if not already started) which starts feeding the tracks.
> Right?

Done.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:692:
GetWebRtcAudioDevice() &&
On 2013/06/05 09:09:20, henrika wrote:
> Same comment as above. It is not clear why you must check for an existing ADM
> here. Can't say by looking at the following code why this is needed.

Removed.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:694:
static_cast<WebRtcLocalAudioTrack*>(
On 2013/06/05 09:09:20, henrika wrote:
> You call start when a track is added and stop when the source is stopped. What
> happens when a track is removed then? Feels like there is an asymmetry here.

StopLocalAudioSource is the wrong name, it should be renamed as
StopLocalAudioTrack, I will create an issue to Tommy.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
File content/renderer/media/webaudio_capturer_source.cc (left):

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webaudio_capturer_source.cc:40: capturer_->Start();
On 2013/06/05 09:09:20, henrika wrote:
> Why was this line removed?

capturer_ will be automatically started when the first audio track is connected.
No need to explicitly start it anymore

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_capturer.cc:200: // Start the source if a
real audio track is connected to the capturer.
On 2013/06/05 09:09:20, henrika wrote:
> Can you please extend the comment and give an example on what is not a "real"
> audio track.

Done.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_capturer.cc:205: // Verify that |track| is
not already added to the list.
On 2013/06/05 09:09:20, henrika wrote:
> What happens if it is (in release mode)?

It is a push_back, then we will have two reference to the same audio track in
the list.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_capturer.cc:246: if (tracks_.size() == 1 &&
default_sink_ &&
On 2013/06/05 09:09:20, henrika wrote:
> You must really add clear comments here explaining exactly what has to be
> fulfilled for Stop to be called. Can't tracks_.size() be 0 if only one track
had
> been added and you have called erase above? OK, covered in else.

Done.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
File content/renderer/media/webrtc_audio_capturer.h (right):

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_capturer.h:55: // Called by the
WebRtcAudioDeviceImpl to add the ADM as the default sink to
On 2013/06/05 09:09:20, henrika wrote:
> Please add some comments about the effects are by calling this method. Are
unit
> tests modifications needed?

Not the existing unittests, but I have already added a test case in
webrtc_local_audio_track_unittest.cc to address this new API.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_capturer.h:140: WebRtcAudioCapturerSink*
default_sink_;
On 2013/06/05 09:09:20, henrika wrote:
> Add comment.

Done.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_impl.cc:229: // TODO(xians): Register
the ADM as the sink of the audio track if WebRTC
On 2013/06/05 09:09:20, henrika wrote:
> Any existing crbug that you could add this this TODO?

Done

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_impl.cc:231:
capturer_->SetDefaultSink(this);
On 2013/06/05 09:09:20, henrika wrote:
> What happens with the capturer if only non-default sinks are added? Is default
> really a good name BTW?

I am open to another more meaningful name, any suggestion on the naming?

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_unittest.cc:587: // Adds a local
audio track to trigger starting the capturer.
On 2013/06/05 09:09:20, henrika wrote:
> Could you make this comment a bit more clear. "..to trigger starting..." is a
> bit vague. Do you mean, "create local audio track and connect it to the source
> and then start the source by...." 

Done.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_unittest.cc:592:
local_audio_track->Start();
On 2013/06/05 09:09:20, henrika wrote:
> No EXPECT?

It is void

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_unittest.cc:755: // Adds a local
audio track to trigger starting the capturer.
On 2013/06/05 09:09:20, henrika wrote:
> dito

Done.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_unittest.cc:830: // Adds a local
audio track to trigger starting the capturer.
On 2013/06/05 09:09:20, henrika wrote:
> same here

Done.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_local_audio_track.cc:38: DCHECK(!capturer_.get());
On 2013/06/05 09:09:20, henrika wrote:
> What ensures that RemoveSink i called now?

Right, I also realized it this morning, we should handle it instead of using a
DCHECK here.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
File content/renderer/media/webrtc_local_audio_track.h (right):

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_local_audio_track.h:47: // Start the local audio
track. Called on the main render thread and
On 2013/06/05 09:09:20, henrika wrote:
> nit, Starts

Done.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_local_audio_track.h:52: // should be called only
once when audio track going away.
On 2013/06/05 09:09:20, henrika wrote:
> Who is responsible for ensuring that it is only called once? What happens if
it
> is called more?

Good question, in the real life, Stop() will be called when JS calls stop() on
the audio track, and I assume the audio track will be invalid after that, so
they can't be called once.

Anyhow, I agree that it should be allowed to be called multiple times, and only
the first one has effect.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
File content/renderer/media/webrtc_local_audio_track_unittest.cc (right):

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_local_audio_track_unittest.cc:159: track->Stop();
On 2013/06/05 09:09:20, henrika wrote:
> What happens if you call Stop before RemoveSink?

Nothing will happen.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_local_audio_track_unittest.cc:204:
EXPECT_CALL(*capturer_source_.get(), Start()).WillOnce(Return());
On 2013/06/05 09:09:20, henrika wrote:
> Who is default here and what is the difference when no default is used?

It does not have the default_sink here.

The default_sink is used by the capturer to send data to WebRtcAudioDeviceImpl,
so it does not matter for this unittest.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_local_audio_track_unittest.cc:276:
track_1->Start();
On 2013/06/05 09:09:20, henrika wrote:
> The sequence is track calls AddSink on default sink and it therefore calls
> Start, right?

It does not matter on the order, but mostly it is
WebRtcAudioDeviceImpl adds itself as default sink to the capturer,
track->Start(), then track->AddSink

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_local_audio_track_unittest.cc:281:
EXPECT_CALL(*capturer_source_.get(), Start()).Times(0);
On 2013/06/05 09:09:20, henrika wrote:
> Should you also verify that default start is not called?

It is done here:
Note that it is Times(0);

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_local_audio_track_unittest.cc:288:
track_1->Stop();
On 2013/06/05 09:09:20, henrika wrote:
> Any difference if you stop #2 first?

No difference.

https://codereview.chromium.org/15979027/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_local_audio_track_unittest.cc:299:
capturer_->SetDefaultSink(NULL);
On 2013/06/05 09:09:20, henrika wrote:
> Is this required?

Yes, ADM does it this way.

[no longer working on chromium, 2013-06-08 19:57:14]

Ping. Henrik, I know you are quite busy, but there is a new bug popping up for
howord, this CL seems to be helpful.

Do you have time to take another look?

SX

[henrika (OOO until Aug 14), 2013-06-11 07:48:55]

LGTM. Please ensure that it works on all bots as well...

[commit-bot: I haz the power, 2013-06-11 14:38:36]

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

[commit-bot: I haz the power, 2013-06-11 18:34:17]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-06-11 18:36:00]

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

[commit-bot: I haz the power, 2013-06-11 18:36:17]

Change committed as 205595