87896 2012-05-30 13:39:37 -0700 Large number constant in TransformationMatrix::projectPoint overflows FractionalLayoutUnits with sub-pixel layout enabled 2012-06-01 20:18:18 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 webkit.review.bot webkit.review.bot eae jamesr leviw shawnsingh simon.fraser oldest_to_newest 637643 0 webkit.review.bot 2012-05-30 13:39:37 -0700 ERROR: !(isInBounds(value)) in FractionalLayoutUnit.h on ducks.nhl.com with fractional layout ratio of 60:1 Requested by jamesr_ on #webkit. 637677 1 leviw 2012-05-30 14:05:57 -0700 Updating title to reflect that many of these overflows are caused by the following lines: // Using int max causes overflow when other code uses the projected point. To // represent infinity yet reduce the risk of overflow, we use a large but // not-too-large number here when clamping. const int kLargeNumber = 10000000; 637714 2 shawnsingh 2012-05-30 14:46:57 -0700 (In reply to comment #1) > Updating title to reflect that many of these overflows are caused by the following lines: > > // Using int max causes overflow when other code uses the projected point. To > // represent infinity yet reduce the risk of overflow, we use a large but > // not-too-large number here when clamping. > const int kLargeNumber = 10000000; An explanation is needed here. This code is not the cause of overflow. If anything, it actually reduced the occurrences of overflow... before I had created kLargeNumber, the code was using int_max, and that was also overflowing, even more quickly. The actual cause of overflow is the way w < 0 special case is handled in TransformationMatrix::projectPoint(), code which I did not change. In that code, projectPoint is clamping to "infinity" when w < 0. The most correct fix, however, should not be clamping these values when w <0. Instead, it really needs to perform clipping in homogeneous coordinates. An example of how clipping is implemented for chromium is in platform/graphics/chromium/cc/CCMathUtil.* -- which I had written exactly because of this problem for our own rendering code. Please feel free to adapt that code into WebCore TransformationMatrix. However, please be aware that for hit testing, this may very likely require supporting hit testing for arbitrary polygons of up to 8 vertices. Unfortunately, adding proper clipping to usage of transforms in WebCore is a very nontrivial task that will take one person several weeks, maybe even months. Unfortunately I won't have time to help with that, but I'm willing to discuss and answer questions, and the CCMathUtil code should be a helpful start for anyone who does take on this task. Another alternate possibility is to require various code to be aware of the concept of "infinity" rather than directly using INT_MAX. I'm not sure what this would entail either, but I have encountered similar issues related to "infiniteRects" that were being operated on, thus causing overflow as well. 639786 3 145389 leviw 2012-06-01 15:26:22 -0700 Created attachment 145389 Patch 639805 4 145389 jamesr 2012-06-01 15:52:26 -0700 Comment on attachment 145389 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145389&action=review > Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:579 > + const int kLargeNumber = 100000000 / kFixedPointDenominator; style not: WebKit style for constants is to name them the same way all other variables are named, not kBlahBlah 639806 5 jamesr 2012-06-01 15:53:58 -0700 a "style not" is halfway between a note and a nit. Or maybe it's just a typo. We'll never know the truth... 639811 6 shawnsingh 2012-06-01 15:55:33 -0700 If it works, its OK with me. But... one little issue is gnawing at my mind about this. In theory, kLargeNumber is supposed to represent infinity, and that's the only place its used in code where its supposed to represent infinity. So, why does behavior get fixed when dividing by 60 to make it smaller? 639813 7 leviw 2012-06-01 15:56:48 -0700 Because 100000000 is too big for FractionalLayoutUnit's tiny mind. 639815 8 shawnsingh 2012-06-01 15:58:45 -0700 (In reply to comment #7) > Because 100000000 is too big for FractionalLayoutUnit's tiny mind. Got it. so it overflows sooner beacuse of the more limited range. anyway, if reviewrs are OK with it, its good for me. 639816 9 leviw 2012-06-01 15:58:57 -0700 (In reply to comment #7) > Because 100000000 is too big for FractionalLayoutUnit's tiny mind. Or more specifically, we'll later represent the projected point using LayoutUnits instead of integers. I'm more curious why we don't do clamping instead of having an arbitrary constant to begin with, but in the meantime I'd like to fix hit testing with 3d transforms ;) 639817 10 145395 leviw 2012-06-01 15:59:57 -0700 Created attachment 145395 Patch for landing 639820 11 shawnsingh 2012-06-01 16:04:40 -0700 (In reply to comment #9) > (In reply to comment #7) > > Because 100000000 is too big for FractionalLayoutUnit's tiny mind. > > Or more specifically, we'll later represent the projected point using LayoutUnits instead of integers. I'm more curious why we don't do clamping instead of having an arbitrary constant to begin with, but in the meantime I'd like to fix hit testing with 3d transforms ;) we don't do clamping to infinity because it causes overflow even more quickly than kLargeNumber. It wouldn't be correct to "clamp" in any other way, the correct solution is to "clipping" in homogeneous coordinates, before divide-by-w. we don't do "clipping" in homogeneous coordinates, before divide-by-w because of the situation described in Comment #2. It will take a gargantuan effort to support that in WebCore at this time. hope that answers the question? We could always just discuss it offline later, too. 639936 12 145395 webkit.review.bot 2012-06-01 20:18:13 -0700 Comment on attachment 145395 Patch for landing Clearing flags on attachment: 145395 Committed r119310: <http://trac.webkit.org/changeset/119310> 639937 13 webkit.review.bot 2012-06-01 20:18:18 -0700 All reviewed patches have been landed. Closing bug. 145389 2012-06-01 15:26:22 -0700 2012-06-01 15:59:53 -0700 Patch bug-87896-20120601152621.patch text/plain 2166 leviw SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDExOTI4OCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIyIEBACisyMDEyLTA2LTAxICBMZXZpIFdl aW50cmF1YiAgPGxldml3QGNocm9taXVtLm9yZz4KKworICAgICAgICBMYXJnZSBudW1iZXIgY29u c3RhbnQgaW4gVHJhbnNmb3JtYXRpb25NYXRyaXg6OnByb2plY3RQb2ludCBvdmVyZmxvd3MgRnJh Y3Rpb25hbExheW91dFVuaXRzIHdpdGggc3ViLXBpeGVsIGxheW91dCBlbmFibGVkCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD04Nzg5NgorCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIGtMYXJnZU51bWJlciBpcyBt ZWFudCB0byBhdm9pZCBvdmVyZmxvd2luZyB3aGVuIHByb2plY3RpbmcgYSBwb2ludCB0aHJvdWdo IGEgdHJhbnNmb3JtLiBVbmZvcnR1bmF0ZWx5LAorICAgICAgICBkdWUgdG8gRnJhY3Rpb25hbExh eW91dFVuaXQncyBkaW1pbmlzaGVkIHJhbmdlIGNvbXBhcmVkIHRvIGludGVnZXJzLCB3ZSB3ZXJl IG92ZXJmbG93aW5nIGFueXdheXMuIFRoaXMKKyAgICAgICAgY2hhbmdlIGFkanVzdHMgb3VyIGxh cmdlIG51bWJlciBieSB0aGUgc2FtZSB2YWx1ZSwgYnV0IGFkanVzdGVkIGZvciBvdXIgZGVub21p bmF0b3IuCisKKyAgICAgICAgVGhpcyB3YXMgb3JpZ2luYWxseSBtaXNzZWQgYmVjYXVzZSB0cmFu c2Zvcm1zLzNkIGlzIG1hcmtlZCBhcyBwYXNzL2ZhaWwgaW4gQ2hyb21pdW0ncyB0ZXN0X2V4cGVj dGF0aW9ucy50eHQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTY2OTg5IHRyYWNrcyByZW1vdmluZyB0aGlzIHByb2JsZW0uCisKKyAgICAgICAgTm8gbmV3 IHRlc3RzLiBDb3ZlcmVkIGJ5IHRyYW5zZm9ybXMvM2QvaGl0LXRlc3RpbmcvcGVyc3BlY3RpdmUt Y2xpcHBlZC5odG1sCisKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy90cmFuc2Zvcm1zL1Ry YW5zZm9ybWF0aW9uTWF0cml4LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlRyYW5zZm9ybWF0aW9u TWF0cml4Ojpwcm9qZWN0UG9pbnQpOgorCiAyMDEyLTA2LTAxICBKYW1lcyBSb2JpbnNvbiAgPGph bWVzckBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgW2Nocm9taXVtXSBVbnJldmlld2VkIGNvbXBp bGUgZml4IGZvciByMTE5MjgzCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGlj cy90cmFuc2Zvcm1zL1RyYW5zZm9ybWF0aW9uTWF0cml4LmNwcAo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3Vy Y2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy90cmFuc2Zvcm1zL1RyYW5zZm9ybWF0aW9uTWF0 cml4LmNwcAkocmV2aXNpb24gMTE5MjQ4KQorKysgU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3Jh cGhpY3MvdHJhbnNmb3Jtcy9UcmFuc2Zvcm1hdGlvbk1hdHJpeC5jcHAJKHdvcmtpbmcgY29weSkK QEAgLTU3Niw3ICs1NzYsNyBAQCBGbG9hdFBvaW50IFRyYW5zZm9ybWF0aW9uTWF0cml4Ojpwcm9q ZWN0CiAgICAgICAgIC8vIFVzaW5nIGludCBtYXggY2F1c2VzIG92ZXJmbG93IHdoZW4gb3RoZXIg Y29kZSB1c2VzIHRoZSBwcm9qZWN0ZWQgcG9pbnQuIFRvCiAgICAgICAgIC8vIHJlcHJlc2VudCBp bmZpbml0eSB5ZXQgcmVkdWNlIHRoZSByaXNrIG9mIG92ZXJmbG93LCB3ZSB1c2UgYSBsYXJnZSBi dXQKICAgICAgICAgLy8gbm90LXRvby1sYXJnZSBudW1iZXIgaGVyZSB3aGVuIGNsYW1waW5nLgot ICAgICAgICBjb25zdCBpbnQga0xhcmdlTnVtYmVyID0gMTAwMDAwMDAwOworICAgICAgICBjb25z dCBpbnQga0xhcmdlTnVtYmVyID0gMTAwMDAwMDAwIC8ga0ZpeGVkUG9pbnREZW5vbWluYXRvcjsK ICAgICAgICAgb3V0WCA9IGNvcHlzaWduKGtMYXJnZU51bWJlciwgb3V0WCk7CiAgICAgICAgIG91 dFkgPSBjb3B5c2lnbihrTGFyZ2VOdW1iZXIsIG91dFkpOwogICAgICAgICBpZiAoY2xhbXBlZCkK 145395 2012-06-01 15:59:57 -0700 2012-06-01 20:18:13 -0700 Patch for landing bug-87896-20120601155956.patch text/plain 2311 leviw SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDExOTI4OCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIyIEBACisyMDEyLTA2LTAxICBMZXZpIFdl aW50cmF1YiAgPGxldml3QGNocm9taXVtLm9yZz4KKworICAgICAgICBMYXJnZSBudW1iZXIgY29u c3RhbnQgaW4gVHJhbnNmb3JtYXRpb25NYXRyaXg6OnByb2plY3RQb2ludCBvdmVyZmxvd3MgRnJh Y3Rpb25hbExheW91dFVuaXRzIHdpdGggc3ViLXBpeGVsIGxheW91dCBlbmFibGVkCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD04Nzg5NgorCisgICAgICAg IFJldmlld2VkIGJ5IEphbWVzIFJvYmluc29uLgorCisgICAgICAgIGtMYXJnZU51bWJlciBpcyBt ZWFudCB0byBhdm9pZCBvdmVyZmxvd2luZyB3aGVuIHByb2plY3RpbmcgYSBwb2ludCB0aHJvdWdo IGEgdHJhbnNmb3JtLiBVbmZvcnR1bmF0ZWx5LAorICAgICAgICBkdWUgdG8gRnJhY3Rpb25hbExh eW91dFVuaXQncyBkaW1pbmlzaGVkIHJhbmdlIGNvbXBhcmVkIHRvIGludGVnZXJzLCB3ZSB3ZXJl IG92ZXJmbG93aW5nIGFueXdheXMuIFRoaXMKKyAgICAgICAgY2hhbmdlIGFkanVzdHMgb3VyIGxh cmdlIG51bWJlciBieSB0aGUgc2FtZSB2YWx1ZSwgYnV0IGFkanVzdGVkIGZvciBvdXIgZGVub21p bmF0b3IuCisKKyAgICAgICAgVGhpcyB3YXMgb3JpZ2luYWxseSBtaXNzZWQgYmVjYXVzZSB0cmFu c2Zvcm1zLzNkIGlzIG1hcmtlZCBhcyBwYXNzL2ZhaWwgaW4gQ2hyb21pdW0ncyB0ZXN0X2V4cGVj dGF0aW9ucy50eHQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTY2OTg5IHRyYWNrcyByZW1vdmluZyB0aGlzIHByb2JsZW0uCisKKyAgICAgICAgTm8gbmV3 IHRlc3RzLiBDb3ZlcmVkIGJ5IHRyYW5zZm9ybXMvM2QvaGl0LXRlc3RpbmcvcGVyc3BlY3RpdmUt Y2xpcHBlZC5odG1sCisKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy90cmFuc2Zvcm1zL1Ry YW5zZm9ybWF0aW9uTWF0cml4LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlRyYW5zZm9ybWF0aW9u TWF0cml4Ojpwcm9qZWN0UG9pbnQpOgorCiAyMDEyLTA2LTAxICBKYW1lcyBSb2JpbnNvbiAgPGph bWVzckBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgW2Nocm9taXVtXSBVbnJldmlld2VkIGNvbXBp bGUgZml4IGZvciByMTE5MjgzCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGlj cy90cmFuc2Zvcm1zL1RyYW5zZm9ybWF0aW9uTWF0cml4LmNwcAo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3Vy Y2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy90cmFuc2Zvcm1zL1RyYW5zZm9ybWF0aW9uTWF0 cml4LmNwcAkocmV2aXNpb24gMTE5MjQ4KQorKysgU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3Jh cGhpY3MvdHJhbnNmb3Jtcy9UcmFuc2Zvcm1hdGlvbk1hdHJpeC5jcHAJKHdvcmtpbmcgY29weSkK QEAgLTU3Niw5ICs1NzYsOSBAQCBGbG9hdFBvaW50IFRyYW5zZm9ybWF0aW9uTWF0cml4Ojpwcm9q ZWN0CiAgICAgICAgIC8vIFVzaW5nIGludCBtYXggY2F1c2VzIG92ZXJmbG93IHdoZW4gb3RoZXIg Y29kZSB1c2VzIHRoZSBwcm9qZWN0ZWQgcG9pbnQuIFRvCiAgICAgICAgIC8vIHJlcHJlc2VudCBp bmZpbml0eSB5ZXQgcmVkdWNlIHRoZSByaXNrIG9mIG92ZXJmbG93LCB3ZSB1c2UgYSBsYXJnZSBi dXQKICAgICAgICAgLy8gbm90LXRvby1sYXJnZSBudW1iZXIgaGVyZSB3aGVuIGNsYW1waW5nLgot ICAgICAgICBjb25zdCBpbnQga0xhcmdlTnVtYmVyID0gMTAwMDAwMDAwOwotICAgICAgICBvdXRY ID0gY29weXNpZ24oa0xhcmdlTnVtYmVyLCBvdXRYKTsKLSAgICAgICAgb3V0WSA9IGNvcHlzaWdu KGtMYXJnZU51bWJlciwgb3V0WSk7CisgICAgICAgIGNvbnN0IGludCBsYXJnZU51bWJlciA9IDEw MDAwMDAwMCAvIGtGaXhlZFBvaW50RGVub21pbmF0b3I7CisgICAgICAgIG91dFggPSBjb3B5c2ln bihsYXJnZU51bWJlciwgb3V0WCk7CisgICAgICAgIG91dFkgPSBjb3B5c2lnbihsYXJnZU51bWJl ciwgb3V0WSk7CiAgICAgICAgIGlmIChjbGFtcGVkKQogICAgICAgICAgICAgKmNsYW1wZWQgPSB0 cnVlOwogICAgIH0gZWxzZSBpZiAodyAhPSAxKSB7Cg==