Add VideoLayout message.

Add VideoLayout message.

Added new VideoLayout message that's sent from host to client when
using WebRTC protocol. The message is used to notify the client about
screen configuration. Currently both host and client support only a
single video stream, but in the future the same VideoLayout message
can be used to specify layout of multiple video streams.
When using ICE protocol the message is not sent and instead it's
generated on the client side by ClientVideoDispatcher.

Committed: https://crrev.com/00a67b14ac400858d86fd572bf46490e22ff0f25
Cr-Commit-Position: refs/heads/master@{#384440}

[Sergey Ulanov, 2016-03-30 23:25:55]

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

[Jamie, 2016-03-31 00:35:53]

https://codereview.chromium.org/1844143002/diff/20001/remoting/client/chromot...
File remoting/client/chromoting_client.cc (right):

https://codereview.chromium.org/1844143002/diff/20001/remoting/client/chromot...
remoting/client/chromoting_client.cc:153: if (layout.video_track_size() < 1) {
I assume that the name video_track_size is auto-generated because there's a
repeated field called video_track? It's an unfortunate choice of name because
the size of the video track would typically be expressed as (width, height).
Perhaps assign this to a variable called something like num_video_tracks so that
the code is a bit more self-documenting?

https://codereview.chromium.org/1844143002/diff/20001/remoting/client/chromot...
remoting/client/chromoting_client.cc:169: track_layout.height() * y_dpi /
kDefaultDpi),
Why divide by kDefaultDpi? If it's to convert to DIPs, then the interface should
make it explicit that that's expected unit of measurement. It's also not obvious
why dividing by something called kDefaultDpi is the right way to convert px to
dips (https://en.wikipedia.org/wiki/Device_independent_pixel, for example,
suggests that 96 is the correct factor for Windows, but not Android).

https://codereview.chromium.org/1844143002/diff/20001/remoting/client/plugin/...
File remoting/client/plugin/chromoting_instance.cc (right):

https://codereview.chromium.org/1844143002/diff/20001/remoting/client/plugin/...
remoting/client/plugin/chromoting_instance.cc:488: data->SetInteger("y_dpi",
dpi.y());
Do these still need to be conditional? I don't think we ever pass a zero DPI.

https://codereview.chromium.org/1844143002/diff/20001/remoting/protocol/webrt...
File remoting/protocol/webrtc_video_capturer_adapter.h (right):

https://codereview.chromium.org/1844143002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_capturer_adapter.h:48: SizeCallback;
Do we need to define this twice?

[Sergey Ulanov, 2016-03-31 06:25:14]

https://codereview.chromium.org/1844143002/diff/20001/remoting/client/chromot...
File remoting/client/chromoting_client.cc (right):

https://codereview.chromium.org/1844143002/diff/20001/remoting/client/chromot...
remoting/client/chromoting_client.cc:153: if (layout.video_track_size() < 1) {
On 2016/03/31 00:35:53, Jamie wrote:
> I assume that the name video_track_size is auto-generated because there's a
> repeated field called video_track? It's an unfortunate choice of name because
> the size of the video track would typically be expressed as (width, height).
> Perhaps assign this to a variable called something like num_video_tracks so
that
> the code is a bit more self-documenting?

Done.

https://codereview.chromium.org/1844143002/diff/20001/remoting/client/chromot...
remoting/client/chromoting_client.cc:169: track_layout.height() * y_dpi /
kDefaultDpi),
On 2016/03/31 00:35:53, Jamie wrote:
> Why divide by kDefaultDpi? If it's to convert to DIPs, then the interface
should
> make it explicit that that's expected unit of measurement. 

It's to convert to physical pixels. Added a comment in client_user_interface.h
to make it clear that |size| is in physical pixels.

> It's also not obvious
> why dividing by something called kDefaultDpi is the right way to convert px to
> dips (https://en.wikipedia.org/wiki/Device_independent_pixel, for example,
> suggests that 96 is the correct factor for Windows, but not Android).

Currently everywhere in chromoting we use DPI=96 for standard resolution, 192 -
for hi-dpi ("Retina") screens. E.g. see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
. This is also consistent with CSS which defines pixel as 1/96 of an inch, see
https://www.w3.org/TR/css3-values/#absolute-lengths . Note that CSS spec defines
"reference pixel" in terms of visual angle. This definition it is compatible
with Android's definition of DIPs (1/96 inch on a desktop screen has about the
same visual size as 1/160 inch on a hand-held device). The CSS's definition of
"reference pixels" makes sense for our application as well. E.g. when connecting
from desktop to a host on android we wouldn't want to show the screen with the
same physical size as on the device itself. So I think we want to use the CSS
pixels as DIPs. We already do that in the webapp (while Android and iOS clients
ignored host DPI).

