Bug 814952: Further cleanup state management surrounding paths and pathbuilders. r=jrmuizel
☠☠ backed out by edd575426780 ☠ ☠
authorBas Schouten <bschouten@mozilla.com>
Wed, 12 Dec 2012 21:37:09 +0100
changeset 115832 3157073496063475f594b52a6228345b42b5000f
parent 115831 362510d94a1bcf96fda34a3fa4610918d8e23609
child 115833 9744174142664ea24640ae27240b3056fbcdea7e
push id24028
push useremorley@mozilla.com
push dateThu, 13 Dec 2012 15:56:02 +0000
treeherderautoland@9db79b97abbb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjrmuizel
bugs814952
milestone20.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 814952: Further cleanup state management surrounding paths and pathbuilders. r=jrmuizel
gfx/thebes/gfxContext.cpp
gfx/thebes/gfxContext.h
--- a/gfx/thebes/gfxContext.cpp
+++ b/gfx/thebes/gfxContext.cpp
@@ -202,28 +202,19 @@ gfxContext::Restore()
 
     if (CurrentState().clipWasReset &&
         CurrentState().drawTarget == mStateStack[mStateStack.Length() - 2].drawTarget) {
       PushClipsToDT(mDT);
     }
 
     mStateStack.RemoveElementAt(mStateStack.Length() - 1);
 
-    if ((mPathBuilder || mPath || mPathIsRect) && !mTransformChanged) {
-      // Support here isn't fully correct if the path is continued -after-
-      // the restore. We don't currently have users that do this and we should
-      // make sure there will not be any. Sadly we can't assert this easily.
-      mTransformChanged = true;
-      mPathTransform = mTransform;
-    }
-
     mDT = CurrentState().drawTarget;
 
