1350493 - Speed up and clean up SVGTransformableElement::PrependLocalTransformsTo and its overrides

Status: RESOLVED FIXED

(Core :: SVG, enhancement)

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 3

[Robert Longson [:longsonr]]

Comment on attachment 8851185 [details] [diff] [review]
patch

>   if (IsInner()) {
>     float x, y;
>     const_cast<SVGSVGElement*>(this)->GetAnimatedLengthValues(&x, &y, nullptr);
>-    if (aWhich == eAllTransforms) {
>-      // the common case
>-      return ThebesMatrix(GetViewBoxTransform()) * gfxMatrix::Translation(x, y) * fromUserSpace;
>-    }
>-    MOZ_ASSERT(aWhich == eChildToUserSpace, "Unknown TransformTypes");
>-    return ThebesMatrix(GetViewBoxTransform()) * gfxMatrix::Translation(x, y) * aMatrix;
>+    childToUser = ThebesMatrix(GetViewBoxTransform().PostTranslate(x, y));
>+  } else if (IsRoot()) {
>+    childToUser = ThebesMatrix(GetViewBoxTransform()
>+                                 .PostScale(mCurrentScale, mCurrentScale)
>+                                 .PostTranslate(mCurrentTranslate.GetX(),
>+                                                mCurrentTranslate.GetY()));

Is this the right order? scale then translate?

>+  } else {
>+    // outer-<svg>, but inline in some other content:
>+    childToUser = ThebesMatrix(GetViewBoxTransform());
>   }
> 
>-  if (IsRoot()) {
>-    gfxMatrix zoomPanTM;
>-    zoomPanTM.Translate(gfxPoint(mCurrentTranslate.GetX(), mCurrentTranslate.GetY()));
>-    zoomPanTM.Scale(mCurrentScale, mCurrentScale);

i.e. wasn't it translate then scale here?

>+
>+  // We get here either when the identity matrix has been passed because our
>+  // caller just wants our eChildToUserSpace transform, or when aMatrix
>+  // already contains our eUserSpaceToParent transform (such as when we're
>+  // called from PaintSVG).

I guess you could assert that is true if you think it would make things more robust.

>+  return childToUser * aMatrix;
> }
> 

> {
>-  return SVGContentUtils::PrependLocalTransformsTo(
>-    aMatrix, aWhich, mAnimateMotionTransform, mTransforms);
>+  if (aWhich == eChildToUserSpace) {
>+    // We get here either when the identity matrix has been passed because our
>+    // caller just wants our eChildToUserSpace transform, or when aMatrix
>+    // already contains our eUserSpaceToParent transform (such as when we're
>+    // called from PaintSVG).

worth asserting that?

>+    // Anyway, we don't have any eUserSpaceToParent transforms. (Sub-classes
>+    // that do need to override this function.

What did you mean to say in this last partially bracketed sentence?

>+    return aMatrix;
>+  }
>+  return GetUserToParentTransform(mAnimateMotionTransform, mTransforms) * aMatrix;

>-    return toUserSpace * aMatrix;

>+
>+  // We get here either when the identity matrix has been passed because our
>+  // caller just wants our eChildToUserSpace transform, or when aMatrix
>+  // already contains our eUserSpaceToParent transform (such as when we're
>+  // called from PaintSVG).

maybe assert?

Comment 4

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #3)
> Comment on attachment 8851185 [details] [diff] [review]
> patch
> 
> >   if (IsInner()) {
> >     float x, y;
> >     const_cast<SVGSVGElement*>(this)->GetAnimatedLengthValues(&x, &y, nullptr);
> >-    if (aWhich == eAllTransforms) {
> >-      // the common case
> >-      return ThebesMatrix(GetViewBoxTransform()) * gfxMatrix::Translation(x, y) * fromUserSpace;
> >-    }
> >-    MOZ_ASSERT(aWhich == eChildToUserSpace, "Unknown TransformTypes");
> >-    return ThebesMatrix(GetViewBoxTransform()) * gfxMatrix::Translation(x, y) * aMatrix;
> >+    childToUser = ThebesMatrix(GetViewBoxTransform().PostTranslate(x, y));
> >+  } else if (IsRoot()) {
> >+    childToUser = ThebesMatrix(GetViewBoxTransform()
> >+                                 .PostScale(mCurrentScale, mCurrentScale)
> >+                                 .PostTranslate(mCurrentTranslate.GetX(),
> >+                                                mCurrentTranslate.GetY()));
> 
> Is this the right order? scale then translate?

In the removed code we first set 'zoomPanTM' to the translation:

  zoomPanTM.Translate(gfxPoint(mCurrentTranslate.GetX(), mCurrentTranslate.GetY()));

then pre-multiply it by the scale:

  zoomPanTM.Scale(mCurrentScale, mCurrentScale);

So we effectively have |scale * translate|. Then at:

  return ThebesMatrix(GetViewBoxTransform()) * zoomPanTM * fromUserSpace;

we end up with |viewBox * scale * translate|.

The new code does things in exactly the same order but using post-multiplication to avoid the full matrix multiplication in the removed code. (The scale ends up between the viewBox and translation in both cases.)

> >+  // We get here either when the identity matrix has been passed because our
> >+  // caller just wants our eChildToUserSpace transform, or when aMatrix
> >+  // already contains our eUserSpaceToParent transform (such as when we're
> >+  // called from PaintSVG).
> 
> I guess you could assert that is true if you think it would make things more
> robust.

Ah, I think the word "contains" here is confusing. How about I make this "or when our eUserSpaceToParent transform is already included in aMatrix". In the PaintSVG case, aMatrix includes the transforms from some ancestor through to this element including its eUserSpaceToParent transform. So it could contain anything, preventing us from usefully asserting.

> >+    // Anyway, we don't have any eUserSpaceToParent transforms. (Sub-classes
> >+    // that do need to override this function.
> 
> What did you mean to say in this last partially bracketed sentence?

Ah, thanks. I'm missing a comma (and the closing parenthesis). I meant to say "(Sub-classes that do, need to override this function.)"

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch

Here's a patch with the comments that caused issues rewritten. Hopefully this is clearer.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2de4332125a3
Speed up and clean up SVGTransformableElement::PrependLocalTransformsTo and its overrides. r=longsonr

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2de4332125a3