Pass VideoRenderer interface to ConnectionToHost.

Pass VideoRenderer interface to ConnectionToHost.

WebrtcConnectionToHost will need to access VideoRenderer to be able to
render incoming video frames. Previously it was taking VideoStub which
accepts only encoded frame.

BUG=547158
Committed: https://crrev.com/0b57073aeb86bb2ed5977d873f0276aa03a7f6f3
Cr-Commit-Position: refs/heads/master@{#367692}

[Sergey Ulanov, 2016-01-04 22:10:58]

sergeyu@chromium.org changed reviewers:
+ jamiewalch@chromium.org

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

https://codereview.chromium.org/1559933003/diff/1/remoting/protocol/ice_conne...
File remoting/protocol/ice_connection_to_host.cc (left):

https://codereview.chromium.org/1559933003/diff/1/remoting/protocol/ice_conne...
remoting/protocol/ice_connection_to_host.cc:82: base::Unretained(this))));
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:23]

https://codereview.chromium.org/1559933003/diff/1/remoting/protocol/ice_conne...
File remoting/protocol/ice_connection_to_host.cc (left):

https://codereview.chromium.org/1559933003/diff/1/remoting/protocol/ice_conne...
remoting/protocol/ice_connection_to_host.cc:82: base::Unretained(this))));
On 2016/01/04 22:41:15, Jamie wrote:
> 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)?

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]

lgtm

https://codereview.chromium.org/1559933003/diff/1/remoting/protocol/ice_conne...
File remoting/protocol/ice_connection_to_host.cc (left):

https://codereview.chromium.org/1559933003/diff/1/remoting/protocol/ice_conne...
remoting/protocol/ice_connection_to_host.cc:82: base::Unretained(this))));
On 2016/01/04 23:35:22, Sergey Ulanov wrote:
> On 2016/01/04 22:41:15, Jamie wrote:
> > 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)?
> 
> 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.

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]

https://codereview.chromium.org/1559933003/diff/1/remoting/protocol/ice_conne...
File remoting/protocol/ice_connection_to_host.cc (left):

https://codereview.chromium.org/1559933003/diff/1/remoting/protocol/ice_conne...
remoting/protocol/ice_connection_to_host.cc:82: base::Unretained(this))));
On 2016/01/04 23:40:49, Jamie wrote:
> On 2016/01/04 23:35:22, Sergey Ulanov wrote:
> > On 2016/01/04 22:41:15, Jamie wrote:
> > > 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)?
> > 
> > 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.
> 
> Maybe a DCHECK that MonitoredVideoStub is null in that case (I think it will
do
> the wrong thing otherwise).

Done.

[Jamie, 2016-01-05 19:21:44]

lgtm

[Sergey Ulanov, 2016-01-05 20:37:34]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2016-01-05 20:37:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1559933003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1559933003/20001

[commit-bot: I haz the power, 2016-01-05 20:53:35]

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

[commit-bot: I haz the power, 2016-01-05 20:53:35]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sergey Ulanov, 2016-01-05 22:20:08]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-01-05 22:20:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/1559933003/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-01-05 22:20:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1559933003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1559933003/40001

[commit-bot: I haz the power, 2016-01-05 22:35:55]

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

[commit-bot: I haz the power, 2016-01-05 22:35:55]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sergey Ulanov, 2016-01-05 22:54:37]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-01-05 22:54:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/1559933003/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-01-05 22:54:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1559933003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1559933003/60001

[commit-bot: I haz the power, 2016-01-05 23:36:53]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-05 23:38:08]

Description was changed from

==========
Pass VideoRenderer interface to ConnectionToHost.

WebrtcConnectionToHost will need to access VideoRenderer to be able to
render incoming video frames. Previously it was taking VideoStub which
accepts only encoded frame.

BUG=547158
==========

to

==========
Pass VideoRenderer interface to ConnectionToHost.

WebrtcConnectionToHost will need to access VideoRenderer to be able to
render incoming video frames. Previously it was taking VideoStub which
accepts only encoded frame.

BUG=547158

Committed: https://crrev.com/0b57073aeb86bb2ed5977d873f0276aa03a7f6f3
Cr-Commit-Position: refs/heads/master@{#367692}
==========

[commit-bot: I haz the power, 2016-01-05 23:38:09]

Patchset 4 (id:??) landed as
https://crrev.com/0b57073aeb86bb2ed5977d873f0276aa03a7f6f3
Cr-Commit-Position: refs/heads/master@{#367692}