-    mTransform = CurrentState().transform;
-    mDT->SetTransform(GetDTTransform());
+    ChangeTransform(CurrentState().transform, false);
   }
 }
 
 // drawing
 void
 gfxContext::NewPath()
 {
   if (mCairo) {
@@ -285,16 +276,18 @@ gfxContext::CurrentPoint()
 void
 gfxContext::Stroke()
 {
   if (mCairo) {
     cairo_stroke_preserve(mCairo);
   } else {
     AzureState &state = CurrentState();
     if (mPathIsRect) {
+      MOZ_ASSERT(!mTransformChanged);
+
       mDT->StrokeRect(mRect, GeneralPattern(this),
                       state.strokeOptions,
                       DrawOptions(1.0f, GetOp(), state.aaMode));
     } else {
       EnsurePath();
 
       mDT->Stroke(mPath, GeneralPattern(this), state.strokeOptions,
                   DrawOptions(1.0f, GetOp(), state.aaMode));
@@ -469,20 +462,20 @@ gfxContext::Rectangle(const gfxRect& rec
         rec = ToRect(mat.TransformBounds(newRect));
       }
     }
 
     if (!mPathBuilder && !mPathIsRect) {
       mPathIsRect = true;
       mRect = rec;
       return;
-    } else if (!mPathBuilder) {
-      EnsurePathBuilder();
     }
-    
+
+    EnsurePathBuilder();
+
     mPathBuilder->MoveTo(rec.TopLeft());
     mPathBuilder->LineTo(rec.TopRight());
     mPathBuilder->LineTo(rec.BottomRight());
     mPathBuilder->LineTo(rec.BottomLeft());
     mPathBuilder->Close();
   }
 }
 
@@ -1133,17 +1126,19 @@ gfxContext::Clip(const gfxRect& rect)
 }
 
 void
 gfxContext::Clip()
 {
   if (mCairo) {
     cairo_clip_preserve(mCairo);
   } else {
-    if (mPathIsRect && !mTransformChanged) {
+    if (mPathIsRect) {
+      MOZ_ASSERT(!mTransformChanged);
+
       AzureState::PushedClip clip = { NULL, mRect, mTransform };
       CurrentState().pushedClips.AppendElement(clip);
       mDT->PushClipRect(mRect);
     } else {
       EnsurePath();
       mDT->PushClip(mPath);
       AzureState::PushedClip clip = { mPath, Rect(), mTransform };
       CurrentState().pushedClips.AppendElement(clip);
@@ -1945,56 +1940,75 @@ gfxContext::EnsurePath()
   EnsurePathBuilder();
   mPath = mPathBuilder->Finish();
   mPathBuilder = NULL;
 }
 
 void
 gfxContext::EnsurePathBuilder()
 {
-  if (mPathBuilder) {
+  if (mPathBuilder && !mTransformChanged) {
     return;
   }
 
   if (mPath) {
-    mPathBuilder = mPath->CopyToBuilder(CurrentState().fillRule);
-    mPath = NULL;
+    if (!mTransformChanged) {
+      mPathBuilder = mPath->CopyToBuilder(CurrentState().fillRule);
+      mPath = NULL;
+    } else {
+      Matrix invTransform = mTransform;
+      invTransform.Invert();
+      Matrix toNewUS = mPathTransform * invTransform;
+      mPathBuilder = mPath->TransformedCopyToBuilder(toNewUS, CurrentState().fillRule);
+    }
+    return;
   }
 
-  mPathBuilder = mDT->CreatePathBuilder(CurrentState().fillRule);
+  DebugOnly<PathBuilder*> oldPath = mPathBuilder;
+
+  if (!mPathBuilder) {
+    mPathBuilder = mDT->CreatePathBuilder(CurrentState().fillRule);
+
+    if (mPathIsRect) {
+      mPathBuilder->MoveTo(mRect.TopLeft());
+      mPathBuilder->LineTo(mRect.TopRight());
+      mPathBuilder->LineTo(mRect.BottomRight());
+      mPathBuilder->LineTo(mRect.BottomLeft());
+      mPathBuilder->Close();
+    }
+  }
 
-  if (mPathIsRect && !mTransformChanged) {
-    mPathBuilder->MoveTo(mRect.TopLeft());
-    mPathBuilder->LineTo(mRect.TopRight());
-    mPathBuilder->LineTo(mRect.BottomRight());
-    mPathBuilder->LineTo(mRect.BottomLeft());
-    mPathBuilder->Close();
-  } else if (mPathIsRect) {
-    mTransformChanged = false;
-    Matrix mat = mTransform;
-    mat.Invert();
-    mat = mPathTransform * mat;
-    mPathBuilder->MoveTo(mat * mRect.TopLeft());
-    mPathBuilder->LineTo(mat * mRect.TopRight());
-    mPathBuilder->LineTo(mat * mRect.BottomRight());
-    mPathBuilder->LineTo(mat * mRect.BottomLeft());
-    mPathBuilder->Close();
+  if (mTransformChanged) {
+    // This could be an else if since this should never happen when
+    // mPathBuilder is NULL and mPath is NULL. But this way we can assert
+    // if all the state is as expected.
+    MOZ_ASSERT(oldPath);
+    MOZ_ASSERT(!mPathIsRect);
+
+    Matrix invTransform = mTransform;
+    invTransform.Invert();
+    Matrix toNewUS = mPathTransform * invTransform;
+
+    RefPtr<Path> path = mPathBuilder->Finish();
+    mPathBuilder = path->TransformedCopyToBuilder(toNewUS, CurrentState().fillRule);
   }
 
   mPathIsRect = false;
 }
 
 void
 gfxContext::FillAzure(Float aOpacity)
 {
   AzureState &state = CurrentState();
 
   CompositionOp op = GetOp();
 
-  if (mPathIsRect && !mTransformChanged) {
+  if (mPathIsRect) {
+    MOZ_ASSERT(!mTransformChanged);
+
     if (state.opIsClear) {
       mDT->ClearRect(mRect);
     } else if (op == OP_SOURCE) {
       // Emulate cairo operator source which is bound by mask!
       mDT->ClearRect(mRect);
       mDT->FillRect(mRect, GeneralPattern(this), DrawOptions(aOpacity));
     } else {
       mDT->FillRect(mRect, GeneralPattern(this), DrawOptions(aOpacity, op, state.aaMode));
@@ -2064,55 +2078,62 @@ gfxContext::GetOp()
       return OP_SOURCE;
     }
   }
 }
 
 /* SVG font code can change the transform after having set the pattern on the
  * context. When the pattern is set it is in user space, if the transform is
  * changed after doing so the pattern needs to be converted back into userspace.
- * We just store the old pattern here so that we only do the work needed here
- * if the pattern is actually used.
+ * We just store the old pattern transform here so that we only do the work
+ * needed here if the pattern is actually used.
+ * We need to avoid doing this when this ChangeTransform comes from a restore,
+ * 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)
+gfxContext::ChangeTransform(const Matrix &aNewMatrix, bool aUpdatePatternTransform)
 {
   AzureState &state = CurrentState();
 
-  if ((state.pattern || state.sourceSurface)
+  if (aUpdatePatternTransform && (state.pattern || state.sourceSurface)
       && !state.patternTransformChanged) {
     state.patternTransform = mTransform;
     state.patternTransformChanged = true;
   }
 
-  if (mPathBuilder || mPathIsRect) {
+  if (mPathIsRect) {
     Matrix invMatrix = aNewMatrix;
     
     invMatrix.Invert();
 
     Matrix toNewUS = mTransform * invMatrix;
 
-    if (toNewUS.IsRectilinear() && mPathIsRect) {
+    if (toNewUS.IsRectilinear()) {
       mRect = toNewUS.TransformBounds(mRect);
       mRect.NudgeToIntegers();
-    } else if (mPathIsRect) {
+    } else {
       mPathBuilder = mDT->CreatePathBuilder(CurrentState().fillRule);
       
       mPathBuilder->MoveTo(toNewUS * mRect.TopLeft());
       mPathBuilder->LineTo(toNewUS * mRect.TopRight());
       mPathBuilder->LineTo(toNewUS * mRect.BottomRight());
       mPathBuilder->LineTo(toNewUS * mRect.BottomLeft());
       mPathBuilder->Close();
-    } else {
-      RefPtr<Path> path = mPathBuilder->Finish();
-      // Create path in device space.
-      mPathBuilder = path->TransformedCopyToBuilder(toNewUS);
+
+      mPathIsRect = false;
     }
+
     // No need to consider the transform changed now!
     mTransformChanged = false;
+  } else if ((mPath || mPathBuilder) && !mTransformChanged) {
+    mTransformChanged = true;
+    mPathTransform = mTransform;
   }
 
   mTransform = aNewMatrix;
 
   mDT->SetTransform(GetDTTransform());
 }
 
 Rect
--- a/gfx/thebes/gfxContext.h
+++ b/gfx/thebes/gfxContext.h
@@ -760,17 +760,17 @@ private:
 
   // 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(mozilla::gfx::Float aOpacity);
   void PushClipsToDT(mozilla::gfx::DrawTarget *aDT);
   CompositionOp GetOp();
-  void ChangeTransform(const mozilla::gfx::Matrix &aNewMatrix);
+  void ChangeTransform(const mozilla::gfx::Matrix &aNewMatrix, bool aUpdatePatternTransform = true);
   Rect GetAzureDeviceSpaceClipBounds();
   Matrix GetDeviceTransform() const;
   Matrix GetDTTransform() const;
   void PushNewDT(gfxASurface::gfxContentType content);
 
   bool mPathIsRect;
   bool mTransformChanged;
   Matrix mPathTransform;