Update MediaStreamTrack::Stop to latest draft.

Update MediaStreamTrack::Stop to latest draft.
MediaStreamTrack::Stop now only stop the source if there are no other tracks using the same source.

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

[perkj_chrome, 2014-03-31 17:22:04]

Hej
Can you please review?

[no longer working on chromium, 2014-04-01 18:29:58]

Great work, thanks.

https://codereview.chromium.org/218763007/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_center.cc (right):

https://codereview.chromium.org/218763007/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_center.cc:131: return false;
Can this happen? Add an error log if it can. otherwise add a DCHECK.

BTW, if the users call Stop on the track multiple times, we always return a
true, right?

https://codereview.chromium.org/218763007/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_center.cc:168: // need to support both
MediaStream::Stop and  MediaStreamTrack::Stop.
nit, remove the extra space

https://codereview.chromium.org/218763007/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_impl_unittest.cc (right):

https://codereview.chromium.org/218763007/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_impl_unittest.cc:233:
TEST_F(MediaStreamImplTest, StopLocalTracks) {
does any of the test cases verify that the source will be stopped when the last
track is stopped ?
And the source won't be stopped if there is any active track?

https://codereview.chromium.org/218763007/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/218763007/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.cc:276: }
you also need to set source_ to NULL to prevent starting the source again.

https://codereview.chromium.org/218763007/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.cc:278: if (stop_source) {
nit, remove the {}

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

https://codereview.chromium.org/218763007/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.h:220: MediaStreamAudioSource*
audio_source_;
const

https://codereview.chromium.org/218763007/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_track_unittest.cc (right):

https://codereview.chromium.org/218763007/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track_unittest.cc:339:
capturer_->Stop();
I think you should skip this capturer_->Stop(); but try to verify that the
source has been stopped by the tracks' stop()

[perkj_chrome, 2014-04-02 13:35:49]

PTAL- 


I know there is an error in MediaStreamVideoSource unittests that I need to
fix....

https://codereview.chromium.org/218763007/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_center.cc (right):

https://codereview.chromium.org/218763007/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_center.cc:131: return false;
On 2014/04/01 18:29:58, xians1 wrote:
> Can this happen? Add an error log if it can. otherwise add a DCHECK.
> 
> BTW, if the users call Stop on the track multiple times, we always return a
> true, right?

Done.

Yes. In the latest spec Stop is a void method so blink can clean this up if
wanted.

https://codereview.chromium.org/218763007/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_center.cc:168: // need to support both
MediaStream::Stop and  MediaStreamTrack::Stop.
On 2014/04/01 18:29:58, xians1 wrote:
> nit, remove the extra space

Done.

https://codereview.chromium.org/218763007/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_impl_unittest.cc (right):

https://codereview.chromium.org/218763007/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_impl_unittest.cc:233:
TEST_F(MediaStreamImplTest, StopLocalTracks) {
On 2014/04/01 18:29:58, xians1 wrote:
> does any of the test cases verify that the source will be stopped when the
last
> track is stopped ?
> And the source won't be stopped if there is any active track?

These tests verify that the ms_dispatcher is notified that a source has been
stopped. It is the MediaStreamAudioSource and MediaStreamVideoSource that
trigger the callback when the last track has been stopped.

For testing with multiple tracks using the same source, see 
StopLocalTracksWhenTwoStreamUseSameDevices.

https://codereview.chromium.org/218763007/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/218763007/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.cc:276: }
On 2014/04/01 18:29:58, xians1 wrote:
> you also need to set source_ to NULL to prevent starting the source again.

Please see my comment. I don't think that should be done here.

https://codereview.chromium.org/218763007/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.cc:278: if (stop_source) {
On 2014/04/01 18:29:58, xians1 wrote:
> nit, remove the {}

I just added a long comment here.

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

https://codereview.chromium.org/218763007/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.h:220: MediaStreamAudioSource*
audio_source_;
On 2014/04/01 18:29:58, xians1 wrote:
> const

Done.

https://codereview.chromium.org/218763007/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_track_unittest.cc (right):

https://codereview.chromium.org/218763007/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track_unittest.cc:339:
capturer_->Stop();
On 2014/04/01 18:29:58, xians1 wrote:
> I think you should skip this capturer_->Stop(); but try to verify that the
> source has been stopped by the tracks' stop()


Do you have any plans to fix http://crbug.com/295418 and enable this?
This test fails long before you reach here.

I added StartTwoAudioTracks and modified StartOneAudioTrack to test this.

[no longer working on chromium, 2014-04-02 13:50:56]

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.cc:284:
audio_source_->StopSource();
you will trigger WebRtcAudioCapturerer::Stop() here, but if a new track is added
to the capturer after the Stop(), the new track will call Start() again. You can
look at AddTrack for details.
We talked about that before, and you said that the source should not be started
again after being stopped, is this still the case?

[tommi (sloooow) - chröme, 2014-04-03 08:53:16]

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

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_impl.cc:526:
DeleteUserMediaRequestInfo(request);
Previously we didn't delete the request on success. Was that a bug?

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_impl_unittest.cc (right):

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_impl_unittest.cc:66:
blink::WebUserMediaRequest* request_info,
fix indent

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_impl_unittest.cc:69: state_ = 
REQUEST_FAILED;
fix spacing

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_track.h (right):

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_track.h:38: // TODO(xians): Make pure
virtual when remote audio tracks has
nit: s/has/have

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_track.h:40: virtual void StopTrack();
Isn't it reduntant to have a MediaStreamTrack::StopTrack?  Just Stop()?

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_video_track_unittest.cc (right):

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_video_track_unittest.cc:74: const
blink::WebMediaStreamSource blink_source() const {
did you intend to return a const&?

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

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
content/renderer/media/mock_media_stream_dependency_factory.cc:533: return;
nit: remove this line

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

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.h:216: // blink guarantee that the
blink::WebMediaStreamSource outlives any
guarantees

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.cc:41: // Users might not call
StopTrack() on the track.
is this something we should rather enforce than work around?

[perkj_chrome, 2014-04-03 11:58:33]

PTAL

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

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_impl.cc:526:
DeleteUserMediaRequestInfo(request);
On 2014/04/03 08:53:16, tommi wrote:
> Previously we didn't delete the request on success. Was that a bug?

No- We kept it all the time since we needed to support MediaStream::Stop that
worked differently from MediaStreamTrack::Stop. Now I implement
MediaStream::Stop and simply looping through all tracks in the mediastream and
call MediaStreamTrack::Stop.

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_impl_unittest.cc (right):

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_impl_unittest.cc:66:
blink::WebUserMediaRequest* request_info,
On 2014/04/03 08:53:16, tommi wrote:
> fix indent

Done.

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_impl_unittest.cc:69: state_ = 
REQUEST_FAILED;
On 2014/04/03 08:53:16, tommi wrote:
> fix spacing

Done.

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_track.h (right):

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_track.h:38: // TODO(xians): Make pure
virtual when remote audio tracks has
On 2014/04/03 08:53:16, tommi wrote:
> nit: s/has/have

remote audio track has ...

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_track.h:40: virtual void StopTrack();
On 2014/04/03 08:53:16, tommi wrote:
> Isn't it reduntant to have a MediaStreamTrack::StopTrack?  Just Stop()?

Done.

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_video_track_unittest.cc (right):

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_video_track_unittest.cc:74: const
blink::WebMediaStreamSource blink_source() const {
On 2014/04/03 08:53:16, tommi wrote:
> did you intend to return a const&?

Done.

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

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
content/renderer/media/mock_media_stream_dependency_factory.cc:533: return;
On 2014/04/03 08:53:16, tommi wrote:
> nit: remove this line

Done.

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.cc:284:
audio_source_->StopSource();
On 2014/04/02 13:50:56, xians1 wrote:
> you will trigger WebRtcAudioCapturerer::Stop() here, but if a new track is
added
> to the capturer after the Stop(), the new track will call Start() again. You
can
> look at AddTrack for details.
> We talked about that before, and you said that the source should not be
started
> again after being stopped, is this still the case?

We discussed offline and decided to start the capturere during initialization.
It stopped either when the last track is removed or on destruction. It can never
be restarted.

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

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.h:216: // blink guarantee that the
blink::WebMediaStreamSource outlives any
On 2014/04/03 08:53:16, tommi wrote:
> guarantees

Done.

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.cc:41: // Users might not call
StopTrack() on the track.
On 2014/04/03 08:53:16, tommi wrote:
> is this something we should rather enforce than work around?

I would rather just remove the comment...
StopTrack (or Stop) is triggered from JS so, no I think this is what we want.

[no longer working on chromium, 2014-04-03 12:15:13]

lgtm on audio.

[perkj_chrome, 2014-04-07 08:15:37]

Tommi?

[tommi (sloooow) - chröme, 2014-04-07 10:05:43]

lgtm % nits

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_track.h (right):

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_track.h:38: // TODO(xians): Make pure
virtual when remote audio tracks has
On 2014/04/03 11:58:34, perkj wrote:
> On 2014/04/03 08:53:16, tommi wrote:
> > nit: s/has/have
> 
> remote audio track has ...

"tracks" is plural, so it should be "remote audio tracks have"... maybe it's the
perl substitute operator that you're asking about? :)

You could also rephrase this "Make this pure virtual when Stop[Track] has been
implemented for remote audio tracks."

https://codereview.chromium.org/218763007/diff/100001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/218763007/diff/100001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:30:
MediaStreamVideoSource::kMaxAspectRatio,
2 space indent

https://codereview.chromium.org/218763007/diff/100001/content/renderer/media/...
File content/renderer/media/media_stream_video_source_unittest.cc (right):

https://codereview.chromium.org/218763007/diff/100001/content/renderer/media/...
content/renderer/media/media_stream_video_source_unittest.cc:411:
"googCpuAdaptation"));
is this constant nowhere specified?
if no, can you add a TODO to use the constant when it's available?

https://codereview.chromium.org/218763007/diff/100001/content/renderer/media/...
File content/renderer/media/mock_media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/218763007/diff/100001/content/renderer/media/...
content/renderer/media/mock_media_stream_dependency_factory.cc:288: std::string
id,
const &

[perkj_chrome, 2014-04-07 15:33:51]

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_track.h (right):

https://codereview.chromium.org/218763007/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_track.h:38: // TODO(xians): Make pure
virtual when remote audio tracks has
On 2014/04/07 10:05:43, tommi wrote:
> On 2014/04/03 11:58:34, perkj wrote:
> > On 2014/04/03 08:53:16, tommi wrote:
> > > nit: s/has/have
> > 
> > remote audio track has ...
> 
> "tracks" is plural, so it should be "remote audio tracks have"... maybe it's
the
> perl substitute operator that you're asking about? :)
> 
> You could also rephrase this "Make this pure virtual when Stop[Track] has been
> implemented for remote audio tracks."

Done.

https://codereview.chromium.org/218763007/diff/100001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/218763007/diff/100001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:30:
MediaStreamVideoSource::kMaxAspectRatio,
On 2014/04/07 10:05:44, tommi wrote:
> 2 space indent

Done.

https://codereview.chromium.org/218763007/diff/100001/content/renderer/media/...
File content/renderer/media/media_stream_video_source_unittest.cc (right):

https://codereview.chromium.org/218763007/diff/100001/content/renderer/media/...
content/renderer/media/media_stream_video_source_unittest.cc:411:
"googCpuAdaptation"));
On 2014/04/07 10:05:44, tommi wrote:
> is this constant nowhere specified?
> if no, can you add a TODO to use the constant when it's available?

