Bug 1592026 - Replace separate mReadySurface field with a bool mMutatedFrontSurface. r=jrmuizel
authorMarkus Stange <mstange@themasta.com>
Sun, 29 Dec 2019 12:19:31 +0000
changeset 508474 b041f2395c0543a9764fd0afd41e02d09c0f72ed
parent 508473 2effeae528d35e468eea66891334947c2756e924
child 508475 8d7fd1220a4dacdca1a14b502008ced946c1da8b
push id104012
push usermstange@themasta.com
push dateSun, 29 Dec 2019 12:53:08 +0000
treeherderautoland@781f53bf9c78 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjrmuizel
bugs1592026
milestone73.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1592026 - Replace separate mReadySurface field with a bool mMutatedFrontSurface. r=jrmuizel Differential Revision: https://phabricator.services.mozilla.com/D57065
gfx/layers/NativeLayerCA.h
gfx/layers/NativeLayerCA.mm
--- a/gfx/layers/NativeLayerCA.h
+++ b/gfx/layers/NativeLayerCA.h
@@ -195,75 +195,47 @@ class NativeLayerCA : public NativeLayer
   // Controls access to all fields of this class.
   Mutex mMutex;
 
   // Each IOSurface is initially created inside NextSurface.
   // The surface stays alive until the recycling mechanism in NextSurface
   // determines it is no longer needed (because the swap chain has grown too
   // long) or until DiscardBackbuffers() is called or the layer is destroyed.
   // During the surface's lifetime, it will continuously move through the fields
-  // mInProgressSurface, mReadySurface, mFrontSurface, and back to front through
-  // the mSurfaces queue:
+  // mInProgressSurface, mFrontSurface, and back to front through the mSurfaces
+  // queue:
   //
   //  mSurfaces.front()
   //  ------[NextSurface()]-----> mInProgressSurface
-  //  --[NotifySurfaceReady()]--> mReadySurface
-  //  ----[ApplyChanges()]------> mFrontSurface
-  //  ----[ApplyChanges()]------> mSurfaces.back()  --> .... -->
+  //  --[NotifySurfaceReady()]--> mFrontSurface
+  //  --[NotifySurfaceReady()]--> mSurfaces.back()  --> .... -->
   //  mSurfaces.front()
   //
   // We mark an IOSurface as "in use" as long as it is either in
-  // mInProgressSurface or in mReadySurface. When it is in mFrontSurface or in
-  // the mSurfaces queue, it is not marked as "in use" by us - but it can be "in
-  // use" by the window server. Consequently, IOSurfaceIsInUse on a surface from
-  // mSurfaces reflects whether the window server is still reading from the
-  // surface, and we can use this indicator to decide when to recycle the
-  // surface.
+  // mInProgressSurface. When it is in mFrontSurface or in the mSurfaces queue,
+  // it is not marked as "in use" by us - but it can be "in use" by the window
+  // server. Consequently, IOSurfaceIsInUse on a surface from mSurfaces reflects
+  // whether the window server is still reading from the surface, and we can use
+  // this indicator to decide when to recycle the surface.
   //
   // Users of NativeLayerCA normally proceed in this order:
   //  1. Begin a frame by calling NextSurface to get the surface.
   //  2. Draw to the surface.
   //  3. Mark the surface as done by calling NotifySurfaceReady.
-  //  4. Trigger a CoreAnimation transaction, and call ApplyChanges within the
-  //  transaction.
-  //
-  // For two consecutive frames, this results in the following ordering of
-  // calls:
-  // I. NextSurface, NotifySurfaceReady, ApplyChanges, NextSurface,
-  //    NotifySurfaceReady, ApplyChanges
-  //
-  // In this scenario, either mInProgressSurface or mReadySurface is always
-  // Nothing().
-  //
-  // However, sometimes we see the following ordering instead:
-  // II. NextSurface, NotifySurfaceReady, NextSurface, ApplyChanges,
-  //     NotifySurfaceReady, ApplyChanges
-  //
-  // This has the NextSurface and ApplyChanges calls in the middle reversed.
-  //
-  // In that scenario, both mInProgressSurface and mReadySurface will be Some()
-  // between the calls to NextSurface and ApplyChanges in the middle, and the
-  // two ApplyChanges invocations will submit two different surfaces. It is
-  // important that we don't simply discard the first surface during the second
-  // call to NextSurface in this scenario.
+  //  4. Call NativeLayerRoot::CommitToScreen(), which calls ApplyChanges()
+  //     during a CATransaction.
 
   // The surface we returned from the most recent call to NextSurface, before
   // the matching call to NotifySurfaceReady.
   // Will only be Some() between calls to NextSurface and NotifySurfaceReady.
   Maybe<SurfaceWithInvalidRegion> mInProgressSurface;
 
   // The surface that the most recent call to NotifySurfaceReady was for.
