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_ &&

[enne (OOO), 2014/05/15 17:40:06]

This conditional and the one in the following block are very confusing to read
now.  Can you break this into more readable subexpressions?

[reveman, 2014/05/16 19:17:57]

Are all these checks here necessary or just an optimization? What if we just
made this something like:

if (twin_layer_ &&
    tilings_->num_tilings() <= 2 &&
    tilings_->num_tilings() == twin_layer_->tilings_->num_tilings()) {
  twin_low_res = low_res ? GetTwinTiling(low_res) : NULL;
  twin_high_res = high_res ? GetTwinTiling(high_res) : NULL;
}

[Sergey, 2014/05/19 08:18:27]

Done.

+      tilings_->num_tilings() == twin_layer_->tilings_->num_tilings() &&
+      tilings_->num_tilings() <= 2) {
+    if (should_have_low_res)
+      twin_low_res = GetTwinTiling(low_res);
+    if (twin_low_res || !should_have_low_res)
       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) ||

[Sergey, 2014/05/16 02:26:12]

@enne, do you mean this one? Unfortunately, the only idea I have how to break is
the on in patch set 1. But I think this one is actually better and is
readable... No?

It is essentially the same, as before the patch, except changing the part "or we
didn't get twin low res" to "or we didn't get twin low res, while we should".

[reveman, 2014/05/16 19:17:57]

Maybe something like this would help:

bool missing_twin_low_res = ShouldHaveLowResTiling() && !twin_low_res;
bool missing_twin_high_res = twin_high_res;

Btw, why are we checking "!twin_high_res || !twin_low_res" here at all? Do they
both have to be set to NULL if one of them is NULL? Or is this just an
optimization to avoid comparing transforms?

[enne (OOO), 2014/05/16 20:46:07]

Assuming that you only do this in the case that a layer has a twin and they each
have either a high res or both a high and low res, then you have four cases for
this optimization.

(1) Both layers only have high res: ok
(2) Both layers have high res and low res: ok
(3) Pending layer has both, active layer has only high res: ok (but no need for
a second pass, previously not handled as a simplification)
(4) Pending layer has only high res, active layer has both: not ok (this is not
handled in this patch correctly)

#4 is not ok because the active layer could be displaying low res tiles, and if
you are not able to mark low res tiles as required, you could flash from low res
to missing.

I think this logic would make more sense if the ShouldHaveLowResTiling() was not
consulted and you just allowed the first 3 cases above.

[Sergey, 2014/05/19 08:18:27]

Done.

       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)) {
     // 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);
+    // content. Only suitable, when low res is enabled.
+    if (should_have_low_res) {
+      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,
     const Region& missing_region) const {
   bool twin_had_missing_tile = false;
@@ skipping 678 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