Implement a source for remote video tracks.

Implement a source for remote video tracks.
MediaStreamRemoteVideoSource implements the MediaStreamVideoSource interface
for video tracks received on a PeerConnection. The purpose of the class is
to make sure there is no difference between a video track where the source is
a local source and a video track where the source is a remote video track.

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

[perkj_chrome, 2014-03-17 19:19:11]

Hej
Can you please take a first pass on the implementation of a s ource for remote
video tracks?

[Jói, 2014-03-17 22:00:40]

This change is full of awesome.

I just have minor nits and questions.

Cheers,
Jói

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.h:128: State GetState() const {
return state_; }
Our naming conventions would typically name this method state() since it is a
simple getter.

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

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/m...
content/renderer/media/mock_media_stream_dependency_factory.cc:306: void
MockLocalVideoTrack::RemoveRenderer(VideoRendererInterface* renderer) {
The interface here is a little weird unless you plan to allow multiple renderers
in the future. If it's just the one renderer, then a SetRenderer would seem
simpler.

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right):

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:21:
track_state_(remote_track_->state()),
As a minor suggestion, perhaps this should be called last_state_ as a way to
self-document that this is used to detect changes in state?

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:35: if
(format_.IsValid()) {
If format_ is not valid, is it acceptable to never call OnSupportedFormats?

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:64:
base::TimeDelta timestamp = base::TimeDelta::FromMilliseconds(
Note that there is also a FromMicroseconds factory, in case that would provide
better precision.

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc/media_stream_remote_video_source_unittest.cc
(right):

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source_unittest.cc:58:
(cricket::VideoCapturer*) NULL)),
nit: static_cast

[Ronghua Wu (Left Chromium), 2014-03-17 22:45:35]

