Hookup the MediaStream glue for Adding and Removing tracks to an existing MediaStream.

Hookup the MediaStream glue for Adding and Removing tracks to an existing MediaStream.

https://code.google.com/p/webrtc/issues/detail?id=382

This cl refactor the way Chrome create the native repressentation for MediaStreams by splitting up creating MediaStreams and tracks in two functions, of for MediaStreams and one for Tracks (AddNativeLocalMediaTrack).

AddNativeLocalMediaTrack is hooked up to MediaStreamCenter::didAddMediaStreamTrack to allow adding tracks to existing MediaStreams.

Two content_browsertests are added for testing adding and removing tracks from MediaStreams.

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

[perkj_chrome, 2013-04-09 12:58:36]

henrika, tommi can you please review the Chrome changes? 

phoglund, can you please review the tests?

[phoglund_chromium, 2013-04-09 13:16:28]

https://codereview.chromium.org/13496009/diff/2001/content/browser/media/webr...
File content/browser/media/webrtc_browsertest.cc (right):

https://codereview.chromium.org/13496009/diff/2001/content/browser/media/webr...
content/browser/media/webrtc_browsertest.cc:57: 
The test no longer stops anything, so this name is incorrect. Perhaps the file
should be called basic_getusermedia.html and this test to
getUserMediaAndVerifyVideoIsPlaying or something? Also consider re-adding the
stop() test in some form.

https://codereview.chromium.org/13496009/diff/2001/content/test/data/media/ge...
File content/test/data/media/getusermedia_and_stop.html (right):

https://codereview.chromium.org/13496009/diff/2001/content/test/data/media/ge...
content/test/data/media/getusermedia_and_stop.html:22: // be rendered.
Maybe getUserMediaAndClone is a better name here.

https://codereview.chromium.org/13496009/diff/2001/content/test/data/media/ge...
content/test/data/media/getusermedia_and_stop.html:35: 
Rename to displayAndWaitForVideo?

