Add VideoLayout message.

remoting/client/chromoting_client.cc

Index: remoting/client/chromoting_client.cc
diff --git a/remoting/client/chromoting_client.cc b/remoting/client/chromoting_client.cc
index bed7215f84175b13e91b106ede5ae0265d6af8cd..d20c7e9646e0f2ac69834496537d0daf7f9e4809 100644
--- a/remoting/client/chromoting_client.cc
+++ b/remoting/client/chromoting_client.cc
@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "remoting/base/capabilities.h"
+#include "remoting/base/constants.h"
 #include "remoting/client/audio_decode_scheduler.h"
 #include "remoting/client/audio_player.h"
 #include "remoting/client/client_context.h"
@@ -22,6 +23,7 @@
 #include "remoting/protocol/video_renderer.h"
 #include "remoting/protocol/webrtc_connection_to_host.h"
 #include "remoting/signaling/jid_util.h"
+#include "third_party/webrtc/modules/desktop_capture/desktop_geometry.h"
 
 namespace remoting {
 
@@ -147,6 +149,27 @@ void ChromotingClient::DeliverHostMessage(
   user_interface_->DeliverHostMessage(message);
 }
 
+void ChromotingClient::SetVideoLayout(const protocol::VideoLayout& layout) {
+  if (layout.video_track_size() < 1) {

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

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?

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

Done.

+    LOG(ERROR) << "Received VideoLayout message with 0 tracks.";
+    return;
+  }
+
+  if (layout.video_track_size() > 2) {
+    LOG(WARNING) << "Received VideoLayout message with "
+                 << layout.video_track_size()
+                 << " tracks. Only one track is supported.";
+  }
+
+  const protocol::VideoTrackLayout& track_layout = layout.video_track(0);
+  int x_dpi = track_layout.has_x_dpi() ? track_layout.x_dpi() : kDefaultDpi;
+  int y_dpi = track_layout.has_y_dpi() ? track_layout.y_dpi() : kDefaultDpi;
+  user_interface_->SetDesktopSize(
+      webrtc::DesktopSize(track_layout.width() * x_dpi / kDefaultDpi,
+                          track_layout.height() * y_dpi / kDefaultDpi),

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

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).

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

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.
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).

+      webrtc::DesktopVector(x_dpi, y_dpi));
+}
+
 void ChromotingClient::InjectClipboardEvent(
     const protocol::ClipboardEvent& event) {
   DCHECK(thread_checker_.CalledOnValidThread());