this is not related to this cl but I changed this to "something unsupported"

https://codereview.chromium.org/218763007/diff/100001/content/renderer/media/...
File content/renderer/media/mock_media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/218763007/diff/100001/content/renderer/media/...
content/renderer/media/mock_media_stream_dependency_factory.cc:288: std::string
id,
On 2014/04/07 10:05:44, tommi wrote:
> const &

Done.

[perkj_chrome, 2014-04-07 15:34:13]

The CQ bit was checked by perkj@chromium.org

[commit-bot: I haz the power, 2014-04-07 15:34:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/218763007/120001

[commit-bot: I haz the power, 2014-04-07 15:59:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-07 15:59:36]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[perkj_chrome, 2014-04-08 07:44:12]

phoglund, would you mind taking a look at the change in getusermedia.html?

alpha, can you help explain why TabCaptureApiTest.ApiTestsAudio fail on
invalidAudioConstraints ? I don't see how this test works and why this change
affect it?

[perkj_chrome, 2014-04-08 10:05:56]

And actually add phoglund and hclam.

phoglund, can you take a look at getusermedia.html.

Alpha, can you help explain why TabCaptureApiTest.ApiTestsAudio fail on
invalidAudioConstraints ? I don't see how this test works and why this change
affect it?

[phoglund_chromium, 2014-04-08 12:04:21]

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

https://codereview.chromium.org/218763007/diff/140001/content/test/data/media...
content/test/data/media/getusermedia.html:174:
stopBothVideoTracksAndVerify(stream),
This is wrong; you're supposed to pass in a callback as the second argument to
displayIntoVideoElement but you are making a function call which evaluates to
void. The only way this would be correct would be if stopBoth... was a higher
order function which returned a function itself, but that seems like fancy
stuff.

I think what you want is a closure like so:

function() {
  stopBoth..(stream);
}

https://codereview.chromium.org/218763007/diff/140001/content/test/data/media...
content/test/data/media/getusermedia.html:181:
waitForVideoToStop('local-view-2');
Hm. This is confusing, move the wait-for-video-to-stop to after the actual stop.
I don't think that can race.

https://codereview.chromium.org/218763007/diff/140001/content/test/data/media...
content/test/data/media/getusermedia.html:183: // Make sure the original track
is still playing and then stop it.
So both tracks are no longer stopped by stopping their common source?

[perkj_chrome, 2014-04-09 15:31:31]

Patrik, can you please take another look?

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

https://codereview.chromium.org/218763007/diff/140001/content/test/data/media...
content/test/data/media/getusermedia.html:174:
stopBothVideoTracksAndVerify(stream),
On 2014/04/08 12:04:21, phoglund wrote:
> This is wrong; you're supposed to pass in a callback as the second argument to
> displayIntoVideoElement but you are making a function call which evaluates to
> void. The only way this would be correct would be if stopBoth... was a higher
> order function which returned a function itself, but that seems like fancy
> stuff.
> 
> I think what you want is a closure like so:
> 
> function() {
>   stopBoth..(stream);
> }
Good catch

https://codereview.chromium.org/218763007/diff/140001/content/test/data/media...
content/test/data/media/getusermedia.html:181:
waitForVideoToStop('local-view-2');
On 2014/04/08 12:04:21, phoglund wrote:
> Hm. This is confusing, move the wait-for-video-to-stop to after the actual
stop.
> I don't think that can race.

Done.

https://codereview.chromium.org/218763007/diff/140001/content/test/data/media...
content/test/data/media/getusermedia.html:183: // Make sure the original track
is still playing and then stop it.
On 2014/04/08 12:04:21, phoglund wrote:
> So both tracks are no longer stopped by stopping their common source?
Right- stop is per track.

[phoglund_chromium, 2014-04-09 15:51:30]

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

https://codereview.chromium.org/218763007/diff/140001/content/test/data/media...
content/test/data/media/getusermedia.html:183: // Make sure the original track
is still playing and then stop it.
On 2014/04/09 15:31:32, perkj wrote:
> On 2014/04/08 12:04:21, phoglund wrote:
> > So both tracks are no longer stopped by stopping their common source?
> Right- stop is per track.

Ok, so you're stopping video track 0 on #183, then you ensure gLocalStream is
still playing, after which you stop track 0 on that. So the stream you are
getting into this function is != gLocalStream because it comes from the second 
getUserMedia?

This is very confusing, I wonder if we can make it clearer. For instance rename
stream -> streamPlayingInLocalView2 or something like that, and add a comment
that gLocalStream is playing in local view 1.

We should think a bit about getting rid of the gLocalView global if we can; it's
really hard to use in the context where we have several active streams at once.

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

https://codereview.chromium.org/218763007/diff/160001/content/test/data/media...
content/test/data/media/getusermedia.html:160: // streams are detected to be
rolling, we stop the local video tracks.
... One at a time.

[perkj_chrome, 2014-04-10 11:25:08]

Alpha, do you have time to help me with the failing TabAudio tests?

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

https://codereview.chromium.org/218763007/diff/140001/content/test/data/media...
content/test/data/media/getusermedia.html:183: // Make sure the original track
is still playing and then stop it.
On 2014/04/09 15:51:31, phoglund wrote:
> On 2014/04/09 15:31:32, perkj wrote:
> > On 2014/04/08 12:04:21, phoglund wrote:
> > > So both tracks are no longer stopped by stopping their common source?
> > Right- stop is per track.
> 
> Ok, so you're stopping video track 0 on #183, then you ensure gLocalStream is
> still playing, after which you stop track 0 on that. So the stream you are
> getting into this function is != gLocalStream because it comes from the second

> getUserMedia?
> 
> This is very confusing, I wonder if we can make it clearer. For instance
rename
> stream -> streamPlayingInLocalView2 or something like that, and add a comment
> that gLocalStream is playing in local view 1.
> 
> We should think a bit about getting rid of the gLocalView global if we can;
it's
> really hard to use in the context where we have several active streams at
once.

Done.

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

https://codereview.chromium.org/218763007/diff/160001/content/test/data/media...
content/test/data/media/getusermedia.html:160: // streams are detected to be
rolling, we stop the local video tracks.
On 2014/04/09 15:51:31, phoglund wrote:
> ... One at a time.

Done.

[Alpha Left Google, 2014-04-10 20:58:36]

I think the problem is that instead of calling stop on a MediaStream you want to
call it to the track:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e...

[phoglund_chromium, 2014-04-11 11:12:14]

lgtm

[perkj_chrome, 2014-04-15 09:11:31]

Thanks a lot Alpha for your pointers. It turned out that the tab capture test
provided value and pointed out an error in this cl. 

I have fixed that and added a unit test for it. Xians, please take a look at the
last patchset.

[no longer working on chromium, 2014-04-15 10:33:27]

lgtm with two nits.

https://codereview.chromium.org/218763007/diff/250001/content/renderer/media/...
File content/renderer/media/media_stream_audio_source.cc (right):

https://codereview.chromium.org/218763007/diff/250001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.cc:40:
if(!factory_->InitializeMediaStreamAudioSource(render_view_id_,
nit, add an empty space.

https://codereview.chromium.org/218763007/diff/250001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.cc:52: }
add, add an empty line.

[perkj_chrome, 2014-04-15 11:00:43]

done

https://codereview.chromium.org/218763007/diff/250001/content/renderer/media/...
File content/renderer/media/media_stream_audio_source.cc (right):

https://codereview.chromium.org/218763007/diff/250001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.cc:40:
if(!factory_->InitializeMediaStreamAudioSource(render_view_id_,
On 2014/04/15 10:33:28, xians1 wrote:
> nit, add an empty space.

Done.

https://codereview.chromium.org/218763007/diff/250001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.cc:52: }
On 2014/04/15 10:33:28, xians1 wrote:
> add, add an empty line.

Done.

[perkj_chrome, 2014-04-15 11:01:45]

The CQ bit was checked by perkj@chromium.org

[commit-bot: I haz the power, 2014-04-15 11:02:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/218763007/270001

[perkj_chrome, 2014-04-15 12:36:00]

The CQ bit was unchecked by perkj@chromium.org

[perkj_chrome, 2014-04-16 05:20:04]

The CQ bit was checked by perkj@chromium.org

[commit-bot: I haz the power, 2014-04-16 05:20:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/218763007/270001

[commit-bot: I haz the power, 2014-04-16 09:31:05]

Change committed as 264155