Bug 1385929 - Part 1. Check whether the content of the persisted state change. draft
authorcku <cku@mozilla.com>
Tue, 01 Aug 2017 16:33:08 +0800
changeset 642379 94dc9a63313c00b8d04934cd8e3a8d3ced8944bd
parent 642204 65507616792c990b1230888612dd7ffc13ed32b4
child 642380 6b0509ae013b3f0d8d1a933c9c8df096289fdac5
push id72717
push userbmo:cku@mozilla.com
push dateTue, 08 Aug 2017 03:21:20 +0000
bugs1385929
milestone57.0a1
Bug 1385929 - Part 1. Check whether the content of the persisted state change. Since gfxContext::Save keep appear on my screen when I did profile, so I think we should find a way to prevent unecessary usage of this function. By this patch, an assertion message will be dump if we save and restore an unchanged AzureState. MozReview-Commit-ID: 5lH1Y5T5K7t
gfx/thebes/gfxContext.cpp
gfx/thebes/gfxContext.h
--- a/gfx/thebes/gfxContext.cpp
+++ b/gfx/thebes/gfxContext.cpp
@@ -29,16 +29,22 @@
 #include "mozilla/gfx/DeviceManagerDx.h"
 #endif
 
 using namespace mozilla;
 using namespace mozilla::gfx;
 
 UserDataKey gfxContext::sDontUseAsSourceKey;
 