-  // Will only be Some() between calls to NotifySurfaceReady and the next call
-  // to ApplyChanges.
-  // Both mInProgressSurface and mReadySurface can be Some() at the same time.
-  Maybe<SurfaceWithInvalidRegion> mReadySurface;
-
-  // The surface that the most recent call to ApplyChanges set on the CALayer.
-  // Will be Some() after the first sequence of NextSurface, NotifySurfaceReady,
-  // ApplyChanges calls, for the rest of the layer's life time.
+  // Will be Some() after the first call to NotifySurfaceReady, for the rest of
+  // the layer's life time.
   Maybe<SurfaceWithInvalidRegion> mFrontSurface;
 
   // The queue of surfaces which make up the rest of our "swap chain".
   // mSurfaces.front() is the next surface we'll attempt to use.
   // mSurfaces.back() is the one that was used most recently.
   std::vector<SurfaceWithInvalidRegionAndCheckCount> mSurfaces;
 
   // Non-null between calls to NextSurfaceAsDrawTarget and NotifySurfaceReady.
@@ -285,14 +257,15 @@ class NativeLayerCA : public NativeLayer
 
   float mBackingScale = 1.0f;
   bool mSurfaceIsFlipped = false;
   const bool mIsOpaque = false;
   bool mMutatedBackingScale = false;
   bool mMutatedSurfaceIsFlipped = false;
   bool mMutatedPosition = false;
   bool mMutatedClipRect = false;
+  bool mMutatedFrontSurface = false;
 };
 
 }  // namespace layers
 }  // namespace mozilla
 
 #endif  // mozilla_layers_NativeLayerCA_h
--- a/gfx/layers/NativeLayerCA.mm
+++ b/gfx/layers/NativeLayerCA.mm
@@ -196,20 +196,16 @@ NativeLayerCA::~NativeLayerCA() {
   if (mInProgressLockedIOSurface) {
     mInProgressLockedIOSurface->Unlock(false);
     mInProgressLockedIOSurface = nullptr;
   }
   if (mInProgressSurface) {
     IOSurfaceDecrementUseCount(mInProgressSurface->mSurface.get());
     mSurfacePoolHandle->ReturnSurfaceToPool(mInProgressSurface->mSurface);
   }
-  if (mReadySurface) {
-    IOSurfaceDecrementUseCount(mReadySurface->mSurface.get());
-    mSurfacePoolHandle->ReturnSurfaceToPool(mReadySurface->mSurface);
-  }
   if (mFrontSurface) {
     mSurfacePoolHandle->ReturnSurfaceToPool(mFrontSurface->mSurface);
   }
   for (const auto& surf : mSurfaces) {
     mSurfacePoolHandle->ReturnSurfaceToPool(surf.mEntry.mSurface);
   }
 
   [mContentCALayer release];
@@ -284,19 +280,16 @@ Maybe<gfx::IntRect> NativeLayerCA::ClipR
 }
 
 void NativeLayerCA::InvalidateRegionThroughoutSwapchain(const MutexAutoLock&,
                                                         const IntRegion& aRegion) {
   IntRegion r = aRegion;
   if (mInProgressSurface) {
     mInProgressSurface->mInvalidRegion.OrWith(r);
   }
-  if (mReadySurface) {
-    mReadySurface->mInvalidRegion.OrWith(r);
-  }
   if (mFrontSurface) {
     mFrontSurface->mInvalidRegion.OrWith(r);
   }
   for (auto& surf : mSurfaces) {
     surf.mEntry.mInvalidRegion.OrWith(r);
   }
 }
 
