(In JavaScript) If A and B are each an SVGMatrix then A.multiply(B) multiplies them. According to my best reading of the spec the product should be AB. Safari computes BA. Firefox computes AB. Opera (as far as I can tell) computes AB. URL is a bookmarklet that alerts the results of multiplying matrix(0 1 1 0 0 0) by translate(1 0). It should produce the 6 numbers: 0,1,1,0,0,1 (for example, on Firefox). Safari produces: 0,1,1,0,1,0. There's more discussion at a blog article I wrote, http://drj11.wordpress.com/2007/11/19/the-trouble-with-matrix-multiplication/ , but you probably don't need that. If you think WebKit is right and Gecko is wrong, you can duke it out on the Moz bug I made earlier: https://bugzilla.mozilla.org/show_bug.cgi?id=404221
Oops. This should be easy to fix. :)
This probably regressed during the initial import of KDOM and conversion of SVGMatrix to use AffineTransform. AffineTransform does matrix math inverted from how SVGMatrix used to. We probably never swapped this multiply correctly. I'll investigate.
Ok, trying this code with AffineTransform: { AffineTransform L(0,1,1,0,0,0); AffineTransform R(0,0,0,0,1,0); AffineTransform LR = L*R; AffineTransform RL = R*L; printf("LR: %f,%f,%f,%f,%f,%f\n", LR.a(), LR.b(), LR.c(), LR.d(), LR.e(), LR.f()); printf("RL: %f,%f,%f,%f,%f,%f\n", RL.a(), RL.b(), RL.c(), RL.d(), RL.e(), RL.f()); } Produced: LR: 0.000000,0.000000,0.000000,0.000000,1.000000,0.000000 RL: 0.000000,0.000000,0.000000,0.000000,0.000000,1.000000 Which is reversed from what is expected, according to http://drj11.wordpress.com/2007/11/19/the-trouble-with-matrix-multiplication/ Interesting to note, the matrix layout shown: http://www.w3.org/TR/SVG/coords.html#TransformMatrixDefined is different from the one used by CG: http://developer.apple.com/documentation/GraphicsImaging/Reference/CGAffineTransform/Reference/reference.html#//apple_ref/c/func/CGAffineTransformMake
If I did my math right, the differing matrix layouts could cause this issue: When laid out the SVG-way my hand-calculated result of LR = 0,0,0,0,0,1, when laid out the CG way: 0,0,0,0,1,0.
(In reply to comment #3) > > Interesting to note, the matrix layout shown: > http://www.w3.org/TR/SVG/coords.html#TransformMatrixDefined > is different from the one used by CG: > http://developer.apple.com/documentation/GraphicsImaging/Reference/CGAffineTransform/Reference/reference.html#//apple_ref/c/func/CGAffineTransformMake > Oh-oh. SVG is using the column vector convention where a matrix M operates on a column vector V by the multiplication MV. Note V is a 3 row by 1 column vector so MV is too. Looks like CG is using the row-vector convention so a matrix M operates on a vector V by VM; this time V is a 1 row by 3 column vector so VM is too. Oh, that couldn't possibly be confusing could it?
I think it's just that AffineTransform has got it wrong. I'm seeing the same problem with CSS transforms.
(In reply to comment #6) > I think it's just that AffineTransform has got it wrong. I'm seeing the same > problem with CSS transforms. I think it's just that SVG/CSS use column vectors, and CG/AffineTransform uses row vectors. As far as I can tell AffineTransform has it right. As you know matrices operate on vectors and can be considered as linear transformations in a vector space. But it matters whether you talk about row vectors or column vectors. A matrix M operates on a column vector v by acting on the left, we form the product Mv. A matrix operates on a row vector by acting on the right: vM. Essentially CG operates in a "transpose world" compared to SVG. So instead of doing the operating Mv, in CG we do (v^T)(M^T) where (@^T) denote transpose. That's why Eric notes in comment 3 that CG and SVG use different matrix layouts (they are transposes of each). Essentially when SVG asks for a vector transformation Mv, what you actually perform in CG is ((v^T)(M^T))^T. The transposition comes "for free" as you write the entries into the vector and matrix and read the result out. This is no trouble until you need to multiply matrices. If SVG asks you to compute ABv then in transpose world you need to compute (v^T)(B^T)(A^T) because (AB)^T = (B^T)(A^T).
(In reply to comment #7) > As far as I can tell AffineTransform has it right. Yeah, I agree now. Callers need to change. I am going to change CSS transforms.
Removing cairo keyword, as this doesn't seem to be cairo specific.
Created attachment 42446 [details] Test case exhibiting the problem WebKit browsers get the multiplication incorrect (m.e = 200!) while all other browsers pass
Did this implementation change since Comment #7? I don't find it to match what the test case exhibits. In the test I take two matrices: 1 0 100 0 1 0 0 0 1 and 2 0 0 0 1 0 0 0 1 Multiplying these together gives: 2 0 100 0 1 0 0 0 1 But WebKit gives: 2 0 200 0 1 0 0 0 1 And I can't figure out any matrix operation that would generate that result.
That's the expected behavior from the "multiply method is wrong way around." Your first matrix (A) is a translation by 100 in x. The second (B) is a scale by a factor of 2 in x. In this case, B is applied in the global coordinates, so a space already translated by 100 in x gets scaled, translating to 200. You are trying to multiply on the right (A*B), but since WebKit stores matrices as the transpose of what you have written there, a multiply of a transpose by a transpose on the right (A^T*B^T) is equivalent to a multiply on the left of the matrices you have written there (B*A).
Yep - got it now, thanks.
See also bug 23243.
Created attachment 42492 [details] Patch adds a custom method to JSVGMatrix to ensure proper matrix multiplication
Comment on attachment 42492 [details] Patch adds a custom method to JSVGMatrix to ensure proper matrix multiplication Tabs in the file. No ChangeLog. I'm not sure this is the right place to fix this.
Jeff and I talked this over, and we think this is the right approach. But I'm not sure if JSSVGMatrix::multiply() needs to check the number of args before getting args.at(0). JSSVGMatrix::rotateFromVector() may also be bad in this regard. The patch also needs a testcase.
The patch should make SVGMatrix functionally equivalent to the transpose of its actual representation, so this does seem like the best place to make the change. Any thoughts on if a similar change should be applied to WebKitCSSMatrix? The draft spec is less explicit than SVG's, but the implication seems to be that it should follow SVGMatrix in convention.
Created attachment 42505 [details] Updated patch: spaces instead of tabs, added test and changelogs
@Brendan: no idea about CSSMatrix, sorry. @Simon: For your arglist checking question, I found JavaScriptCore/runtime/ArgList.h which has ArgList::at() defined: JSValue at(size_t idx) const { if (idx < m_argCount) return m_args[idx]; return jsUndefined(); } This means that if there are no arguments, an at(0) will return jsUndefined() - which should fail this if and return a Type Error: if (!args.at(0).inherits(&JSSVGMatrix::s_info)) I checked DOM2 and DOM3 and there is no standard exception as far as I can tell for 'not enough arguments'. My own belief is that a Type Error is just fine, so that's what the patch has. Mozilla throws a NS_ERROR_XPC_NOT_ENOUGH_ARGS exception. Does WebKit have something similar? If so, I can check args.size() first and throw that if you prefer. I cannot comment on rotateFromVector() and would suggest a separate patch for it.
Hm, I found this in about 20 places in WebKit: if (args.size() < 1) return throwError(exec, SyntaxError, "Not enough arguments"); Do we prefer that?
Created attachment 42507 [details] Updated patch: added check for enough arguments. Also added copyright as this change is now over 10 lines.
Since I prefer that (from a author/debugging point of view), I went ahead and made the syntax error check in multiply().
Comment on attachment 42507 [details] Updated patch: added check for enough arguments. Also added copyright as this change is now over 10 lines. I think this is good, but you might want to consider adding the test to SVGMatrix-interface.svg rather than creating a new test. Ideally SVGMatrix-interface.svg would be a much more comprehensive test of the SVGMatrix interface. See LayoutTests/transforms/cssmatrix-2d-interface.xhtml which could probably be converted to SVGMatrix easily.
Created attachment 42527 [details] Reworked CSSMatrix test and added further tests for all other SVGMatrix functionality.
If this patch is accepted, we can then remove the following test files as they are no longer needed: LayoutTests/svg/custom/SVGMatrix-interface.svg LayoutTests/platform/mac/svg/custom/SVGMatrix-interface-* (there is no reason to test this in platform-specific code, this is just a DOM object that has no visual representation)
Comment on attachment 42527 [details] Reworked CSSMatrix test and added further tests for all other SVGMatrix functionality. > Index: WebCore/ChangeLog > =================================================================== > +>>>>>>> .r50529 You have a conflict marker in the patch. > Index: LayoutTests/ChangeLog > =================================================================== > +>>>>>>> .r50529 Ditto. r=me with those fixed. You should attach a clean patch and we can put it in the commit-queue.
Thanks Jeff for fixing this bug! Patch looks good to me as well.
Created attachment 42535 [details] Remove conflict markers from changelog in patch.
Comment on attachment 42535 [details] Remove conflict markers from changelog in patch. Patches have to be marked r+ as well as cq+. Patches marked with just a cq+ get ignored by the commit-queue. I guess I could teach it to reject patches with just a cq+, but that seems draconian. The issue is that a committer can cq+ a patch, but it takes a reviewer's OK as well for something to get committed.
Thanks again for the patch Jeff. And thanks for the quick review Simon!
Comment on attachment 42535 [details] Remove conflict markers from changelog in patch. It seems an even slicker solution would have been to have a custom JSSVGStaticPODTypeWrapper<TransformationMatrix> subclass called SVGMatrix which defined a special mutiply() function. As is, this will require v8 to fix it the exact same way. But this is OK for now.
Comment on attachment 42535 [details] Remove conflict markers from changelog in patch. Rejecting patch 42535 from commit-queue. Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Eric Seidel', '--force']" exit_code: 2 patching file WebCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patch: **** malformed patch at line 28: patching file WebCore/bindings/js/JSSVGMatrixCustom.cpp patching file WebCore/svg/SVGMatrix.idl patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patch: **** malformed patch at line 25: patching file LayoutTests/svg/dom/SVGMatrix-interface-expected.txt patching file LayoutTests/svg/dom/SVGMatrix-interface.xhtml
Ha! You edited the patch file directly it looks like. Unless you know how to speak patch, that will just get you in trouble. :) You can read the documentation on unified diffs here: http://en.wikipedia.org/wiki/Diff#Unified_format But the better thing to do is to just always re-make the patch instead of modifying the patch file.
Comment on attachment 42535 [details] Remove conflict markers from changelog in patch. This can't be landed as is with the broken patch file, so marking r-.
Created attachment 42556 [details] Updated correct patch via svn-create-patch with apologies :)
Sorry about the malformed patch, Eric. About Comment #32, I didn't realize (but I should have) that this would require separate fixing from V8 side. Looks like it's time for me to check out chromium :)
Comment on attachment 42556 [details] Updated correct patch via svn-create-patch with apologies :) Rejecting patch 42556 from commit-queue. codedread@gmail.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.
Comment on attachment 42556 [details] Updated correct patch via svn-create-patch with apologies :) Going to try commit-queue? again
Comment on attachment 42556 [details] Updated correct patch via svn-create-patch with apologies :) Clearing flags on attachment: 42556 Committed r50561: <http://trac.webkit.org/changeset/50561>
All reviewed patches have been landed. Closing bug.