952977 - Switch some uses of gfx3DMatrix to gfx::Matrix4x4

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: IPDL changes

Comment 2

[David Zbarsky (:dzbarsky)]

Attachment: PrependLocalTransformsTo

Comment 3

[David Zbarsky (:dzbarsky)]

Attachment: GetCanvasTM and friends

Comment 4

[David Zbarsky (:dzbarsky)]

Attachment: IsSVGTransformed

Comment 5

[David Zbarsky (:dzbarsky)]

Attachment: SVGIntegrationUtils

Comment 6

[David Zbarsky (:dzbarsky)]

Attachment: GetBBoxContribution

Comment 7

[David Zbarsky (:dzbarsky)]

Attachment: GetMarkerTransform

Comment 8

[David Zbarsky (:dzbarsky)]

Attachment: GetViewBoxTransform

Comment 9

[David Zbarsky (:dzbarsky)]

Attachment: nsSVGImageFrame

Comment 10

[David Zbarsky (:dzbarsky)]

Attachment: GeneratePath

Comment 11

[Nicolas Silva [:nical]]

Comment on attachment 8351268 [details] [diff] [review]
GetCanvasTM and friends

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

::: layout/svg/SVGTextFrame.cpp
@@ +3575,3 @@
>                        mRect.y / appUnitsPerDevPixel,
>                        mRect.width / appUnitsPerDevPixel,
>                        mRect.height / appUnitsPerDevPixel);

ultra lame nit: indentation is off

Comment 12

[David Zbarsky (:dzbarsky)]

Attachment: Remove sRotationMatrix

Looks like we used to set the world transform to this matrix when using OpenGL layers, but now we have AsyncCompositionManager::ComputeRotation()

Comment 13

[David Zbarsky (:dzbarsky)]

Attachment: ScaleAndTranslate

Comment 14

[David Zbarsky (:dzbarsky)]

Attachment: nsSVGTextFrame

Comment 15

[David Zbarsky (:dzbarsky)]

Attachment: GetCTM

Comment 16

[David Zbarsky (:dzbarsky)]

Attachment: GetStrokeTransform

Comment 17

[David Zbarsky (:dzbarsky)]

Attachment: SVG DOM changes

Comment 18

[David Zbarsky (:dzbarsky)]

Attachment: Remove DocumentRendererShmem and DocumentRendererNativeID

Comment 19

[David Zbarsky (:dzbarsky)]

Try run here:
https://tbpl.mozilla.org/?tree=Try&rev=5c43a1024d8e

The linux failures start with the GetCanvasTM patch, but there's no platform-specific code there.  Perhaps it has something to do with deprecated textures?

Comment 20

[David Zbarsky (:dzbarsky)]

I built on linux locally with clang and couldn't reproduce it, so it could also potentially be a float/double gcc compiler bug.

Comment 21

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ce43cb764d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/b04bd18e5692
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a2a444ec574

Comment 22

[David Zbarsky (:dzbarsky)]

Backed out PrependSVG https://hg.mozilla.org/integration/mozilla-inbound/rev/8eacfa0523bd

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4ce43cb764d5
https://hg.mozilla.org/mozilla-central/rev/4a2a444ec574

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e80575278469

Comment 25

[Nicolas Silva [:nical]]

