Bug 1422057 - Avoid caching clips across stacking contexts with non-identity transforms. r?mstange draft
authorKartikaya Gupta <kgupta@mozilla.com>
Sat, 06 Jan 2018 09:52:58 -0500
changeset 716814 19eba7048daaba8e263632ff259346ef54066c0f
parent 716813 ebe05b6d8667ff08365e0298dfebdffb9ebcf7ea
child 716815 b082d005d5e04d547750561281120d9f5ee51464
push id94505
push userkgupta@mozilla.com
push dateSat, 06 Jan 2018 16:27:17 +0000
reviewersmstange
bugs1422057
milestone59.0a1
Bug 1422057 - Avoid caching clips across stacking contexts with non-identity transforms. r?mstange When a clip is defined in WebRender, any transforms on the containing stacking contexts are baked into the clip's position. Therefore, trying to use a clip that was defined inside a transformed stacking context in other parts of the WR display list doesn't work properly. This was a latent bug in ScrollingLayersHelper that was previously not exposed because in these cases Gecko itself creates separate DisplayItemClipChain items. Now that we are going to deduplicate those DisplayItemClipChain items, it exposes this latent bug which we need to fix. MozReview-Commit-ID: Icd7L1JuY8s
gfx/layers/wr/ScrollingLayersHelper.cpp
gfx/layers/wr/ScrollingLayersHelper.h
gfx/layers/wr/StackingContextHelper.h
gfx/layers/wr/WebRenderCommandBuilder.cpp
--- a/gfx/layers/wr/ScrollingLayersHelper.cpp
+++ b/gfx/layers/wr/ScrollingLayersHelper.cpp
@@ -30,43 +30,51 @@ ScrollingLayersHelper::ScrollingLayersHe
 void
 ScrollingLayersHelper::BeginBuild(WebRenderLayerManager* aManager,
                                   wr::DisplayListBuilder& aBuilder)
 {
   MOZ_ASSERT(!mManager);
   mManager = aManager;
   MOZ_ASSERT(!mBuilder);
   mBuilder = &aBuilder;
-  MOZ_ASSERT(mCache.empty());
+  MOZ_ASSERT(mCacheStack.empty());
+  mCacheStack.emplace_back();
   MOZ_ASSERT(mScrollParents.empty());
   MOZ_ASSERT(mItemClipStack.empty());
 }
 
 void
 ScrollingLayersHelper::EndBuild()
 {
   mBuilder = nullptr;
   mManager = nullptr;
-  mCache.clear();
+  mCacheStack.pop_back();
+  MOZ_ASSERT(mCacheStack.empty());
   mScrollParents.clear();
   MOZ_ASSERT(mItemClipStack.empty());
 }
 
 void
-ScrollingLayersHelper::BeginList()
+ScrollingLayersHelper::BeginList(const StackingContextHelper& aStackingContext)
 {
+  if (aStackingContext.IsReferenceFrame()) {
+    mCacheStack.emplace_back();
+  }
   mItemClipStack.emplace_back(nullptr, nullptr);
 }
 
 void
