Added implementation of RemoteMediaStreams.

Added implementation of RemoteMediaStreams. 
This implement an observer of a remote webrtc MediaStream and propagates changes to a WebKit MediaStream.
It propagates adding and removing of remote tracks to existing mediastreams as well as if a remote track has ended.

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

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

[perkj_chrome, 2013-04-16 13:28:22]

Hej
Can you please review? Tommi- the Chromium code and Patrik the content
browsertest?

The unit tests RemoveAndAddAudioTrackFromRemoteStream and 
RemoveAndAddVideoTrackFromRemoteStream don't pass at the moment due to how
WebKit creates the stream implementation. I will ask Tommy W about that but I
might decide to remove them. The feature is also tested by the content
browsertest.

[tommi (sloooow) - chröme, 2013-04-16 13:54:31]

There seems to be something wrong with the diff.  Can you upload again?

[perkj_chrome, 2013-04-16 13:59:38]

done


On Tue, Apr 16, 2013 at 3:54 PM, <tommi@chromium.org> wrote:

> There seems to be something wrong with the diff.  Can you upload again?
>
>
https://codereview.chromium.**org/14200016/<https://codereview.chromium.org/1...
>

[phoglund_chromium, 2013-04-16 14:44:58]

https://codereview.chromium.org/14200016/diff/5004/content/test/data/media/pe...
File content/test/data/media/peerconnection-call.html (right):

