116542 2013-05-21 08:45:46 -0700 [BlackBerry] Don't use reinterpret_cast to convert to TransformationMatrix 2014-01-09 21:08:27 -0800 1 1 1 Unclassified WebKit WebKit BlackBerry 528+ (Nightly build) Unspecified Unspecified RESOLVED INVALID P2 Normal --- 1 jpetsovits jpetsovits andersca anilsson commit-queue rwlbuis oldest_to_newest 891795 0 jpetsovits 2013-05-21 08:45:46 -0700 We're currently using reinterpret_cast in WebPageCompositor::render() to convert from a BlackBerry::Platform::TransformationMatrix to a WebCore::TransformationMatrix. This is especially bad since the BlackBerry::Platform one uses column-major storage and the WebCore one stores its values in row-major layout. It basically only worked because so far we're using an extremely limited subset of potential transformation matrices. Fixed by the patch below. 891797 1 202435 jpetsovits 2013-05-21 08:48:42 -0700 Created attachment 202435 Patch 891922 2 anilsson 2013-05-21 11:45:14 -0700 While the reinterpret_cast may be ugly, I don't agree that WebCore::TransformationMatrix is row major - I think the commit message is misleading. TransformationMatrix order was swapped by Chromium devs quite a while ago. 891923 3 anilsson 2013-05-21 11:48:52 -0700 I also note that the range of transformation matrices used by CSS3 Transforms is not at all limited, and converts successfully to BB::P::TransformationMatrix every frame. It's then passed directly to OpenGL down in platform, and OpenGL expects column major matrices (according to some accounts, not 100% sure about GL ES2 actually)... 891954 4 jpetsovits 2013-05-21 12:48:35 -0700 (In reply to comment #2) > While the reinterpret_cast may be ugly, I don't agree that WebCore::TransformationMatrix is row major - I think the commit message is misleading. TransformationMatrix order was swapped by Chromium devs quite a while ago. My math is not great, but let me get the facts together here. (1) http://en.wikipedia.org/wiki/Row-major_order row-major order means that an array of 1 2 3 4 5 6 is laid out in memory as { { 1, 2, 3 }, { 4, 5, 6 } }. That means the element with value 4 is located at m_matrix[1][0]. (2) According to the handy graphic at http://en.wikipedia.org/wiki/Matrix_%28mathematics%29 and its caption, a matrix descriptor a(2,1) represents the element in the second row, first column of the matrix. (3) Wolfram Alpha, http://en.wikipedia.org/wiki/Translation_%28geometry%29 as well as your favourite math textbook state that translation elements are on the right edge of a 4x4 matrix, starting with x from the top. That means x translation is at a(1,4), y is at a(2,4) and z at a(3,4). (4) For the x and y translation elements, aliased as methods e() and f(), WebCore::TransformationMatrix returns m_matrix[3][0] and m_matrix[3][1]. In a row-major matrix, these values would correspond to a(4,1) and a(4,2). However according to (3) this is not the case, and therefore WebCore::TransformationMatrix is column-major. (i.e. you're right) (5) WebCore::TransformationMatrix::m14() returns m_matrix[0][3], which means WebCore's matrix accessor methods are an outright misnomer, or confusing at best. (6) The BlackBerry::Platform::TransformationMatrix constructor assigns its arguments in the same order and with the same names as WebCore::TransformationMatrix, but then assigns them to the mathematically correct position instead. As determined above, mathematically correct means codewise incorrect. Consequently, I'll go off to change BlackBerry::Platform instead and will hopefully figure out what to do about the mIJ() accessor naming. It seems this patch won't be necessary, then. Let me confirm and close later if this is the case. 891995 5 anilsson 2013-05-21 13:54:45 -0700 I'm probably guilty of the BB::P::TransformationMatrix constructor being wrong - because of all the reinterpret_cast going on, it was probably dead code until now... . 202435 2013-05-21 08:48:42 -0700 2013-05-21 20:07:51 -0700 Patch bug-116542-20130521154719.patch text/plain 2504 jpetsovits U3VidmVyc2lvbiBSZXZpc2lvbjogMTQ5NjI0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2Js YWNrYmVycnkvQ2hhbmdlTG9nIGIvU291cmNlL1dlYktpdC9ibGFja2JlcnJ5L0NoYW5nZUxvZwpp bmRleCA3ZjczYmZjNGY0ZjI2MDMxNWQxZGExYjBiYzJmNmM5YTUwNTQ3YjExLi43ZmY4NjBlZDBm MWU2Mzk3ZmNmZjNiMDk0MmQzNDU2ZjA3NDIwMzA1IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 L2JsYWNrYmVycnkvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQvYmxhY2tiZXJyeS9DaGFu Z2VMb2cKQEAgLTEsMyArMSwyMCBAQAorMjAxMy0wNS0xNyAgSmFrb2IgUGV0c292aXRzICA8anBl dHNvdml0c0BibGFja2JlcnJ5LmNvbT4KKworICAgICAgICBbQmxhY2tCZXJyeV0gRG9uJ3QgdXNl IHJlaW50ZXJwcmV0X2Nhc3QgdG8gY29udmVydCB0byBUcmFuc2Zvcm1hdGlvbk1hdHJpeC4KKyAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTExNjU0MgorICAg ICAgICBJbnRlcm5hbCBQUiAxODk3NzUKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICBUaGlzIGlzIGVzcGVjaWFsbHkgYmFkIHNpbmNlIHRoZSBCbGFja0Jl cnJ5OjpQbGF0Zm9ybSBvbmUKKyAgICAgICAgdXNlcyBjb2x1bW4tbWFqb3Igc3RvcmFnZSBhbmQg dGhlIFdlYkNvcmUgb25lIHN0b3JlcyBpdHMKKyAgICAgICAgdmFsdWVzIGluIHJvdy1tYWpvciBs YXlvdXQuIEl0IGJhc2ljYWxseSBvbmx5IHdvcmtlZAorICAgICAgICBiZWNhdXNlIHNvIGZhciB3 ZSdyZSB1c2luZyBhbiBleHRyZW1lbHkgbGltaXRlZCBzdWJzZXQKKyAgICAgICAgb2YgcG90ZW50 aWFsIHRyYW5zZm9ybWF0aW9uIG1hdHJpY2VzLgorCisgICAgICAgICogQXBpL1dlYlBhZ2VDb21w b3NpdG9yLmNwcDoKKyAgICAgICAgKEJsYWNrQmVycnk6OldlYktpdDo6V2ViUGFnZUNvbXBvc2l0 b3I6OnJlbmRlcik6CisKIDIwMTMtMDUtMjEgIEpha29iIFBldHNvdml0cyAgPGpwZXRzb3ZpdHNA YmxhY2tiZXJyeS5jb20+CiAKICAgICAgICAgW0JsYWNrQmVycnldIE1ha2Ugc2NyZWVuIHVwZGF0 ZXMgZGVwZW5kZW50IG9uIHRoZSBleGlzdGVuY2Ugb2YgYSBkcmF3aW5nIGJ1ZmZlci4KZGlmZiAt LWdpdCBhL1NvdXJjZS9XZWJLaXQvYmxhY2tiZXJyeS9BcGkvV2ViUGFnZUNvbXBvc2l0b3IuY3Bw IGIvU291cmNlL1dlYktpdC9ibGFja2JlcnJ5L0FwaS9XZWJQYWdlQ29tcG9zaXRvci5jcHAKaW5k ZXggMWMyZjk5ZDAxOWY2NjBkMmNhN2JhZGQ0YmY3MzAxODI5ZjgzY2M0NC4uMjMxYjRmYmQ5ZmI4 N2QzMGMyNzhjNTE2MjcwZGUwOWE3ZDhjMzc3ZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9i bGFja2JlcnJ5L0FwaS9XZWJQYWdlQ29tcG9zaXRvci5jcHAKKysrIGIvU291cmNlL1dlYktpdC9i bGFja2JlcnJ5L0FwaS9XZWJQYWdlQ29tcG9zaXRvci5jcHAKQEAgLTM0Nyw4ICszNDcsMTQgQEAg dm9pZCBXZWJQYWdlQ29tcG9zaXRvcjo6cHJlcGFyZUZyYW1lKFBsYXRmb3JtOjpHcmFwaGljczo6 R0xFUzJDb250ZXh0KiBjb250ZXh0LAogCiB2b2lkIFdlYlBhZ2VDb21wb3NpdG9yOjpyZW5kZXIo UGxhdGZvcm06OkdyYXBoaWNzOjpHTEVTMkNvbnRleHQqIGNvbnRleHQsIGNvbnN0IFBsYXRmb3Jt OjpJbnRSZWN0JiB0YXJnZXRSZWN0LCBjb25zdCBQbGF0Zm9ybTo6SW50UmVjdCYgY2xpcFJlY3Qs IGNvbnN0IFBsYXRmb3JtOjpUcmFuc2Zvcm1hdGlvbk1hdHJpeCYgdHJhbnNmb3JtLCBjb25zdCBQ bGF0Zm9ybTo6RmxvYXRSZWN0JiBkb2N1bWVudENvbnRlbnRzLCBjb25zdCBQbGF0Zm9ybTo6Rmxv YXRSZWN0JiB2aWV3cG9ydCkKIHsKKyAgICBUcmFuc2Zvcm1hdGlvbk1hdHJpeCB0ZigKKyAgICAg ICAgdHJhbnNmb3JtLm0xMSgpLCB0cmFuc2Zvcm0ubTEyKCksIHRyYW5zZm9ybS5tMTMoKSwgdHJh bnNmb3JtLm0xNCgpLAorICAgICAgICB0cmFuc2Zvcm0ubTIxKCksIHRyYW5zZm9ybS5tMjIoKSwg dHJhbnNmb3JtLm0yMygpLCB0cmFuc2Zvcm0ubTI0KCksCisgICAgICAgIHRyYW5zZm9ybS5tMzEo KSwgdHJhbnNmb3JtLm0zMigpLCB0cmFuc2Zvcm0ubTMzKCksIHRyYW5zZm9ybS5tMzQoKSwKKyAg ICAgICAgdHJhbnNmb3JtLm00MSgpLCB0cmFuc2Zvcm0ubTQyKCksIHRyYW5zZm9ybS5tNDMoKSwg dHJhbnNmb3JtLm00NCgpKTsKKwogICAgIGQtPnNldENvbnRleHQoY29udGV4dCk7Ci0gICAgZC0+ cmVuZGVyKHRhcmdldFJlY3QsIGNsaXBSZWN0LCBUcmFuc2Zvcm1hdGlvbk1hdHJpeChyZWludGVy cHJldF9jYXN0PGNvbnN0IFRyYW5zZm9ybWF0aW9uTWF0cml4Jj4odHJhbnNmb3JtKSksIGRvY3Vt ZW50Q29udGVudHMsIHZpZXdwb3J0KTsKKyAgICBkLT5yZW5kZXIodGFyZ2V0UmVjdCwgY2xpcFJl Y3QsIHRmLCBkb2N1bWVudENvbnRlbnRzLCB2aWV3cG9ydCk7CiB9CiAKIHZvaWQgV2ViUGFnZUNv bXBvc2l0b3I6OmNsZWFudXAoUGxhdGZvcm06OkdyYXBoaWNzOjpHTEVTMkNvbnRleHQqKQo=