Fixing crash in PictureLayerImpl::MarkVisibleResourcesAsRequired when low-res tiles are disabled

cc/layers/picture_layer_impl.cc

 // Copyright 2012 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 "cc/layers/picture_layer_impl.h"
 
 #include <algorithm>
 #include <limits>
 
 #include "base/time/time.h"
@@ skipping 772 matching lines @@
   // simpification just below), then high res tiles will be required to fill any
   // holes left by the first pass above.  If the pointers are valid, then this
   // layer is allowed to skip any tiles that are not ready on its twin.
   const PictureLayerTiling* twin_high_res = NULL;
   const PictureLayerTiling* twin_low_res = NULL;
 
   // As a simplification, only allow activating to skip twin tiles that the
   // active layer is also missing when both this layer and its twin have 2
   // tilings (high and low).  This avoids having to iterate/track coverage of
   // non-ideal tilings during the last draw call on the active layer.
-  if (high_res && low_res && tilings_->num_tilings() == 2 &&
-      twin_layer_ && twin_layer_->tilings_->num_tilings() == 2) {
-    twin_low_res = GetTwinTiling(low_res);
-    if (twin_low_res)
+  if (ShouldHaveLowResTiling()) {

[reveman, 2014/05/12 02:30:52]

why this change? is this broken before this change?

[Sergey, 2014/05/12 08:07:21]

If you set create_low_res_tiling=false, you will get
twin_layer_->tilings_->num_tilings() = 1. Thus, the original condition will turn
to false and there would not be a try to get high resolution tiling from twin
layer. Thus, made it two cases.

Logically this change seemed correct... Is there some misunderstanding from my
side?

[reveman, 2014/05/12 15:44:04]

Probably not but if you're fixing a bug in the implementation and not just some
unit tests you might want to do that separately or at least make it clear in the
description of the patch that this is currently broken. Does the patch currently
include a test that will fail without this change?

Is there any way you can make this look less like something that was tacked on
as side effect of making low res tiling optional and more like the initial
design?

[Sergey, 2014/05/12 23:37:47]

There was one, let me make sure we have it... Actually, I am not sure, that we
will have a failing test, as according to the MarkVisibleTilesAsRequired
function finding twin tiling is optional (we are searching here for a pointer,
that will be passed into the MarkVisibleTilesAsRequired as optional_twin_tiling
parameter). But I guess we should try our best to actually find it.
Agree, let me try.

[Sergey, 2014/05/13 05:37:44]

Found that test cases once again: NothingRequiredIfAllHighResTilesShared and
NothingRequiredIfActiveMissingTiles. Added them for low res disabled case.
Done. Please, check, if now it looks better.

+    if (high_res && low_res && tilings_->num_tilings() == 2 &&
+        twin_layer_ && twin_layer_->tilings_->num_tilings() == 2) {
+      twin_low_res = GetTwinTiling(low_res);
+      if (twin_low_res)
+        twin_high_res = GetTwinTiling(high_res);
+    }
+  } else {
+    if (high_res && tilings_->num_tilings() == 1 &&
+        twin_layer_ && twin_layer_->tilings_->num_tilings() == 1) {
       twin_high_res = GetTwinTiling(high_res);
+    }
   }
+
   // If this layer and its twin have different transforms, then don't compare
   // them and only allow activating to high res tiles, since tiles on each layer
   // will be in different places on screen.
-  if (!twin_high_res || !twin_low_res ||
-      twin_layer_->layer_tree_impl()->RequiresHighResToDraw() ||
-      draw_properties().screen_space_transform !=
-          twin_layer_->draw_properties().screen_space_transform) {
-    twin_high_res = NULL;
-    twin_low_res = NULL;
+
+  if (ShouldHaveLowResTiling()) {
+    if (!twin_high_res || !twin_low_res ||
+        twin_layer_->layer_tree_impl()->RequiresHighResToDraw() ||
+        draw_properties().screen_space_transform !=
+            twin_layer_->draw_properties().screen_space_transform) {
+      twin_high_res = NULL;
+      twin_low_res = NULL;
+    }
+  } else {
+    if (!twin_high_res ||
+        twin_layer_->layer_tree_impl()->RequiresHighResToDraw() ||
+        draw_properties().screen_space_transform !=
+            twin_layer_->draw_properties().screen_space_transform) {
+      twin_high_res = NULL;
+    }
   }
 
   // As a second pass, mark as required any visible high res tiles not filled in
   // by acceptable non-ideal tiles from the first pass.
   if (MarkVisibleTilesAsRequired(
-      high_res, twin_high_res, contents_scale_x(), rect, missing_region)) {
+      high_res, twin_high_res, contents_scale_x(), rect, missing_region) &&
+      ShouldHaveLowResTiling()) {

[reveman, 2014/05/12 15:44:04]

is this required?

[Sergey, 2014/05/12 23:37:47]

Actually, this one is more required, then the above :)
Without it we get into MarkVisibleTilesAsRequired with low_res = NULL and crash.

     // As an optional third pass, if a high res tile was skipped because its
     // twin was also missing, then fall back to mark low res tiles as required
     // in case the active twin is substituting those for missing high res
     // content.
     MarkVisibleTilesAsRequired(
         low_res, twin_low_res, contents_scale_x(), rect, missing_region);
   }
 }
 
 bool PictureLayerImpl::MarkVisibleTilesAsRequired(
@@ skipping 684 matching lines @@
   return iterator_index_ < iterators_.size();
 }
 
 bool PictureLayerImpl::LayerEvictionTileIterator::IsCorrectType(
     PictureLayerTiling::TilingEvictionTileIterator* it) const {
   return it->get_type() == iteration_stage_ &&
          (**it)->required_for_activation() == required_for_activation_;
 }
 
 }  // namespace cc