https://codereview.chromium.org/14200016/diff/5004/content/test/data/media/pe...
content/test/data/media/peerconnection-call.html:124:
navigator.webkitGetUserMedia({audio:true, video:true},
Nit: line length

https://codereview.chromium.org/14200016/diff/5004/content/test/data/media/pe...
content/test/data/media/peerconnection-call.html:130: // Set an event handler
for when video is playing.
Side note:  We should consider if we make this event handling framework clearer.
It would be better in this case if we could say something like
whenVideoIsPlaying(function() { ... }); instead of setAllEventsOccurredHandler,
which is less clear. We don't have to do it now though.

https://codereview.chromium.org/14200016/diff/5004/content/test/data/media/pe...
content/test/data/media/peerconnection-call.html:138: // Add an expected event
that onaddtrack will be called on the remote
So you call addTrack and removeTrack and _later_ set up the expectations related
to them? I guess the thing that makes it work is that this function will finish
executing before any callbacks get a chance to run, but I think it makes more
sense from a code perspective to call the add and remove functions as the last
thing you do rather than the first.

https://codereview.chromium.org/14200016/diff/5004/content/test/data/media/pe...
content/test/data/media/peerconnection-call.html:277:
displayAndRemember(localStream);
This change doesn't make sense, accidental?

[tommi (sloooow) - chröme, 2013-04-16 15:59:59]

https://codereview.chromium.org/14200016/diff/5004/content/renderer/media/moc...
File content/renderer/media/mock_media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/14200016/diff/5004/content/renderer/media/moc...
content/renderer/media/mock_media_stream_dependency_factory.cc:95: observer_ =
observer;
first DCHECK(!observer_)?

https://codereview.chromium.org/14200016/diff/5004/content/renderer/media/rem...
File content/renderer/media/remote_media_stream_impl.cc (right):

https://codereview.chromium.org/14200016/diff/5004/content/renderer/media/rem...
content/renderer/media/remote_media_stream_impl.cc:33: virtual void OnChanged()
OVERRIDE;
make private?

https://codereview.chromium.org/14200016/diff/5004/content/renderer/media/rem...
content/renderer/media/remote_media_stream_impl.cc:62: state_ = state;
nit: empty line above

https://codereview.chromium.org/14200016/diff/5004/content/renderer/media/rem...
content/renderer/media/remote_media_stream_impl.cc:66: /* kInitializing is not
ready state. */
//

I'm not sure what this comment means though?

https://codereview.chromium.org/14200016/diff/5004/content/renderer/media/rem...
content/renderer/media/remote_media_stream_impl.cc:77: NOTREACHED();
nit: add a break;

https://codereview.chromium.org/14200016/diff/5004/content/renderer/media/rem...
content/renderer/media/remote_media_stream_impl.cc:81: static void
InitializeWebkitTrack(
should this be in anonymous namespace near the top, rather than directly in the
content namespace?
(I think that's the convention in content/ but I might be wrong)

https://codereview.chromium.org/14200016/diff/5004/content/renderer/media/rem...
content/renderer/media/remote_media_stream_impl.cc:98: if
((*it)->observered_track() == track) {
nit: no {}

[perkj_chrome, 2013-04-18 14:46:57]

PTAL

https://codereview.chromium.org/14200016/diff/5004/content/renderer/media/moc...
File content/renderer/media/mock_media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/14200016/diff/5004/content/renderer/media/moc...
content/renderer/media/mock_media_stream_dependency_factory.cc:95: observer_ =
observer;
On 2013/04/16 15:59:59, tommi wrote:
> first DCHECK(!observer_)?

Done.

https://codereview.chromium.org/14200016/diff/5004/content/renderer/media/rem...
File content/renderer/media/remote_media_stream_impl.cc (right):

https://codereview.chromium.org/14200016/diff/5004/content/renderer/media/rem...
content/renderer/media/remote_media_stream_impl.cc:33: virtual void OnChanged()
OVERRIDE;
On 2013/04/16 15:59:59, tommi wrote:
> make private?

Done.

https://codereview.chromium.org/14200016/diff/5004/content/renderer/media/rem...
content/renderer/media/remote_media_stream_impl.cc:62: state_ = state;
On 2013/04/16 15:59:59, tommi wrote:
> nit: empty line above

Done.

https://codereview.chromium.org/14200016/diff/5004/content/renderer/media/rem...
content/renderer/media/remote_media_stream_impl.cc:66: /* kInitializing is not
ready state. */
On 2013/04/16 15:59:59, tommi wrote:
> //
> 
> I'm not sure what this comment means though?

Done.

https://codereview.chromium.org/14200016/diff/5004/content/renderer/media/rem...
content/renderer/media/remote_media_stream_impl.cc:77: NOTREACHED();
On 2013/04/16 15:59:59, tommi wrote:
> nit: add a break;

Done.

https://codereview.chromium.org/14200016/diff/5004/content/renderer/media/rem...
content/renderer/media/remote_media_stream_impl.cc:81: static void
InitializeWebkitTrack(
On 2013/04/16 15:59:59, tommi wrote:
> should this be in anonymous namespace near the top, rather than directly in
the
> content namespace?
> (I think that's the convention in content/ but I might be wrong)

I use RemoteMediaStreamTrackObserver in FindTrackObserver.So I can not put it
first..

https://codereview.chromium.org/14200016/diff/5004/content/renderer/media/rem...
content/renderer/media/remote_media_stream_impl.cc:98: if
((*it)->observered_track() == track) {
On 2013/04/16 15:59:59, tommi wrote:
> nit: no {}

Done.

https://codereview.chromium.org/14200016/diff/5004/content/test/data/media/pe...
File content/test/data/media/peerconnection-call.html (right):

https://codereview.chromium.org/14200016/diff/5004/content/test/data/media/pe...
content/test/data/media/peerconnection-call.html:124:
navigator.webkitGetUserMedia({audio:true, video:true},
On 2013/04/16 14:44:58, phoglund wrote:
> Nit: line length

Done.

https://codereview.chromium.org/14200016/diff/5004/content/test/data/media/pe...
content/test/data/media/peerconnection-call.html:130: // Set an event handler
for when video is playing.
On 2013/04/16 14:44:58, phoglund wrote:
> Side note:  We should consider if we make this event handling framework
clearer.
> It would be better in this case if we could say something like
> whenVideoIsPlaying(function() { ... }); instead of
setAllEventsOccurredHandler,
> which is less clear. We don't have to do it now though.

I was considering that. But one feature I like with this is that you can setup
multiple events to occur and don't care about the order. 

If you do care about the order you just change setAllEventsOccuredHandler after
every step.  So it has pros and cons.

https://codereview.chromium.org/14200016/diff/5004/content/test/data/media/pe...
content/test/data/media/peerconnection-call.html:138: // Add an expected event
that onaddtrack will be called on the remote
On 2013/04/16 14:44:58, phoglund wrote:
> So you call addTrack and removeTrack and _later_ set up the expectations
related
> to them? I guess the thing that makes it work is that this function will
finish
> executing before any callbacks get a chance to run, but I think it makes more
> sense from a code perspective to call the add and remove functions as the last
> thing you do rather than the first.

Nothing happens until you do the renegotiation. That said- I will move add and
remove tracks anyway.

https://codereview.chromium.org/14200016/diff/5004/content/test/data/media/pe...
content/test/data/media/peerconnection-call.html:277:
displayAndRemember(localStream);
On 2013/04/16 14:44:58, phoglund wrote:
> This change doesn't make sense, accidental?

Nope- I want to reuse createNewVideoStreamAndAddToBothConnections. ButI wanted
to makes sure gLocalStream is set to what getusermedia returns. 
So therefore copied what I needed from addStreamToBothConnectionsAndNegotiate
here instead.

[tommi (sloooow) - chröme, 2013-04-18 16:33:31]

lgtm

https://codereview.chromium.org/14200016/diff/14001/content/renderer/media/re...
File content/renderer/media/remote_media_stream_impl.cc (right):

https://codereview.chromium.org/14200016/diff/14001/content/renderer/media/re...
content/renderer/media/remote_media_stream_impl.cc:14: namespace  content {
only one space (looks like an accidental change)

https://codereview.chromium.org/14200016/diff/14001/content/renderer/media/re...
content/renderer/media/remote_media_stream_impl.cc:96: // kInitializing is not a
WebMediaStreamSource::ReadyState.
nit: // Ignore the kInitializing state since there is no match in
WebMediaStreamSource::ReadyState.

[perkj_chrome, 2013-04-18 17:04:39]

Avi, do you mind helping me too with gypi ?

Thanks

[phoglund_chromium, 2013-04-18 17:18:40]

lgtm

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

https://codereview.chromium.org/14200016/diff/14001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:176: negotiate();
Nit: End of...

By the way, that you need to add this comment suggests that the above code
should move to a named function instead. I don't think it pulls in any arguments
from the outer code anyway. Just a suggestion.

[perkj_chrome, 2013-04-19 06:23:20]

Fixed nits. Avi, can you help me by approving the GYP change?

Thanks
Per

https://codereview.chromium.org/14200016/diff/14001/content/renderer/media/re...
File content/renderer/media/remote_media_stream_impl.cc (right):

https://codereview.chromium.org/14200016/diff/14001/content/renderer/media/re...
content/renderer/media/remote_media_stream_impl.cc:14: namespace  content {
On 2013/04/18 16:33:31, tommi wrote:
> only one space (looks like an accidental change)

Done.

https://codereview.chromium.org/14200016/diff/14001/content/renderer/media/re...
content/renderer/media/remote_media_stream_impl.cc:96: // kInitializing is not a
WebMediaStreamSource::ReadyState.
On 2013/04/18 16:33:31, tommi wrote:
> nit: // Ignore the kInitializing state since there is no match in
> WebMediaStreamSource::ReadyState.

Done.

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

https://codereview.chromium.org/14200016/diff/14001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:176: negotiate();
In that case I would like to keep this as is. Yes it can be pulled out to a
separate function but it only make sence in the context of this test I think. So
at least I think it is easier to see what is happening this way.
On 2013/04/18 17:18:40, phoglund wrote:
> Nit: End of...
> 
> By the way, that you need to add this comment suggests that the above code
> should move to a named function instead. I don't think it pulls in any
arguments
> from the outer code anyway. Just a suggestion.

[Avi (use Gerrit), 2013-04-19 08:39:50]

gypi lgtm

Where are your BUGs? At least link this change and others to a feature bug.

[perkj_chrome, 2013-04-19 10:09:12]

On 2013/04/19 08:39:50, Avi wrote:
> gypi lgtm
> 
> Where are your BUGs? At least link this change and others to a feature bug.

Thanks- there is a link to the webrtc tracker in the description. But I created
a bug in crbug and referred to it as well.

[commit-bot: I haz the power, 2013-04-19 10:09:37]

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

[commit-bot: I haz the power, 2013-04-19 14:36:05]

Change committed as 195186

[Jered, 2013-04-22 21:02:37]

Hi,

When trying to run tot chrome today on chromeos, I see the following error:
out/Debug/chrome: symbol lookup error:
/usr/local/google/chromium.git/src/out/Debug/lib/libcontent.so: undefined
symbol:
_ZN6WebKit20WebMediaStreamSource10initializeERKNS_9WebStringENS0_4TypeES3_

This change is the most recent reference I could find to the initialize()
method. Is it possible there's some missing build or link dependency here?

On 2013/04/19 10:09:12, perkj wrote:
> On 2013/04/19 08:39:50, Avi wrote:
> > gypi lgtm
> > 
> > Where are your BUGs? At least link this change and others to a feature bug.
> 
> Thanks- there is a link to the webrtc tracker in the description. But I
created
> a bug in crbug and referred to it as well.

[Jered, 2013-04-22 21:04:48]

https://chromiumcodereview.appspot.com/14200016/diff/28001/content/content_re...
File content/content_renderer.gypi (right):

https://chromiumcodereview.appspot.com/14200016/diff/28001/content/content_re...
content/content_renderer.gypi:150: 'renderer/media/remote_media_stream_impl.cc',
Should this be built even if enable_webrtc is 0?

[Jered, 2013-04-22 21:13:14]

On 2013/04/22 21:04:48, Jered wrote:
>
https://chromiumcodereview.appspot.com/14200016/diff/28001/content/content_re...
> File content/content_renderer.gypi (right):
> 
>
https://chromiumcodereview.appspot.com/14200016/diff/28001/content/content_re...
> content/content_renderer.gypi:150:
'renderer/media/remote_media_stream_impl.cc',
> Should this be built even if enable_webrtc is 0?

I sent 14150011.