1181240 - Replace gfx3DMatrix with Matrix4x4

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[:kip (Kearwood Gilbert)]

gfx3DMatrix and Matrix4x4 have the same functionality, with Matrix4x4 having greater support for special cases such as transforming behind the w=0 plane.

By refactoring the code base to eliminate gfx3DMatrix, we will not need to add this functionality to gfx3DMatrix

Comment 1

[:kip (Kearwood Gilbert)]

Attachment: Bug 1181240 - Refactor to replace gfx3DMatrix with Matrix4x4

I have refactored out gfx3DMatrix, updating callsites to use Matrix4x4.
Methods in gfx3DMatrix that act on double-precision floating point values in classes such as gfxRect, gfxQuad, gfxPoint, and gfxMatrix have been moved to Matrix4x4.
Some of these callsites may not require double-precision floating point values, but that could be reviewed afterwards.
This refactoring should have no functional effect except to clean up the code and reducing the size of upcoming patches that require these callsites to use functions only present in Matrix4x4.

Comment 2

[:kip (Kearwood Gilbert)]

I have pushed to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4d6508403d2

I have included all platforms and tests due to the number of files affected in platform specific code.

Comment 3

[:kip (Kearwood Gilbert)]

I anticipate potential platform specific build failures due to header include dependencies and such.  I’ll weed those out with try runs and let all tests run before requesting review in case I need to iterate on the patch.

Comment 4

[:kip (Kearwood Gilbert)]

First try run only ran builds.  Now pushing to try with tests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b60b37ef9bda

Comment 5

[:kip (Kearwood Gilbert)]

Attachment: Bug 1181240 - Part 1: Move methods from gfx3DMatrix to Matrix4x4

- Moved methods from gfx3DMatrix to Matrix4x4 that operate on the
double-precision classes gfxMatrix, gfxRect, gfxQuad, and gfxPoint.

Comment 6

[:kip (Kearwood Gilbert)]

Attachment: Bug 1181240 - Part 2: Replace gfx3DMatrix with Matrix4x4 in layout

- Refactored code to use Matrix4x4 instead of gfx3DMatrix.
- There is not expected to be any functional effect.

Comment 7

[:kip (Kearwood Gilbert)]

Attachment: Bug 1181240 - Part 3: Replace gfx3DMatrix with Matrix4x4 in gfx

- Refactored code to use Matrix4x4 instead of gfx3DMatrix.
- There is not expected to be any functional effect.

Comment 8

[:kip (Kearwood Gilbert)]

Attachment: Bug 1181240 - Part 4: Remove gfx3DMatrix

- Removed the gfx3DMatrix class, which has been replaced with Matrix4x4

Comment 9

[:kip (Kearwood Gilbert)]

I have split the patches for easier review.

Also, just prior to final formatting adjustments, I had pushed to try to check for any unintended regressions:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=95eab82e8dae

(Try is green)

Comment 10

[:kip (Kearwood Gilbert)]

Attachment: MozReview Request: Bug 1181240 - Part 1: Copy methods from gfx3DMatrix

Bug 1181240 - Part 1: Move methods from gfx3DMatrix to Matrix4x4
- Moved methods from gfx3DMatrix to Matrix4x4 that operate on the
  double-precision classes gfxMatrix, gfxRect, gfxQuad, and gfxPoint.

Comment 11

[:kip (Kearwood Gilbert)]

Attachment: MozReview Request: Bug 1181240 - Part 2: Replace gfx3DMatrix with Matrix4x4 in layout

Bug 1181240 - Part 2: Replace gfx3DMatrix with Matrix4x4 in layout
- Refactored code to use Matrix4x4 instead of gfx3DMatrix.
- There is not expected to be any functional effect.

Comment 12

[:kip (Kearwood Gilbert)]

Attachment: MozReview Request: Bug 1181240 - Part 3: Replace gfx3DMatrix with Matrix4x4 in gfx

Bug 1181240 - Part 3: Replace gfx3DMatrix with Matrix4x4 in gfx
- Refactored code to use Matrix4x4 instead of gfx3DMatrix.
- There is not expected to be any functional effect.

Comment 13

[:kip (Kearwood Gilbert)]

Attachment: MozReview Request: Bug 1181240 - Part 4: Remove gfx3DMatrix

Bug 1181240 - Part 4: Remove gfx3DMatrix
- Removed the gfx3DMatrix class, which has been replaced with Matrix4x4

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8633016 [details] [diff] [review]
Bug 1181240 - Part 1: Move methods from gfx3DMatrix to Matrix4x4

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

::: gfx/2d/Matrix.h
@@ +15,5 @@
>  #include "mozilla/DebugOnly.h"
>  #include "mozilla/FloatingPoint.h"
> +#include "gfxMatrix.h"
> +#include "gfxRect.h"
> +#include "gfxQuad.h"

