Summary: | [chromium] Remove mapRect and mapQuad from WebTransformationMatrix | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Robinson <jamesr> | ||||||||||
Component: | New Bugs | Assignee: | 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
James Robinson
2012-06-28 17:41:26 PDT
Created attachment 150050 [details]
Patch
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. 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. Created attachment 150064 [details]
Patch
Second patch uses CCMathUtil::projectPoint inside of projectQuad. That just leaves the mapPoint functions behind. 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 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. 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 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? Created attachment 150224 [details]
Patch
Updated - I'd like a bit more guidance on how to handle the appendQuadsToFillScreen(). 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.
>
> 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.
Created attachment 150234 [details]
add comment in CCRenderPass::appendQuadsToFillScreen
Comment on attachment 150234 [details]
add comment in CCRenderPass::appendQuadsToFillScreen
LGTM. R=me.
Committed r121583: <http://trac.webkit.org/changeset/121583> |