Closed Bug 1416267 Opened 2 years ago Closed 2 years ago

Avoid unnecessary float<->double conversions with gfxContext::mTransform

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file)

gfxContext::mTransform [1] is a gfx::Matrix, which uses floats internally. However, the CurrentMatrix() and SetMatrix() functions [2] on gfxContext take the similar-sounding but different gfxMatrix class, which uses doubles internally. So anytime CurrentMatrix() or SetMatrix() is called, there is conversion between floats and doubles.

However, many of the call sites of CurrentMatrix() and SetMatrix() start out with gfx::Matrix, which means they have to do an artificial round-trip from floats to doubles and then back to floats in order to use these APIs. This is wasteful.

While waiting for compiles I started writing a patch to clean this up. My plan is to change the existing CurrentMatrix() and SetMatrix() functions to take the float-using Matrix, since that matches the internal representation of mTransform. The call sites that can avoid the round-trip will avoid it. For the call sites that use a double gfxMatrix as their native representation, or that are non-trivial to convert, I'm adding two new functions CurrentMatrixDouble() and SetMatrixDouble() that continue to do the conversion that the current functions do.

This change should be functionally a no-op (assuming that converting from floats to doubles and back doesn't change them) and the structure of the patch should ensure that the compiler catches all the relevant call sites.

[1] https://searchfox.org/mozilla-central/rev/30ead7d1ae5bf95b8bc0fd67b950cd46ca05e32c/gfx/thebes/gfxContext.h#538
[2] https://searchfox.org/mozilla-central/rev/30ead7d1ae5bf95b8bc0fd67b950cd46ca05e32c/gfx/thebes/gfxContext.cpp#320-331
Comment on attachment 8927437 [details]
Bug 1416267 - Update gfxContext matrix functions to avoid flip-flopping between float and double matrices.

https://reviewboard.mozilla.org/r/198764/#review203892

::: gfx/thebes/gfxContext.cpp:328
(Diff revision 1)
> +  CURRENTSTATE_CHANGED()
> +  ChangeTransform(matrix);
> +}
> +
> +void
> +gfxContext::SetMatrixDouble(const gfxMatrix& matrix)

Maybe just call SetMatrix() instead of duplicating its contents?
Attachment #8927437 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8927437 [details]
Bug 1416267 - Update gfxContext matrix functions to avoid flip-flopping between float and double matrices.

https://reviewboard.mozilla.org/r/198764/#review203894

Hopefully this doesn't cause any test failures :P
Comment on attachment 8927437 [details]
Bug 1416267 - Update gfxContext matrix functions to avoid flip-flopping between float and double matrices.

https://reviewboard.mozilla.org/r/198764/#review203892

> Maybe just call SetMatrix() instead of duplicating its contents?

Will do
Comment on attachment 8927437 [details]
Bug 1416267 - Update gfxContext matrix functions to avoid flip-flopping between float and double matrices.

https://reviewboard.mozilla.org/r/198764/#review203894

The try push above was green so hopefully all's good. I'll do another build-only try push on autoland to make sure nothing's changed out from under me.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b8f533c5a270
Update gfxContext matrix functions to avoid flip-flopping between float and double matrices. r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/b8f533c5a270
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.