https://codereview.chromium.org/13496009/diff/2001/content/test/data/media/ge...
content/test/data/media/getusermedia_and_stop.html:36: function
okCallback(stream) {
We are no longer testing the stop() method. Can you make sure it gets tested
somewhere?

https://codereview.chromium.org/13496009/diff/2001/content/test/data/media/ge...
content/test/data/media/getusermedia_and_stop.html:38:
waitForVideo('local-view');
So this wait function happens to be asynchronous, but I think the reader will
feel better if it's at the bottom of this function. Otherwise it will look like
we wait first and hook everything up later.

https://codereview.chromium.org/13496009/diff/2001/content/test/data/media/ge...
content/test/data/media/getusermedia_and_stop.html:58: 
You should be able to extract all code 59-116 to a .js file which the two .html
files share. Just move the code and the corresponding global variables and
include before this script tag.

[tommi (sloooow) - chröme, 2013-04-09 13:28:49]

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

https://codereview.chromium.org/13496009/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_center.cc:104: if (!rtc_factory_) {
no {}

https://codereview.chromium.org/13496009/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_center.cc:116: if (type ==
WebKit::WebMediaStreamSource::TypeAudio) {
This looks a bit hard to read and spot the three return statements.  What about
doing this instead (I'm assuming that since you have a NOTREACHED(), the type
will always be either audio or video):

webrtc::MediaStreamInterface* native_stream =  GetNativeMediaStream(stream);
WebKit::WebMediaStreamSource::Type type = track.source().type();
DCHECK(type == WebKit::WebMediaStreamSource::TypeAudio ||
       type == WebKit::WebMediaStreamSource::TypeVideo);

return type == WebKit::WebMediaStreamSource::TypeAudio ?
    native_stream->RemoveTrack(native_stream->FindAudioTrack(track_id)) :
    native_stream->RemoveTrack(native_stream->FindVideoTrack(track_id))

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

https://codereview.chromium.org/13496009/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:350: NOTREACHED();
if this is something that will never happen, this if() clause is not needed. 
Instead you can just DCHECK that the native_stream pointer is valid.

https://codereview.chromium.org/13496009/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:365: // We're adding a
WebAudio MediaStream.
Is that the only possibility? Is there a way to verify that somehow?

https://codereview.chromium.org/13496009/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:379: if (source.type()
== WebKit::WebMediaStreamSource::TypeAudio) {
nit: if type() can only ever be audio or video, then I'd prefer a dcheck instead
of if/else and three return statements.

[henrika (OOO until Aug 14), 2013-04-09 16:53:43]

Nice work Per, like the new structure.

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

https://codereview.chromium.org/13496009/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_center.cc:101: bool
MediaStreamCenter::didAddMediaStreamTrack(
Why do we return bool here but void for didCreateMediaStream?
What happens in the false case?

https://codereview.chromium.org/13496009/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_center.cc:107: return
rtc_factory_->AddNativeLocalMediaTrack(stream, track);
Why is the rtc_factory used for didAdd but bot for didRemove? I just feels like
an odd design to me (feels asymmetric that's all).

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

https://codereview.chromium.org/13496009/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:321: for (size_t i =
0; i < audio_tracks.size(); ++i) {
Nice! Much better structure even if I find it a bit tricky to figure out what 

WebKit::WebVector<WebKit::WebMediaStreamTrack> audio_tracks;
 description->audioSources(audio_tracks);
description->audioTracks(audio_tracks);

actually does.

https://codereview.chromium.org/13496009/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:362: NOTIMPLEMENTED();
What does the combo of NOTIMPLEMENTED() + return false do actually? What happens
in release and debug mode?

https://codereview.chromium.org/13496009/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:369: // Create a
special source where default WebAudio constraints are used.
Nit, the TODO is not really connected to the audio constraint part. Perhaps
separate these comments?

https://codereview.chromium.org/13496009/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:384: if
(GetWebRtcAudioDevice()) {
Could you add a TODO(henrika,xians) here saying that we should refactor this
part and harmonize better with video (se below)?

[perkj_chrome, 2013-04-10 19:58:00]

Hi Henrik, Tommi, can you please take another look. I have updated the code and
added a unit test. But I have not yet addressed Patriks comments in
content_browsertests.

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

https://codereview.chromium.org/13496009/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_center.cc:101: bool
MediaStreamCenter::didAddMediaStreamTrack(
On 2013/04/09 16:53:43, henrika wrote:
> Why do we return bool here but void for didCreateMediaStream?
> What happens in the false case?

Good question- this is WebKit magic. Normally an exception is created in JS if
we return false. void functions can't throw an exception.

https://codereview.chromium.org/13496009/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_center.cc:104: if (!rtc_factory_) {
On 2013/04/09 13:28:49, tommi wrote:
> no {}

Done.

https://codereview.chromium.org/13496009/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_center.cc:107: return
rtc_factory_->AddNativeLocalMediaTrack(stream, track);
On 2013/04/09 16:53:43, henrika wrote:
> Why is the rtc_factory used for didAdd but bot for didRemove? I just feels
like
> an odd design to me (feels asymmetric that's all).
make sence. Moved Removed.

https://codereview.chromium.org/13496009/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_center.cc:116: if (type ==
WebKit::WebMediaStreamSource::TypeAudio) {
On 2013/04/09 13:28:49, tommi wrote:
> This looks a bit hard to read and spot the three return statements.  What
about
> doing this instead (I'm assuming that since you have a NOTREACHED(), the type
> will always be either audio or video):
> 
> webrtc::MediaStreamInterface* native_stream =  GetNativeMediaStream(stream);
> WebKit::WebMediaStreamSource::Type type = track.source().type();
> DCHECK(type == WebKit::WebMediaStreamSource::TypeAudio ||
>        type == WebKit::WebMediaStreamSource::TypeVideo);
> 
> return type == WebKit::WebMediaStreamSource::TypeAudio ?
>     native_stream->RemoveTrack(native_stream->FindAudioTrack(track_id)) :
>     native_stream->RemoveTrack(native_stream->FindVideoTrack(track_id))

Done.

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

https://codereview.chromium.org/13496009/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:321: for (size_t i =
0; i < audio_tracks.size(); ++i) {
On 2013/04/09 16:53:43, henrika wrote:
> Nice! Much better structure even if I find it a bit tricky to figure out what 
> 
> WebKit::WebVector<WebKit::WebMediaStreamTrack> audio_tracks;
>  description->audioSources(audio_tracks);
> description->audioTracks(audio_tracks);
> 
> actually does.
..
audioSources(audio_tracks) has changed name to audioTracks(audio_tracks);

https://codereview.chromium.org/13496009/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:350: NOTREACHED();
On 2013/04/09 13:28:49, tommi wrote:
> if this is something that will never happen, this if() clause is not needed. 
> Instead you can just DCHECK that the native_stream pointer is valid.

Done.

https://codereview.chromium.org/13496009/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:362: NOTIMPLEMENTED();
On 2013/04/09 16:53:43, henrika wrote:
> What does the combo of NOTIMPLEMENTED() + return false do actually? What
happens
> in release and debug mode?

NOTIMPLEMENTED prints a warning in the log output.

https://codereview.chromium.org/13496009/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:365: // We're adding a
WebAudio MediaStream.
On 2013/04/09 13:28:49, tommi wrote:
> Is that the only possibility? Is there a way to verify that somehow?
source.requiresAudioConsumer() means WebAudio in Blinkish language. Switched if
case to make that more clear.

https://codereview.chromium.org/13496009/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:369: // Create a
special source where default WebAudio constraints are used.
On 2013/04/09 16:53:43, henrika wrote:
> Nit, the TODO is not really connected to the audio constraint part. Perhaps
> separate these comments?

Done.

https://codereview.chromium.org/13496009/diff/2001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:379: if (source.type()
== WebKit::WebMediaStreamSource::TypeAudio) {
On 2013/04/09 13:28:49, tommi wrote:
> nit: if type() can only ever be audio or video, then I'd prefer a dcheck
instead
> of if/else and three return statements.

Done.

[tommi (sloooow) - chröme, 2013-04-11 03:26:05]

lgtm (didn't review the js code)

https://codereview.chromium.org/13496009/diff/12001/content/renderer/media/me...
File content/renderer/media/media_stream_center.cc (right):

https://codereview.chromium.org/13496009/diff/12001/content/renderer/media/me...
content/renderer/media/media_stream_center.cc:114: return false;
fix indent

https://codereview.chromium.org/13496009/diff/12001/content/renderer/media/me...
File content/renderer/media/media_stream_center.h (right):

https://codereview.chromium.org/13496009/diff/12001/content/renderer/media/me...
content/renderer/media/media_stream_center.h:51: const
WebKit::WebMediaStreamTrack& track) OVERRIDE;;
semicolon

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

https://codereview.chromium.org/13496009/diff/12001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:307: DVLOG(1) <<
"EnsurePeerConnectionFactory() failed!";
can you fix the indentation?

https://codereview.chromium.org/13496009/diff/12001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:321: for (size_t i =
0; i < audio_tracks.size(); ++i) {
nit: can remove {} now

[henrika (OOO until Aug 14), 2013-04-11 04:18:00]

LGTM

[perkj_chrome, 2013-04-11 10:12:41]

Patrik- your turn please.

Thanks

https://codereview.chromium.org/13496009/diff/2001/content/browser/media/webr...
File content/browser/media/webrtc_browsertest.cc (right):

https://codereview.chromium.org/13496009/diff/2001/content/browser/media/webr...
content/browser/media/webrtc_browsertest.cc:57: 
On 2013/04/09 13:16:28, phoglund wrote:
> The test no longer stops anything, so this name is incorrect. Perhaps the file
> should be called basic_getusermedia.html and this test to
> getUserMediaAndVerifyVideoIsPlaying or something? Also consider re-adding the
> stop() test in some form.

This is testing stop in the same way as before.

https://codereview.chromium.org/13496009/diff/2001/content/test/data/media/ge...
File content/test/data/media/getusermedia_and_stop.html (right):

https://codereview.chromium.org/13496009/diff/2001/content/test/data/media/ge...
content/test/data/media/getusermedia_and_stop.html:22: // be rendered.
On 2013/04/09 13:16:28, phoglund wrote:
> Maybe getUserMediaAndClone is a better name here.

Done.

https://codereview.chromium.org/13496009/diff/2001/content/test/data/media/ge...
content/test/data/media/getusermedia_and_stop.html:35: 
On 2013/04/09 13:16:28, phoglund wrote:
> Rename to displayAndWaitForVideo?

Done.

https://codereview.chromium.org/13496009/diff/2001/content/test/data/media/ge...
content/test/data/media/getusermedia_and_stop.html:36: function
okCallback(stream) {
On 2013/04/09 13:16:28, phoglund wrote:
> We are no longer testing the stop() method. Can you make sure it gets tested
> somewhere?
Humm- I did not really want to change the behavior of your tests and added the
stop to the when everything is good. Please let me know if you like this-
otherwise I can revert and skip local preview test here.

https://codereview.chromium.org/13496009/diff/2001/content/test/data/media/ge...
content/test/data/media/getusermedia_and_stop.html:38:
waitForVideo('local-view');
On 2013/04/09 13:16:28, phoglund wrote:
> So this wait function happens to be asynchronous, but I think the reader will
> feel better if it's at the bottom of this function. Otherwise it will look
like
> we wait first and hook everything up later.

Done.

https://codereview.chromium.org/13496009/diff/2001/content/test/data/media/ge...
content/test/data/media/getusermedia_and_stop.html:58: 
On 2013/04/09 13:16:28, phoglund wrote:
> You should be able to extract all code 59-116 to a .js file which the two
.html
> files share. Just move the code and the corresponding global variables and
> include before this script tag.

Done.

https://codereview.chromium.org/13496009/diff/12001/content/renderer/media/me...
File content/renderer/media/media_stream_center.cc (right):

https://codereview.chromium.org/13496009/diff/12001/content/renderer/media/me...
content/renderer/media/media_stream_center.cc:114: return false;
On 2013/04/11 03:26:05, tommi wrote:
> fix indent

Done.

https://codereview.chromium.org/13496009/diff/12001/content/renderer/media/me...
File content/renderer/media/media_stream_center.h (right):

https://codereview.chromium.org/13496009/diff/12001/content/renderer/media/me...
content/renderer/media/media_stream_center.h:51: const
WebKit::WebMediaStreamTrack& track) OVERRIDE;;
On 2013/04/11 03:26:05, tommi wrote:
> semicolon

Done.

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

https://codereview.chromium.org/13496009/diff/12001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:321: for (size_t i =
0; i < audio_tracks.size(); ++i) {
On 2013/04/11 03:26:05, tommi wrote:
> nit: can remove {} now

Prefer not to.

[phoglund_chromium, 2013-04-11 11:16:47]

Looks good in general, just some small comments.

https://codereview.chromium.org/13496009/diff/35001/content/browser/media/web...
File content/browser/media/webrtc_browsertest.cc (right):

https://codereview.chromium.org/13496009/diff/35001/content/browser/media/web...
content/browser/media/webrtc_browsertest.cc:159: // This test will make a
PeerConnection-based call and send a new Video
This sentence is hard to understand. Created based on = cloned from?

https://codereview.chromium.org/13496009/diff/35001/content/test/data/media/g...
File content/test/data/media/getusermedia_and_stop.html (right):

https://codereview.chromium.org/13496009/diff/35001/content/test/data/media/g...
content/test/data/media/getusermedia_and_stop.html:1: <html>
Rename this file to something like basic_getusermedia since it does more than
getusermedia and stop now.

https://codereview.chromium.org/13496009/diff/35001/content/test/data/media/g...
content/test/data/media/getusermedia_and_stop.html:10: 
I'd prefer if this is done through some accessor. See comment in
webrtc_test_utilities.js.

https://codereview.chromium.org/13496009/diff/35001/content/test/data/media/p...
File content/test/data/media/peerconnection-call.html (right):

https://codereview.chromium.org/13496009/diff/35001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:214: // |localStream| is added.
Also removes the audio track.

https://codereview.chromium.org/13496009/diff/35001/content/test/data/media/w...
File content/test/data/media/webrtc_test_utilities.js (right):

https://codereview.chromium.org/13496009/diff/35001/content/test/data/media/w...
content/test/data/media/webrtc_test_utilities.js:11: 
Write a small accessor to overwrite this guy which is then called by the two
other files. I think we should avoid overwriting globals in other files
directly.

[perkj_chrome, 2013-04-11 11:50:48]

PTAL

https://codereview.chromium.org/13496009/diff/35001/content/test/data/media/g...
File content/test/data/media/getusermedia_and_stop.html (right):

https://codereview.chromium.org/13496009/diff/35001/content/test/data/media/g...
content/test/data/media/getusermedia_and_stop.html:1: <html>
On 2013/04/11 11:16:48, phoglund wrote:
> Rename this file to something like basic_getusermedia since it does more than
> getusermedia and stop now.

Done.

https://codereview.chromium.org/13496009/diff/35001/content/test/data/media/g...
content/test/data/media/getusermedia_and_stop.html:10: 
On 2013/04/11 11:16:48, phoglund wrote:
> I'd prefer if this is done through some accessor. See comment in
> webrtc_test_utilities.js.

Done.

https://codereview.chromium.org/13496009/diff/35001/content/test/data/media/w...
File content/test/data/media/webrtc_test_utilities.js (right):

https://codereview.chromium.org/13496009/diff/35001/content/test/data/media/w...
content/test/data/media/webrtc_test_utilities.js:11: 
On 2013/04/11 11:16:48, phoglund wrote:
> Write a small accessor to overwrite this guy which is then called by the two
> other files. I think we should avoid overwriting globals in other files
> directly.

Done.

[phoglund_chromium, 2013-04-11 12:00:51]

lgtm

[commit-bot: I haz the power, 2013-04-11 12:10:02]

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

[commit-bot: I haz the power, 2013-04-11 12:10:12]

Presubmit check for 13496009-38014 failed and returned exit status 1.

INFO:root:Found 10 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit Warnings **
License must match:
.*? Copyright (\(c\) )?(2013|2012|2011|2010|2009|2008|2007|2006|2006-2008) The
Chromium Authors\. All rights reserved\.\n.*? Use of this source code is
governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?:
\*/)?\n
Found a bad license header in these files:
  content/test/data/media/webrtc_test_utilities.js

Presubmit checks took 1.1s to calculate.

[commit-bot: I haz the power, 2013-04-11 12:49:15]

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

[commit-bot: I haz the power, 2013-04-11 14:02:21]

Retried try job too often on linux_chromeos for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-04-12 08:29:50]

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

[commit-bot: I haz the power, 2013-04-12 12:30:25]

Change committed as 193908