Large number constant in TransformationMatrix::projectPoint overflows FractionalLayoutUnits with sub-pixel layout enabled

• Component: New Bugs
• Status: RESOLVED FIXED

[WebKit Review Bot]

ERROR: !(isInBounds(value)) in FractionalLayoutUnit.h on ducks.nhl.com with fractional layout ratio of 60:1
Requested by jamesr_ on #webkit.

[Levi Weintraub]

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;

[Shawn Singh]

(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.

[Levi Weintraub]

Created attachment 145389 [details]
Patch

[James Robinson]

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

[James Robinson]

a "style not" is halfway between a note and a nit.  Or maybe it's just a typo.  We'll never know the truth...

[Shawn Singh]

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?

[Levi Weintraub]

Because 100000000 is too big for FractionalLayoutUnit's tiny mind.

[Shawn Singh]

(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.

[Levi Weintraub]

(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 ;)

[Levi Weintraub]

Created attachment 145395 [details]
Patch for landing

[Shawn Singh]

(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.

[WebKit Review Bot]

Comment on attachment 145395 [details]
Patch for landing

Clearing flags on attachment: 145395

Committed r119310: <http://trac.webkit.org/changeset/119310>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.