Improve list of display zoom values

This patch changes the logic the list of zoom values are computed for a
given display. The new logic has 2 scenarios for listing the zoom values
for a given display.

1) Displays with device scale factors assigned to them, will now have
   zoom values ranging from the inverse of device scale factor to
   device scale factor. If there are still slider ticks avaiable we use
   them to add zoom levels beyond the device scale factor.
   Doing this allows the user to go to the native resolution of the
   display and on the other hand it also allows them to zoom in if
   required. We no longer allow the user to go to a zoom below the native
   resolution of the display as this is a very unlikely scenario and
   introduces artifacts. This also gives finer control to the user in
   setting the zoom level.
   How this effects a pixelbook for example?
   On pixelbook we used to have a zoom range of 50% to 175%. Due to the
   wide range, the consecutive values were too far apart and users wanted
   zoom values that were mostly in the range of 70% to 100%.
   With the new change, the range will go from 50% to 130%.

2) Displays with no device scale factors assigned to them will use a
   static list of initialized zoom values.

Bug: 845634
Change-Id: I69a761856dab4e5b37b85420f6f6dfebdb2dead5
Component: Display zoom, display util
Reviewed-on: https://chromium-review.googlesource.com/1069561
Commit-Queue: Malay Keshav <malaykeshav@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563050}

ash/display/cros_display_config_unittest.cc

