Add Matrix5x4::operator* for color matrix multiplication

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

Trunk
mozilla35
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

Multiplying color matrices is something people might want to do, and I even have a use case in bug 1050804, so let's add it.
Attachment #8475276 - Flags: review?(bas)
Comment on attachment 8475276 [details] [diff] [review]
add-colormatrix-multiplication

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

Perfect.  Need it to clarify some code in bug 1016539 and bug 1055891.  Also, are unit tests possible here?
Attachment #8475276 - Flags: feedback+
This is now "needed" for 34, as the code for the dependent bug 1055891 is much clearer.
(In reply to Milan Sreckovic [:milan] from comment #1)
> Also, are unit tests possible here?

I can add one like http://dxr.mozilla.org/mozilla-central/source/gfx/2d/unittest/TestPoint.cpp
While writing the test I realized that it would also be nice to be able to compare color matrices to each other.
Attachment #8475939 - Flags: review?(bas)
Attachment #8475941 - Flags: review?(bas)
(In reply to Markus Stange [:mstange] from comment #4)
> Created attachment 8475939 [details] [diff] [review]
> add operator== and operator!=
> 
> While writing the test I realized that it would also be nice to be able to
> compare color matrices to each other.

And if we got greedy, we'd also go for operator*= :)
Posted patch add operator *= (obsolete) — Splinter Review
Getting mighty greedy here!
Attachment #8476032 - Flags: review?(bas)
Comment on attachment 8476032 [details] [diff] [review]
add operator *=

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

::: Matrix.h
@@ +638,5 @@
>  
> +  Matrix5x4& operator*=(const Matrix5x4 &aMatrix)
> +  {
> +    Matrix5x4 resultMatrix = *this * aMatrix;
> +    return *this = resultMatrix;

This is weirding me out (compared to *this = resultMatrix; return *this;), but that's just a matter of style.
Attachment #8476032 - Flags: feedback+
Attachment #8475276 - Flags: review?(bas) → review+
Comment on attachment 8475939 [details] [diff] [review]
add operator== and operator!=

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

::: Matrix.h
@@ +599,5 @@
> +    return _11 == o._11 && _12 == o._12 && _13 == o._13 && _14 == o._14 &&
> +           _21 == o._21 && _22 == o._22 && _23 == o._23 && _24 == o._24 &&
> +           _31 == o._31 && _32 == o._32 && _33 == o._33 && _34 == o._34 &&
> +           _41 == o._41 && _42 == o._42 && _43 == o._43 && _44 == o._44 &&
> +           _51 == o._51 && _52 == o._52 && _53 == o._53 && _54 == o._54;

I feel we should do a fuzzy comparison here.. am I wrong? If I'm wrong, I feel a memcmp is probably a faster alternative, although the compiler might figure this out.
Comment on attachment 8475941 [details] [diff] [review]
add basic unit test

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

You rock.
Attachment #8475941 - Flags: review?(bas) → review+
Comment on attachment 8476032 [details] [diff] [review]
add operator *=

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

::: Matrix.h
@@ +638,5 @@
>  
> +  Matrix5x4& operator*=(const Matrix5x4 &aMatrix)
> +  {
> +    Matrix5x4 resultMatrix = *this * aMatrix;
> +    return *this = resultMatrix;

I'm with Milan on this one :). But feel free to pick your favorite style.
Attachment #8476032 - Flags: review?(bas) → review+
My favorite way of writing this is actually
*this = *this * aMatrix;
return *this;

but in the patch I just copied the existing operator*= from Matrix, because usually being consistent with the rest of the file trumps all style considerations ;-)

I'll just change both.
(In reply to Bas Schouten (:bas.schouten) from comment #9)
> Comment on attachment 8475939 [details] [diff] [review]
> add operator== and operator!=
> 
> Review of attachment 8475939 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: Matrix.h
> @@ +599,5 @@
> > +    return _11 == o._11 && _12 == o._12 && _13 == o._13 && _14 == o._14 &&
> > +           _21 == o._21 && _22 == o._22 && _23 == o._23 && _24 == o._24 &&
> > +           _31 == o._31 && _32 == o._32 && _33 == o._33 && _34 == o._34 &&
> > +           _41 == o._41 && _42 == o._42 && _43 == o._43 && _44 == o._44 &&
> > +           _51 == o._51 && _52 == o._52 && _53 == o._53 && _54 == o._54;
> 
> I feel we should do a fuzzy comparison here.. am I wrong? If I'm wrong, I
> feel a memcmp is probably a faster alternative, although the compiler might
> figure this out.

I'll use FuzzyEquals. "Matrix" uses that as well, but "Matrix4x4" doesn't. And about the memcmp, Matrix4x4::operator== has this comment in it:
// XXX would be nice to memcmp here, but that breaks IEEE 754 semantics
Posted patch add operator== and operator!= (obsolete) — Splinter Review
Attachment #8475939 - Attachment is obsolete: true
Attachment #8475939 - Flags: review?(bas)
Attachment #8476601 - Flags: review?(bas)
Attachment #8476032 - Attachment is obsolete: true
Comment on attachment 8476601 [details] [diff] [review]
add operator== and operator!=

wait, "Matrix" has its own FuzzyEqual with a fixed epsilon, this doesn't actually compile
Attachment #8476601 - Attachment is obsolete: true
Attachment #8476601 - Flags: review?(bas)
Comment on attachment 8476606 [details] [diff] [review]
add operator== and operator!= with fuzzy comparison

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

I know Bas asked for a FuzzyEquals, and we're getting into a longer conversations here, but I'm wondering if we should leave == be actually ==, and if necessary add a separate FuzzyEquals method.  That way, it's a bit more obvious, and given that we currently have no users of this method, we could teach people to think about what they actually want, instead of us choosing for them.  Especially since the cost of operator== goes up as it becomes fuzzy...
Good point. Whatever choice we make, we should be consistent between our Matrix types.

I get the feeling that this issue needs more discussion, but I don't want it to block operator* from landing, so tomorrow I'll just land the already reviewed patches and file a new bug for operator==.
(I'm thinking I'll just comment out the lines in the test that use == and !=.)
Comment on attachment 8476606 [details] [diff] [review]
add operator== and operator!= with fuzzy comparison

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

I'm fine with FuzzyEquals or no FuzzyEquals. It all depends on the usecases we have for this function. I was mainly suggesting FuzzyEquals for consistency with other places in the tree.
Attachment #8476606 - Flags: review?(bas) → review+
I didn't get a chance to land this today, and I won't be able to until Tuesday, so frel free to land whichever parts you need to complete your work on the other bug, Milan.
Comment on attachment 8475276 [details] [diff] [review]
add-colormatrix-multiplication

Tagged this one as checked in.
Attachment #8475276 - Flags: checkin+
Leave open until the other patches land.
Keywords: leave-open
Comment on attachment 8475276 [details] [diff] [review]
add-colormatrix-multiplication

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

::: gfx/2d/Matrix.h
@@ +669,5 @@
>      , _41(a41), _42(a42), _43(a43), _44(a44)
>      , _51(a51), _52(a52), _53(a53), _54(a54)
>    {}
> +
> +  Matrix5x4 operator*(const Matrix5x4 &aMatrix) const

This is way too big to be in a header. The compiler won't/shouldn't inline something this big so we're just going to work the linker harder to resolve this.

Should this be a loop? That way we can let the compiler decide to unroll this or not. I don't know if compilers can do the reverse a 'roll' something up into a loop.
I believe Markus was just following the previously established pattern in this file - this is how it was done for a 4x4 matrix, 5x4 isn't that much worse.  So, we should probably open a separate bug with "investigate which of the inline methods in Matrix.h should stop being inline, moved to Matrix.cpp, and optimized for performance" or something like that?
No longer blocks: 1055891
I'm leaning towards making fuzzy equality checks explicit. I don't currently have a use case for fuzzy color matrix comparison, so I'm just going to land operator== with == comparison.

For consistency it would be nice to change |Matrix|, too, but I'm not going to do that in this bug. But I've checked the callers of the Matrix version: Most are assertions that I think will always pass even with exact comparison. There's one caller that might benefit from a fuzzy comparison in order to avoid some unnecessary work, and that's SVGTransformableElement::SetAnimateMotionTransform.
Attachment #8476606 - Attachment description: add operator== and operator!= → add operator== and operator!= with fuzzy comparison
Attachment #8476606 - Attachment is obsolete: true
Attachment #8475939 - Attachment is obsolete: false
https://hg.mozilla.org/mozilla-central/rev/c412208c4841
https://hg.mozilla.org/mozilla-central/rev/4a969a77fd31
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.