Add VideoStream interface.

Add VideoStream interface.

Added new VideoStream interface. VideoFramePump implements it. Later
there will be a separate implementation for WebRTC-based protocol.
ConnectionToClient is responsible for VideoStream creation.

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

[Sergey Ulanov, 2015-11-25 00:09:35]

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

[Jamie, 2015-11-25 01:32:46]

LGTM with nits

https://codereview.chromium.org/1472873005/diff/1/remoting/host/client_sessio...
File remoting/host/client_session.cc (right):

https://codereview.chromium.org/1472873005/diff/1/remoting/host/client_sessio...
remoting/host/client_session.cc:119: DCHECK(!screen_controls_);
No DCHECK for video_stream_?

https://codereview.chromium.org/1472873005/diff/1/remoting/protocol/connectio...
File remoting/protocol/connection_to_client.h (right):

https://codereview.chromium.org/1472873005/diff/1/remoting/protocol/connectio...
remoting/protocol/connection_to_client.h:88: // sends it to the client.
s/sends/send/

https://codereview.chromium.org/1472873005/diff/1/remoting/protocol/video_fra...
File remoting/protocol/video_frame_pump.cc (right):

https://codereview.chromium.org/1472873005/diff/1/remoting/protocol/video_fra...
remoting/protocol/video_frame_pump.cc:125: size_callback_.Run(frame->size());
You're calling this on every capture, not just when the size changes as the
interface comment suggests. My preference would be do what the comment says--ie,
remember the last-notified size here--but you could just change the comment and
have all implementations deal with duplicate sizes.

https://codereview.chromium.org/1472873005/diff/1/remoting/protocol/video_str...
File remoting/protocol/video_stream.h (right):

https://codereview.chromium.org/1472873005/diff/1/remoting/protocol/video_str...
remoting/protocol/video_stream.h:29: // Should be called whenever input event is
received.
s/input/an input/

[Sergey Ulanov, 2015-11-27 22:11:34]

https://codereview.chromium.org/1472873005/diff/1/remoting/host/client_sessio...
File remoting/host/client_session.cc (right):

https://codereview.chromium.org/1472873005/diff/1/remoting/host/client_sessio...
remoting/host/client_session.cc:119: DCHECK(!screen_controls_);
On 2015/11/25 01:32:46, Jamie wrote:
> No DCHECK for video_stream_?

Done.

https://codereview.chromium.org/1472873005/diff/1/remoting/protocol/connectio...
File remoting/protocol/connection_to_client.h (right):

https://codereview.chromium.org/1472873005/diff/1/remoting/protocol/connectio...
remoting/protocol/connection_to_client.h:88: // sends it to the client.
On 2015/11/25 01:32:46, Jamie wrote:
> s/sends/send/

I think "sends" is correct here. The VideoStream _sends_ screen content to the
client. Reworded this comment to make it clearer.

https://codereview.chromium.org/1472873005/diff/1/remoting/protocol/video_fra...
File remoting/protocol/video_frame_pump.cc (right):

https://codereview.chromium.org/1472873005/diff/1/remoting/protocol/video_fra...
remoting/protocol/video_frame_pump.cc:125: size_callback_.Run(frame->size());
On 2015/11/25 01:32:46, Jamie wrote:
> You're calling this on every capture, not just when the size changes as the
> interface comment suggests. My preference would be do what the comment
says--ie,
> remember the last-notified size here--but you could just change the comment
and
> have all implementations deal with duplicate sizes.

Done.

https://codereview.chromium.org/1472873005/diff/1/remoting/protocol/video_str...
File remoting/protocol/video_stream.h (right):

https://codereview.chromium.org/1472873005/diff/1/remoting/protocol/video_str...
remoting/protocol/video_stream.h:29: // Should be called whenever input event is
received.
On 2015/11/25 01:32:46, Jamie wrote:
> s/input/an input/

Done.

[Sergey Ulanov, 2015-11-27 22:12:47]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2015-11-27 22:12:48]

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/1472873005/#ps20001
(title: " ")

[commit-bot: I haz the power, 2015-11-27 22:12:59]

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

[commit-bot: I haz the power, 2015-11-27 22:14:33]

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

[commit-bot: I haz the power, 2015-11-27 22:14:34]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  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, 2015-11-30 05:04:27]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2015-11-30 05:04:28]

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/1472873005/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-11-30 05:04:46]

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

[commit-bot: I haz the power, 2015-11-30 05:26:13]

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

[commit-bot: I haz the power, 2015-11-30 05:26:14]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[Sergey Ulanov, 2015-11-30 05:52:01]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2015-11-30 05:52:01]

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/1472873005/#ps50021
(title: " ")

[commit-bot: I haz the power, 2015-11-30 05:52:23]

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

[commit-bot: I haz the power, 2015-11-30 06:25:48]

Committed patchset #4 (id:50021)

[commit-bot: I haz the power, 2015-11-30 06:26:33]

Description was changed from

==========
Add VideoStream interface.

Added new VideoStream interface. VideoFramePump implements it. Later
there will be a separate implementation for WebRTC-based protocol.
ConnectionToClient is responsible for VideoStream creation.

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

to

==========
Add VideoStream interface.

Added new VideoStream interface. VideoFramePump implements it. Later
there will be a separate implementation for WebRTC-based protocol.
ConnectionToClient is responsible for VideoStream creation.

BUG=547158

Committed: https://crrev.com/a609b7a407e9a009d963f5c3c799a0bccc1be658
Cr-Commit-Position: refs/heads/master@{#362096}
==========

[commit-bot: I haz the power, 2015-11-30 06:26:34]

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

[Wez, 2015-11-30 18:30:39]

<drive-by> What's going on with MouseClampingFilter - looks like it's being
deleted, but the CL description doesn't mention why, and some of the CL seem to
use it, which suggests it's actually moved somewhere else...?

[Sergey Ulanov, 2015-11-30 18:41:24]

MouseClampingFilter was  a wrapper around protocol::MouseInputFilter that
was also implementing VideoStub to get the frame size. This approach will
not work with WebRTC-based protocol where VideoStub is not applicable. IMO
it was also confusing. So I changed ClientSession to use
protocol::MouseInputFilter directly. ClientSession gets VideoSize
notification from VideoStream and passes that size to the filter. Agree I
should've mentioned it in the CL description.

On Mon, Nov 30, 2015 at 10:30 AM, <wez@chromium.org> wrote:

> <drive-by> What's going on with MouseClampingFilter - looks like it's being
> deleted, but the CL description doesn't mention why, and some of the CL
> seem to
> use it, which suggests it's actually moved somewhere else...?
>
> https://codereview.chromium.org/1472873005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Wez, 2015-11-30 18:53:31]

OK, SGTM!

On 30 November 2015 at 10:41, Sergey Ulanov <sergeyu@chromium.org> wrote:

> MouseClampingFilter was  a wrapper around protocol::MouseInputFilter that
> was also implementing VideoStub to get the frame size. This approach will
> not work with WebRTC-based protocol where VideoStub is not applicable. IMO
> it was also confusing. So I changed ClientSession to use
> protocol::MouseInputFilter directly. ClientSession gets VideoSize
> notification from VideoStream and passes that size to the filter. Agree I
> should've mentioned it in the CL description.
>
> On Mon, Nov 30, 2015 at 10:30 AM, <wez@chromium.org> wrote:
>
>> <drive-by> What's going on with MouseClampingFilter - looks like it's
>> being
>> deleted, but the CL description doesn't mention why, and some of the CL
>> seem to
>> use it, which suggests it's actually moved somewhere else...?
>>
>> https://codereview.chromium.org/1472873005/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.