Bug 151575 – Use Optional for matrix inverses

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Use Optional for matrix inverses

[Alex Christensen]

Created attachment 266103 [details]
Patch

[Build Bot]

Comment on attachment 266103 [details]
Patch

Attachment 266103 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/469367

New failing tests:
fast/canvas/pointInPath.html

[Build Bot]

Created attachment 266104 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 266103 [details]
Patch

Attachment 266103 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/469377

New failing tests:
fast/canvas/pointInPath.html

[Build Bot]

Created attachment 266105 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 266103 [details]
Patch

Attachment 266103 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/469375

New failing tests:
svg/dom/SVGMatrix-interface.xhtml
fast/canvas/pointInPath.html

[Build Bot]

Created attachment 266106 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Alex Christensen]

Created attachment 266109 [details]
Patch

[Alex Christensen]

Created attachment 266110 [details]
Patch

[Alex Christensen]

Created attachment 266112 [details]
Patch

[Alex Christensen]

Created attachment 266113 [details]
Patch

[Myles C. Maxfield]

Looks like you forgot to update CoordinatedGraphicsLayer.cpp

[Build Bot]

Comment on attachment 266113 [details]
Patch

Attachment 266113 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/470197

New failing tests:
svg/dom/SVGMatrix-interface.xhtml

[Build Bot]

Created attachment 266115 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Alex Christensen]

Created attachment 266118 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 266113 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266113&action=review

r+ assuming you fix mac-debug and efl.

> Source/WebCore/css/WebKitCSSMatrix.cpp:104
> +    Optional<TransformationMatrix> inverse = m_matrix.inverse();

if (auto inverse = m_matrix.inverse()) return WebKitCSSMatrix::create(inverse.value());

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:349
> +    if (Optional<AffineTransform> inverse = state().transform.inverse())

