Bug 50633 - Make WebKitCSSMatrix more efficient for use in WebGL
Summary: Make WebKitCSSMatrix more efficient for use in WebGL
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Chris Marrin
URL:
Keywords:
: 52488 (view as bug list)
Depends on:
Blocks: 110001
  Show dependency treegraph
 
Reported: 2010-12-07 09:15 PST by Chris Marrin
Modified: 2022-02-28 04:04 PST (History)
12 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Marrin 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 Simon Fraser (smfr) 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 Igor Trindade Oliveira 2011-06-13 10:22:15 PDT
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.
Comment 3 Kenneth Russell 2011-06-13 14:24:46 PDT
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.
Comment 4 Chris Marrin 2011-06-13 14:28:19 PDT
(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?
Comment 5 Igor Trindade Oliveira 2011-06-24 11:48:20 PDT
Created attachment 98521 [details]
Patch

Update patch with the changes proposed by Kenneth Russell.
Comment 6 Kenneth Russell 2011-06-24 14:40:43 PDT
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.
Comment 7 Simon Fraser (smfr) 2011-06-24 14:50:47 PDT
selfMultiply() is pretty ugly.

I wish the non-const methods were like 'multiplied', and the mutable versions were 'multiply'.
Comment 8 Dean Jackson 2011-06-24 14:52:13 PDT
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 Dean Jackson 2011-06-24 15:04:27 PDT
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 Chris Marrin 2011-06-24 15:17:21 PDT
(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 James Robinson 2011-07-07 17:41:14 PDT
Comment on attachment 98521 [details]
Patch

R-, consensus is against the self* naming convention.
Comment 12 Chris Marrin 2011-08-29 19:47:45 PDT
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 Igor Trindade Oliveira 2011-10-18 16:36:39 PDT
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.
Comment 14 WebKit Review Bot 2011-10-18 16:41:37 PDT
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 Simon Fraser (smfr) 2011-10-18 16:47:03 PDT
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 WebKit Review Bot 2011-10-18 17:58:57 PDT
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
Comment 17 Igor Trindade Oliveira 2011-10-19 06:11:32 PDT
(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 Simon Fraser (smfr) 2011-10-19 07:22:49 PDT
We should avoid adding new web-facing API without it being on a standards track.
Comment 19 Igor Trindade Oliveira 2011-10-19 08:57:50 PDT
Created attachment 111631 [details]
Patch

Proposed patch. Implement mutable methods for WebKitCSSMatrix.
Comment 20 Igor Trindade Oliveira 2011-10-19 11:27:11 PDT
Created attachment 111651 [details]
Patch

Updated patch. Implement mutable methods for WebKitCSSMatrix.
Comment 21 Dean Jackson 2011-10-19 15:06:14 PDT
(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 Igor Trindade Oliveira 2011-10-20 17:03:57 PDT
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 Simon Fraser (smfr) 2011-10-24 15:05:35 PDT
*** Bug 52488 has been marked as a duplicate of this bug. ***
Comment 24 Chris Marrin 2011-12-21 16:23:55 PST
Comment on attachment 111651 [details]
Patch

Patch looks good. But please fix Mac build issue and resubmit
Comment 25 Igor Trindade Oliveira 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])
> Patch looks good. But please fix Mac build issue and resubmit
Comment 26 Chris Marrin 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])
> > 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 Daniel Murphy 2012-05-25 15:45:18 PDT
Any developments with this?  What's the current status?
Comment 28 Igor Trindade Oliveira 2012-05-25 15:51:03 PDT
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 Dean Jackson 2012-05-25 17:54:34 PDT
(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!