+#ifdef DEBUG
+  #define CURRENTSTATE_CHANGED() \
+    CurrentState().mContentChanged = true;
+#else
+  #define CURRENTSTATE_CHANGED()
+#endif
 
 PatternFromState::operator mozilla::gfx::Pattern&()
 {
   gfxContext::AzureState &state = mContext->CurrentState();
 
   if (state.pattern) {
     return *state.pattern->GetPattern(mContext->mDT, state.patternTransformChanged ? &state.patternTransform : nullptr);
   }
@@ -119,21 +125,60 @@ gfxContext::~gfxContext()
 }
 
 void
 gfxContext::Save()
 {
   CurrentState().transform = mTransform;
   mStateStack.AppendElement(AzureState(CurrentState()));
   CurrentState().pushedClips.Clear();
+#ifdef DEBUG
+  CurrentState().mContentChanged = false;
+#endif
 }
 
 void
 gfxContext::Restore()
 {
+#ifdef DEBUG
+  // gfxContext::Restore is used to restore AzureState. We need to restore it
+  // only if it was altered. The following APIs do change the content of
+  // AzureState, a user should save the state before using them and restore it
+  // after finishing painting:
+  // 1. APIs to setup how to paint, such as SetColor()/SetAntialiasMode(). All
+  //    gfxContext SetXXXX public functions belong to this category, except
+  //    gfxContext::SetPath & gfxContext::SetMatrix.
+  // 2. Clip functions, such as Clip() or PopClip(). You may call PopClip()
+  //    directly instead of using gfxContext::Save if the clip region is the
+  //    only thing that you altered in the target context.
+  // 3. Function of setup transform matrix, such as Multiply() and
+  //    SetMatrix(). Using gfxContextMatrixAutoSaveRestore is more recommended
+  //    if transform data is the only thing that you are going to alter.
+  //
+  // You will hit the assertion message below if there is no above functions
+  // been used between a pair of gfxContext::Save and gfxContext::Restore.
+  // Considerate to remove that pair of Save/Restore if hitting that assertion.
+  //
+  // In the other hand, the following APIs do not alter the content of the
+  // current AzureState, therefore, there is no need to save & restore
+  // AzureState:
+  // 1. constant member functions of gfxContext.
+  // 2. Paint calls, such as Line()/Rectangle()/Fill(). Those APIs change the
+  //    content of drawing buffer, which is not part of AzureState.
+  // 3. Path building APIs, such as SetPath()/MoveTo()/LineTo()/NewPath().
+  //    Surprisingly, path information is not stored in AzureState either.
+  // Save current AzureState before using these type of APIs does nothing but
+  // make performance worse.
+  NS_ASSERTION(CurrentState().mContentChanged ||
+               CurrentState().pushedClips.Length() > 0,
+               "The context of the current AzureState is not altered after "
+               "Save() been called. you may consider to remove this pair of "
+               "gfxContext::Save/Restore.");
+#endif
+
   for (unsigned int c = 0; c < CurrentState().pushedClips.Length(); c++) {
     mDT->PopClip();
   }
 
   mStateStack.RemoveElementAt(mStateStack.Length() - 1);
 
   mDT = CurrentState().drawTarget;
 
@@ -253,22 +298,24 @@ gfxContext::Rectangle(const gfxRect& rec
   mPathBuilder->LineTo(rec.BottomLeft());
   mPathBuilder->Close();
 }
 
 // transform stuff
 void
 gfxContext::Multiply(const gfxMatrix& matrix)
 {
+  CURRENTSTATE_CHANGED()
   ChangeTransform(ToMatrix(matrix) * mTransform);
 }
 
 void
 gfxContext::SetMatrix(const gfxMatrix& matrix)
 {
+  CURRENTSTATE_CHANGED()
   ChangeTransform(ToMatrix(matrix));
 }
 
 gfxMatrix
 gfxContext::CurrentMatrix() const
 {
   return ThebesMatrix(mTransform);
 }
@@ -377,28 +424,30 @@ gfxContext::UserToDevicePixelSnapped(gfx
   pt = UserToDevice(pt);
   pt.Round();
   return true;
 }
 
 void
 gfxContext::SetAntialiasMode(AntialiasMode mode)
 {
+  CURRENTSTATE_CHANGED()
   CurrentState().aaMode = mode;
 }
 
 AntialiasMode
 gfxContext::CurrentAntialiasMode() const
 {
   return CurrentState().aaMode;
 }
 
 void
 gfxContext::SetDash(gfxFloat *dashes, int ndash, gfxFloat offset)
 {
+  CURRENTSTATE_CHANGED()
   AzureState &state = CurrentState();
 
   state.dashPattern.SetLength(ndash);
   for (int i = 0; i < ndash; i++) {
     state.dashPattern[i] = Float(dashes[i]);
   }
   state.strokeOptions.mDashLength = ndash;
   state.strokeOptions.mDashOffset = Float(offset);
@@ -441,52 +490,56 @@ gfxFloat
 gfxContext::CurrentLineWidth() const
 {
   return CurrentState().strokeOptions.mLineWidth;
 }
 
 void
 gfxContext::SetOp(CompositionOp aOp)
 {
+  CURRENTSTATE_CHANGED()
   CurrentState().op = aOp;
 }
 
 CompositionOp
 gfxContext::CurrentOp() const
 {
   return CurrentState().op;
 }
 
 void
 gfxContext::SetLineCap(CapStyle cap)
 {
+  CURRENTSTATE_CHANGED()
   CurrentState().strokeOptions.mLineCap = cap;
 }
 
 CapStyle
 gfxContext::CurrentLineCap() const
 {
   return CurrentState().strokeOptions.mLineCap;
 }
 
 void
 gfxContext::SetLineJoin(JoinStyle join)
 {
+  CURRENTSTATE_CHANGED()
   CurrentState().strokeOptions.mLineJoin = join;
 }
 
 JoinStyle
 gfxContext::CurrentLineJoin() const
 {
   return CurrentState().strokeOptions.mLineJoin;
 }
 
 void
 gfxContext::SetMiterLimit(gfxFloat limit)
 {
+  CURRENTSTATE_CHANGED()
   CurrentState().strokeOptions.mMiterLimit = Float(limit);
 }
 
 gfxFloat
 gfxContext::CurrentMiterLimit() const
 {
   return CurrentState().strokeOptions.mMiterLimit;
 }
@@ -623,25 +676,27 @@ gfxContext::ClipContainsRect(const gfxRe
   return clipBounds.Contains(ToRect(aRect));
 }
 
 // rendering sources
 
 void
 gfxContext::SetColor(const Color& aColor)
 {
+  CURRENTSTATE_CHANGED()
   CurrentState().pattern = nullptr;
   CurrentState().sourceSurfCairo = nullptr;
   CurrentState().sourceSurface = nullptr;
   CurrentState().color = ToDeviceColor(aColor);
 }
 
 void
 gfxContext::SetDeviceColor(const Color& aColor)
 {
+  CURRENTSTATE_CHANGED()
   CurrentState().pattern = nullptr;
   CurrentState().sourceSurfCairo = nullptr;
   CurrentState().sourceSurface = nullptr;
   CurrentState().color = aColor;
 }
 
 bool
 gfxContext::GetDeviceColor(Color& aColorOut)
@@ -655,30 +710,32 @@ gfxContext::GetDeviceColor(Color& aColor
 
   aColorOut = CurrentState().color;
   return true;
 }
 
 void
 gfxContext::SetSource(gfxASurface *surface, const gfxPoint& offset)
 {
+  CURRENTSTATE_CHANGED()
   CurrentState().surfTransform = Matrix(1.0f, 0, 0, 1.0f, Float(offset.x), Float(offset.y));
   CurrentState().pattern = nullptr;
   CurrentState().patternTransformChanged = false;
   // Keep the underlying cairo surface around while we keep the
   // sourceSurface.
   CurrentState().sourceSurfCairo = surface;
   CurrentState().sourceSurface =
   gfxPlatform::GetPlatform()->GetSourceSurfaceForSurface(mDT, surface);
   CurrentState().color = Color(0, 0, 0, 0);
 }
 
 void
 gfxContext::SetPattern(gfxPattern *pattern)
 {
+  CURRENTSTATE_CHANGED()
   CurrentState().sourceSurfCairo = nullptr;
   CurrentState().sourceSurface = nullptr;
   CurrentState().patternTransformChanged = false;
   CurrentState().pattern = pattern;
 }
 
 already_AddRefed<gfxPattern>
 gfxContext::GetPattern()
@@ -694,16 +751,17 @@ gfxContext::GetPattern()
     pat = new gfxPattern(state.color);
   }
   return pat.forget();
 }
 
 void
 gfxContext::SetFontSmoothingBackgroundColor(const Color& aColor)
 {
+  CURRENTSTATE_CHANGED()
   CurrentState().fontSmoothingBackgroundColor = aColor;
 }
 
 Color
 gfxContext::GetFontSmoothingBackgroundColor()
 {
   return CurrentState().fontSmoothingBackgroundColor;
 }