https://codereview.chromium.org/1844143002/diff/20001/remoting/client/plugin/...
File remoting/client/plugin/chromoting_instance.cc (right):

https://codereview.chromium.org/1844143002/diff/20001/remoting/client/plugin/...
remoting/client/plugin/chromoting_instance.cc:488: data->SetInteger("y_dpi",
dpi.y());
On 2016/03/31 00:35:53, Jamie wrote:
> Do these still need to be conditional? I don't think we ever pass a zero DPI.

Done.

https://codereview.chromium.org/1844143002/diff/20001/remoting/protocol/webrt...
File remoting/protocol/webrtc_video_capturer_adapter.h (right):

https://codereview.chromium.org/1844143002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_capturer_adapter.h:48: SizeCallback;
On 2016/03/31 00:35:53, Jamie wrote:
> Do we need to define this twice?

Removed this one now.

[Jamie, 2016-03-31 22:36:44]

lgtm

https://codereview.chromium.org/1844143002/diff/40001/remoting/client/chromot...
File remoting/client/chromoting_client.cc (right):

https://codereview.chromium.org/1844143002/diff/40001/remoting/client/chromot...
remoting/client/chromoting_client.cc:159: if (layout.video_track_size() > 2) {
Replace this call (and the next one) with num_video_tracks as well.

[Sergey Ulanov, 2016-03-31 23:05:25]

https://codereview.chromium.org/1844143002/diff/40001/remoting/client/chromot...
File remoting/client/chromoting_client.cc (right):

https://codereview.chromium.org/1844143002/diff/40001/remoting/client/chromot...
remoting/client/chromoting_client.cc:159: if (layout.video_track_size() > 2) {
On 2016/03/31 22:36:44, Jamie wrote:
> Replace this call (and the next one) with num_video_tracks as well.

Done.

[Sergey Ulanov, 2016-03-31 23:05:30]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-03-31 23:05:31]

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/1844143002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-03-31 23:06:23]

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

[commit-bot: I haz the power, 2016-04-01 00:07:09]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-01 00:09:40]

Description was changed from

==========
Add VideoLayout message.

Added new VideoLayout message that's sent from host to client when
using WebRTC protocol. The message is used to notify the client about
screen configuration. Currently both host and client support only a
single video stream, but in the future the same VideoLayout message
can be used to specify layout of multiple video streams.
When using ICE protocol the message is not sent and instead it's
generated on the client side by ClientVideoDispatcher.
==========

to

==========
Add VideoLayout message.

Added new VideoLayout message that's sent from host to client when
using WebRTC protocol. The message is used to notify the client about
screen configuration. Currently both host and client support only a
single video stream, but in the future the same VideoLayout message
can be used to specify layout of multiple video streams.
When using ICE protocol the message is not sent and instead it's
generated on the client side by ClientVideoDispatcher.

Committed: https://crrev.com/00a67b14ac400858d86fd572bf46490e22ff0f25
Cr-Commit-Position: refs/heads/master@{#384440}
==========

[commit-bot: I haz the power, 2016-04-01 00:09:41]

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