1055661 - Add Matrix5x4::operator* for color matrix multiplication

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Markus Stange [:mstange]]

Attachment: add-colormatrix-multiplication

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.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

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?

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

This is now "needed" for 34, as the code for the dependent bug 1055891 is much clearer.

Comment 3

[Markus Stange [:mstange]]

(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

Comment 4

[Markus Stange [:mstange]]

Attachment: 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.

Comment 5

[Markus Stange [:mstange]]

Attachment: add basic unit test

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

(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*= :)

Comment 7

[Markus Stange [:mstange]]

Attachment: add operator *=

Getting mighty greedy here!

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 9

[Bas Schouten (:bas.schouten)]

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 10

[Bas Schouten (:bas.schouten)]

Comment on attachment 8475941 [details] [diff] [review]
add basic unit test

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

You rock.

Comment 11

[Bas Schouten (:bas.schouten)]

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.

Comment 12

[Markus Stange [:mstange]]

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.

Comment 13

[Markus Stange [:mstange]]

(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

Comment 14

[Markus Stange [:mstange]]

Attachment: add operator== and operator!=

Comment 15

[Markus Stange [:mstange]]

Attachment: add operator*=

Comment 16

[Markus Stange [:mstange]]

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

Comment 17

[Markus Stange [:mstange]]

Attachment: add operator== and operator!= with fuzzy comparison

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

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...

Comment 19

[Markus Stange [:mstange]]

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 20

[Bas Schouten (:bas.schouten)]

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.

Comment 21

[Markus Stange [:mstange]]

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 22

[Milan Sreckovic [:milan] (needinfo for best results)]

Landing the first part.  This patch was "try"-ed with https://tbpl.mozilla.org/?tree=Try&rev=4b140534a5a1 as a part of testing bug 1055891.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5dbd6c1bf6c

Comment 23

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8475276 [details] [diff] [review]
add-colormatrix-multiplication

Tagged this one as checked in.

Comment 24

[Milan Sreckovic [:milan] (needinfo for best results)]

Leave open until the other patches land.

Comment 25

[Benoit Girard (:BenWa)]

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.

Comment 26

[Milan Sreckovic [:milan] (needinfo for best results)]

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?

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d5dbd6c1bf6c

Comment 28

[Markus Stange [:mstange]]

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.

Comment 29

[Markus Stange [:mstange]]

http://hg.mozilla.org/users/bschouten_mozilla.com/moz2d/rev/6ba7df386a32
http://hg.mozilla.org/users/bschouten_mozilla.com/moz2d/rev/c3692f049e32
http://hg.mozilla.org/users/bschouten_mozilla.com/moz2d/rev/0c6ff6c4955b

https://hg.mozilla.org/integration/mozilla-inbound/rev/c412208c4841
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a969a77fd31

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c412208c4841
https://hg.mozilla.org/mozilla-central/rev/4a969a77fd31