--- a/gfx/thebes/gfxContext.h
+++ b/gfx/thebes/gfxContext.h
@@ -473,16 +473,19 @@ private:
 
   struct AzureState {
     AzureState()
       : op(mozilla::gfx::CompositionOp::OP_OVER)
       , color(0, 0, 0, 1.0f)
       , aaMode(mozilla::gfx::AntialiasMode::SUBPIXEL)
       , patternTransformChanged(false)
       , mBlendOpacity(0.0f)
+#ifdef DEBUG
+      , mContentChanged(false)
+#endif
     {}
 
     mozilla::gfx::CompositionOp op;
     Color color;
     RefPtr<gfxPattern> pattern;
     RefPtr<gfxASurface> sourceSurfCairo;
     RefPtr<SourceSurface> sourceSurface;
     mozilla::gfx::Point sourceSurfaceDeviceOffset;
@@ -504,16 +507,18 @@ private:
     // This is used solely for using minimal intermediate surface size.
     mozilla::gfx::Point deviceOffset;
     // Support groups
     mozilla::gfx::Float mBlendOpacity;
     RefPtr<SourceSurface> mBlendMask;
     Matrix mBlendMaskTransform;
 #ifdef DEBUG
     bool mWasPushedForBlendBack;
+    // Whether the content of this AzureState changed after construction.
+    bool mContentChanged;
 #endif
   };
 
   // This ensures mPath contains a valid path (in user space!)
   void EnsurePath();
   // This ensures mPathBuilder contains a valid PathBuilder (in user space!)
   void EnsurePathBuilder();
   void FillAzure(const Pattern& aPattern, mozilla::gfx::Float aOpacity);