(In reply to David Zbarsky (:dzbarsky) from comment #19)
> Try run here:
> https://tbpl.mozilla.org/?tree=Try&rev=5c43a1024d8e
> 
> The linux failures start with the GetCanvasTM patch, but there's no
> platform-specific code there.  Perhaps it has something to do with
> deprecated textures?

Deprecated/new textures are an OMTC-only thing, so unless you forced OMTC and GL layers I don't think it is related.

Comment 26

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f29ac8c53779
https://hg.mozilla.org/integration/mozilla-inbound/rev/983b63dc75e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/8821d96ad478

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can we do something to prevent dropping mozilla::gfx:: prefixes all over the place? Perhaps add a "typedef mozilla::gfx::Matrix Matrix;" to nsIFrame.h?

Comment 28

[David Zbarsky (:dzbarsky)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> Can we do something to prevent dropping mozilla::gfx:: prefixes all over the
> place? Perhaps add a "typedef mozilla::gfx::Matrix Matrix;" to nsIFrame.h?

I added that in this push.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b412a92a92c
https://hg.mozilla.org/integration/mozilla-inbound/rev/19983d377f58
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f2c1da63eeb

Comment 29

[David Zbarsky (:dzbarsky)]

Attachment: World transform

Comment 30

[David Zbarsky (:dzbarsky)]

Attachment: GetUserSpaceToFilterSpaceTransform

Comment 31

[David Zbarsky (:dzbarsky)]

Attachment: CreateMatrix

Comment 32

[David Zbarsky (:dzbarsky)]

Attachment: SVGFilterInstance

Comment 33

[David Zbarsky (:dzbarsky)]

Attachment: imgIContainer::Draw

Comment 34

[David Zbarsky (:dzbarsky)]

Attachment: Allow gfx::Matrix with gfxContext

Comment 35

[David Zbarsky (:dzbarsky)]

Attachment: DrawPixelSnapped

Comment 36

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3cf7d0ef7869
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8e39fa87ba3

Comment 37

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f29ac8c53779
https://hg.mozilla.org/mozilla-central/rev/983b63dc75e3
https://hg.mozilla.org/mozilla-central/rev/8821d96ad478
https://hg.mozilla.org/mozilla-central/rev/4b412a92a92c
https://hg.mozilla.org/mozilla-central/rev/19983d377f58
https://hg.mozilla.org/mozilla-central/rev/0f2c1da63eeb
https://hg.mozilla.org/mozilla-central/rev/7fb529b7829a
https://hg.mozilla.org/mozilla-central/rev/3cf7d0ef7869
https://hg.mozilla.org/mozilla-central/rev/f8e39fa87ba3

Comment 38

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a8e09381a51

Comment 39

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1a8e09381a51

Comment 40

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f9f55f174e7

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5f9f55f174e7

Comment 42

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b15232e11c19

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b15232e11c19

Comment 44

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0fd2401968c

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a0fd2401968c

Comment 46

[David Zbarsky (:dzbarsky)]

Attachment: GetTextureTransform

Comment 47

[David Zbarsky (:dzbarsky)]

Attachment: GetEffectiveTransform

Comment 48

[David Zbarsky (:dzbarsky)]

Attachment: SnapTransforms

Comment 49

[David Zbarsky (:dzbarsky)]

Attachment: ComputeEffectiveTransforms

Comment 50

[David Zbarsky (:dzbarsky)]

Attachment: GetLocalTransform

Comment 51

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1aed8b03154a
https://hg.mozilla.org/integration/mozilla-inbound/rev/87b0be26bea9

Comment 52

[David Zbarsky (:dzbarsky)]

Attachment: SetBaseTransform

Comment 53

[David Zbarsky (:dzbarsky)]

Attachment: GetTransform

Comment 54

[David Zbarsky (:dzbarsky)]

Attachment: Layer's mTransform

Comment 55

[David Zbarsky (:dzbarsky)]

Attachment: shadow transform

Comment 56

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1aed8b03154a
https://hg.mozilla.org/mozilla-central/rev/87b0be26bea9

Comment 57

[David Zbarsky (:dzbarsky)]

Attachment: Cleanup

Comment 58

[David Zbarsky (:dzbarsky)]

Attachment: OGLShaderProgram

Comment 59

[David Zbarsky (:dzbarsky)]

Attachment: PrepareViewport

Comment 60

[David Zbarsky (:dzbarsky)]

Attachment: DrawTo

Comment 61

[David Zbarsky (:dzbarsky)]

Attachment: CompositingRenderTargetOGL

Comment 62

[David Zbarsky (:dzbarsky)]

Attachment: AutoMaskData

Comment 63

[David Zbarsky (:dzbarsky)]

Attachment: AsyncCompositionManager

Comment 64

[Nicolas Silva [:nical]]

Comment on attachment 8365098 [details] [diff] [review]
GetLocalTransform

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

::: gfx/2d/Matrix.h
@@ +284,5 @@
>  
>      return Matrix(_11, _12, _21, _22, _41, _42);
>    }
>  
> +  bool CanDraw2D(Matrix* aMatrix = nullptr) const {

Please add doc comments (as a followup if you want) to the new methods (especially when the abundance of existing Is2D, To2D, etc. make method names like this one not obvious).

Comment 65

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7f4f11fa70cf
https://hg.mozilla.org/mozilla-central/rev/17e04ba8fe30
https://hg.mozilla.org/mozilla-central/rev/558fb0c6a045
https://hg.mozilla.org/mozilla-central/rev/6d8a33dcab14
https://hg.mozilla.org/mozilla-central/rev/0ae9e982bf2f
https://hg.mozilla.org/mozilla-central/rev/7776f7bdaa8d
https://hg.mozilla.org/mozilla-central/rev/bafd28ce2b87

Comment 66

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48317587ccef
https://hg.mozilla.org/integration/mozilla-inbound/rev/70a77a2cb353
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc6547917f0d
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec4df9a9a9c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ffe3c056420
https://hg.mozilla.org/integration/mozilla-inbound/rev/b25ba289e7df
https://hg.mozilla.org/integration/mozilla-inbound/rev/59320a8f2536

Comment 67

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/48317587ccef
https://hg.mozilla.org/mozilla-central/rev/70a77a2cb353
https://hg.mozilla.org/mozilla-central/rev/cc6547917f0d
https://hg.mozilla.org/mozilla-central/rev/ec4df9a9a9c2
https://hg.mozilla.org/mozilla-central/rev/9ffe3c056420
https://hg.mozilla.org/mozilla-central/rev/b25ba289e7df
https://hg.mozilla.org/mozilla-central/rev/59320a8f2536

Comment 68

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f17694ec185b
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2d4f16ed64a

Comment 69

[David Zbarsky (:dzbarsky)]

Attachment: SVG DOM changes

Comment 70

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f17694ec185b
https://hg.mozilla.org/mozilla-central/rev/c2d4f16ed64a
https://hg.mozilla.org/mozilla-central/rev/27a2c7c78e8a

Comment 71

[David Zbarsky (:dzbarsky)]

Attachment: More gfx/layers cleanup

Comment 72

[David Zbarsky (:dzbarsky)]

Attachment: More AsyncCompositionManager changes

Comment 73

[David Zbarsky (:dzbarsky)]

Attachment: LayerTreeInvalidation

Comment 74

[David Zbarsky (:dzbarsky)]

Attachment: LayerManagerComposite

Comment 75

[David Zbarsky (:dzbarsky)]

Attachment: Layer tests

Comment 76

[David Zbarsky (:dzbarsky)]

Attachment: nsDisplayTransform

Comment 77

[David Zbarsky (:dzbarsky)]

Attachment: TiledContentClient

Comment 78

[David Zbarsky (:dzbarsky)]

Attachment: APZC

Comment 79

[David Zbarsky (:dzbarsky)]

Attachment: Various small changes

Comment 80

[David Zbarsky (:dzbarsky)]

Attachment: FrameLayerBuilder

Comment 81

[David Zbarsky (:dzbarsky)]

Attachment: Various other cleanups

Comment 82

[David Zbarsky (:dzbarsky)]

Attachment: Remove gfxPointH3D

This adds the operator[] API to Matrix4x4.  Not sure if we want to do that, but it does make certain operations simpler and matches gfx3DMatrix.

Comment 83

[Nicolas Silva [:nical]]

Comment on attachment 8463174 [details] [diff] [review]
Remove gfxPointH3D

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

::: gfx/thebes/gfx3DMatrix.cpp
@@ +493,5 @@
>  
>  gfx3DMatrix
>  gfx3DMatrix::Inverse() const
>  {
> +  if (_11 == 0 && _21 == 0 && _31 == 0 && _41 == 1) {

TransposedVector(aIndex) goes (&_11)+aIndex, etc. Looks like you forgot to offset by 3 here.

Comment 84

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

All backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/05eb059f70bf for this reftest-no-accel failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=44848645&tree=Mozilla-Inbound

Comment 85

[Kartikaya Gupta (email:[e-mail])]

Is it possible to add a Inverse(Matrix4x4 m) function somewhere? It seems rather cumbersome to always have to create a local temp variable to invert a matrix.

Comment 86

[Kartikaya Gupta (email:[e-mail])]

(Specifically for code like http://hg.mozilla.org/integration/mozilla-inbound/rev/765b7f67163e#l1.353)

Comment 87

[David Zbarsky (:dzbarsky)]

Kats, that's a good idea. We can just put it on Matrix4x4 to match the gfx3DMatrix API.  I'll do it next time I get a chance to work on this stuff.

Relanded without the offending commit:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/849e11005fc9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/941a77086c1a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9cd72d8361cb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1df59831403a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5e7fdb35499e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8924abe4f277
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7fbae4092ca9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6643bed21a3e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d764f90842f3

Try run: https://tbpl.mozilla.org/?tree=Try&rev=2c67fa3069e6

Comment 88

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/849e11005fc9
https://hg.mozilla.org/mozilla-central/rev/941a77086c1a
https://hg.mozilla.org/mozilla-central/rev/9cd72d8361cb
https://hg.mozilla.org/mozilla-central/rev/1df59831403a
https://hg.mozilla.org/mozilla-central/rev/5e7fdb35499e
https://hg.mozilla.org/mozilla-central/rev/8924abe4f277
https://hg.mozilla.org/mozilla-central/rev/7fbae4092ca9
https://hg.mozilla.org/mozilla-central/rev/6643bed21a3e
https://hg.mozilla.org/mozilla-central/rev/d764f90842f3

Comment 89

[David Zbarsky (:dzbarsky)]

Attachment: Remove gfxPointH3D v2

Good catch on the Inverse bug.  That's the only thing I've changed here

Comment 90

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3325dc888b50

Comment 91

[David Zbarsky (:dzbarsky)]

Attachment: nsLayoutUtils

Comment 92

[Ed Morley [:emorley]]

(In reply to David Zbarsky (:dzbarsky) from comment #90)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3325dc888b50

Reverted for build failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=46475233&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=46477375&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=46475271&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/36ffb9b24c56

Comment 93

[David Zbarsky (:dzbarsky)]

Attachment: GetResultingTransformMatrix

Comment 94

[David Zbarsky (:dzbarsky)]

Attachment: ReadTransforms

Comment 95

[David Zbarsky (:dzbarsky)]

Looks good on try: https://tbpl.mozilla.org/?tree=Try&rev=5215e602b598

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/76987e4e811c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d02d8790e5c5

Comment 96

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

https://hg.mozilla.org/mozilla-central/rev/76987e4e811c
https://hg.mozilla.org/mozilla-central/rev/d02d8790e5c5

Comment 97

[Nicolas Silva [:nical]]

Comment on attachment 8476729 [details] [diff] [review]
nsLayoutUtils

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

Looks good. The parts of this patch in FrameLayerBuilder already made it to central (maybe some other parts too but not all of the patch). Looks like your patch queue is getting a bit messed up?

Comment 98

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8430feba6148

Comment 99

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8430feba6148

Comment 100

[Jonathan Watt [:jwatt]]

David, what's the status here? It would be good to have this finished off so that we can get rid of the overhead of the ToMatrix4x4 and To3DMatrix calls.

Comment 101

[David Zbarsky (:dzbarsky)]

I ran into reftest failures on linux, presumably due to rounding error.  I haven't had time to investigate and I'm not actively working on this bug.

Comment 102

[David Zbarsky (:dzbarsky)]

This bug already has a ton of patches; we can do more in a followup.