984278 - Convert all uses of gfxMatrix to Moz2D Matrix in content/svg/

Status: RESOLVED WONTFIX

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Part of the Moz2Dification is to convert all uses of gfxMatrix to Moz2D Matrix in content/svg/.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Rename SVGMatrix::Matrix and SVGTransform::Matrix to SVGMatrix::GetMatrix and SVGTransform::GetMatrix, respectively

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8392089 [details] [diff] [review]
part 1 - Rename SVGMatrix::Matrix and SVGTransform::Matrix to SVGMatrix::GetMatrix and SVGTransform::GetMatrix, respectively

(The motivation behind this rename, for anyone else who's curious: it's a recipe for disaster to have a method called Matrix() and a class called Matrix in the same file, because then the expression "Matrix()" is ambiguous (are you calling the local method or constructing an anonymous Matrix?) and causes compile errors and/or human error.)

r=me

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Convert all uses of gfxMatrix to Moz2D Matrix in content/svg/

Comment 4

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/341c371eb41b

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/341c371eb41b

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Convert all uses of gfxMatrix to Moz2D Matrix in content/svg/

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8393317 [details] [diff] [review]
part 2 -  Convert all uses of gfxMatrix to Moz2D Matrix in content/svg/

>+++ b/content/svg/content/src/SVGMatrix.h
>  * It's important to note that the matrix convention used in the SVG standard
>  * is the opposite convention to the one used in the Mozilla code or, more
>- * specifically, the convention used in Thebes code (code using gfxMatrix).
>+ * specifically, the convention used in Thebes code (code using Matrix).

I think this comment needs s/Thebes/Moz2D/g, right? (gfxMatrix is Thebes, whereas Matrix is Moz2D, IIUC)

(There are several mentions of "Thebes" later in this comment, too.)

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8393317 [details] [diff] [review]
part 2 -  Convert all uses of gfxMatrix to Moz2D Matrix in content/svg/

PARTIAL REVIEW:
>+++ b/content/svg/content/src/SVGMarkerElement.cpp
>@@ -354,17 +354,17 @@ SVGMarkerElement::GetViewBoxTransform()
>                                            viewbox.width, viewbox.height,
>                                            mPreserveAspectRatio);
> 
>     float refX = mLengthAttributes[REFX].GetAnimValue(mCoordCtx);
>     float refY = mLengthAttributes[REFY].GetAnimValue(mCoordCtx);
> 
>     gfx::Point ref = viewBoxTM * gfx::Point(refX, refY);
> 
>-    gfx::Matrix TM = viewBoxTM * gfx::Matrix().Translate(-ref.x, -ref.y);
>+    gfx::Matrix TM = viewBoxTM * gfx::Matrix::Translation(-ref.x, -ref.y);
> 
>     mViewBoxToViewportTransform = new gfx::Matrix(TM);

