Bug 90230

Summary: [chromium] Remove mapRect and mapQuad from WebTransformationMatrix
Product: WebKit Reporter: James Robinson <jamesr>
Component: New BugsAssignee: James Robinson <jamesr>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cc-bugs, danakj, dglazkov, enne, fishd, shawnsingh, tkent+wkapi, webkit.review.bot, wjmaclean
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
add comment in CCRenderPass::appendQuadsToFillScreen enne: review+

Description James Robinson 2012-06-28 17:41:26 PDT
[chromium] Remove mapRect and mapQuad from WebTransformationMatrix
Comment 1 James Robinson 2012-06-28 17:43:50 PDT
Created attachment 150050 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-28 17:45:29 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 James Robinson 2012-06-28 17:51:27 PDT
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.
Comment 4 James Robinson 2012-06-28 19:00:42 PDT
Created attachment 150064 [details]
Patch
Comment 5 James Robinson 2012-06-28 19:01:26 PDT
Second patch uses CCMathUtil::projectPoint inside of projectQuad.  That just leaves the mapPoint functions behind.
Comment 6 Dana Jansens 2012-06-29 08:41:57 PDT
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?
Comment 7 Shawn Singh 2012-06-29 11:07:58 PDT
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.
Comment 8 Shawn Singh 2012-06-29 11:09:51 PDT
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.
Comment 9 James Robinson 2012-06-29 11:33:51 PDT
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?
Comment 10 James Robinson 2012-06-29 11:34:24 PDT
Created attachment 150224 [details]
Patch
Comment 11 James Robinson 2012-06-29 11:34:33 PDT
Updated - I'd like a bit more guidance on how to handle the appendQuadsToFillScreen().
Comment 12 Dana Jansens 2012-06-29 11:40:16 PDT
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.
Comment 13 Shawn Singh 2012-06-29 11:54:14 PDT
> 
> 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.
Comment 14 James Robinson 2012-06-29 12:25:54 PDT
Created attachment 150234 [details]
add comment in CCRenderPass::appendQuadsToFillScreen
Comment 15 Adrienne Walker 2012-06-29 12:33:05 PDT
Comment on attachment 150234 [details]
add comment in CCRenderPass::appendQuadsToFillScreen

LGTM.  R=me.
Comment 16 James Robinson 2012-06-29 12:57:44 PDT
Committed r121583: <http://trac.webkit.org/changeset/121583>