We aren't sure that this matrix is invertible. Maybe remembering the old matrices is better?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:687
> +    if (!sx || !sy) {

I assume this means we maintain the invariant that all stored matrices are invertible? (I hope we ASSERT that)

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:761
> +    Optional<AffineTransform> inverse = newTransform.inverse();

Probably should flip this around like above.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1104
> +    FloatPoint transformedPoint = state().transform.inverse().valueOr(AffineTransform()).mapPoint(FloatPoint(x, y));

If the transform is not invertible, paths have 0 area, so I would intuitively guess that no points are "in" any paths. Perhaps we can simply return false if the matrix isn't invertible. Or, if we maintain the invariant like I mention above, we can ASSERT.

What does the spec say?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1124
> +    FloatPoint transformedPoint = state().transform.inverse().valueOr(AffineTransform()).mapPoint(FloatPoint(x, y));

Ditto.

> Source/WebCore/platform/graphics/GraphicsContext.cpp:735
> +            origin = inverse.value().mapPoint(deviceOrigin);

else, origin should be set to "point". (You might as well initialize it to that and remove line 712)

> Source/WebCore/platform/graphics/ShadowBlur.cpp:402
> +        layerRect = transform.inverse().valueOr(AffineTransform()).mapQuad(transformedPolygon).boundingBox();

Same deal.

We have a few places where we do "transform -> op -> transform back". Maybe we should encapsulate that somehow?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2374
> +    FloatRect transformedOutputRect = videoTransform.inverse().valueOr(AffineTransform()).mapRect(outputRect);

outputRect is already in screen space? Yuck. Isn't there a better way to do this? (Perhaps it is outside the scope of this patch.)

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:327
> +    if (coordinateSystem == LogicalCoordinateSystem) {
> +        if (auto inverse = AffineTransform(getUserToBaseCTM(destContext)).inverse())
> +            CGContextConcatCTM(destContext, inverse.value());
> +    } else {
> +        if (auto inverse = AffineTransform(CGContextGetCTM(destContext)).inverse())
> +            CGContextConcatCTM(destContext, inverse.value());

Why didn't you use valueOr() here like above? It would be cleaner. Perhaps this way is faster? (But, then again, I imagine the cost of a matrix multiply is trivial compared to the cost of the rest of this function)

> Source/WebCore/platform/graphics/transforms/AffineTransform.cpp:104
>      return std::isfinite(determinant) && determinant != 0;

I thought you didn't like this? Also, floating point equality.

> Source/WebCore/platform/graphics/transforms/AffineTransform.cpp:107
> +Optional<AffineTransform> AffineTransform::inverse() const

\o/

> Source/WebCore/platform/graphics/transforms/AffineTransform.h:121
> +    bool isInvertible() const; // FIXME: Every use of this is probably not necessary.

Confused by your use of "every." Perhaps you meant something like "if you call this this, you're probably doing it wrong"

> Source/WebCore/platform/graphics/transforms/TransformState.cpp:173
> +    return m_accumulatedTransform->inverse().valueOr(TransformationMatrix()).projectPoint(point, wasClamped);

It's a shame that we have all these valueOr's around. I wish we had the time to investigate each one of them.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1465
> +    if (!WebCore::inverse(m_matrix, invMat.m_matrix))

Can we change the signature of WebCore::inverse() too? Optionals are great.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:204
>      stateSaver.save();

Unbalanced. Yuck.

> Source/WebCore/svg/SVGLocatable.cpp:106
> +        Optional<AffineTransform> inverse = targetCTM.inverse();

I think you should flip this around and put the Optional in the conditional.

> Source/WebCore/svg/SVGMatrix.h:109
> +        Optional<AffineTransform> transform = AffineTransform::inverse();

Ditto.

> Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp:1125
> +    Optional<AffineTransform> inverse = m_pluginToRootViewTransform.inverse();

Ditto.

> Source/WebKit2/WebProcess/Plugins/Netscape/mac/NetscapePluginMac.mm:150
> +    if (!inverse)

Ditto.

[Build Bot]

Comment on attachment 266118 [details]
Patch

Attachment 266118 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/470479

New failing tests:
js/property-name-enumerator-gc-151495.html

[Build Bot]

Created attachment 266125 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Alex Christensen]

Created attachment 266126 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 266126 [details]
Patch

r+ with all my previous comments.

[Alex Christensen]

Comment on attachment 266113 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266113&action=review

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:349
>> +    if (Optional<AffineTransform> inverse = state().transform.inverse())
> 
> We aren't sure that this matrix is invertible. Maybe remembering the old matrices is better?

I'm not sure what you mean by this comment.  This change doesn't change any behavior.  Do we want to change behavior in this patch?

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:687
>> +    if (!sx || !sy) {
> 
> I assume this means we maintain the invariant that all stored matrices are invertible? (I hope we ASSERT that)

The only way a scale operation could make a non-singular matrix singular is if sx or sy were 0 (or infinite, which is covered above).  This preserves behavior without calculating the determinant.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1103
> +    // FIXME: hasInvertibleTransform

I meant to say, hasInvertibleTransform can probably be removed.

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1104
>> +    FloatPoint transformedPoint = state().transform.inverse().valueOr(AffineTransform()).mapPoint(FloatPoint(x, y));
> 
> If the transform is not invertible, paths have 0 area, so I would intuitively guess that no points are "in" any paths. Perhaps we can simply return false if the matrix isn't invertible. Or, if we maintain the invariant like I mention above, we can ASSERT.
> 
> What does the spec say?

We can't do this.  "Points on the path itself must be considered to be inside the path." http://www.w3.org/TR/2dcontext/#dom-context-2d-ispointinpath

>> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:327
>> +            CGContextConcatCTM(destContext, inverse.value());
> 
> Why didn't you use valueOr() here like above? It would be cleaner. Perhaps this way is faster? (But, then again, I imagine the cost of a matrix multiply is trivial compared to the cost of the rest of this function)

I thought it was unnecessary to multiply by the identity.

>> Source/WebCore/platform/graphics/transforms/AffineTransform.cpp:104
>>      return std::isfinite(determinant) && determinant != 0;
> 
> I thought you didn't like this? Also, floating point equality.

I don't, but inverting an affine transformation isn't numerically unstable.  Only when you get larger than a 2x2 determinant is it unstable.

>> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1465
>> +    if (!WebCore::inverse(m_matrix, invMat.m_matrix))
> 
> Can we change the signature of WebCore::inverse() too? Optionals are great.

I'm going to completely get rid of that implementation in another patch.

>> Source/WebCore/rendering/svg/RenderSVGShape.cpp:204
>>      stateSaver.save();
> 
> Unbalanced. Yuck.

I think this is fine.  That's how GraphicsContextStateSavers work.  They save the state, then restore it when they go out of scope if saveAndRestore is true.

[Alex Christensen]

Created attachment 266296 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 266296 [details]
Patch

Clearing flags on attachment: 266296

Committed r192900: <http://trac.webkit.org/changeset/192900>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.