Replace gfx3DMatrix with Matrix4x4

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kip, Assigned: kip)

Tracking

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

(Whiteboard: [webvr][vrm2])

Attachments

(4 attachments, 5 obsolete attachments)

(Assignee)

Description

3 years ago
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
(Assignee)

Updated

3 years ago
Assignee: nobody → kgilbert
(Assignee)

Comment 1

3 years ago
Created attachment 8631880 [details] [diff] [review]
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.
(Assignee)

Comment 2

3 years ago
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.
(Assignee)

Comment 3

3 years ago
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.
(Assignee)

Comment 4

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

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

Comment 5

3 years ago
Created attachment 8633016 [details] [diff] [review]
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.
Attachment #8631880 - Attachment is obsolete: true
(Assignee)

Comment 6

3 years ago
Created attachment 8633030 [details] [diff] [review]
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.
(Assignee)

Comment 7

3 years ago
Created attachment 8633040 [details] [diff] [review]
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.
(Assignee)

Comment 8

3 years ago
Created attachment 8633042 [details] [diff] [review]
Bug 1181240 - Part 4: Remove gfx3DMatrix

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

Comment 9

3 years ago
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)
(Assignee)

Comment 10

3 years ago
Created attachment 8633109 [details]
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.
Attachment #8633109 - Flags: review?(vladimir)
(Assignee)

Comment 11

3 years ago
Created 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.
Attachment #8633110 - Flags: review?(vladimir)
(Assignee)

Comment 12

3 years ago
Created 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.
Attachment #8633111 - Flags: review?(vladimir)
(Assignee)

Comment 13

3 years ago
Created 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
Attachment #8633112 - Flags: review?(vladimir)
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.
Attachment #8633016 - Flags: review-
(Assignee)

Comment 15

3 years ago
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.
Attachment #8633109 - Attachment description: MozReview Request: Bug 1181240 - Part 1: Move methods from gfx3DMatrix to Matrix4x4 → MozReview Request: Bug 1181240 - Part 1: Copy methods from gfx3DMatrix
(Assignee)

Comment 16

3 years ago
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.
(Assignee)

Comment 17

3 years ago
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.
(Assignee)

Comment 18

3 years ago
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
(Assignee)

Updated

3 years ago
Attachment #8633042 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8633016 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8633030 - Attachment is obsolete: true
(Assignee)

Comment 19

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

Correct patches are now on MozReview
Attachment #8633040 - Attachment is obsolete: true
(Assignee)

Comment 20

3 years ago
I have updated the patches to rebase to the current head and remove the dependencies of gfxRect, gfxPoint, and gfxQuad from Matrix.h
(Assignee)

Updated

3 years ago
Whiteboard: [webvr]
(Assignee)

Comment 21

3 years ago
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.
(Assignee)

Comment 22

3 years ago
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.
(Assignee)

Comment 23

3 years ago
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.
(Assignee)

Comment 24

3 years ago
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
(Assignee)

Comment 25

3 years ago
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
(Assignee)

Comment 26

3 years ago
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.
(Assignee)

Comment 27

3 years ago
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.
(Assignee)

Comment 28

3 years ago
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.
(Assignee)

Comment 29

3 years ago
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
(Assignee)

Comment 30

3 years ago
(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 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?
Attachment #8633109 - Flags: review?(vladimir)
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.
Attachment #8633110 - Flags: review?(vladimir)
Comment on attachment 8633111 [details]
MozReview Request: Bug 1181240 - Part 3: Replace gfx3DMatrix with Matrix4x4 in gfx

https://reviewboard.mozilla.org/r/13177/#review12389
Attachment #8633111 - Flags: review?(vladimir) → review+
(Assignee)

Updated

3 years ago
Blocks: 1186575

Updated

3 years ago
Whiteboard: [webvr] → [webvr][vrm2]
(Assignee)

Comment 38

3 years ago
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.
Attachment #8633109 - Flags: review+ → review?(vladimir)
(Assignee)

Updated

3 years ago
Attachment #8633110 - Flags: review+ → review?(vladimir)
(Assignee)

Comment 39

3 years ago
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.
(Assignee)

Comment 40

3 years ago
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.
Attachment #8633111 - Flags: review+ → review?(vladimir)
(Assignee)

Updated

3 years ago
Attachment #8633112 - Flags: review+ → review?(vladimir)
(Assignee)

Comment 41

3 years ago
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
Depends on: 1204824
You need to log in before you can comment on or make changes to this bug.