-ScrollingLayersHelper::EndList()
+ScrollingLayersHelper::EndList(const StackingContextHelper& aStackingContext)
 {
   MOZ_ASSERT(!mItemClipStack.empty());
   mItemClipStack.back().Unapply(mBuilder);
   mItemClipStack.pop_back();
+  if (aStackingContext.IsReferenceFrame()) {
+    mCacheStack.pop_back();
+  }
 }
 
 void
 ScrollingLayersHelper::BeginItem(nsDisplayItem* aItem,
                                  const StackingContextHelper& aStackingContext)
 {
   SLH_LOG("processing item %p\n", aItem);
 
@@ -221,24 +229,25 @@ ScrollingLayersHelper::RecurseAndDefineC
                                             const StackingContextHelper& aSc)
 {
   MOZ_ASSERT(aChain);
 
   // This will hold our return value
   std::pair<Maybe<FrameMetrics::ViewID>, Maybe<wr::WrClipId>> ids;
 
   if (mBuilder->HasExtraClip()) {
-    // We can't use mCache directly. However if there's an out-of-band clip that
+    // We can't use the clip cache directly. However if there's an out-of-band clip that
     // was pushed on top of aChain, we should return the id for that OOB clip,
     // so that anything we want to define as a descendant of aChain we actually
     // end up defining as a descendant of the OOB clip.
     ids.second = mBuilder->GetCacheOverride(aChain);
   } else {
-    auto it = mCache.find(aChain);
-    if (it != mCache.end()) {
+    const ClipIdMap& cache = mCacheStack.back();
+    auto it = cache.find(aChain);
+    if (it != cache.end()) {
       ids.second = Some(it->second);
     }
   }
   if (ids.second) {
     // If we've already got an id for this clip, we can early-exit
     if (aAsr) {
       FrameMetrics::ViewID scrollId = aAsr->GetViewId();
       MOZ_ASSERT(mBuilder->IsScrollLayerDefined(scrollId));
@@ -323,17 +332,17 @@ ScrollingLayersHelper::RecurseAndDefineC
   nsTArray<wr::ComplexClipRegion> wrRoundedRects;
   aChain->mClip.ToComplexClipRegions(aAppUnitsPerDevPixel, aSc, wrRoundedRects);
 
   // Define the clip
   wr::WrClipId clipId = mBuilder->DefineClip(
       ancestorIds.first, ancestorIds.second,
       aSc.ToRelativeLayoutRect(clip), &wrRoundedRects);
   if (!mBuilder->HasExtraClip()) {
-    mCache[aChain] = clipId;
+    mCacheStack.back()[aChain] = clipId;
   }
 
   ids.second = Some(clipId);
   return ids;
 }
 
 std::pair<Maybe<FrameMetrics::ViewID>, Maybe<wr::WrClipId>>
 ScrollingLayersHelper::RecurseAndDefineAsr(nsDisplayItem* aItem,
@@ -368,28 +377,29 @@ ScrollingLayersHelper::RecurseAndDefineA
         auto it = mScrollParents.find(scrollId);
         if (it != mScrollParents.end()) {
           const DisplayItemClipChain* scrollParent = it->second;
           if (DisplayItemClipChain::Equal(scrollParent, aChain)) {
             canonicalChain = scrollParent;
           }
         }
 
-        auto it2 = mCache.find(canonicalChain);
-        // If |it == mCache.end()| here then we have run into a case where the
+        const ClipIdMap& cache = mCacheStack.back();
+        auto it2 = cache.find(canonicalChain);
+        // If |it2 == cache.end()| here then we have run into a case where the
         // scroll layer was previously defined with a specific parent clip, and
         // now here it has a different parent clip. Gecko can create display
         // lists like this because it treats the ASR chain and clipping chain
         // more independently, but we can't yet represent this in WR. This is
         // tracked by bug 1409442. For now we'll just leave ids.second as
         // Nothing() which will effectively ignore the clip |aChain|. Once WR
         // supports multiple ancestors on a scroll layer we can deal with this
         // better. The layout/reftests/text/wordwrap-08.html has a Text display
         // item that exercises this case.
-        if (it2 == mCache.end()) {
+        if (it2 == cache.end()) {
           // leave ids.second as Nothing(). This should only happen if we didn't
           // pick up a better canonicalChain above, either because it didn't
           // exist, or because it was not ::Equal to aChain. Therefore
           // canonicalChain must still be equal to aChain here.
           MOZ_ASSERT(canonicalChain == aChain);
         } else {
           ids.second = Some(it2->second);
         }
@@ -481,17 +491,17 @@ ScrollingLayersHelper::EnclosingClipAndS
     }
   }
   return Nothing();
 }
 
 ScrollingLayersHelper::~ScrollingLayersHelper()
 {
   MOZ_ASSERT(!mBuilder);
-  MOZ_ASSERT(mCache.empty());
+  MOZ_ASSERT(mCacheStack.empty());
   MOZ_ASSERT(mItemClipStack.empty());
 }
 
 ScrollingLayersHelper::ItemClips::ItemClips(const ActiveScrolledRoot* aAsr,
                                             const DisplayItemClipChain* aChain)
   : mAsr(aAsr)
   , mChain(aChain)
 {
--- a/gfx/layers/wr/ScrollingLayersHelper.h
+++ b/gfx/layers/wr/ScrollingLayersHelper.h
@@ -34,18 +34,18 @@ class ScrollingLayersHelper
 {
 public:
   ScrollingLayersHelper();
 
   void BeginBuild(WebRenderLayerManager* aManager,
                   wr::DisplayListBuilder& aBuilder);
   void EndBuild();
 
-  void BeginList();
-  void EndList();
+  void BeginList(const StackingContextHelper& aStackingContext);
+  void EndList(const StackingContextHelper& aStackingContext);
 
   void BeginItem(nsDisplayItem* aItem,
                  const StackingContextHelper& aStackingContext);
   ~ScrollingLayersHelper();
 
 private:
   typedef std::pair<FrameMetrics::ViewID, Maybe<wr::WrClipId>> ClipAndScroll;
 
@@ -81,17 +81,26 @@ private:
   // is true, and that will incur a performance penalty for all the hashmap
   // operations, so is probably not worth it. With the current code we might
   // end up creating multiple clips in WR that are effectively identical but
   // have separate clip ids. Hopefully this won't happen very often.
   typedef std::unordered_map<const DisplayItemClipChain*, wr::WrClipId> ClipIdMap;
 
   WebRenderLayerManager* MOZ_NON_OWNING_REF mManager;
   wr::DisplayListBuilder* mBuilder;
-  ClipIdMap mCache;
+  // Stack of clip caches. There is one entry in the stack for each reference
+  // frame that is currently pushed in WR (a reference frame is a stacking
+  // context with a non-identity transform). Each entry contains a map that
+  // maps gecko DisplayItemClipChain objects to webrender WrClipIds, which
+  // allows us to avoid redefining identical clips in WR. We need to keep a
+  // separate cache per reference frame because the DisplayItemClipChain items
+  // themselves get deduplicated without regard to reference frames, but on the
+  // WR side we need to create different clips if they are in different
+  // reference frames.
+  std::vector<ClipIdMap> mCacheStack;
 
   typedef std::unordered_map<FrameMetrics::ViewID, const DisplayItemClipChain*> ScrollParentMap;
   ScrollParentMap mScrollParents;
 
   struct ItemClips {
     ItemClips(const ActiveScrolledRoot* aAsr,
               const DisplayItemClipChain* aChain);
 
--- a/gfx/layers/wr/StackingContextHelper.h
+++ b/gfx/layers/wr/StackingContextHelper.h
@@ -69,16 +69,17 @@ public:
   gfx::Size GetInheritedScale() const { return mScale; }
 
   const gfx::Matrix& GetInheritedTransform() const
   {
     return mInheritedTransform;
   }
 
   bool IsBackfaceVisible() const { return mTransform.IsBackfaceVisible(); }
+  bool IsReferenceFrame() const { return !mTransform.IsIdentity(); }
 
 private:
   wr::DisplayListBuilder* mBuilder;
   gfx::Matrix4x4 mTransform;
   gfx::Size mScale;
   gfx::Matrix mInheritedTransform;
 };
 
--- a/gfx/layers/wr/WebRenderCommandBuilder.cpp
+++ b/gfx/layers/wr/WebRenderCommandBuilder.cpp
@@ -106,17 +106,17 @@ WebRenderCommandBuilder::BuildWebRenderC
 
 void
 WebRenderCommandBuilder::CreateWebRenderCommandsFromDisplayList(nsDisplayList* aDisplayList,
                                                                 nsDisplayListBuilder* aDisplayListBuilder,
                                                                 const StackingContextHelper& aSc,
                                                                 wr::DisplayListBuilder& aBuilder,
                                                                 wr::IpcResourceUpdateQueue& aResources)
 {
-  mScrollingHelper.BeginList();
+  mScrollingHelper.BeginList(aSc);
 
   bool apzEnabled = mManager->AsyncPanZoomEnabled();
   EventRegions eventRegions;
 
   FlattenedDisplayItemIterator iter(aDisplayListBuilder, aDisplayList);
   while (nsDisplayItem* i = iter.GetNext()) {
     nsDisplayItem* item = i;
     DisplayItemType itemType = item->GetType();
@@ -256,17 +256,17 @@ WebRenderCommandBuilder::CreateWebRender
   // the most recently created layer data will have been created by an item
   // with matching ASRs.
   if (!eventRegions.IsEmpty()) {
     MOZ_ASSERT(apzEnabled);
     MOZ_ASSERT(!mLayerScrollData.empty());
     mLayerScrollData.back().AddEventRegions(eventRegions);
   }
 
-  mScrollingHelper.EndList();
+  mScrollingHelper.EndList(aSc);
 }
 
 Maybe<wr::ImageKey>
 WebRenderCommandBuilder::CreateImageKey(nsDisplayItem* aItem,
                                         ImageContainer* aContainer,
                                         mozilla::wr::DisplayListBuilder& aBuilder,
                                         mozilla::wr::IpcResourceUpdateQueue& aResources,
                                         const StackingContextHelper& aSc,