Bug 50633 – Make WebKitCSSMatrix more efficient for use in WebGL

• Component: WebGL
• Status: NEW

[Chris Marrin]

Currently CSSMatrix has an immutable API. None of the calls for multiply(), inverse(), etc. change the CSSMatrix, but rather they return a new CSSMatrix with the changed value. A set of API's that mutate the CSSMatrix would result in many fewer objects being created, which would significantly improve performance in WebGL. These API could be added to the existing CSSMatrix. API needed would be: multiply(), inverse(), translate(), scale(), rotate() and rotateAxisAngle(). We could add a suffix to the new functions, such as invertMatrix().

Also, there is currently no way to get the values out of a CSSMatrix other than using the mXX attributes and picking out each value separately. I experimentally added a copy() function to CSSMatrix. It is passed a Float32Array with space for 16 elements, and the values are placed in it. This gave me a 30% increase in WebGL performance on the spinning box demo.

Adding these 2 features to CSSMatrix would greatly improve WebGL.

[Simon Fraser (smfr)]

Maybe it's time to rename CSSMatrix to make it non CSS-specific. It should really be shared with SVGTransform too.

I think these new suggestions are fine, as long as we can maintain compatibility with the existing API.

[Igor Trindade Oliveira]

Created attachment 96968 [details]
Patch

Proposed Patch.
The patch was tested using https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/demos/webkit/MatrixTest.html test with modifications. Together with callgrind analysis.

[Kenneth Russell]

Comment on attachment 96968 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96968&action=review

This mostly looks okay, but the naming of the copy operations should be made consistent. Couple of other minor issues.

> LayoutTests/transforms/cssmatrix-3d-interface.xhtml:244
> +shouldBe('parseFloat(m.m11)', '538');

I'm not sure this is the best way to write this. I've seen problems in other tests where floating point values like "1.0e12" weren't handled correctly. For situations where you're doing a simple truncation to integer I would suggest using Math.floor(). For example: shouldBe('Math.floor(m.m11)', '538');

> Source/WebCore/css/WebKitCSSMatrix.h:106
>      // The following math function return a new matrix with the 
>      // specified operation applied. The this value is not modified.

This comment isn't accurate any more. Either group the in-place operations together in their own section or adjust this comment. Also, the new entry points need documentation.

> Source/WebCore/css/WebKitCSSMatrix.idl:63
> +        void copy(in Float32Array array);
> +        void copy64(in Float64Array array);

These methods can and should be named identically; WebKit's code generators (both JSC and V8) support overloaded methods. Personally I would probably name these "copyTo" or "toArray".

> Source/WebCore/css/WebKitCSSMatrix.idl:67
> +        void multiplyMatrix(in WebKitCSSMatrix secondMatrix);

This and all of the other new entry points need documentation.

[Chris Marrin]

