Android: Adjust tile-size using 'real' resolution in JB-MR1

content/browser/gpu/gpu_data_manager_impl_private.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/gpu/gpu_data_manager_impl_private.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/debug/trace_event.h"
@@ skipping 229 matching lines @@
 void ApplyAndroidWorkarounds(const gpu::GPUInfo& gpu_info,
                              CommandLine* command_line) {
   std::string vendor(StringToLowerASCII(gpu_info.gl_vendor));
   std::string renderer(StringToLowerASCII(gpu_info.gl_renderer));
   bool is_img =
       gpu_info.gl_vendor.find("Imagination") != std::string::npos;
 
   gfx::DeviceDisplayInfo info;
   int default_tile_size = 256;
 
-  // For very high resolution displays (eg. Nexus 10), set the default
-  // tile size to be 512. This should be removed in favour of a generic
-  // hueristic that works across all platforms and devices, once that
-  // exists: http://crbug.com/159524. This switches to 512 for screens
-  // containing 40 or more 256x256 tiles, such that 1080p devices do
-  // not use 512x512 tiles (eg. 1920x1280 requires 37.5 tiles)
-  int numTiles = (info.GetDisplayWidth() *
-                  info.GetDisplayHeight()) / (256 * 256);
-  if (numTiles >= 40)
-    default_tile_size = 512;
+  // TODO(epenner): Now that this is somewhat generic, maybe we can
+  // unify this for all platforms (http://crbug.com/159524)
+
+  bool real_size_supported = false;
+  int display_width = info.GetRealDisplayWidth(real_size_supported);

[no sievers, 2013/12/20 19:24:25]

I don't see these functions in ui/gfx/android/device_display_info.h, do they
depend on another patch?

[epennerAtGoogle, 2013/12/20 19:34:22]

Yep:
https://codereview.chromium.org/93933016/

+  int display_height = info.GetRealDisplayHeight(real_size_supported);
+  int portrait_width = std::min(display_width, display_height);
+  int landscape_width = std::max(display_width, display_height);
+
+  if (real_size_supported) {

[no sievers, 2013/12/20 19:24:25]

where does that ever get set to |true|?

[epennerAtGoogle, 2013/12/20 19:34:22]

It's an out-param by reference. I could use a pointer if that would make it more
clear (I don't recall the guideline for which is better).

[no sievers, 2013/12/20 19:43:34]

I think per style guide it's const-ref or pointer if modified.

[epenner, 2013/12/20 23:00:56]

Done.

+    // Maximum HD dimensions should be 768x1280
+    // Maximum FHD dimensions should be 1200x1920
+    if (portrait_width > 768 || landscape_width > 1280)

[no sievers, 2013/12/20 19:24:25]

nit: is it more readable to maybe put all of this incl. the logic in 267-273 in
a single if-else-if-else-if-... statement?

if (portrait_width > 1200 || landscape_width > 1920)
  default_tile_size = 512;
else if (portrait_width == 1200 && landscape_width > 1280)
  default_tile_size = 416;  
else if (portrait_width > 768 || landscape_width > 1280)
  default_tile_size = 384;
else if (portrait_width == 768)
  default_tile_size = 288;

I hope I got it right :)

[epennerAtGoogle, 2013/12/20 19:34:22]

I'm not sure about that one. That combines several bits of 'conceptual' logic
into one logical statement. I'd rather this be a bit longer with each adjustment
clearly explained.  For example, where would the comments go in your version?

[no sievers, 2013/12/20 19:43:34]

I just find it a bit easier to read, because to follow your code we might
actually change the size twice, i.e. you would have to go back from the logic
further down to above to piece it together. But I don't feel strongly, if you
prefer it this way it's fine, you are the one tweaking the logic anyways :)
You could just put it Into each branch.

[epenner, 2013/12/20 23:00:56]

I get your point and we do change it more than once. If you look below, we might
even change it 3 times on IMG! :P

I'll see if I can clean it up when I rebase on the real_size_supported change.
But currently I favor redundancy over concise logic here, since each step
corresponds to a conceptual reason why we are making these changes.

+       default_tile_size = 384;
+    if (portrait_width > 1200 || landscape_width > 1920)
+       default_tile_size = 512;
+
+    // Adjust for some resolutions that barely straddle an extra
+    // tile when in portrait mode. This helps worst case scroll/raster
+    // by not needing a full extra tile for each row.
+    if (default_tile_size == 256 && portrait_width == 768)
+      default_tile_size += 32;
+    if (default_tile_size == 384 && portrait_width == 1200)
+      default_tile_size += 32;
+  } else {
+    // We don't know the exact resolution due to screen controls etc.
+    // So this just estimates the values above using tile counts.
+    int numTiles = (display_width * display_height) / (256 * 256);
+    if (numTiles > 16)
+      default_tile_size = 384;
+    if (numTiles >= 40)
+      default_tile_size = 512;
+  }
 
   // IMG: Fast async texture uploads only work with non-power-of-two,
   // but still multiple-of-eight sizes.
   // http://crbug.com/168099
   if (is_img)
     default_tile_size -= 8;
 
   // Set the command line if it isn't already set and we changed
   // the default tile size.
   if (default_tile_size != 256 &&
@@ skipping 907 matching lines @@
 
 void GpuDataManagerImplPrivate::OnGpuProcessInitFailure() {
   gpu_process_accessible_ = false;
   gpu_info_.finalized = true;
   complete_gpu_info_already_requested_ = true;
   // Some observers might be waiting.
   NotifyGpuInfoUpdate();
 }
 
 }  // namespace content