Bug 50633 - Make WebKitCSSMatrix more efficient for use in WebGL
: Make WebKitCSSMatrix more efficient for use in WebGL
Status: NEW
: WebKit
WebGL
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 110001
  Show dependency treegraph
 
Reported: 2010-12-07 09:15 PST by
Modified: 2014-03-19 03:39 PST (History)


Attachments
Patch (23.64 KB, patch)
2011-06-13 10:22 PST, Igor Trindade Oliveira
kbr: review-
Review Patch | Details | Formatted Diff | Diff
Patch (25.06 KB, patch)
2011-06-24 11:48 PST, Igor Trindade Oliveira
jamesr: review-
Review Patch | Details | Formatted Diff | Diff
Patch (88.26 KB, patch)
2011-10-18 16:36 PST, Igor Trindade Oliveira
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch (31.06 KB, patch)
2011-10-19 08:57 PST, Igor Trindade Oliveira
no flags Review Patch | Details | Formatted Diff | Diff
Patch (28.68 KB, patch)
2011-10-19 11:27 PST, Igor Trindade Oliveira
cmarrin: review-
igor.oliveira: commit‑queue?
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-12-07 09:15:29 PST
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.
------- Comment #1 From 2010-12-07 10:07:44 PST -------
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.
------- Comment #2 From 2011-06-13 10:22:15 PST -------
Created an attachment (id=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.
------- Comment #3 From 2011-06-13 14:24:46 PST -------
(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.

> 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.
------- Comment #4 From 2011-06-13 14:28:19 PST -------
(In reply to comment #3)
> (From update of attachment 96968 [details] [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?
------- Comment #5 From 2011-06-24 11:48:20 PST -------
Created an attachment (id=98521) [details]
Patch

Update patch with the changes proposed by Kenneth Russell.
------- Comment #6 From 2011-06-24 14:40:43 PST -------
(From update of attachment 98521 [details])
Thanks for the updates. This looks fine to me. I think Chris or Simon should take a look and make the final call.
------- Comment #7 From 2011-06-24 14:50:47 PST -------
selfMultiply() is pretty ugly.

I wish the non-const methods were like 'multiplied', and the mutable versions were 'multiply'.
------- Comment #8 From 2011-06-24 14:52:13 PST -------
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.
------- Comment #9 From 2011-06-24 15:04:27 PST -------
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.
------- Comment #10 From 2011-06-24 15:17:21 PST -------
(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.
------- Comment #11 From 2011-07-07 17:41:14 PST -------
(From update of attachment 98521 [details])
R-, consensus is against the self* naming convention.
------- Comment #12 From 2011-08-29 19:47:45 PST -------
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
------- Comment #13 From 2011-10-18 16:36:39 PST -------
Created an attachment (id=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.
------- Comment #14 From 2011-10-18 16:41:37 PST -------
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.
------- Comment #15 From 2011-10-18 16:47:03 PST -------
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?
------- Comment #16 From 2011-10-18 17:58:57 PST -------
(From update of attachment 111526 [details])
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
------- Comment #17 From 2011-10-19 06:11:32 PST -------
(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?
------- Comment #18 From 2011-10-19 07:22:49 PST -------
We should avoid adding new web-facing API without it being on a standards track.
------- Comment #19 From 2011-10-19 08:57:50 PST -------
Created an attachment (id=111631) [details]
Patch

Proposed patch. Implement mutable methods for WebKitCSSMatrix.
------- Comment #20 From 2011-10-19 11:27:11 PST -------
Created an attachment (id=111651) [details]
Patch

Updated patch. Implement mutable methods for WebKitCSSMatrix.
------- Comment #21 From 2011-10-19 15:06:14 PST -------
(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 public-webapps@w3.org and see if anyone speaks up against it before landing this?
------- Comment #22 From 2011-10-20 17:03:57 PST -------
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 public-webapps@w3.org and see if anyone speaks up against it before landing this?
------- Comment #23 From 2011-10-24 15:05:35 PST -------
*** Bug 52488 has been marked as a duplicate of this bug. ***
------- Comment #24 From 2011-12-21 16:23:55 PST -------
(From update of attachment 111651 [details])
Patch looks good. But please fix Mac build issue and resubmit
------- Comment #25 From 2012-01-11 18:19:43 PST -------
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
------- Comment #26 From 2012-01-12 13:08:36 PST -------
(In reply to comment #25)
> i will be updating the patch soon.
> (In reply to comment #24)
> > (From update of attachment 111651 [details] [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...
------- Comment #27 From 2012-05-25 15:45:18 PST -------
Any developments with this?  What's the current status?
------- Comment #28 From 2012-05-25 15:51:03 PST -------
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?
------- Comment #29 From 2012-05-25 17:54:34 PST -------
(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!