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.
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.
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 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.
(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?
Created attachment 98521 [details] Patch Update patch with the changes proposed by 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.
selfMultiply() is pretty ugly. I wish the non-const methods were like 'multiplied', and the mutable versions were 'multiply'.
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.
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.
(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 on attachment 98521 [details] Patch R-, consensus is against the self* naming convention.
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
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.
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.
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 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
(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?
We should avoid adding new web-facing API without it being on a standards track.
Created attachment 111631 [details] Patch Proposed patch. Implement mutable methods for WebKitCSSMatrix.
Created attachment 111651 [details] Patch Updated patch. Implement mutable methods for WebKitCSSMatrix.
(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?
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?
*** Bug 52488 has been marked as a duplicate of this bug. ***
Comment on attachment 111651 [details] Patch Patch looks good. But please fix Mac build issue and resubmit
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
(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...
Any developments with this? What's the current status?
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?
(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!