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..5c8da3902efdc1047b657f977248fb389bbbb7b0 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_ &&
+      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) {
@@ -810,11 +816,12 @@ void PictureLayerImpl::MarkVisibleResourcesAsRequired() const {
   // 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);
   }