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 619565 e24c2a3a98c6243c974a30f1eeb80322f0d4b7d9
parent 619024 031fa984499deeaac1f843be682a133fb2344204
child 619566 5a787f09ddeded2f3097a995f267fbce8d88d493
push id71719
push userbmo:cku@mozilla.com
push dateWed, 02 Aug 2017 07:36:27 +0000
bugs1385929
milestone56.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(_changed) \
+    CurrentState().mContentChanged |= _changed;
+#else
+  #define CURRENTSTATE_CHANGED(_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,59 @@ 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 context 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 bellow 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,
+               "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;
 
@@ -377,28 +421,30 @@ gfxContext::UserToDevicePixelSnapped(gfx
   pt = UserToDevice(pt);
   pt.Round();
   return true;
 }
 
 void
 gfxContext::SetAntialiasMode(AntialiasMode mode)
 {
+  CURRENTSTATE_CHANGED(true)
   CurrentState().aaMode = mode;
 }
 
 AntialiasMode
 gfxContext::CurrentAntialiasMode() const
 {
   return CurrentState().aaMode;
 }
 
 void
 gfxContext::SetDash(gfxFloat *dashes, int ndash, gfxFloat offset)
 {
+  CURRENTSTATE_CHANGED(true)
   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,88 +487,96 @@ gfxFloat
 gfxContext::CurrentLineWidth() const
 {
   return CurrentState().strokeOptions.mLineWidth;
 }
 
 void
 gfxContext::SetOp(CompositionOp aOp)
 {
+  CURRENTSTATE_CHANGED(true)
   CurrentState().op = aOp;
 }
 
 CompositionOp
 gfxContext::CurrentOp() const
 {
   return CurrentState().op;
 }
 
 void
 gfxContext::SetLineCap(CapStyle cap)
 {
+  CURRENTSTATE_CHANGED(true)
   CurrentState().strokeOptions.mLineCap = cap;
 }
 
 CapStyle
 gfxContext::CurrentLineCap() const
 {
   return CurrentState().strokeOptions.mLineCap;
 }
 
 void
 gfxContext::SetLineJoin(JoinStyle join)
 {
+  CURRENTSTATE_CHANGED(true)
   CurrentState().strokeOptions.mLineJoin = join;
 }
 
 JoinStyle
 gfxContext::CurrentLineJoin() const
 {
   return CurrentState().strokeOptions.mLineJoin;
 }
 
 void
 gfxContext::SetMiterLimit(gfxFloat limit)
 {
+  CURRENTSTATE_CHANGED(true)
   CurrentState().strokeOptions.mMiterLimit = Float(limit);
 }
 
 gfxFloat
 gfxContext::CurrentMiterLimit() const
 {
   return CurrentState().strokeOptions.mMiterLimit;
 }
 
 // clipping
 void
 gfxContext::Clip(const Rect& rect)
 {
+  CURRENTSTATE_CHANGED(true)
   AzureState::PushedClip clip = { nullptr, rect, mTransform };
   CurrentState().pushedClips.AppendElement(clip);
   mDT->PushClipRect(rect);
   NewPath();
 }
 
 void
 gfxContext::Clip(const gfxRect& rect)
 {
+  CURRENTSTATE_CHANGED(true)
   Clip(ToRect(rect));
 }
 
 void
 gfxContext::Clip(Path* aPath)
 {
+  CURRENTSTATE_CHANGED(true)
   mDT->PushClip(aPath);
   AzureState::PushedClip clip = { aPath, Rect(), mTransform };
   CurrentState().pushedClips.AppendElement(clip);
 }
 
 void
 gfxContext::Clip()
 {
+  CURRENTSTATE_CHANGED(true)
   if (mPathIsRect) {
     MOZ_ASSERT(!mTransformChanged);
 
     AzureState::PushedClip clip = { nullptr, mRect, mTransform };
     CurrentState().pushedClips.AppendElement(clip);
     mDT->PushClipRect(mRect);
   } else {
     EnsurePath();
@@ -530,16 +584,17 @@ gfxContext::Clip()
     AzureState::PushedClip clip = { mPath, Rect(), mTransform };
     CurrentState().pushedClips.AppendElement(clip);
   }
 }
 
 void
 gfxContext::PopClip()
 {
+  CURRENTSTATE_CHANGED(true)
   MOZ_ASSERT(CurrentState().pushedClips.Length() > 0);
 
   CurrentState().pushedClips.RemoveElementAt(CurrentState().pushedClips.Length() - 1);
   mDT->PopClip();
 }
 
 gfxRect
 gfxContext::GetClipExtents()
@@ -623,25 +678,27 @@ gfxContext::ClipContainsRect(const gfxRe
   return clipBounds.Contains(ToRect(aRect));
 }
 
 // rendering sources
 
 void
 gfxContext::SetColor(const Color& aColor)
 {
+  CURRENTSTATE_CHANGED(true)
   CurrentState().pattern = nullptr;
   CurrentState().sourceSurfCairo = nullptr;
   CurrentState().sourceSurface = nullptr;
   CurrentState().color = ToDeviceColor(aColor);
 }
 
 void
 gfxContext::SetDeviceColor(const Color& aColor)
 {
+  CURRENTSTATE_CHANGED(true)
   CurrentState().pattern = nullptr;
   CurrentState().sourceSurfCairo = nullptr;
   CurrentState().sourceSurface = nullptr;
   CurrentState().color = aColor;
 }
 
 bool
 gfxContext::GetDeviceColor(Color& aColorOut)
@@ -655,30 +712,32 @@ gfxContext::GetDeviceColor(Color& aColor
 
   aColorOut = CurrentState().color;
   return true;
 }
 
 void
 gfxContext::SetSource(gfxASurface *surface, const gfxPoint& offset)
 {
+  CURRENTSTATE_CHANGED(true)
   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(true)
   CurrentState().sourceSurfCairo = nullptr;
   CurrentState().sourceSurface = nullptr;
   CurrentState().patternTransformChanged = false;
   CurrentState().pattern = pattern;
 }
 
 already_AddRefed<gfxPattern>
 gfxContext::GetPattern()
@@ -694,16 +753,17 @@ gfxContext::GetPattern()
     pat = new gfxPattern(state.color);
   }
   return pat.forget();
 }
 
 void
 gfxContext::SetFontSmoothingBackgroundColor(const Color& aColor)
 {
+  CURRENTSTATE_CHANGED(true)
   CurrentState().fontSmoothingBackgroundColor = aColor;
 }
 
 Color
 gfxContext::GetFontSmoothingBackgroundColor()
 {
   return CurrentState().fontSmoothingBackgroundColor;
 }
@@ -974,16 +1034,17 @@ gfxContext::GetOp()
  * since the current pattern and the current transform are both part of the
  * state we know the new CurrentState()'s values are valid. But if we assume
  * a change they might become invalid since patternTransformChanged is part of
  * the state and might be false for the restored AzureState.
  */
 void
 gfxContext::ChangeTransform(const Matrix &aNewMatrix, bool aUpdatePatternTransform)
 {
+  CURRENTSTATE_CHANGED(aUpdatePatternTransform)
   AzureState &state = CurrentState();
 
   if (aUpdatePatternTransform && (state.pattern || state.sourceSurface)
       && !state.patternTransformChanged) {
     state.patternTransform = GetDTTransform();
     state.patternTransformChanged = true;
   }
 
--- 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);