@@ -396,15 +396,15 @@ TEST_F(CrosDisplayConfigTest, GetDisplayUnitInfoListZoomFactor) {
   const std::vector<double>& zoom_factors =
       info_0.available_display_zoom_factors;
   EXPECT_EQ(9u, zoom_factors.size());
-  EXPECT_FLOAT_EQ(0.6, zoom_factors[0]);
-  EXPECT_FLOAT_EQ(0.7, zoom_factors[1]);
-  EXPECT_FLOAT_EQ(0.8, zoom_factors[2]);
-  EXPECT_FLOAT_EQ(0.9, zoom_factors[3]);
-  EXPECT_FLOAT_EQ(1.0, zoom_factors[4]);
-  EXPECT_FLOAT_EQ(1.1, zoom_factors[5]);
-  EXPECT_FLOAT_EQ(1.2, zoom_factors[6]);
-  EXPECT_FLOAT_EQ(1.3, zoom_factors[7]);
-  EXPECT_FLOAT_EQ(1.4, zoom_factors[8]);
+  EXPECT_FLOAT_EQ(0.90f, zoom_factors[0]);
+  EXPECT_FLOAT_EQ(0.95f, zoom_factors[1]);
+  EXPECT_FLOAT_EQ(1.f, zoom_factors[2]);
+  EXPECT_FLOAT_EQ(1.05f, zoom_factors[3]);
+  EXPECT_FLOAT_EQ(1.10f, zoom_factors[4]);
+  EXPECT_FLOAT_EQ(1.15f, zoom_factors[5]);
+  EXPECT_FLOAT_EQ(1.20f, zoom_factors[6]);
+  EXPECT_FLOAT_EQ(1.25f, zoom_factors[7]);
+  EXPECT_FLOAT_EQ(1.30f, zoom_factors[8]);
 }
 
 TEST_F(CrosDisplayConfigTest, SetDisplayPropertiesPrimary) {

ui/display/manager/display_util.cc

@@ -6,6 +6,7 @@
 
 #include <stddef.h>
 #include <algorithm>
+#include <cmath>
 
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
@@ -16,44 +17,38 @@
 namespace display {
 namespace {
 
-// The maximum logical resolution width allowed when zooming out for a display.
-constexpr int kDefaultMaxZoomWidth = 4096;
-
-// The minimum logical resolution width allowed when zooming in for a display.
-constexpr int kDefaultMinZoomWidth = 640;
-
 // The total number of display zoom factors to enumerate.
 constexpr int kNumOfZoomFactors = 9;
 
+// A pair representing the list of zoom values for a given minimum display
+// resolution width.
+using ZoomListBucket = std::pair<int, std::array<float, kNumOfZoomFactors>>;
+
+// For displays with a device scale factor of unity, we use a static list of
+// initialized zoom values. For a given resolution width of a display, we can
+// find its associated list of zoom values by simply finding the last bucket
+// with a width less than the given resolution width.
+// Ex. A resolution width of 1024, we will use the bucket with the width of 960.
+constexpr std::array<ZoomListBucket, 8> kZoomListBuckets{{
+    {0, {0.60f, 0.65f, 0.70f, 0.75f, 0.80f, 0.85f, 0.90f, 0.95f, 1.f}},
+    {720, {0.70f, 0.75f, 0.80f, 0.85f, 0.90f, 0.95f, 1.f, 1.05f, 1.10f}},
+    {800, {0.75f, 0.80f, 0.85f, 0.90f, 0.95f, 1.f, 1.05f, 1.10f, 1.15f}},
+    {960, {0.90f, 0.95f, 1.f, 1.05f, 1.10f, 1.15f, 1.20f, 1.25f, 1.30f}},
+    {1280, {1.f, 1.10f, 1.15f, 1.20f, 1.25f, 1.30f, 1.50f, 1.70f, 1.80f}},
+    {1920, {1.f, 1.10f, 1.15f, 1.20f, 1.30f, 1.40f, 1.50f, 1.75f, 2.00f}},
+    {3840, {1.f, 1.10f, 1.20f, 1.40f, 1.60f, 1.80f, 2.00f, 2.20f, 2.40f}},
+    {5120, {1.f, 1.25f, 1.50f, 1.75f, 2.00f, 2.25f, 2.50f, 2.75f, 3.00f}},
+}};
+
 bool WithinEpsilon(float a, float b) {
   return std::abs(a - b) < std::numeric_limits<float>::epsilon();
 }
 
-// Clamps the delta between consecutive zoom factors to a user friendly and UI
-// friendly value.
-float ClampToUserFriendlyDelta(float delta) {
-  // NOTE: If these thresholds are updated, please also update aura-shell.xml.
-  // The list of deltas between two consecutive zoom level. Any display must
-  // have one of these values as the difference between two consecutive zoom
-  // level.
-  // The array of pair represents which user friendly delta value must the given
-  // raw |delta| be clamped to.
-  // std::pair::first - represents the threshold.
-  // std::pair::second - represents the user friendly clamped delta
-  constexpr std::array<std::pair<float, float>, 7> kZoomFactorDeltas = {
-      {{0.05f, 0.05f},
-       {0.1f, 0.1f},
-       {0.15f, 0.15f},
-       {0.2f, 0.2f},
-       {0.25f, 0.25f},
-       {0.7f, 0.3f},
-       {1.f, 0.5f}}};
-  std::size_t delta_index = 0;
-  while (delta_index < kZoomFactorDeltas.size() &&
-         delta >= kZoomFactorDeltas[delta_index].first) {
-    delta_index++;
-  }
-  return kZoomFactorDeltas[delta_index - 1].second;
+// Returns the user friendly delta to be used for the given |dsf|.
+float FindDeltaForDsf(float dsf) {
+  DCHECK_GT(dsf, 1.f);
+  const float raw_delta = (1.f - 1.f / dsf) / (kNumOfZoomFactors - 1.f);
+  return raw_delta > 0.05f ? 0.1f : 0.05f;
 }
 
 }  // namespace
@@ -117,59 +112,60 @@ bool IsPhysicalDisplayType(DisplayConnectionType type) {
 }
 
 std::vector<float> GetDisplayZoomFactors(const ManagedDisplayMode& mode) {
+  // Internal displays have an internal device scale factor greater than 1
+  // associated with them. This means that if we use the usual logic, we would
+  // end up with a list of zoom levels that the user may not find very useful.
+  // Take for example the pixelbook with device scale factor of 2. Based on the
+  // usual approach, we would get a zoom range of 100% to 180% with a step of
+  // 10% between each consecutive levels. This means:
+  //   1. Users will not be able to go to the native resolution which is
+  //      achieved at 50% zoom level.
+  //   2. Due to the device scale factor, the display already has a low DPI and
+  //      users dont want to zoom in, they mostly want to zoom out and add more
+  //      pixels to the screen. But we only provide a zoom list of 90% to 130%.
+  // This clearly shows we need a different logic to handle internal displays
+  // which have lower DPI due to the device scale factor associated with them.
+  //
+  // OTOH if we look at an external display with a device scale factor of 1 but
+  // the same resolution as the pixel book, the DPI would usually be very high
+  // and users mostly want to zoom in to reduce the number of pixels on the
+  // screen. So having a range of 90% to 130% makes sense.
+  // TODO(malaykeshav): Investigate if we can use DPI instead of resolution or
+  // device scale factor to decide the list of zoom levels.
+  if (mode.device_scale_factor() > 1.f)
+    return GetDisplayZoomFactorForDsf(mode.device_scale_factor());
+
+  // There may be cases where the device scale factor is less than 1. This can
+  // happen during testing or local linux builds.
   const int effective_width = std::round(
       static_cast<float>(mode.size().width()) / mode.device_scale_factor());
 
-  // We want to support displays greater than 4K. This is added to ensure the
-  // zoom does not break in such cases.
-  const int max_width = std::max(effective_width, kDefaultMaxZoomWidth);
-  const int min_width = std::min(effective_width, kDefaultMinZoomWidth);
-
-  // The logical resolution will vary from half of the mode resolution to double
-  // the mode resolution.
-  int max_effective_width =
-      std::min(static_cast<int>(std::round(effective_width * 2.f)), max_width);
-  int min_effective_width =
-      std::max(static_cast<int>(std::round(effective_width / 2.f)), min_width);
-
-  // If either the maximum width or minimum width was reached in the above step
-  // and clamping was performed, then update the total range of logical
-  // resolutions and ensure that everything lies within the maximum and minimum
-  // resolution range.
-  const int interval = std::round(static_cast<float>(effective_width) * 1.5f);
-  if (max_effective_width == max_width)
-    min_effective_width = std::max(max_effective_width - interval, min_width);
-  if (min_effective_width == min_width)
-    max_effective_width = std::min(min_effective_width + interval, max_width);
-
-  float max_zoom = static_cast<float>(effective_width) /
-                   static_cast<float>(min_effective_width);
-  float min_zoom = static_cast<float>(effective_width) /
-                   static_cast<float>(max_effective_width);
-
-  float delta =
-      (max_zoom - min_zoom) / static_cast<float>(kNumOfZoomFactors - 1);
-
-  // Number of zoom values above 100% zoom.
-  const int zoom_in_count = std::round((max_zoom - 1.f) / delta);
-
-  // Number of zoom values below 100% zoom.
-  const int zoom_out_count = kNumOfZoomFactors - zoom_in_count - 1;
-
-  delta = ClampToUserFriendlyDelta(delta);
-
-  // Populate the zoom values list.
-  min_zoom = 1.f - delta * zoom_out_count;
+  std::size_t index = kZoomListBuckets.size() - 1;
+  while (index > 0 && effective_width < kZoomListBuckets[index].first)
+    index--;
+  DCHECK_GE(effective_width, kZoomListBuckets[index].first);
+
+  const auto& zoom_array = kZoomListBuckets[index].second;
+  return std::vector<float>(zoom_array.begin(), zoom_array.end());
+}
+
+std::vector<float> GetDisplayZoomFactorForDsf(float dsf) {
+  DCHECK(!WithinEpsilon(dsf, 1.f));
+  DCHECK_GT(dsf, 1.f);
+
+  const float delta = FindDeltaForDsf(dsf);
+  const float inverse_dsf = 1.f / dsf;
+  int zoom_out_count = std::ceil((1.f - inverse_dsf) / delta);
+  zoom_out_count = std::min(zoom_out_count, kNumOfZoomFactors - 1);
+
+  const float min_zoom = 1.f - delta * zoom_out_count;
+
   std::vector<float> zoom_values;
   for (int i = 0; i < kNumOfZoomFactors; i++)
     zoom_values.push_back(min_zoom + i * delta);
 
-  // Make sure the inverse of the internal device scale factor is included in
-  // the list. This ensures that the users have a way to go to the native
-  // resolution and 1.0 effective device scale factor.
-  InsertDsfIntoList(&zoom_values, 1.f / mode.device_scale_factor());
-
-  DCHECK_EQ(zoom_values.size(), static_cast<std::size_t>(kNumOfZoomFactors));
+  // Ensure the inverse dsf is in the list.
+  zoom_values[0] = inverse_dsf;
   return zoom_values;
 }
 
@@ -194,7 +190,7 @@ void InsertDsfIntoList(std::vector<float>* zoom_values, float dsf) {
 
   // We dont need to add |dsf| to the list if it is already in the list.
   auto it = std::lower_bound(zoom_values->begin(), zoom_values->end(), dsf);
-  if (WithinEpsilon(*it, dsf))
+  if (it != zoom_values->end() && WithinEpsilon(*it, dsf))
     return;
 
   if (it == zoom_values->begin()) {

ui/display/manager/display_util.h

@@ -40,6 +40,11 @@ bool IsPhysicalDisplayType(DisplayConnectionType type);
 std::vector<float> DISPLAY_MANAGER_EXPORT
 GetDisplayZoomFactors(const ManagedDisplayMode& mode);
 
+// Returns a list of display zooms based on the provided |dsf| of the display.
+// This is useful for displays that have a non unity device scale factors
+// applied to them.
+std::vector<float> DISPLAY_MANAGER_EXPORT GetDisplayZoomFactorForDsf(float dsf);
+
 // This function adds |dsf| to the vector of |zoom_values| by replacing
 // the element it is closest to in the list. It also ensures that it never
 // replaces the default zoom value of 1.0 from the list and that the size of the

ui/display/manager/display_utils_unittest.cc

@@ -16,127 +16,43 @@ namespace test {
 
 namespace {
 constexpr std::size_t kNumOfZoomFactors = 9;
-constexpr int kDefaultMaxZoomWidth = 4096;
-constexpr int kDefaultMinZoomWidth = 640;
-
-// Returns true if |a| and |b| are equal within the error limits.
-bool WithinEpsilon(float a, float b) {
-  return std::abs(a - b) < std::numeric_limits<float>::epsilon();
-}
+using ZoomListBucket = std::pair<int, std::array<float, kNumOfZoomFactors>>;
 
 }  // namespace
 using DisplayUtilTest = testing::Test;
 
 TEST_F(DisplayUtilTest, DisplayZooms) {
-  // A vector of pairs where each pair is the resolution width correspoinding
-  // to its list of available display zoom values.
-  const std::vector<std::pair<int, std::vector<float>>> expected_zoom_values{
-      {480, {0.60f, 0.65f, 0.70f, 0.75f, 0.80f, 0.85f, 0.90f, 0.95f, 1.f}},
-      {640, {0.60f, 0.65f, 0.70f, 0.75f, 0.80f, 0.85f, 0.90f, 0.95f, 1.f}},
-      {720, {0.65f, 0.70f, 0.75f, 0.80f, 0.85f, 0.90f, 0.95f, 1.f, 1.05f}},
-      {800, {0.40f, 0.50f, 0.60f, 0.70f, 0.80f, 0.90f, 1.f, 1.10f, 1.20f}},
-      {960, {0.60f, 0.70f, 0.80f, 0.90f, 1.f, 1.10f, 1.20f, 1.30f, 1.40f}},
-      {1024, {0.60f, 0.70f, 0.80f, 0.90f, 1.f, 1.10f, 1.20f, 1.30f, 1.40f}},
-      {1280, {0.55f, 0.70f, 0.85f, 1.f, 1.15f, 1.30f, 1.45f, 1.60f, 1.75f}},
-      {1366, {0.55f, 0.70f, 0.85f, 1.f, 1.15f, 1.30f, 1.45f, 1.60f, 1.75f}},
-      {1440, {0.55f, 0.70f, 0.85f, 1.f, 1.15f, 1.30f, 1.45f, 1.60f, 1.75f}},
-      {1600, {0.55f, 0.70f, 0.85f, 1.f, 1.15f, 1.30f, 1.45f, 1.60f, 1.75f}},
-      {1920, {0.55f, 0.70f, 0.85f, 1.f, 1.15f, 1.30f, 1.45f, 1.60f, 1.75f}},
-      {2160, {0.60f, 0.80f, 1.f, 1.20f, 1.40f, 1.60f, 1.80f, 2.00f, 2.20f}},
-      {2400, {0.75f, 1.f, 1.25f, 1.50f, 1.75f, 2.00f, 2.25f, 2.50f, 2.75f}},
-      {2560, {0.75f, 1.f, 1.25f, 1.50f, 1.75f, 2.00f, 2.25f, 2.50f, 2.75f}},
-      {2880, {0.75f, 1.f, 1.25f, 1.50f, 1.75f, 2.00f, 2.25f, 2.50f, 2.75f}},
-      {3200, {1.f, 1.25f, 1.50f, 1.75f, 2.00f, 2.25f, 2.50f, 2.75f, 3.00f}},
-      {3840, {1.f, 1.25f, 1.50f, 1.75f, 2.00f, 2.25f, 2.50f, 2.75f, 3.00f}},
-      {4096, {1.f, 1.25f, 1.50f, 1.75f, 2.00f, 2.25f, 2.50f, 2.75f, 3.00f}},
-      {5120, {1.f, 1.30f, 1.60f, 1.90f, 2.20f, 2.50f, 2.80f, 3.10f, 3.40f}},
-      {7680, {1.f, 1.50f, 2.00f, 2.50f, 3.00f, 3.50f, 4.00f, 4.50f, 5.00f}},
-      {8192, {1.f, 1.50f, 2.00f, 2.50f, 3.00f, 3.50f, 4.00f, 4.50f, 5.00f}},
-  };
-
-  for (const auto& pair : expected_zoom_values) {
-    const int size = pair.first;
-    ManagedDisplayMode mode(gfx::Size(size, size), 60, false, true, 1.f, 1.f);
+  // The expected zoom list for the width given by |first| of the pair should be
+  //  equal to the |second| of the pair.
+  constexpr std::array<ZoomListBucket, 4> kTestData{{
+      {240, {0.60f, 0.65f, 0.70f, 0.75f, 0.80f, 0.85f, 0.90f, 0.95f, 1.f}},
+      {720, {0.70f, 0.75f, 0.80f, 0.85f, 0.90f, 0.95f, 1.f, 1.05f, 1.10f}},
+      {1024, {0.90f, 0.95f, 1.f, 1.05f, 1.10f, 1.15f, 1.20f, 1.25f, 1.30f}},
+      {2400, {1.f, 1.10f, 1.15f, 1.20f, 1.30f, 1.40f, 1.50f, 1.75f, 2.00f}},
+  }};
+  for (const auto& data : kTestData) {
+    ManagedDisplayMode mode(gfx::Size(data.first, data.first), 60, false, true,
+                            1.f, 1.f);
     const std::vector<float> zoom_values = GetDisplayZoomFactors(mode);
-    EXPECT_FLOAT_EQ(zoom_values.size(), kNumOfZoomFactors);
-    for (std::size_t j = 0; j < kNumOfZoomFactors; j++) {
-      EXPECT_FLOAT_EQ(zoom_values[j], pair.second[j]);
-
-      // Display pref stores the zoom value as double. This check ensures that
-      // the values dont lose precision.
-      // Before changing this line please ensure that you have updated
-      // chromeos/display/display_prefs.cc
-      double double_value = static_cast<double>(zoom_values[j]);
-      float float_value = static_cast<float>(double_value);
-      EXPECT_FLOAT_EQ(zoom_values[j], float_value);
-    }
-
-    const int effective_minimum_width_possible = size / zoom_values.back();
-    const int effective_maximum_width_possible = size * zoom_values.front();
-
-    const int allowed_minimum_width = std::min(kDefaultMinZoomWidth, size);
-    const int allowed_maximum_width = std::max(kDefaultMaxZoomWidth, size);
-
-    EXPECT_GE(effective_minimum_width_possible, allowed_minimum_width);
-    EXPECT_LE(effective_maximum_width_possible, allowed_maximum_width);
+    for (std::size_t j = 0; j < kNumOfZoomFactors; j++)
+      EXPECT_FLOAT_EQ(zoom_values[j], data.second[j]);
   }
 }
 
 TEST_F(DisplayUtilTest, DisplayZoomsWithInternalDsf) {
-  const std::vector<int> sizes = {1280, 1366, 1440, 1600, 1920,
-                                  2160, 2400, 2560, 2880, 3200,
-                                  3840, 4096, 5120, 7680, 8192};
-
-  const std::vector<float> dsfs = {1.25f, 1.5f, 1.6f, 1.8f, 2.f, 2.25f};
+  const std::vector<std::pair<float, std::vector<float>>> expected_zoom_values{
+      {1.25f, {1.f / 1.25f, 0.85f, 0.9f, 0.95f, 1.f, 1.05f, 1.1f, 1.15f, 1.2f}},
+      {1.5f, {1.f / 1.5f, 0.7f, 0.75f, 0.8f, 0.85f, 0.9f, 0.95f, 1.f, 1.05f}},
+      {1.6f, {1.f / 1.6f, 0.65f, 0.7f, 0.75f, 0.8f, 0.85f, 0.9f, 0.95f, 1.f}},
+      {1.8f, {1.f / 1.8f, 0.6f, 0.7f, 0.8f, 0.9f, 1.f, 1.1f, 1.2f, 1.3f}},
+      {2.f, {1.f / 2.f, 0.6f, 0.7f, 0.8f, 0.9f, 1.f, 1.1f, 1.2f, 1.3f}},
+      {2.25f, {1.f / 2.25f, 0.5f, 0.6f, 0.7f, 0.8f, 0.9f, 1.f, 1.1f, 1.2f}}};
 
-  for (float dsf : dsfs) {
-    for (int size : sizes) {
-      ManagedDisplayMode mode(gfx::Size(size, size), 60, false, true, 1.f, dsf);
-      const std::vector<float> zoom_values = GetDisplayZoomFactors(mode);
-
-      const int effective_size = std::round(static_cast<float>(size) / dsf);
-      ManagedDisplayMode expected_mode(
-          gfx::Size(effective_size, effective_size), 60, false, true, 1.f, 1.f);
-      const std::vector<float> expected_zoom_values =
-          GetDisplayZoomFactors(expected_mode);
-      EXPECT_EQ(zoom_values.size(), kNumOfZoomFactors);
-
-      // There should be exactly 1 entry for the inverse dsf in the list.
-      const float inverse_dsf = 1.f / dsf;
-      const std::size_t count_of_inverse_dsf =
-          std::count_if(zoom_values.begin(), zoom_values.end(),
-                        [inverse_dsf](float zoom_value) -> bool {
-                          return WithinEpsilon(zoom_value, inverse_dsf);
-                        });
-      EXPECT_EQ(count_of_inverse_dsf, 1UL);
-
-      // Check all values in |zoom_values| are present in |expected_zoom_values|
-      // except for |inverse_dsf|.
-      auto it = expected_zoom_values.begin();
-      for (const auto& zoom_value : zoom_values) {
-        // |inverse_dsf| is not expected to be in the expected list since it was
-        // added later on.
-        if (WithinEpsilon(zoom_value, inverse_dsf))
-          continue;
-
-        // Check if |zoom_value| is present in |expected_zoom_values|.
-        it = std::find_if(it, expected_zoom_values.end(),
-                          [&zoom_value](float expected_zoom) -> bool {
-                            return WithinEpsilon(zoom_value, expected_zoom);
-                          });
-        EXPECT_TRUE(it != expected_zoom_values.end())
-            << zoom_value << " not found";
-      }
-
-      const int effective_minimum_width_possible = size / zoom_values.back();
-      const int effective_maximum_width_possible = size * zoom_values.front();
-
-      const int allowed_minimum_width = std::min(kDefaultMinZoomWidth, size);
-      const int allowed_maximum_width = std::max(kDefaultMaxZoomWidth, size);
-
-      EXPECT_GE(effective_minimum_width_possible, allowed_minimum_width);
-      EXPECT_LE(effective_maximum_width_possible, allowed_maximum_width);
-    }
+  for (const auto& pair : expected_zoom_values) {
+    const std::vector<float> zoom_values =
+        GetDisplayZoomFactorForDsf(pair.first);
+    for (std::size_t j = 0; j < kNumOfZoomFactors; j++)
+      EXPECT_FLOAT_EQ(zoom_values[j], pair.second[j]);
   }
 }