So this is going to be a problem -- stuff from gfx/2d should be largely self-contained, and especially not not depend on other gfx stuff (only on high-level MFBT (mozilla/*) bits).

You'll need to move these methods elsewhere -- maybe to gfxMatrix/gfxPoint/gfxRect directly, or an adapter class, or add some static helpers to gfxUtils until we convert gfxMatrix/gfxPoint/gfxRect to the moz2d equivalents.

Comment 15

[:kip (Kearwood Gilbert)]

Comment on attachment 8633109 [details]
MozReview Request: Bug 1181240 - Part 1: Copy methods from gfx3DMatrix

Bug 1181240 - Part 1: Copy methods from gfx3DMatrix
- Copied methods from gfx3DMatrix to Matrix4x4, gfxPoint, and gfxRect that
  with at double-precision floating point.

Comment 16

[:kip (Kearwood Gilbert)]

Comment on attachment 8633110 [details]
MozReview Request: Bug 1181240 - Part 2: Replace gfx3DMatrix with Matrix4x4 in layout

Bug 1181240 - Part 2: Replace gfx3DMatrix with Matrix4x4 in layout
- Refactored code to use Matrix4x4 instead of gfx3DMatrix.
- There is not expected to be any functional effect.

Comment 17

[:kip (Kearwood Gilbert)]

Comment on attachment 8633111 [details]
MozReview Request: Bug 1181240 - Part 3: Replace gfx3DMatrix with Matrix4x4 in gfx

Bug 1181240 - Part 3: Replace gfx3DMatrix with Matrix4x4 in gfx
- Refactored code to use Matrix4x4 instead of gfx3DMatrix.
- There is not expected to be any functional effect.

Comment 18

[:kip (Kearwood Gilbert)]

Comment on attachment 8633112 [details]
MozReview Request: Bug 1181240 - Part 4: Remove gfx3DMatrix

Bug 1181240 - Part 4: Remove gfx3DMatrix
- Removed the gfx3DMatrix class, which has been replaced with Matrix4x4

Comment 19

[:kip (Kearwood Gilbert)]

Comment on attachment 8633040 [details] [diff] [review]
Bug 1181240 - Part 3: Replace gfx3DMatrix with Matrix4x4 in gfx

Correct patches are now on MozReview

Comment 20

[:kip (Kearwood Gilbert)]

I have updated the patches to rebase to the current head and remove the dependencies of gfxRect, gfxPoint, and gfxQuad from Matrix.h

Comment 21

[:kip (Kearwood Gilbert)]

Comment on attachment 8633109 [details]
MozReview Request: Bug 1181240 - Part 1: Copy methods from gfx3DMatrix

Bug 1181240 - Part 1: Copy methods from gfx3DMatrix
- Copied methods from gfx3DMatrix to Matrix4x4, gfxPoint, and gfxRect that
  with at double-precision floating point.

Comment 22

[:kip (Kearwood Gilbert)]

Comment on attachment 8633110 [details]
MozReview Request: Bug 1181240 - Part 2: Replace gfx3DMatrix with Matrix4x4 in layout

Bug 1181240 - Part 2: Replace gfx3DMatrix with Matrix4x4 in layout
- Refactored code to use Matrix4x4 instead of gfx3DMatrix.
- There is not expected to be any functional effect.

Comment 23

[:kip (Kearwood Gilbert)]

Comment on attachment 8633111 [details]
MozReview Request: Bug 1181240 - Part 3: Replace gfx3DMatrix with Matrix4x4 in gfx

Bug 1181240 - Part 3: Replace gfx3DMatrix with Matrix4x4 in gfx
- Refactored code to use Matrix4x4 instead of gfx3DMatrix.
- There is not expected to be any functional effect.

Comment 24

[:kip (Kearwood Gilbert)]

Comment on attachment 8633112 [details]
MozReview Request: Bug 1181240 - Part 4: Remove gfx3DMatrix

Bug 1181240 - Part 4: Remove gfx3DMatrix
- Removed the gfx3DMatrix class, which has been replaced with Matrix4x4

Comment 25

[:kip (Kearwood Gilbert)]

A try run showed that tests were green on Windows and OSX; however, Linux and B2G builds failed:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e573c41d2b1a

I have made a small change in an include directive to correct for case-sensitive file systems on the build machines, and pushed to try again:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=781fac71c34d

Comment 26

[:kip (Kearwood Gilbert)]

Comment on attachment 8633109 [details]
MozReview Request: Bug 1181240 - Part 1: Copy methods from gfx3DMatrix

Bug 1181240 - Part 1: Copy methods from gfx3DMatrix
- Copied methods from gfx3DMatrix to Matrix4x4, gfxPoint, and gfxRect that
  with at double-precision floating point.

Comment 27

[:kip (Kearwood Gilbert)]

Comment on attachment 8633110 [details]
MozReview Request: Bug 1181240 - Part 2: Replace gfx3DMatrix with Matrix4x4 in layout

Bug 1181240 - Part 2: Replace gfx3DMatrix with Matrix4x4 in layout
- Refactored code to use Matrix4x4 instead of gfx3DMatrix.
- There is not expected to be any functional effect.

Comment 28

[:kip (Kearwood Gilbert)]

Comment on attachment 8633111 [details]
MozReview Request: Bug 1181240 - Part 3: Replace gfx3DMatrix with Matrix4x4 in gfx

Bug 1181240 - Part 3: Replace gfx3DMatrix with Matrix4x4 in gfx
- Refactored code to use Matrix4x4 instead of gfx3DMatrix.
- There is not expected to be any functional effect.

Comment 29

[:kip (Kearwood Gilbert)]

Comment on attachment 8633112 [details]
MozReview Request: Bug 1181240 - Part 4: Remove gfx3DMatrix

Bug 1181240 - Part 4: Remove gfx3DMatrix
- Removed the gfx3DMatrix class, which has been replaced with Matrix4x4

Comment 30

[:kip (Kearwood Gilbert)]

(In reply to :kip (Kearwood Gilbert) from comment #25)
> A try run showed that tests were green on Windows and OSX; however, Linux
> and B2G builds failed:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e573c41d2b1a
> 
> I have made a small change in an include directive to correct for
> case-sensitive file systems on the build machines, and pushed to try again:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=781fac71c34d

This try push has found one other include directive that needed to be corrected for case-sensitivity.  I have updated the patches and pushed again:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aed21d3e639d

Comment 31

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8633109 [details]
MozReview Request: Bug 1181240 - Part 1: Copy methods from gfx3DMatrix

https://reviewboard.mozilla.org/r/13173/#review12385

::: gfx/2d/Matrix.cpp:405
(Diff revision 4)
> +FlushToZero(double aVal)

Let's make these static, or stick them in an anonymous namespace?

Comment 32

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8633110 [details]
MozReview Request: Bug 1181240 - Part 2: Replace gfx3DMatrix with Matrix4x4 in layout

https://reviewboard.mozilla.org/r/13175/#review12387

::: layout/base/ActiveLayerTracker.cpp:235
(Diff revision 4)
> -    nsStyleTransformMatrix::ReadTransforms(display->mSpecifiedTransform->mHead,
> +                                           display->mSpecifiedTransform->mHead,

Nit: weird indentation, maybe just leave the nsStyleTransformMatrix::ReadTransforms( on the next line like it was before, newline after the =?  No strong preference.

Comment 33

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8633111 [details]
MozReview Request: Bug 1181240 - Part 3: Replace gfx3DMatrix with Matrix4x4 in gfx

https://reviewboard.mozilla.org/r/13177/#review12389

Comment 34

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8633110 [details]
MozReview Request: Bug 1181240 - Part 2: Replace gfx3DMatrix with Matrix4x4 in layout

https://reviewboard.mozilla.org/r/13175/#review12391

Comment 35

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8633112 [details]
MozReview Request: Bug 1181240 - Part 4: Remove gfx3DMatrix

https://reviewboard.mozilla.org/r/13179/#review12393

Comment 36

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8633109 [details]
MozReview Request: Bug 1181240 - Part 1: Copy methods from gfx3DMatrix

https://reviewboard.mozilla.org/r/13173/#review12395

Comment 37

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

https://reviewboard.mozilla.org/r/13173/#review12397

Ship It!

Comment 38

[:kip (Kearwood Gilbert)]

Comment on attachment 8633109 [details]
MozReview Request: Bug 1181240 - Part 1: Copy methods from gfx3DMatrix

Bug 1181240 - Part 1: Copy methods from gfx3DMatrix
- Copied methods from gfx3DMatrix to Matrix4x4, gfxPoint, and gfxRect that
  with at double-precision floating point.

Comment 39

[:kip (Kearwood Gilbert)]

Comment on attachment 8633110 [details]
MozReview Request: Bug 1181240 - Part 2: Replace gfx3DMatrix with Matrix4x4 in layout

Bug 1181240 - Part 2: Replace gfx3DMatrix with Matrix4x4 in layout
- Refactored code to use Matrix4x4 instead of gfx3DMatrix.
- There is not expected to be any functional effect.

Comment 40

[:kip (Kearwood Gilbert)]

Comment on attachment 8633111 [details]
MozReview Request: Bug 1181240 - Part 3: Replace gfx3DMatrix with Matrix4x4 in gfx

Bug 1181240 - Part 3: Replace gfx3DMatrix with Matrix4x4 in gfx
- Refactored code to use Matrix4x4 instead of gfx3DMatrix.
- There is not expected to be any functional effect.

Comment 41

[:kip (Kearwood Gilbert)]

Comment on attachment 8633112 [details]
MozReview Request: Bug 1181240 - Part 4: Remove gfx3DMatrix

Bug 1181240 - Part 4: Remove gfx3DMatrix
- Removed the gfx3DMatrix class, which has been replaced with Matrix4x4

Comment 42

[:kip (Kearwood Gilbert)]

Full try run is green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=52ea010f62ab

Comment 43

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/24c93e2d0d68
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d03e5f96614
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f1489a600e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/292fbdb78dde

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/24c93e2d0d68
https://hg.mozilla.org/mozilla-central/rev/5d03e5f96614
https://hg.mozilla.org/mozilla-central/rev/7f1489a600e8
https://hg.mozilla.org/mozilla-central/rev/292fbdb78dde