Bug 90230 – [chromium] Remove mapRect and mapQuad from WebTransformationMatrix

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

[James Robinson]

[chromium] Remove mapRect and mapQuad from WebTransformationMatrix

[James Robinson]

Created attachment 150050 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

Shawn - would you mind checking my math here?  I wrote a tiny bit of new code in CCOverdrawMetrics, the rest of the time I'm just assuming that we do not expect to clip.  The polygon handling in CCOverdrawMetrics may be overkill for what it's trying to do.

[James Robinson]

Created attachment 150064 [details]
Patch

[James Robinson]

Second patch uses CCMathUtil::projectPoint inside of projectQuad.  That just leaves the mapPoint functions behind.

[Dana Jansens]

Comment on attachment 150064 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:99
> -        IntRect layerRect = transformToLayerSpace.mapRect(fillRects[i]);
> +        IntRect layerRect = CCMathUtil::mapClippedRect(transformToLayerSpace, fillRects[i]);

This is using an inverse transform, so shouldn't it actually be project instead of map?

[Shawn Singh]

Comment on attachment 150064 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:666
> +    FloatQuad surfaceQuad = CCMathUtil::mapQuad(contentsDeviceTransform.inverse(), deviceLayerEdges.floatQuad(), clipped);

Can we please add a comment here that says "to2dTransform removed the perspective component of the transform, so projectQuad is unnecessary here."

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:154
> +    // Clipping has been verified above, so mapClippedRect will give correct results.

I think this comment can either be removed or be reworded to say "Clipping was already performed by the transform above, using mapClippedRect here is not expected to clip anything further here."

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:322
> +    // We verify that the possible bounds of this region are not clipped above, so we can use mapClippedRect() safely here.

And similarly for this comment

> Source/WebCore/platform/graphics/chromium/cc/CCOverdrawMetrics.cpp:64
> +    if (numPoints < 2)

if numPoints == 2 then this function would compute an area of 0, so we should be able to say  numPoints < 3 here, if you want.

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:99
>> +        IntRect layerRect = CCMathUtil::mapClippedRect(transformToLayerSpace, fillRects[i]);
> 
> This is using an inverse transform, so shouldn't it actually be project instead of map?

I think Dana is right. If projectQuad causes any regressions versus mapRect, I would really love to see that scenario and analyze it...  Otherwise I think the general rule is that inverse() usually means project rather than map.

[Shawn Singh]

Yeah, the math seems fine to me.   I think the most likely thing that might go wrong with this patch is if clipped rects break culling -- and if that happens, it's not really a fault of this patch, instead its more likely that another bug gets revealed that should be fixed anyway.

[James Robinson]

Comment on attachment 150064 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:666
>> +    FloatQuad surfaceQuad = CCMathUtil::mapQuad(contentsDeviceTransform.inverse(), deviceLayerEdges.floatQuad(), clipped);
> 
> Can we please add a comment here that says "to2dTransform removed the perspective component of the transform, so projectQuad is unnecessary here."

done

>> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:154
>> +    // Clipping has been verified above, so mapClippedRect will give correct results.
> 
> I think this comment can either be removed or be reworded to say "Clipping was already performed by the transform above, using mapClippedRect here is not expected to clip anything further here."

done

>> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:322
>> +    // We verify that the possible bounds of this region are not clipped above, so we can use mapClippedRect() safely here.
> 
> And similarly for this comment

done

>> Source/WebCore/platform/graphics/chromium/cc/CCOverdrawMetrics.cpp:64
>> +    if (numPoints < 2)
> 
> if numPoints == 2 then this function would compute an area of 0, so we should be able to say  numPoints < 3 here, if you want.

Good point!

>>> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:99
>>> +        IntRect layerRect = CCMathUtil::mapClippedRect(transformToLayerSpace, fillRects[i]);
>> 
>> This is using an inverse transform, so shouldn't it actually be project instead of map?
> 
> I think Dana is right. If projectQuad causes any regressions versus mapRect, I would really love to see that scenario and analyze it...  Otherwise I think the general rule is that inverse() usually means project rather than map.

This function is used to fill out background color quads for solid-color checkerboarding.  It's only used on the root layer, so there should never be any perspective here (only translation/scales) and it shouldn't make a difference, if I understand correctly.  Doing a project seems like overkill.

Should I instead ASSERT() or comment the expectations here?

[James Robinson]

Created attachment 150224 [details]
Patch

[James Robinson]

Updated - I'd like a bit more guidance on how to handle the appendQuadsToFillScreen().

[Dana Jansens]

Comment on attachment 150064 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:99
>>>> +        IntRect layerRect = CCMathUtil::mapClippedRect(transformToLayerSpace, fillRects[i]);
>>> 
>>> This is using an inverse transform, so shouldn't it actually be project instead of map?
>> 
>> I think Dana is right. If projectQuad causes any regressions versus mapRect, I would really love to see that scenario and analyze it...  Otherwise I think the general rule is that inverse() usually means project rather than map.
> 
> This function is used to fill out background color quads for solid-color checkerboarding.  It's only used on the root layer, so there should never be any perspective here (only translation/scales) and it shouldn't make a difference, if I understand correctly.  Doing a project seems like overkill.
> 
> Should I instead ASSERT() or comment the expectations here?

Oh, ya good observation. ASSERT() would be nice but I don't think we have any nice helpers to check for this, comment will be fine IMO.

[Shawn Singh]

> 
> This function is used to fill out background color quads for solid-color checkerboarding.  It's only used on the root layer, so there should never be any perspective here (only translation/scales) and it shouldn't make a difference, if I understand correctly.  Doing a project seems like overkill.
> 
> Should I instead ASSERT() or comment the expectations here?

Yeah, I think its OK to avoid projectQuad based on your point.  Comment about this seems appropriate and necessary.

we could add ASSERT(!rootLayer->parent()) or something similar that makes sure that rootLayer really is the root layer.

[James Robinson]

Created attachment 150234 [details]
add comment in CCRenderPass::appendQuadsToFillScreen

[Adrienne Walker]

Comment on attachment 150234 [details]
add comment in CCRenderPass::appendQuadsToFillScreen

LGTM.  R=me.

[James Robinson]

Committed r121583: <http://trac.webkit.org/changeset/121583>