931769 - Various patches to prepare for moving nsSVGPatternFrame from using gfxASurface to using Moz2D APIs

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch to make gfxPattern::GetPattern use its mExtend

SVG patterns aren't repeated without this.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch to add IsSingular and operator*= methods to Matrix

Comment 3

[Bas Schouten (:bas.schouten)]

Comment on attachment 823335 [details] [diff] [review]
patch to add IsSingular and operator*= methods to Matrix

Review of attachment 823335 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/Matrix.h
@@ +123,5 @@
>    }
>  
> +  Matrix& operator*=(const Matrix &aMatrix)
> +  {
> +    Matrix resultMatrix = *this * aMatrix;

See IRC convo.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch to switch away from gfxASurface

Comment 5

[Robert Longson [:longsonr]]

pattern->SetExtend(gfxPattern::EXTEND_REPEAT);

is now called twice.

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: patch to move MaxExpansion() from nsSVGUtils to nsSVGPatternFrame, and convert it to Moz2D

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: patch to switch away from gfxASurface

Comment 8

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #5)
> is now called twice.

Yeah, I forgot to remove that from when I was trying to work out what was going on with patterns not repeating any more.

BTW, feel free to steal these reviews if you're looking at the patches anyway.

Comment 9

[Robert Longson [:longsonr]]

Comment on attachment 823376 [details] [diff] [review]
patch to switch away from gfxASurface

>   if (NS_FAILED(GetTargetGeometry(&callerBBox,
>                                   viewBox,
>                                   patternContentUnits, patternUnits,
>                                   aSource,
>-                                  aContextMatrix,
>+                                  ThebesMatrix(aContextMatrix),
>                                   aOverrideBounds)))

Have a temporary for ThebesMatrix(aContextMatrix) as it's used below too.

> 
>   // Construct the CTM that we will provide to our children when we
>   // render them into the tile.
>   gfxMatrix ctm = ConstructCTM(viewBox, patternContentUnits, patternUnits,
>-                               callerBBox, aContextMatrix, aSource);
>+                               callerBBox, ThebesMatrix(aContextMatrix),
>+                               aSource);

i.e. here.

> 
>   // Get the bounding box of the pattern.  This will be used to determine
>   // the size of the surface, and will also be used to define the bounding
>   // box for the pattern tile.
>-  gfxRect bbox = GetPatternRect(patternUnits, callerBBox, aContextMatrix, aSource);
>+  gfxRect bbox = GetPatternRect(patternUnits, callerBBox,
>+                                ThebesMatrix(aContextMatrix), aSource);

and here.

>   // revert the vector effect transform so that the pattern appears unchanged
>   if (aFillOrStroke == &nsStyleSVG::mStroke) {
>-    patternTransform.Multiply(nsSVGUtils::GetStrokeTransform(aSource).Invert());
>+    patternTransform *= ToMatrix(nsSVGUtils::GetStrokeTransform(aSource).Invert());

Did you want to add the If !Identity code here that you added last time you semi-converted non-scaling-stroke?

>-  nsRefPtr<gfxPattern> pattern = new gfxPattern(surface);
>+  nsRefPtr<gfxPattern> pattern = new gfxPattern(surface, Matrix());

Presumably you should pass ThebesMatrix(pMatrix) here and omit the SetMatrix call below.

> {
>+  typedef mozilla::gfx::Matrix Matrix;
>+  typedef mozilla::gfx::SourceSurface SourceSurface;
>+

I thought we generally don't do this in headers but write the stuff out completely there and then put these in the cpp.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 823369 [details] [diff] [review]
patch to move MaxExpansion() from nsSVGUtils to nsSVGPatternFrame, and convert it to Moz2D

>diff --git a/layout/svg/nsSVGPatternFrame.cpp b/layout/svg/nsSVGPatternFrame.cpp
> //----------------------------------------------------------------------
>-// Helper classes
>+// Helpers
[...]
>+/* Calculate the maximum expansion of a matrix */
>+static float
>+MaxExpansion(const Matrix &aMatrix)
>+{

This cpp file has a pre-existing "Helper functions" area (where e.g. GetPatternMatrix lives). This new function belongs there, not up here in the "Helper classes" section.

>diff --git a/layout/svg/nsSVGPatternFrame.cpp b/layout/svg/nsSVGPatternFrame.cpp
>@@ -167,17 +187,17 @@ GetPatternMatrix(uint16_t aPatternUnits,
>-  float scale = 1.0f / nsSVGUtils::MaxExpansion(callerCTM);
>+  float scale = 1.0f / MaxExpansion(ToMatrix(callerCTM));
[...]
>@@ -194,17 +214,17 @@ GetTargetGeometry(gfxRect *aBBox,
>-  float scale = nsSVGUtils::MaxExpansion(aContextMatrix);
>+  float scale = MaxExpansion(ToMatrix(aContextMatrix));
[etc]

All four of the MaxExpansion(ToMatrix(...)) clients in your patch take a gfxMatrix argument, and call ToMatrix() on it.

As it happens, I believe they're each only called once, by the same function -- PaintPattern -- which could be responsible for doing a single ToMatrix() call, rather than doing it four times in each of these helper-functions.

Please do that (and change these function-signatures) to avoid needless duplicate matrix-conversion.

r=me with that.

Comment 11

[Jonathan Watt [:jwatt]]

Landed parts 1-3:

https://hg.mozilla.org/integration/mozilla-inbound/rev/194ba1b8f191
https://hg.mozilla.org/integration/mozilla-inbound/rev/6123376c20f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/05d9c6a32d8b

Part 4 is going to need to wait until filters use a Moz2D backed temporary draw target, since without that these two test fail:

layout/reftests/svg/filters/filter-patterned-rect-01.svg
layout/reftests/svg/filters/filter-patterned-rect-02.svg

Comment 12

[Jonathan Watt [:jwatt]]

I span part 4 out into bug 932198.

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/194ba1b8f191
https://hg.mozilla.org/mozilla-central/rev/6123376c20f1
https://hg.mozilla.org/mozilla-central/rev/05d9c6a32d8b