Improve DPI matching & scaling logic.

remoting/webapp/crd/js/client_session.js

Index: remoting/webapp/crd/js/client_session.js
diff --git a/remoting/webapp/crd/js/client_session.js b/remoting/webapp/crd/js/client_session.js
index 73bf54e140d656e608692780f81aaf345da29b51..3918e13bd90b543a1d7d2a013f3a8defba50e742 100644
--- a/remoting/webapp/crd/js/client_session.js
+++ b/remoting/webapp/crd/js/client_session.js
@@ -1244,43 +1244,115 @@ remoting.ClientSession.prototype.updateDimensions = function() {
     return;
   }
 
-  var clientArea = this.getClientArea_();
-  var desktopWidth = this.plugin_.getDesktopWidth();
-  var desktopHeight = this.plugin_.getDesktopHeight();
-
-  // When configured to display a host at its original size, we aim to display
-  // it as close to its physical size as possible, without losing data:
-  // - If client and host have matching DPI, render the host pixel-for-pixel.
-  // - If the host has higher DPI then still render pixel-for-pixel.
-  // - If the host has lower DPI then let Chrome up-scale it to natural size.
-
-  // We specify the plugin dimensions in Density-Independent Pixels, so to
-  // render pixel-for-pixel we need to down-scale the host dimensions by the
-  // devicePixelRatio of the client. To match the host pixel density, we choose
-  // an initial scale factor based on the client devicePixelRatio and host DPI.
-
-  // Determine the effective device pixel ratio of the host, based on DPI.
-  var hostPixelRatioX = Math.ceil(this.plugin_.getDesktopXDpi() / 96);
-  var hostPixelRatioY = Math.ceil(this.plugin_.getDesktopYDpi() / 96);
+  var desktopSize = { width: this.plugin_.getDesktopWidth(),
+                      height: this.plugin_.getDesktopHeight() };
+  var desktopDpi = { x: this.plugin_.getDesktopXDpi(),
+                     y: this.plugin_.getDesktopYDpi() };
+  var newSize = remoting.ClientSession.choosePluginSize(
+      this.getClientArea_(), window.devicePixelRatio,
+      desktopSize, desktopDpi, this.desktopScale_,
+      remoting.fullscreen.isActive(), this.shrinkToFit_);
+
+  // Resize the plugin if necessary.
+  // TODO(wez): Handle high-DPI to high-DPI properly (crbug.com/135089).
+  this.plugin_.element().style.width = newSize.width + 'px';
+  this.plugin_.element().style.height = newSize.height + 'px';
+
+  // Position the container.
+  // Note that clientWidth/Height take into account scrollbars.
+  var clientWidth = document.documentElement.clientWidth;
+  var clientHeight = document.documentElement.clientHeight;
+  var parentNode = this.plugin_.element().parentNode;
+
+  console.log('plugin dimensions: ' +
+              parentNode.style.left + ',' +
+              parentNode.style.top + '-' +
+              newSize.width + 'x' + newSize.height + '.');
+
+  // When we receive the first plugin dimensions from the host, we know that
+  // remote host has started.
+  remoting.app.onVideoStreamingStarted();
+}
+
+/**
+ * Helper function accepting client and host dimensions, and returning a chosen
+ * size for the plugin element, in DIPs.
+ *
+ * @param {{width: number, height: number}} clientSizeDips Available client
+ *     dimensions, in DIPs.
+ * @param {number} clientPixelRatio Number of physical pixels per client DIP.
+ * @param {{width: number, height: number}} desktopSize Size of the host desktop
+ *     in physical pixels.
+ * @param {{x: number, y: number}} desktopDpi DPI of the host desktop in both
+ *     dimensions.
+ * @param {number} desktopScale The scale factor configured for the host.
+ * @param {boolean} isFullscreen True if full-screen mode is active.
+ * @param {boolean} shrinkToFit True if shrink-to-fit should be applied.
+ * @return {{width: number, height: number}}
+ */
+remoting.ClientSession.choosePluginSize = function(
+    clientSizeDips, clientPixelRatio, desktopSize, desktopDpi, desktopScale,
+    isFullscreen, shrinkToFit) {

[Jamie, 2015/02/04 23:18:30]

Maybe assert that any value used as a numerator (or even all values, if that
makes sense) is not zero.

[Wez, 2015/02/06 01:29:42]

Done.

+  // We have the following goals in sizing the desktop display at the client:
+  //  1. Avoid losing detail by down-scaling beyond 1:1 host:device pixels.
+  //  2. Avoid up-scaling if that will cause the client to need scrollbars.
+  //  3. Avoid introducing blurriness with non-integer up-scaling factors.
+  //  4. Avoid having huge "letterboxes" around the desktop, if it's really
+  //     small.

[Sergey Ulanov, 2015/02/04 23:57:41]

I think another goal here should also be to render the screen to match physical
screen size of the host. Without it we can just ignore desktopDpi received from
the host and still satisfy these goals.

[Wez, 2015/02/06 01:29:42]

Exactly; the DPI is only included as part of calculating the "ideal" size,
below.

+  //
+  // To determine the ideal size we follow a four-stage process:
+  //  1. Determine the "natural" size at which to display the desktop.
+  //    a. Initially assume 1:1 mapping of desktop to client device pixels.
+  //    b. If host DPI is less than the client's then up-scale accordingly.
+  //    c. If desktopScale is configured for the host then allow that to
+  //       cancel-out some or all of the up-scaling from (b).

[Jamie, 2015/02/04 23:18:29]

It's not clear what "cancel-out" means in this context. Some examples would
help.

[Wez, 2015/02/06 01:29:42]

Done.

+  //  2. If the natural size of the desktop is smaller than the client device
+  //     then apply up-scaling by an integer scale factor to avoid excessive
+  //     letterboxing.
+  //  3. If shrink-to-fit is configured, and the natural size exceeds the
+  //     client size then apply down-scaling by an arbitrary scale factor.
+  //  4. If the overall scale factor is fractionally over an integer factor
+  //     then reduce it to that integer factor, to avoid blurring.
+
+  // All calculations are performed in device pixels.
+  var clientWidth = clientSizeDips.width * clientPixelRatio;
+  var clientHeight = clientSizeDips.height * clientPixelRatio;
+
+  // 1. Determine a "natural" size at which to display the desktop.
+  var scale = 1.0;
+
+  // Determine the effective host device pixel ratio.
+  var hostPixelRatioX = Math.ceil(desktopDpi.x / 96);

[Sergey Ulanov, 2015/02/04 23:57:41]

can this be Math.floor() instead of Math.ceil()? That way we won't be rendering
low-DPI image on hi-DPI display without upscaling, while still avoiding blurring
when it is possible. IMO it's better to blur the image while upscaling instead
of rendering it small and unreadable.

[Wez, 2015/02/06 01:29:42]

We should actually be able to remove the rounding entirely now that the
snap-to-blurrless is implemented further down, but I'd prefer to make that
change separately.

It's also the case that the larger vs blurry trade-off is subjective, so we
should experiment rather than just changing the behaviour in this CL.

Finally, there's no trade-off in the case of a host having been resized-to-fit
the client - in that case it should end up resized down to dimensions that can
be up-scaled by an integer factor at the client. Step #2 performs further
up-scaling if the host is still "naturally" smaller than the client after taking
DPI into account.

+  var hostPixelRatioY = Math.ceil(desktopDpi.y / 96);
   var hostPixelRatio = Math.min(hostPixelRatioX, hostPixelRatioY);
 
-  // Include the desktopScale in the hostPixelRatio before comparing it with
-  // the client devicePixelRatio to determine the "natural" scale to use.
-  hostPixelRatio *= this.desktopScale_;
+  // Allow up-scaling to account for DPI.
+  scale = Math.max(scale, clientPixelRatio / hostPixelRatio);
 
-  // Down-scale by the smaller of the client and host ratios.
-  var scale = 1.0 / Math.min(window.devicePixelRatio, hostPixelRatio);
+  // Allow some or all of the up-scaling to be cancelled by the desktopScale.
+  if (desktopScale > 1.0) {
+    scale = Math.max(1.0, scale / desktopScale);
+  }
 
-  if (this.shrinkToFit_) {
-    // Reduce the scale, if necessary, to fit the whole desktop in the window.
-    var scaleFitWidth = Math.min(scale, 1.0 * clientArea.width / desktopWidth);
+  // 2. Up-scale to avoid excessive letterboxing, if necessary.
+  if (desktopSize.width * scale <= clientWidth &&
+      desktopSize.height * scale <= clientHeight) {
+    var scaleX = Math.floor(clientWidth / desktopSize.width);
+    var scaleY = Math.floor(clientHeight / desktopSize.height);
+    scale = Math.min(scaleX, scaleY);

[Jamie, 2015/02/04 23:18:30]

It's not immediately obvious from reading the code that scale will be >= 1.0. I
think an assert would document that nicely.

[Wez, 2015/02/06 01:29:42]

Done.

+  }
+
+  // 3. Apply shrink-to-fit, if configured.
+  if (shrinkToFit) {
+    var scaleFitWidth = Math.min(scale, 1.0 * clientWidth / desktopSize.width);
     var scaleFitHeight =
-        Math.min(scale, 1.0 * clientArea.height / desktopHeight);
+        Math.min(scale, 1.0 * clientHeight / desktopSize.height);

[Jamie, 2015/02/04 23:18:30]

I'm not sure what the "1.0 *" was doing in the original code, but I don't think
it's needed.

[Wez, 2015/02/06 01:29:42]

D'oh; thanks for spotting that!

     scale = Math.min(scaleFitHeight, scaleFitWidth);
 
+    // TODO(wez): Fix multi-monitor and wide/tall desktop handling.
     // If we're running full-screen then try to handle common side-by-side
     // multi-monitor combinations more intelligently.
-    if (remoting.fullscreen.isActive()) {
+    if (isFullscreen) {
       // If the host has two monitors each the same size as the client then
       // scale-to-fit will have the desktop occupy only 50% of the client area,
       // in which case it would be preferable to down-scale less and let the
@@ -1298,29 +1370,20 @@ remoting.ClientSession.prototype.updateDimensions = function() {
     }
   }
 
-  var pluginWidth = Math.round(desktopWidth * scale);
-  var pluginHeight = Math.round(desktopHeight * scale);
-
-  // Resize the plugin if necessary.
-  // TODO(wez): Handle high-DPI to high-DPI properly (crbug.com/135089).
-  this.plugin_.element().style.width = pluginWidth + 'px';
-  this.plugin_.element().style.height = pluginHeight + 'px';
-
-  // Position the container.
-  // Note that clientWidth/Height take into account scrollbars.
-  var clientWidth = document.documentElement.clientWidth;
-  var clientHeight = document.documentElement.clientHeight;
-  var parentNode = this.plugin_.element().parentNode;
-
-  console.log('plugin dimensions: ' +
-              parentNode.style.left + ',' +
-              parentNode.style.top + '-' +
-              pluginWidth + 'x' + pluginHeight + '.');
+  // 4. Avoid blurring for close-to-integer up-scaling factors.
+  if (scale > 1.0) {
+    var scaleBlurriness = scale / Math.floor(scale);
+    if (scaleBlurriness < 1.1) {

[Jamie, 2015/02/04 23:18:30]

Optional: I wonder if this would be better expressed as a difference rather than
a ratio? In other words, sacrifice up to x pixels horizontally and vertically in
order to make the image less blurred.

[Wez, 2015/02/06 01:29:42]

I considered that, but I think a ratio makes more sense; if you consider the
difference between 640x480 and 2560x1600, clearly the point at which so much
space is being wasted it's better to allow blurring isn't the same - a 16px
border around 640x480 is a lot more wasted space than it is around 2560x1600.

+      scale = Math.floor(scale);
+    }
+  }
 
-  // When we receive the first plugin dimensions from the host, we know that
-  // remote host has started.
-  remoting.app.onVideoStreamingStarted();
-};
+  // Return the necessary plugin dimensions in DIPs.
+  scale = scale / clientPixelRatio;
+  var pluginWidth = Math.round(desktopSize.width * scale);
+  var pluginHeight = Math.round(desktopSize.height * scale);
+  return { width: pluginWidth, height: pluginHeight };
+}
 
 /**
  * Returns an associative array with a set of stats for this connection.