(In reply to comment #3)
> (From update of attachment 96968 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96968&action=review
> 
> This mostly looks okay, but the naming of the copy operations should be made consistent. Couple of other minor issues.

I agree about the copy operations. I'd also like to see better names for the other functions. translateMatrix() just doesn't seem like it is the mutable sibling of translate(). A while back Dean came up with a naming convention that seemed reasonable, but I don't remember what it was. Dean?

[Igor Trindade Oliveira]

Created attachment 98521 [details]
Patch

Update patch with the changes proposed by Kenneth Russell.

[Kenneth Russell]

Comment on attachment 98521 [details]
Patch

Thanks for the updates. This looks fine to me. I think Chris or Simon should take a look and make the final call.

[Simon Fraser (smfr)]

selfMultiply() is pretty ugly.

I wish the non-const methods were like 'multiplied', and the mutable versions were 'multiply'.

[Dean Jackson]

I don't really like the self* naming because it doesn't really make sense. I can't think of much better, but maybe something like "multiplyInPlace"?

Simon suggests in IRC that the const versions could be called multipliedBy and the non-const versions multiply. That's pretty good.

[Dean Jackson]

Or, let's just bite the bullet and move to a new API -> WebKitMatrix or something.

Have the methods mutable by default, and non-mutable methods available.

[Chris Marrin]

(In reply to comment #9)
> Or, let's just bite the bullet and move to a new API -> WebKitMatrix or something.
> 
> Have the methods mutable by default, and non-mutable methods available.

I like that idea. We can make this class accepted where CSSMatrix and SVGMatrix are accepted today. And eventually this can become the preferred class for all our Matrix needs.

[James Robinson]

Comment on attachment 98521 [details]
Patch

R-, consensus is against the self* naming convention.

[Chris Marrin]

We should also add a copy() function to copy the Matrix into a Float32Array so it can be passed to WebGL. And maybe a ctor that takes a Float32Array would be useful, too

[Igor Trindade Oliveira]

Created attachment 111526 [details]
Patch

Proposed patch. Implement WebKitMatrix, a new API for matrix operations. It has mutable and non mutable methods making it better to be used by WebGL in terms of performance.

[WebKit Review Bot]

Attachment 111526 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/tran..." exit_code: 1

Source/WebCore/WebCore.vcproj/WebCore.vcproj:25640:  mismatched tag  [xml/syntax] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

What's the standardization track for this "Matrix" class? Is it OK to introduce a class with a generic name like "Matrix" into the global namespace?

[WebKit Review Bot]

Comment on attachment 111526 [details]
Patch

Attachment 111526 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10138199

New failing tests:
transforms/webkitmatrix-3d-interface.xhtml

[Igor Trindade Oliveira]

(In reply to comment #15)
> What's the standardization track for this "Matrix" class? Is it OK to introduce a class with a generic name like "Matrix" into the global namespace?

There is not a standard for this "Matrix" class. It was based in the comments #9, #10 and #12. I think the main problem right now is  the CSSMatrix and SVGMatrix interfaces always create new matrices, this additional "new Matrix" creates an overhead. The idea proposed by comments #7 and #8 are fine for me, too. Chris, Dean any opinion here?

[Simon Fraser (smfr)]

We should avoid adding new web-facing API without it being on a standards track.

[Igor Trindade Oliveira]

Created attachment 111631 [details]
Patch

Proposed patch. Implement mutable methods for WebKitCSSMatrix.

[Igor Trindade Oliveira]

Created attachment 111651 [details]
Patch

Updated patch. Implement mutable methods for WebKitCSSMatrix.

[Dean Jackson]

(In reply to comment #17)
> (In reply to comment #15)
> > What's the standardization track for this "Matrix" class? Is it OK to introduce a class with a generic name like "Matrix" into the global namespace?
> 
> There is not a standard for this "Matrix" class. It was based in the comments #9, #10 and #12. I think the main problem right now is  the CSSMatrix and SVGMatrix interfaces always create new matrices, this additional "new Matrix" creates an overhead. The idea proposed by comments #7 and #8 are fine for me, too. Chris, Dean any opinion here?

As Simon said, we need some plan for how this will be interoperable. It's risky to put API in place before there is at least a specification somewhere. (The risk is that one of the WebKit ports ships with it and then we'll have pressure to continue supporting it).

The good news is that Matrix is hopefully a pretty simple API that won't be controversial. We definitely want it - I'm sure Mozilla do too. How about sending the IDL to [e-mail] and see if anyone speaks up against it before landing this?

[Igor Trindade Oliveira]

e-mail sent: 
http://lists.w3.org/Archives/Public/public-webapps/2011OctDec/0376.html


(In reply to comment #21)
> (In reply to comment #17)
> > (In reply to comment #15)
> > > What's the standardization track for this "Matrix" class? Is it OK to introduce a class with a generic name like "Matrix" into the global namespace?
> > 
> > There is not a standard for this "Matrix" class. It was based in the comments #9, #10 and #12. I think the main problem right now is  the CSSMatrix and SVGMatrix interfaces always create new matrices, this additional "new Matrix" creates an overhead. The idea proposed by comments #7 and #8 are fine for me, too. Chris, Dean any opinion here?
> 
> As Simon said, we need some plan for how this will be interoperable. It's risky to put API in place before there is at least a specification somewhere. (The risk is that one of the WebKit ports ships with it and then we'll have pressure to continue supporting it).
> 
> The good news is that Matrix is hopefully a pretty simple API that won't be controversial. We definitely want it - I'm sure Mozilla do too. How about sending the IDL to [e-mail] and see if anyone speaks up against it before landing this?

[Simon Fraser (smfr)]

*** Bug 52488 has been marked as a duplicate of this bug. ***

[Chris Marrin]

Comment on attachment 111651 [details]
Patch

Patch looks good. But please fix Mac build issue and resubmit

[Igor Trindade Oliveira]

i will be updating the patch soon.
(In reply to comment #24)
> (From update of attachment 111651 [details])
> Patch looks good. But please fix Mac build issue and resubmit

[Chris Marrin]

(In reply to comment #25)
> i will be updating the patch soon.
> (In reply to comment #24)
> > (From update of attachment 111651 [details] [details])
> > Patch looks good. But please fix Mac build issue and resubmit

Have you looked at Dean's Matrix4x4 proposal? Please make sure your work and his proposal are aligned, one way or the other. Thanks...

[Daniel Murphy]

Any developments with this?  What's the current status?

[Igor Trindade Oliveira]

After: http://lists.w3.org/Archives/Public/public-fx/2012JanMar/0007.html
I am waiting a updated proposal. Dean, do you mind if i send a new updated proposal? 

(In reply to comment #27)
> Any developments with this?  What's the current status?

[Dean Jackson]

(In reply to comment #28)
> After: http://lists.w3.org/Archives/Public/public-fx/2012JanMar/0007.html
> I am waiting a updated proposal. Dean, do you mind if i send a new updated proposal? 

Go for it!