Closed Bug 952977 Opened 7 years ago Closed 6 years ago

Switch some uses of gfx3DMatrix to gfx::Matrix4x4

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

Details

Attachments

(55 files, 2 obsolete files)

13.55 KB, patch
nical
: review+
Details | Diff | Splinter Review
43.56 KB, patch
nical
: review+
Details | Diff | Splinter Review
93.93 KB, patch
nical
: review+
Details | Diff | Splinter Review
21.04 KB, patch
nical
: review+
Details | Diff | Splinter Review
3.81 KB, patch
nical
: review+
Details | Diff | Splinter Review
23.01 KB, patch
nical
: review+
Details | Diff | Splinter Review
4.72 KB, patch
nical
: review+
Details | Diff | Splinter Review
18.47 KB, patch
nical
: review+
Details | Diff | Splinter Review
9.33 KB, patch
nical
: review+
Details | Diff | Splinter Review
6.76 KB, patch
nical
: review+
Details | Diff | Splinter Review
2.62 KB, patch
mwu
: review+
Details | Diff | Splinter Review
7.86 KB, patch
nical
: review+
Details | Diff | Splinter Review
38.24 KB, patch
nical
: review+
Details | Diff | Splinter Review
7.66 KB, patch
nical
: review+
Details | Diff | Splinter Review
7.44 KB, patch
nical
: review+
Details | Diff | Splinter Review
5.70 KB, patch
nical
: review+
Details | Diff | Splinter Review
28.36 KB, patch
nical
: review+
Details | Diff | Splinter Review
5.94 KB, patch
nical
: review+
Details | Diff | Splinter Review
4.54 KB, patch
nical
: review+
Details | Diff | Splinter Review
16.76 KB, patch
nical
: review+
Details | Diff | Splinter Review
35.99 KB, patch
nical
: review+
Details | Diff | Splinter Review
12.46 KB, patch
nical
: review+
Details | Diff | Splinter Review
16.46 KB, patch
nical
: review+
Details | Diff | Splinter Review
10.09 KB, patch
nical
: review+
Details | Diff | Splinter Review
39.15 KB, patch
nical
: review+
Details | Diff | Splinter Review
23.60 KB, patch
nical
: review+
Details | Diff | Splinter Review
33.26 KB, patch
nical
: review+
Details | Diff | Splinter Review
15.76 KB, patch
nical
: review+
Details | Diff | Splinter Review
27.51 KB, patch
nical
: review+
Details | Diff | Splinter Review
33.84 KB, patch
nical
: review+
Details | Diff | Splinter Review
12.69 KB, patch
nical
: review+
Details | Diff | Splinter Review
15.64 KB, patch
nical
: review+
Details | Diff | Splinter Review
22.49 KB, patch
nical
: review+
Details | Diff | Splinter Review
5.41 KB, patch
nical
: review+
Details | Diff | Splinter Review
15.46 KB, patch
nical
: review+
Details | Diff | Splinter Review
5.35 KB, patch
nical
: review+
Details | Diff | Splinter Review
15.18 KB, patch
nical
: review+
Details | Diff | Splinter Review
8.30 KB, patch
nical
: review+
Details | Diff | Splinter Review
11.55 KB, patch
nical
: review+
Details | Diff | Splinter Review
31.21 KB, patch
Details | Diff | Splinter Review
12.77 KB, patch
nical
: review+
Details | Diff | Splinter Review
13.44 KB, patch
nical
: review+
Details | Diff | Splinter Review
9.63 KB, patch
nical
: review+
Details | Diff | Splinter Review
19.57 KB, patch
nical
: review+
Details | Diff | Splinter Review
5.60 KB, patch
nical
: review+
Details | Diff | Splinter Review
36.24 KB, patch
nical
: review+
Details | Diff | Splinter Review
7.19 KB, patch
nical
: review+
Details | Diff | Splinter Review
50.26 KB, patch
nical
: review+
Details | Diff | Splinter Review
14.10 KB, patch
nical
: review+
Details | Diff | Splinter Review
15.95 KB, patch
nical
: review+
Details | Diff | Splinter Review
7.86 KB, patch
nical
: review+
Details | Diff | Splinter Review
47.90 KB, patch
nical
: review+
Details | Diff | Splinter Review
43.13 KB, patch
nical
: review+
Details | Diff | Splinter Review
22.18 KB, patch
nical
: review+
Details | Diff | Splinter Review
50.85 KB, patch
nical
: review+
Details | Diff | Splinter Review
No description provided.
Attached patch IPDL changesSplinter Review
Assignee: nobody → dzbarsky
Status: NEW → ASSIGNED
Attachment #8351250 - Flags: review?(nical.bugzilla)
Attachment #8351255 - Flags: review?(nical.bugzilla)
Attachment #8351250 - Flags: review?(nical.bugzilla) → review+
Attachment #8351255 - Flags: review?(nical.bugzilla) → review+
Attachment #8351268 - Attachment is patch: true
Attachment #8351268 - Attachment mime type: message/rfc822 → text/plain
Attachment #8351268 - Flags: review?(nical.bugzilla)
Attached patch IsSVGTransformedSplinter Review
Attachment #8351386 - Flags: review?(nical.bugzilla)
Attachment #8351387 - Flags: review?(nical.bugzilla)
Attachment #8351388 - Flags: review?(nical.bugzilla)
Attachment #8351389 - Flags: review?(nical.bugzilla)
Attachment #8351391 - Flags: review?(nical.bugzilla)
Attached patch nsSVGImageFrameSplinter Review
Attachment #8351392 - Flags: review?(nical.bugzilla)
Attached patch GeneratePathSplinter Review
Attachment #8351393 - Flags: review?(nical.bugzilla)
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
Attachment #8351268 - Flags: review?(nical.bugzilla) → review+
Looks like we used to set the world transform to this matrix when using OpenGL layers, but now we have AsyncCompositionManager::ComputeRotation()
Attachment #8351416 - Flags: review?(mwu)
Attachment #8351425 - Flags: review?(nical.bugzilla)
Attached patch nsSVGTextFrameSplinter Review
Attachment #8351426 - Flags: review?(nical.bugzilla)
Attached patch GetCTMSplinter Review
Attachment #8351427 - Flags: review?(nical.bugzilla)
Attachment #8351428 - Flags: review?(nical.bugzilla)
Attached patch SVG DOM changes (obsolete) — Splinter Review
Attachment #8351429 - Flags: review?(nical.bugzilla)
Attachment #8351416 - Flags: review?(mwu) → review+
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?
Flags: needinfo?(nical.bugzilla)
I built on linux locally with clang and couldn't reproduce it, so it could also potentially be a float/double gcc compiler bug.
Attachment #8351386 - Flags: review?(nical.bugzilla) → review+
Attachment #8351387 - Flags: review?(nical.bugzilla) → review+
Attachment #8351388 - Flags: review?(nical.bugzilla) → review+
Attachment #8351389 - Flags: review?(nical.bugzilla) → review+
Attachment #8351391 - Flags: review?(nical.bugzilla) → review+
Attachment #8351392 - Flags: review?(nical.bugzilla) → review+
Attachment #8351393 - Flags: review?(nical.bugzilla) → review+
Attachment #8351425 - Flags: review?(nical.bugzilla) → review+
Attachment #8351426 - Flags: review?(nical.bugzilla) → review+
Attachment #8351427 - Flags: review?(nical.bugzilla) → review+
(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.
Flags: needinfo?(nical.bugzilla)
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?
(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
Attached patch World transformSplinter Review
Attachment #8354702 - Flags: review?(nical.bugzilla)
Attachment #8354704 - Flags: review?(nical.bugzilla)
Attached patch CreateMatrixSplinter Review
Attachment #8354705 - Flags: review?(nical.bugzilla)
Attachment #8354707 - Flags: review?(nical.bugzilla)
Attachment #8354710 - Flags: review?(nical.bugzilla)
Attachment #8354713 - Flags: review?(nical.bugzilla)
Attached patch DrawPixelSnappedSplinter Review
Attachment #8354716 - Flags: review?(nical.bugzilla)
Attachment #8351428 - Flags: review?(nical.bugzilla) → review+
Attachment #8351429 - Flags: review?(nical.bugzilla) → review+
Attachment #8354702 - Flags: review?(nical.bugzilla) → review+
Attachment #8354704 - Flags: review?(nical.bugzilla) → review+
Attachment #8354705 - Flags: review?(nical.bugzilla) → review+
Attachment #8354707 - Flags: review?(nical.bugzilla) → review+
Attachment #8354710 - Flags: review?(nical.bugzilla) → review+
Attachment #8354713 - Flags: review?(nical.bugzilla) → review+
Attachment #8354716 - Flags: review?(nical.bugzilla) → review+
Attachment #8351430 - Flags: review?(nical.bugzilla) → review+
Attachment #8363698 - Flags: review?(nical.bugzilla)
Attachment #8363698 - Flags: review?(nical.bugzilla) → review+
Attachment #8363848 - Flags: review?(nical.bugzilla)
Attachment #8363848 - Flags: review?(nical.bugzilla) → review+
Attached patch SnapTransformsSplinter Review
Attachment #8365095 - Flags: review?(nical.bugzilla)
Attachment #8365097 - Flags: review?(nical.bugzilla)
Attachment #8365098 - Flags: review?(nical.bugzilla)
Attached patch SetBaseTransformSplinter Review
Attachment #8365467 - Flags: review?(nical.bugzilla)
Attached patch GetTransformSplinter Review
Attachment #8365468 - Flags: review?(nical.bugzilla)
Attachment #8365469 - Flags: review?(nical.bugzilla)
Attached patch shadow transformSplinter Review
Attachment #8365470 - Flags: review?(nical.bugzilla)
Attached patch CleanupSplinter Review
Attachment #8365705 - Flags: review?(nical.bugzilla)
Attached patch OGLShaderProgramSplinter Review
Attachment #8365729 - Flags: review?(nical.bugzilla)
Attached patch PrepareViewportSplinter Review
Attachment #8365730 - Flags: review?(nical.bugzilla)
Attached patch DrawToSplinter Review
Attachment #8365731 - Flags: review?(nical.bugzilla)
Attachment #8365732 - Flags: review?(nical.bugzilla)
Attached patch AutoMaskDataSplinter Review
Attachment #8365733 - Flags: review?(nical.bugzilla)
Attachment #8365734 - Flags: review?(nical.bugzilla)
Attachment #8365095 - Flags: review?(nical.bugzilla) → review+
Attachment #8365097 - Flags: review?(nical.bugzilla) → review+
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).
Attachment #8365098 - Flags: review?(nical.bugzilla) → review+
Attachment #8365467 - Flags: review?(nical.bugzilla) → review+
Attachment #8365468 - Flags: review?(nical.bugzilla) → review+
Attachment #8365469 - Flags: review?(nical.bugzilla) → review+
Attachment #8365470 - Flags: review?(nical.bugzilla) → review+
Attachment #8365705 - Flags: review?(nical.bugzilla) → review+
Attachment #8365729 - Flags: review?(nical.bugzilla) → review+
Attachment #8365730 - Flags: review?(nical.bugzilla) → review+
Attachment #8365731 - Flags: review?(nical.bugzilla) → review+
Attachment #8365732 - Flags: review?(nical.bugzilla) → review+
Attachment #8365733 - Flags: review?(nical.bugzilla) → review+
Attachment #8365734 - Flags: review?(nical.bugzilla) → review+
Attached patch SVG DOM changesSplinter Review
Attachment #8351429 - Attachment is obsolete: true
Attachment #8463029 - Flags: review?(nical.bugzilla)
Attachment #8463030 - Flags: review?(nical.bugzilla)
Attachment #8463031 - Flags: review?(nical.bugzilla)
Attachment #8463032 - Flags: review?(nical.bugzilla)
Attached patch Layer testsSplinter Review
Attachment #8463033 - Flags: review?(nical.bugzilla)
Attachment #8463034 - Flags: review?(nical.bugzilla)
Attachment #8463035 - Flags: review?(nical.bugzilla)
Attached patch APZCSplinter Review
Attachment #8463078 - Flags: review?(nical.bugzilla)
Attachment #8463079 - Flags: review?(nical.bugzilla)
Attachment #8463151 - Flags: review?(nical.bugzilla)
Attachment #8463173 - Flags: review?(nical.bugzilla)
Attached patch Remove gfxPointH3D (obsolete) — Splinter Review
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.
Attachment #8463174 - Flags: review?(nical.bugzilla)
Attachment #8463029 - Flags: review?(nical.bugzilla) → review+
Attachment #8463030 - Flags: review?(nical.bugzilla) → review+
Attachment #8463031 - Flags: review?(nical.bugzilla) → review+
Attachment #8463032 - Flags: review?(nical.bugzilla) → review+
Attachment #8463033 - Flags: review?(nical.bugzilla) → review+
Attachment #8463034 - Flags: review?(nical.bugzilla) → review+
Attachment #8463035 - Flags: review?(nical.bugzilla) → review+
Attachment #8463078 - Flags: review?(nical.bugzilla) → review+
Attachment #8463079 - Flags: review?(nical.bugzilla) → review+
Attachment #8463151 - Flags: review?(nical.bugzilla) → review+
Attachment #8463173 - Flags: review?(nical.bugzilla) → review+
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.
Attachment #8463174 - Flags: review?(nical.bugzilla) → review-
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.
Good catch on the Inverse bug.  That's the only thing I've changed here
Attachment #8463174 - Attachment is obsolete: true
Attachment #8470019 - Flags: review?(nical.bugzilla)
Attachment #8470019 - Flags: review?(nical.bugzilla) → review+
Attached patch nsLayoutUtilsSplinter Review
Attachment #8476729 - Flags: review?(nical.bugzilla)
Summary: Switch gfxMatrix to gfx::Matrix → Switch gfx3DMatrix to gfx::Matrix4x4
Attachment #8477200 - Flags: review?(nical.bugzilla)
Attached patch ReadTransformsSplinter Review
Attachment #8477203 - Flags: review?(nical.bugzilla)
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?
Attachment #8476729 - Flags: review?(nical.bugzilla) → review+
Attachment #8477200 - Flags: review?(nical.bugzilla) → review+
Attachment #8477203 - Flags: review?(nical.bugzilla) → review+
Depends on: 1062899
No longer depends on: 1062899
Depends on: 1077961
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.
Flags: needinfo?(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.
Flags: needinfo?(dzbarsky)
This bug already has a ton of patches; we can do more in a followup.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Summary: Switch gfx3DMatrix to gfx::Matrix4x4 → Switch some uses of gfx3DMatrix to gfx::Matrix4x4
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.