Pass VideoRenderer interface to ConnectionToHost.

remoting/protocol/ice_connection_to_host.cc

Index: remoting/protocol/ice_connection_to_host.cc
diff --git a/remoting/protocol/ice_connection_to_host.cc b/remoting/protocol/ice_connection_to_host.cc
index ddb77c73b1d0884ab88ed073670069d41ca0a5b0..02788471bc9b4260b42df5b5945eda001e31f6e1 100644
--- a/remoting/protocol/ice_connection_to_host.cc
+++ b/remoting/protocol/ice_connection_to_host.cc
@@ -21,7 +21,7 @@
 #include "remoting/protocol/errors.h"
 #include "remoting/protocol/ice_transport.h"
 #include "remoting/protocol/transport_context.h"
-#include "remoting/protocol/video_stub.h"
+#include "remoting/protocol/video_renderer.h"
 
 namespace remoting {
 namespace protocol {
@@ -73,13 +73,9 @@ void IceConnectionToHost::set_clipboard_stub(ClipboardStub* clipboard_stub) {
   clipboard_stub_ = clipboard_stub;
 }
 
-void IceConnectionToHost::set_video_stub(VideoStub* video_stub) {
-  DCHECK(video_stub);
-  monitored_video_stub_.reset(new MonitoredVideoStub(
-      video_stub, base::TimeDelta::FromSeconds(
-                      MonitoredVideoStub::kConnectivityCheckDelaySeconds),
-      base::Bind(&IceConnectionToHost::OnVideoChannelStatus,
-                 base::Unretained(this))));

[Jamie, 2016/01/04 22:41:15]

I think it would be clearer to keep initialization of monitored_video_stub_
here, since it's tied to the video_renderer_. What's the benefit of moving it to
OnSessionStateChange(AUTHENTICATED)?

[Sergey Ulanov, 2016/01/04 23:35:22]

There are no very significant benefits, but I think it's cleaner this way:
 - it keeps all video renderer initialization logic in one place.
 - MonitoredVideoStub starts "monitoring" the video stream when it's created,
but the stream cannot be active before connection is authenticated.
 - this is a lower-case function, so it's not supposed to do anything complex
such as memory allocation.
 - This doesn't matter right now, but potentially an implementation of
VideoRenderer interface may require that GetVideoStub() is called only after
OnSessionConfig(), e.g. to create different stabs depending on session
configuration.

[Jamie, 2016/01/04 23:40:49]

Maybe a DCHECK that MonitoredVideoStub is null in that case (I think it will do
the wrong thing otherwise).

[Sergey Ulanov, 2016/01/05 18:03:03]

Done.

+void IceConnectionToHost::set_video_renderer(VideoRenderer* video_renderer) {
+  DCHECK(video_renderer);
+  video_renderer_ = video_renderer;
 }
 
 void IceConnectionToHost::set_audio_stub(AudioStub* audio_stub) {
@@ -101,19 +97,31 @@ void IceConnectionToHost::OnSessionStateChange(Session::State state) {
 
     case Session::AUTHENTICATED:
       SetState(AUTHENTICATED, OK);
+
+      // Setup control channel.
       control_dispatcher_.reset(new ClientControlDispatcher());
       control_dispatcher_->Init(transport_->GetMultiplexedChannelFactory(),
                                 this);
       control_dispatcher_->set_client_stub(client_stub_);
       control_dispatcher_->set_clipboard_stub(clipboard_stub_);
 
+      // Setup event channel.
       event_dispatcher_.reset(new ClientEventDispatcher());
       event_dispatcher_->Init(transport_->GetMultiplexedChannelFactory(), this);
 
+      // Configure video pipeline.
+      video_renderer_->OnSessionConfig(session_->config());
+      monitored_video_stub_.reset(new MonitoredVideoStub(
+          video_renderer_->GetVideoStub(),
+          base::TimeDelta::FromSeconds(
+              MonitoredVideoStub::kConnectivityCheckDelaySeconds),
+          base::Bind(&IceConnectionToHost::OnVideoChannelStatus,
+                     base::Unretained(this))));
       video_dispatcher_.reset(
           new ClientVideoDispatcher(monitored_video_stub_.get()));
       video_dispatcher_->Init(transport_->GetStreamChannelFactory(), this);
 
+      // Configure audio pipeline if necessary.
       if (session_->config().is_audio_enabled()) {
         audio_reader_.reset(new AudioReader(audio_stub_));
         audio_reader_->Init(transport_->GetMultiplexedChannelFactory(), this);