looks pretty good, didn't see major issues

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

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:719: void
MediaStreamDependencyFactory::AddNativeTrackToBlinkTrack(
rename this to indicate audio only?

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

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:355: capture_adapter_ =
factory_->CreateVideoCapturer(is_screeencast);
since we no longer assert factory_, we should check factory_ before use

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/r...
File content/renderer/media/remote_media_stream_impl.cc (right):

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/r...
content/renderer/media/remote_media_stream_impl.cc:74:
RemoteMediaStreamTrackAdapter(webrtc::MediaStreamTrackInterface* webrtc_track,
should this be called native_track? webrtc is too broad for this.

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right):

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:21:
track_state_(remote_track_->state()),
On 2014/03/17 22:00:40, Jói wrote:
> As a minor suggestion, perhaps this should be called last_state_ as a way to
> self-document that this is used to detect changes in state?

+1

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:35: if
(format_.IsValid()) {
On 2014/03/17 22:00:40, Jói wrote:
> If format_ is not valid, is it acceptable to never call OnSupportedFormats?

Should call OnSupportedFormats regardless, otherwise the caller will be waiting
for the callback forever.

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:78: // Aspect
ratio unsupported; DCHECK when there are non-square pixels.
how do you mean by "Aspect ratio unsupported"?

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:80:
DCHECK_EQ(frame->GetPixelHeight(), 1u);
why is this check needed?

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:110:
message_loop_proxy_->PostTask(
Don't deliver the frame before the source is started.

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc/media_stream_remote_video_source.h (right):

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source.h:13: #include
"third_party/libjingle/source/talk/app/webrtc/mediastreaminterface.h"
move one line above

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source.h:62: /// thread
webrtc::VideoRendererInterface::RenderFrame is called on.
remove extra /

[perkj_chrome, 2014-03-19 16:34:47]

PTAL

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

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:719: void
MediaStreamDependencyFactory::AddNativeTrackToBlinkTrack(
On 2014/03/17 22:45:35, Ronghua Wu wrote:
> rename this to indicate audio only?

Done.

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

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:355: capture_adapter_ =
factory_->CreateVideoCapturer(is_screeencast);
On 2014/03/17 22:45:35, Ronghua Wu wrote:
> since we no longer assert factory_, we should check factory_ before use

Other reviewers typically say that is no need. Since it will be obvious in the
crash that it is a null pointer.

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.h:128: State GetState() const {
return state_; }
On 2014/03/17 22:00:40, Jói wrote:
> Our naming conventions would typically name this method state() since it is a
> simple getter.

Done.

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

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/m...
content/renderer/media/mock_media_stream_dependency_factory.cc:306: void
MockLocalVideoTrack::RemoveRenderer(VideoRendererInterface* renderer) {
On 2014/03/17 22:00:40, Jói wrote:
> The interface here is a little weird unless you plan to allow multiple
renderers
> in the future. If it's just the one renderer, then a SetRenderer would seem
> simpler.

This is an overriden method. The real implementation in libjingle support
multiple but since I only need one renderer I DCHECK that is how I used it 
instead. I renamed this class to indicate what it really is.

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/r...
File content/renderer/media/remote_media_stream_impl.cc (right):

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/r...
content/renderer/media/remote_media_stream_impl.cc:74:
RemoteMediaStreamTrackAdapter(webrtc::MediaStreamTrackInterface* webrtc_track,
On 2014/03/17 22:45:35, Ronghua Wu wrote:
> should this be called native_track? webrtc is too broad for this.

I did a code search and it seems we have been fairly consistent with usign
native_track for the implementation in chrome so far. This is a remote webrtc
track so I think its an ok name. Do you feel strong about it and have anotehr
suggestion?

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right):

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:21:
track_state_(remote_track_->state()),
On 2014/03/17 22:00:40, Jói wrote:
> As a minor suggestion, perhaps this should be called last_state_ as a way to
> self-document that this is used to detect changes in state?

Done.

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:21:
track_state_(remote_track_->state()),
On 2014/03/17 22:45:35, Ronghua Wu wrote:
> On 2014/03/17 22:00:40, Jói wrote:
> > As a minor suggestion, perhaps this should be called last_state_ as a way to
> > self-document that this is used to detect changes in state?
> 
> +1

Done.

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:35: if
(format_.IsValid()) {
On 2014/03/17 22:45:35, Ronghua Wu wrote:
> On 2014/03/17 22:00:40, Jói wrote:
> > If format_ is not valid, is it acceptable to never call OnSupportedFormats?
> 
> Should call OnSupportedFormats regardless, otherwise the caller will be
waiting
> for the callback forever.

So the idea here is that format_ gets valid when the first video frame is
received. But that will typically happen after the remote SDP has been received
and the new remote track has been announced.

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:64:
base::TimeDelta timestamp = base::TimeDelta::FromMilliseconds(
On 2014/03/17 22:00:40, Jói wrote:
> Note that there is also a FromMicroseconds factory, in case that would provide
> better precision.

Except that I introduce the video frame pool I kept the rendering part the same
as we have had a long time. Right now its from webrtc_video_sink_adapter.cc and
previously somewhere else. So thanks- but ms precision is good enough. (btw, I
found out that this is currently only used for showing the time in seconds in a
<video>

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:78: // Aspect
ratio unsupported; DCHECK when there are non-square pixels.
On 2014/03/17 22:45:35, Ronghua Wu wrote:
> how do you mean by "Aspect ratio unsupported"?

This function is actually copied from the one I think you wrote originally and
that has been modified by others. I simplified the comment.

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:80:
DCHECK_EQ(frame->GetPixelHeight(), 1u);
On 2014/03/17 22:45:35, Ronghua Wu wrote:
> why is this check needed? Becuse libjingle have the concept of non-square
pixels but Chrome doesn't. So its a DCHECK for that if it is ever used. (This is
not new code).

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:110:
message_loop_proxy_->PostTask(
On 2014/03/17 22:45:35, Ronghua Wu wrote:
> Don't deliver the frame before the source is started.

ok- But we need to do that on the correct thread. Added to
:DoRenderFrameOnMainThread.

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc/media_stream_remote_video_source.h (right):

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source.h:13: #include
"third_party/libjingle/source/talk/app/webrtc/mediastreaminterface.h"
On 2014/03/17 22:45:35, Ronghua Wu wrote:
> move one line above

actually - WebKit includes must come first in third_party includes. Otherwise
the presubmit script complains.

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source.h:62: /// thread
webrtc::VideoRendererInterface::RenderFrame is called on.
On 2014/03/17 22:45:35, Ronghua Wu wrote:
> remove extra /

Done.

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc/media_stream_remote_video_source_unittest.cc
(right):

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source_unittest.cc:58:
(cricket::VideoCapturer*) NULL)),
On 2014/03/17 22:00:40, Jói wrote:
> nit: static_cast

Done.

[Jói, 2014-03-20 11:10:44]

LGTM

[Ronghua Wu (Left Chromium), 2014-03-20 21:55:50]

lgtm

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

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:355: capture_adapter_ =
factory_->CreateVideoCapturer(is_screeencast);
On 2014/03/19 16:34:48, perkj wrote:
> On 2014/03/17 22:45:35, Ronghua Wu wrote:
> > since we no longer assert factory_, we should check factory_ before use
> 
> Other reviewers typically say that is no need. Since it will be obvious in the
> crash that it is a null pointer.

ok. :)

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/r...
File content/renderer/media/remote_media_stream_impl.cc (right):

https://codereview.chromium.org/201583003/diff/20001/content/renderer/media/r...
content/renderer/media/remote_media_stream_impl.cc:74:
RemoteMediaStreamTrackAdapter(webrtc::MediaStreamTrackInterface* webrtc_track,
On 2014/03/19 16:34:48, perkj wrote:
> On 2014/03/17 22:45:35, Ronghua Wu wrote:
> > should this be called native_track? webrtc is too broad for this.
> 
> I did a code search and it seems we have been fairly consistent with usign
> native_track for the implementation in chrome so far. This is a remote webrtc
> track so I think its an ok name. Do you feel strong about it and have anotehr
> suggestion?

In that case keep it as it's then.

[perkj_chrome, 2014-04-05 18:32:29]

The CQ bit was checked by perkj@chromium.org

[commit-bot: I haz the power, 2014-04-05 18:32:36]

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

[commit-bot: I haz the power, 2014-04-05 18:32:48]

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

[commit-bot: I haz the power, 2014-04-05 18:32:48]

Failed to apply patch for
content/renderer/media/media_stream_dependency_factory.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/media/media_stream_dependency_factory.cc
  Hunk #1 FAILED at 344.
  Hunk #2 succeeded at 678 (offset -35 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
content/renderer/media/media_stream_dependency_factory.cc.rej

Patch:       content/renderer/media/media_stream_dependency_factory.cc
Index: content/renderer/media/media_stream_dependency_factory.cc
diff --git a/content/renderer/media/media_stream_dependency_factory.cc
b/content/renderer/media/media_stream_dependency_factory.cc
index
0393fa4af2a6d133e55a615c8c535b2ee40de197..bd78e27112874da9e87cef54ff2e281e6ab627c7
100644
--- a/content/renderer/media/media_stream_dependency_factory.cc
+++ b/content/renderer/media/media_stream_dependency_factory.cc
@@ -344,7 +344,7 @@ bool MediaStreamDependencyFactory::AddNativeVideoMediaTrack(
   webkit_source.initialize(webkit_track_id, type, webkit_track_id);
 
   webkit_track.initialize(webkit_track_id, webkit_source);
-  AddNativeTrackToBlinkTrack(native_track.get(), webkit_track, true);
+  AddNativeAudioTrackToBlinkTrack(native_track.get(), webkit_track, true);
 
   // Add the track to WebMediaStream.
   stream->addTrack(webkit_track);
@@ -713,24 +713,20 @@ MediaStreamDependencyFactory::CreateAudioCapturer(
                                              GetWebRtcAudioDevice());
 }
 
-void MediaStreamDependencyFactory::AddNativeTrackToBlinkTrack(
+void MediaStreamDependencyFactory::AddNativeAudioTrackToBlinkTrack(
     webrtc::MediaStreamTrackInterface* native_track,
     const blink::WebMediaStreamTrack& webkit_track,
     bool is_local_track) {
   DCHECK(!webkit_track.isNull() && !webkit_track.extraData());
+  DCHECK_EQ(blink::WebMediaStreamSource::TypeAudio,
+            webkit_track.source().type());
   blink::WebMediaStreamTrack track = webkit_track;
 
-  if (track.source().type() == blink::WebMediaStreamSource::TypeVideo) {
-    DVLOG(1) << "AddNativeTrackToBlinkTrack() video";
-    track.setExtraData(new WebRtcMediaStreamVideoTrack(
-        static_cast<webrtc::VideoTrackInterface*>(native_track)));
-  } else {
-    DVLOG(1) << "AddNativeTrackToBlinkTrack() audio";
-    track.setExtraData(
-        new MediaStreamTrack(
-            static_cast<webrtc::AudioTrackInterface*>(native_track),
-            is_local_track));
-  }
+  DVLOG(1) << "AddNativeTrackToBlinkTrack() audio";
+  track.setExtraData(
+      new MediaStreamTrack(
+          static_cast<webrtc::AudioTrackInterface*>(native_track),
+          is_local_track));
 }
 
 bool MediaStreamDependencyFactory::OnControlMessageReceived(

[perkj_chrome, 2014-04-06 10:45:09]

The CQ bit was checked by perkj@chromium.org

[commit-bot: I haz the power, 2014-04-06 10:45:11]

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

[commit-bot: I haz the power, 2014-04-06 16:25:02]

Change committed as 262050