@@ -338,31 +331,28 @@ void NativeLayerCA::HandlePartialUpdate(
 
   gfx::IntRegion copyRegion;
   copyRegion.Sub(mInProgressSurface->mInvalidRegion, aUpdateRegion);
   if (!copyRegion.IsEmpty()) {
     // There are parts in mInProgressSurface which are invalid but which are not included in
     // aUpdateRegion. We will obtain valid content for those parts by copying from a previous
     // surface.
     MOZ_RELEASE_ASSERT(
-        mReadySurface || mFrontSurface,
+        mFrontSurface,
         "The first call to NextSurface* must always update the entire layer. If this "
-        "is the second call, mReadySurface or mFrontSurface will be Some().");
+        "is the second call, mFrontSurface will be Some().");
 
-    // NotifySurfaceReady marks the entire surface as valid. The valid surface is then stored in
-    // mReadySurface, and later moves to mFrontSurface. Get the surface that NotifySurfaceReady was
-    // called on most recently.
-    SurfaceWithInvalidRegion& copySource = mReadySurface ? *mReadySurface : *mFrontSurface;
-    MOZ_RELEASE_ASSERT(copySource.mInvalidRegion.Intersect(copyRegion).IsEmpty(),
-                       "copySource should have valid content in the entire copy region, because "
+    // NotifySurfaceReady marks the entirety of mFrontSurface as valid.
+    MOZ_RELEASE_ASSERT(mFrontSurface->mInvalidRegion.Intersect(copyRegion).IsEmpty(),
+                       "mFrontSurface should have valid content in the entire copy region, because "
                        "the only invalidation since NotifySurfaceReady was aUpdateRegion, and "
                        "aUpdateRegion has no overlap with copyRegion.");
 
-    // Now copy the valid content, using a callar-provided copy function.
-    aCopyFn(copySource.mSurface, copyRegion);
+    // Now copy the valid content, using a caller-provided copy function.
+    aCopyFn(mFrontSurface->mSurface, copyRegion);
     mInProgressSurface->mInvalidRegion.SubOut(copyRegion);
   }
 
   MOZ_RELEASE_ASSERT(mInProgressSurface->mInvalidRegion == aUpdateRegion);
 }
 
 RefPtr<gfx::DrawTarget> NativeLayerCA::NextSurfaceAsDrawTarget(const gfx::IntRegion& aUpdateRegion,
                                                                gfx::BackendType aBackendType) {
@@ -431,29 +421,31 @@ Maybe<GLuint> NativeLayerCA::NextSurface
   return fbo;
 }
 
 void NativeLayerCA::NotifySurfaceReady() {
   MutexAutoLock lock(mMutex);
 
   MOZ_RELEASE_ASSERT(mInProgressSurface,
                      "NotifySurfaceReady called without preceding call to NextSurface");
-  if (mReadySurface) {
-    IOSurfaceDecrementUseCount(mReadySurface->mSurface.get());
-    mSurfaces.push_back({*mReadySurface, 0});
-    mReadySurface = Nothing();
-  }
 
   if (mInProgressLockedIOSurface) {
     mInProgressLockedIOSurface->Unlock(false);
     mInProgressLockedIOSurface = nullptr;
   }
 
-  mReadySurface = std::move(mInProgressSurface);
-  mReadySurface->mInvalidRegion = IntRect();
+  if (mFrontSurface) {
+    mSurfaces.push_back({*mFrontSurface, 0});
+    mFrontSurface = Nothing();
+  }
+
+  IOSurfaceDecrementUseCount(mInProgressSurface->mSurface.get());
+  mFrontSurface = std::move(mInProgressSurface);
+  mFrontSurface->mInvalidRegion = IntRect();
+  mMutatedFrontSurface = true;
 }
 
 void NativeLayerCA::DiscardBackbuffers() {
   MutexAutoLock lock(mMutex);
 
   for (const auto& surf : mSurfaces) {
     mSurfacePoolHandle->ReturnSurfaceToPool(surf.mEntry.mSurface);
   }
@@ -552,33 +544,25 @@ void NativeLayerCA::ApplyChanges() {
     if (mSurfaceIsFlipped) {
       CGFloat height = mSize.height / mBackingScale;
       mContentCALayer.affineTransform = CGAffineTransformMake(1.0, 0.0, 0.0, -1.0, 0.0, height);
     } else {
       mContentCALayer.affineTransform = CGAffineTransformIdentity;
     }
   }
 
+  if (mMutatedFrontSurface) {
+    mContentCALayer.contents = (id)mFrontSurface->mSurface.get();
+  }
+
   mMutatedPosition = false;
   mMutatedBackingScale = false;
   mMutatedSurfaceIsFlipped = false;
   mMutatedClipRect = false;
-
-  if (mReadySurface) {
-    mContentCALayer.contents = (id)mReadySurface->mSurface.get();
-    IOSurfaceDecrementUseCount(mReadySurface->mSurface.get());
-
-    if (mFrontSurface) {
-      mSurfaces.push_back({*mFrontSurface, 0});
-      mFrontSurface = Nothing();
-    }
-
-    mFrontSurface = Some(*mReadySurface);
-    mReadySurface = Nothing();
-  }
+  mMutatedFrontSurface = false;
 }
 
 // Called when mMutex is already being held by the current thread.
 Maybe<NativeLayerCA::SurfaceWithInvalidRegion> NativeLayerCA::GetUnusedSurfaceAndCleanUp(
     const MutexAutoLock&) {
   std::vector<SurfaceWithInvalidRegionAndCheckCount> usedSurfaces;
   Maybe<SurfaceWithInvalidRegion> unusedSurface;