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

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 85249812d5fbc9d5ea119537e014ae5e80eb80b2..2212a71c584b4d266f46287e7f2bb70ae9115b82 100644
--- a/cc/layers/picture_layer_impl.cc
+++ b/cc/layers/picture_layer_impl.cc
@@ -787,19 +787,25 @@ void PictureLayerImpl::MarkVisibleResourcesAsRequired() const {
   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) {
@@ -814,9 +820,11 @@ void PictureLayerImpl::MarkVisibleResourcesAsRequired() const {
     // 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);
+    }
   }
 }