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 769 matching lines @@
   DCHECK(high_res) << "There must be one high res tiling";
 
   // If these pointers are null (because no twin, no matching tiling, or the
   // 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)
+  // active layer is also missing when both this layer and its twin have
+  // "simple" sets of tilings: only 2 tilings (high and low) for case, when low
+  // res is enabled; or only 1 high res tiling, when low res is disabled. This
+  // avoids having to iterate/track coverage of non-ideal tilings during the
+  // last draw call on the active layer.
+  bool should_have_low_res = ShouldHaveLowResTiling();
+  if (high_res && (low_res || !should_have_low_res) && twin_layer_ &&
+      tilings_->num_tilings() == twin_layer_->tilings_->num_tilings() &&
+      tilings_->num_tilings() <= 2) {

[reveman, 2014/05/13 15:38:05]

what if |should_have_low_res| is true but tilings_->num_tilings() returns 1?
that would previously cause us to not enter this clause but now it does. Is that
a potential problem?

[Sergey, 2014/05/14 05:53:35]

If |should_have_low_res| is true and tilings_->num_tilings() = 1, then we have
low_res = NULL and condition (low_res || !should_have_low_res) = false. Thus, we
would not enter this block, as before.

[reveman, 2014/05/14 15:13:49]

Could be low_res != NULL if high_res = NULL but we would still not enter the
block so I guess this looks good.

+    if (should_have_low_res)
+      twin_low_res = GetTwinTiling(low_res);
+    if (twin_low_res || !should_have_low_res)

[reveman, 2014/05/13 15:38:05]

Do we need this check or could we just make it unconditional and move it above
line 799?

[Sergey, 2014/05/14 05:53:35]

For some reason before we only executed this if we have found twin tiling for
low res tiles. I would like to leave the logic intact, when we should have low
res. And when we are not supposed to have low res still try to find twin tiles
for high res. Thus this condition.

[reveman, 2014/05/14 15:13:49]

Fine to keep existing logic if you prefer but cleaning it up by removing this
conditional if not necessary would be nice as a follow up.

[Sergey, 2014/05/15 01:08:04]

Ok, let it be my next patch :) And I would like to do some research on it too.

       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 ||
+  if (!twin_high_res || (!twin_low_res && should_have_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;
   }
 
   // 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) &&
+      should_have_low_res) {

[reveman, 2014/05/13 15:38:05]

Maybe move the "should_have_low_res" check into it's own if statement below to
make it more clear that skipping the call to MarkVisibleTilesAsRequired when
should_have_low_res is false is not OK.

[Sergey, 2014/05/14 05:53:35]

Done.

     // 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.
+    // content. Only suitable, when low res is enabled.
     MarkVisibleTilesAsRequired(
         low_res, twin_low_res, contents_scale_x(), rect, missing_region);
   }
 }
 
 bool PictureLayerImpl::MarkVisibleTilesAsRequired(
     PictureLayerTiling* tiling,
     const PictureLayerTiling* optional_twin_tiling,
     float contents_scale,
     const gfx::Rect& rect,
@@ skipping 680 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