Observation: it doesn't look like |TM| is useful here - we immediately copy its value into mViewBoxToViewportTransform and then throw TM away. If I'm not misunderstanding, could you file a followup on cleaning that up? (or perhaps just fix that here, if that's easier)

Also, looks like this file wants some "using namespace mozilla::gfx" love -- it's got gfx::Matrix all over the place. (I noticed other mentions of "gfx::Matrix" in nearby code in other files, too -- e.g. mAnimateMotionTransform at the bottom of SVGTransformableElement.h is of type gfx::Matrix). Could you file a followup on globally replacing gfx::Matrix with Matrix in content/svg (where possible/trivial)?

>+++ b/content/svg/content/src/SVGPathElement.h
> #include "mozilla/gfx/2D.h"
>+#include "mozilla/gfx/Matrix.h"
[...]
> class SVGPathElement MOZ_FINAL : public SVGPathElementBase
> {
> friend class nsSVGPathFrame;
> 
>+  typedef mozilla::gfx::Matrix Matrix;

This file doesn't actually use mozilla::gfx::Matrix, so it's odd that it's adding an #include and typedef here.

Ah; if I remove the typedef, I get a build error for an instance of "Matrix()" in SVGPathElement.cpp, which is an error because the ancestor class "SVGTransformableElement" has its own *private* Matrix typedef.  So the compiler is matching Matrix() against the (private) $ANCESTORCLASS::Matrix() constructor instead of the (public) gfx::mozilla::Matrix() constructor.

This is bad -- the ancestor's convenience typedef is ruining things for all the child classes and forcing them to add their own typedefs as well, even when they don't otherwise need them.

To avoid this problem and the unnecessary typedef-proliferation that it would produce, could you label SVGTransformableElement's Matrix typedef as either 'protected' or 'public'?  (protected seems slightly cleaner, since IIUC this only matters for descendant classes, but I'm happy either way.) Then, you can revert the changes to SVGPathElement.h in this patch. (maybe other files, too?)

>diff --git a/content/svg/content/src/SVGTransform.cpp b/content/svg/content/src/SVGTransform.cpp
>--- a/content/svg/content/src/SVGTransform.cpp
>+++ b/content/svg/content/src/SVGTransform.cpp 
> void
>-SVGTransform::SetMatrix(const gfxMatrix& aMatrix)
>+SVGTransform::SetMatrix(const Matrix& aMatrix)
> {
>   NS_ABORT_IF_FALSE(!mIsAnimValItem,
>       "Attempting to modify read-only transform");
> 
>   if (Transform().Type() == SVG_TRANSFORM_MATRIX &&
>-      nsSVGTransform::MatricesEqual(Matrixgfx(), aMatrix)) {
>+      GetInternalMatrix() == aMatrix) {
>     return;

Note that this isn't quite equivalent -- MatricesEqual() compares the components for exact equality, whereas Matrix::operator== uses fuzzy comparison.

gfxMatrix::operator== *also* uses fuzzy comparison, and we've explicitly chosen not to use it here so far.  Maybe that means we should add a Matrix version of MatricesEqual and use it here?  (Otherwise, SetMatrix() with a subtly-different matrix will be a no-op, which seems incorrect.)

I think this sort of change (s/MatricesEqual/operator==) happened in a few other places, too.

>diff --git a/content/svg/content/src/SVGUseElement.h b/content/svg/content/src/SVGUseElement.h
>+#include "mozilla/gfx/Matrix.h"
[...]
>   // nsSVGElement specializations:
>-  virtual gfxMatrix PrependLocalTransformsTo(const gfxMatrix &aMatrix,
>+  virtual Matrix PrependLocalTransformsTo(const Matrix &aMatrix,
>                       TransformTypes aWhich = eAllTransforms) const MOZ_OVERRIDE;

I don't think you need to #include Matrix.h here -- you can just forward-declare it, like you do in nsSVGElement.h.

I believe this is also true of:
 SVGForeignObjectElement.h
 SVGSVGElement.h
 SVGTransform.h
 SVGUseElement.h (the file quoted above)

> nsSVGTransform::SetTranslate(float aTx, float aTy)
> {
>   mType    = SVG_TRANSFORM_TRANSLATE;
>-  mMatrix.Reset();
>-  mMatrix.x0 = aTx;
>-  mMatrix.y0 = aTy;
>+  mMatrix = Matrix::Translation(aTx, aTy);
[...]
> void
> nsSVGTransform::SetScale(float aSx, float aSy)
> {
>   mType    = SVG_TRANSFORM_SCALE;
>-  mMatrix.Reset();
>-  mMatrix.xx = aSx;
>-  mMatrix.yy = aSy;
>+  mMatrix = Matrix();
>+  mMatrix._11 = aSx;
>+  mMatrix._22 = aSy;

Probably better to replace those last 3 lines with
  mMatrix = Matrix()::Scaling(aSx, aSy);
for brevity & consistency with the above code that uses ::Translation() instead of directly setting components.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8393317 [details] [diff] [review]
part 2 -  Convert all uses of gfxMatrix to Moz2D Matrix in content/svg/

Is there a bug tracking gfxMatrix & ThebesMatrix() removal in layout/svg yet?  (I'm assuming that's one of the next steps.)

If not, please file one & mark it as depending on this bug, since this bug introduces some overhead for Matrix <--> gfxMatrix conversions that will be nice to get rid of. (Particularly the various GetCanvasTM() methods and their calls to their parents' GetCanvasTM() methods -- the parent's rv gets converted from gfxMatrix to Matrix and then back to gfxMatrix when the child returns.)

>diff --git a/layout/svg/nsSVGInnerSVGFrame.cpp b/layout/svg/nsSVGInnerSVGFrame.cpp
>@@ -278,21 +280,21 @@ nsSVGInnerSVGFrame::GetCanvasTM(uint32_t
>-    gfxMatrix tm = content->PrependLocalTransformsTo(
>-        this == aTransformRoot ? gfxMatrix() :
>-                                 parent->GetCanvasTM(aFor, aTransformRoot));
>+    Matrix tm = content->PrependLocalTransformsTo(
>+        ToMatrix(this == aTransformRoot ? gfxMatrix() :
>+                                 parent->GetCanvasTM(aFor, aTransformRoot)));

This doesn't match the pattern you're using in other files here (e.g. nsSVGAFrame, nsSVGForeignObjectFrame, nsSVGPathGeometryFrame).  In those other GetCanvasTM methods, you replace gfxMatrix() with Matrix(), and you invoke ToMatrix() only on the parent->GetCanvasTM() call.

Can we do the same here? (seems like it's worth it, for efficiency and consistency.)

>diff --git a/layout/svg/nsSVGOuterSVGFrame.cpp b/layout/svg/nsSVGOuterSVGFrame.cpp
>@@ -938,16 +938,16 @@ nsSVGOuterSVGAnonChildFrame::HasChildren
>   if (hasTransform && aTransform) {
>     // Outer-<svg> doesn't use x/y, so we can pass eChildToUserSpace here.
>-    gfxMatrix identity;
>-    *aTransform = gfx::ToMatrix(
>+    Matrix identity;
>+    *aTransform =
>       content->PrependLocalTransformsTo(identity,
>-                                        nsSVGElement::eChildToUserSpace));
>+                                        nsSVGElement::eChildToUserSpace);
>   }

Note: if you like, I think we can just pass "Matrix()" as the first arg here, instead of bothering to declare "identity". (That's what we seem to do elsewhere when we want to pass the identity matrix, at least.)


Overall, looks good!  This is basically r=me with the above addressed; but I'm marking feedback+ instead of r+ since it's probably worth having a sanity-check on the MatricesEqual() changes and the other things here. I promise speedy final-review once your next revision is up. :)

Comment 10

[Jonathan Watt [:jwatt]]

In general I've been aiming to move SVG from Thebes classes to Moz2D classes, knowing that also means moving from double to float. I still think that's what we should do, but I'm thinking that gfxMatrix should possibly be an exception. In general the number of arithmetic operations with other types such as gfxRect or low, so the faster error accumulation inherent in using float instead of double is not really significant. Matrix multiplication is a bit different though, since we can end up with a considerable number of matrices between an element and its root. As a consequence we're likely to find that error accumulation will be much more significant for matrices. To cope with that I think we either may want a matrix typedef specifically for SVG so we can switch the SVG code to double based matrix at short notice if necessary, or else we may just want to decide to stay with a double based matrix. I'd like to have the former so that we can experiment easily. I have some WIP patches to vectorize Matrix which of course work much better with float than double, so a float based matrix class is likely to have even better performance than a double based one in future.

Comment 11

[Jonathan Watt [:jwatt]]

This old Try push of the current attached patch shows that for matrix the loss of precision can result in changes in rendering due to more error accumulation:

https://tbpl.mozilla.org/?tree=Try&rev=e62d745d4b63

At least some (all?) the failures already have fuzz, but they now need more. Specifically:

OS X:
layout/reftests/svg/smil/transform/additive-1.svg
max difference: 109, number of differing pixels: 2098.

Win:
layout/reftests/svg/smil/transform/additive-1.svg
max difference: 108, number of differing pixels: 2198 

Win 7/8:
layout/reftests/svg/smil/transform/rotate-angle-3.svg
max difference: 20, number of differing pixels: 91 

Android 4.0 Opt:
layout/reftests/svg/smil/transform/additive-1.svg
max difference: 101, number of differing pixels: 2234

B2G ICS Emulator Opt
layout/reftests/svg/smil/transform/additive-1.svg | image comparison (==)
max difference: 100, number of differing pixels: 1805
layout/reftests/svg/smil/transform/additive-1.svg
max difference: 100, number of differing pixels: 1805

I need to take a closer look at these before proceeding with this bug, but other things are more pressing at the moment.

Comment 12

[Jonathan Watt [:jwatt]]

Given the rounding issues we have, I no longer think we should do this since it will just exacerbate them and further degrade our SVG implementation relative to other implementations.