CC: Tile-size cleanup.

cc/layers/picture_layer_impl.cc

Index: cc/layers/picture_layer_impl.cc
diff --git a/cc/layers/picture_layer_impl.cc b/cc/layers/picture_layer_impl.cc
index 37c83289700ab9fef4cd1fb356d0f9c354d29b0f..7f78821c46cf5496a38f293897a20d719e04d823 100644
--- a/cc/layers/picture_layer_impl.cc
+++ b/cc/layers/picture_layer_impl.cc
@@ -44,6 +44,11 @@ const float kCpuSkewportTargetTimeInFrames = 60.0f;
 // TileManager::BinFromTilePriority).
 const float kGpuSkewportTargetTimeInFrames = 0.0f;
 
+// Even for really wide viewports, at some point GPU raster should use
+// less than 4 tiles to fill the viewport. This is set to 128 as a
+// sane minimum for now, but we might want to increase with tuning.
+const int kMinHeightForGpuRasteredTile = 128;
+
 }  // namespace
 
 namespace cc {
@@ -675,33 +680,44 @@ gfx::Size PictureLayerImpl::CalculateTileSize(
   }
 
   gfx::Size default_tile_size = layer_tree_impl()->settings().default_tile_size;
-  if (layer_tree_impl()->use_gpu_rasterization()) {
-    // TODO(ernstm) crbug.com/365877: We need a unified way to override the
-    // default-tile-size.
-    default_tile_size =
-        gfx::Size(layer_tree_impl()->device_viewport_size().width(),
-                  layer_tree_impl()->device_viewport_size().height() / 4);
-  }
-  default_tile_size.SetToMin(gfx::Size(max_texture_size, max_texture_size));
-
   gfx::Size max_untiled_content_size =
       layer_tree_impl()->settings().max_untiled_layer_size;
+
+  // For GPU rasterization, we pick an ideal tile size using the viewport,
+  // so we don't need the above settings.
+  bool use_gpu = layer_tree_impl()->use_gpu_rasterization();
+  if (use_gpu) {

[vmpstr, 2014/10/09 16:35:43]

For complexity thing, maybe just have the two variables default initialized and
have an explicit if/else.. something like

Foo foo;
if (gpu) 
  foo = expr1;
else
  foo = expr2;

instead of 

Foo foo = expr2;
if (gpu) {
  /* disregard all variables above */
  foo = expr1;
}

That and maybe max_untiled_content_size needs a better name. This might be me,
but it's not evident to me what max_untiled_content_size means.. Maybe
largest_tile_size_possible or something? I don't know, this is a weak point for
me so it doesn't really matter. Largest tile size possible name also conflicts
with what we set it in gpu since 2 * height can be smaller than the default size
of width.

[epenner, 2014/10/09 18:36:33]

I dunno, I think once you see the code you might not like this as much.

I like if/else if there is actually some instructions worth saving, but
otherwise it just adds three negatives IMO: More braces, more indentation, and
more default-or-uninitialized variables that need to be mentally tracked through
all branches.

I did the same thing below for long_and_skinny, and if I made that a full
if/else there would be more code lines etc.

[vmpstr, 2014/10/09 18:38:18]

Fair enough. That's why I'm not really sure how to make it too much better :)
Let me know when the new patch is up, but I'm pretty happy with this as is.

+    int width = layer_tree_impl()->device_viewport_size().width();
+    int height = layer_tree_impl()->device_viewport_size().height() / 4;
+    height = std::max(height, kMinHeightForGpuRasteredTile);
+    default_tile_size = gfx::Size(width, height);
+    // Since the width is already expanded to viewport width, we use
+    // double the height as our max untiled size.
+    max_untiled_content_size = gfx::Size(height * 2, height * 2);
+  }
+
+  default_tile_size.SetToMin(gfx::Size(max_texture_size, max_texture_size));
   max_untiled_content_size.SetToMin(
       gfx::Size(max_texture_size, max_texture_size));
 
-  bool any_dimension_too_large =
-      content_bounds.width() > max_untiled_content_size.width() ||
-      content_bounds.height() > max_untiled_content_size.height();
+  bool both_dimensions_are_small =
+      content_bounds.width() <= max_untiled_content_size.width() &&
+      content_bounds.height() <= max_untiled_content_size.height();
 
-  bool any_dimension_one_tile =
+  bool long_and_skinny =
       content_bounds.width() <= default_tile_size.width() ||
       content_bounds.height() <= default_tile_size.height();
 
+  // Using GPU raster the width is already expanded to the viewport, so we just
+  // use the height to determine if the layer is skinny horizontally.
+  if (use_gpu)
+    long_and_skinny = content_bounds.width() <= default_tile_size.height();

[vmpstr, 2014/10/09 16:35:43]

typo? This should probably compare height with height. This would only make a
difference in content bounds width is larger than default tile size width right?
That is, if content bounds width is also smaller than or equal to viewport, then
even without long and skinny optimization 1 tile would cover the whole content
(I'm just trying to understand the cases when this hits)

[epenner, 2014/10/09 18:36:32]

This is what I intended. But perhaps I need to improve this further and make it
more clear. This is used to expand a tile beyond it's typical size in response
to content being skinny in the other direction. If the content has a skinny
width, we expand the tile height and vice versa... However, we never want to
expand tile-width (it's already ginormous), and the tile-width is not useful to
determine if the content-width is skinny (tile-width is ginormous).

I'll take another pass at making this better/clear.

+
   // If long and skinny, tile at the max untiled content size, and clamp
   // the smaller dimension to the content size, e.g. 1000x12 layer with
   // 500x500 max untiled size would get 500x12 tiles.  Also do this
   // if the layer is small.
-  if (any_dimension_one_tile || !any_dimension_too_large) {
+  if (long_and_skinny || both_dimensions_are_small) {
     int width = std::min(
         std::max(max_untiled_content_size.width(), default_tile_size.width()),
         content_bounds.width());