Bug 87896 - Large number constant in TransformationMatrix::projectPoint overflows FractionalLayoutUnits with sub-pixel layout enabled
Summary: Large number constant in TransformationMatrix::projectPoint overflows Fractio...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: WebKit Review Bot
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-30 13:39 PDT by WebKit Review Bot
Modified: 2012-06-01 20:18 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.12 KB, patch)
2012-06-01 15:26 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch for landing (2.26 KB, patch)
2012-06-01 15:59 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description WebKit Review Bot 2012-05-30 13:39:37 PDT
ERROR: !(isInBounds(value)) in FractionalLayoutUnit.h on ducks.nhl.com with fractional layout ratio of 60:1
Requested by jamesr_ on #webkit.
Comment 1 Levi Weintraub 2012-05-30 14:05:57 PDT
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;
Comment 2 Shawn Singh 2012-05-30 14:46:57 PDT
(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.
Comment 3 Levi Weintraub 2012-06-01 15:26:22 PDT
Created attachment 145389 [details]
Patch
Comment 4 James Robinson 2012-06-01 15:52:26 PDT
Comment on attachment 145389 [details]
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
Comment 5 James Robinson 2012-06-01 15:53:58 PDT
a "style not" is halfway between a note and a nit.  Or maybe it's just a typo.  We'll never know the truth...
Comment 6 Shawn Singh 2012-06-01 15:55:33 PDT
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?
Comment 7 Levi Weintraub 2012-06-01 15:56:48 PDT
Because 100000000 is too big for FractionalLayoutUnit's tiny mind.
Comment 8 Shawn Singh 2012-06-01 15:58:45 PDT
(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.
Comment 9 Levi Weintraub 2012-06-01 15:58:57 PDT
(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 ;)
Comment 10 Levi Weintraub 2012-06-01 15:59:57 PDT
Created attachment 145395 [details]
Patch for landing
Comment 11 Shawn Singh 2012-06-01 16:04:40 PDT
(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.
Comment 12 WebKit Review Bot 2012-06-01 20:18:13 PDT
Comment on attachment 145395 [details]
Patch for landing

Clearing flags on attachment: 145395

Committed r119310: <http://trac.webkit.org/changeset/119310>
Comment 13 WebKit Review Bot 2012-06-01 20:18:18 PDT
All reviewed patches have been landed.  Closing bug.