[Webkit-unassigned] [Bug 87167] [chromium] Cleanup scissor rect computation/use with damage

Wed May 30 07:37:28 PDT 2012

https://bugs.webkit.org/show_bug.cgi?id=87167

--- Comment #9 from zlieber at chromium.org  2012-05-30 07:37:28 PST ---
(From update of attachment 143848)
View in context: https://bugs.webkit.org/attachment.cgi?id=143848&action=review

Implemented all except as indicated.

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:630
>> +    if (!layer->renderSurface()->calcScissorRect(m_capabilities.usingPartialSwap, quad->surfaceDamageRect(), scissorRect))
> 
> Can we use quad->scissorRect() here, which should be == layer->renderSurface()->scissorRect()?
> 
> I guess the root is a special case that we have to deal with. We could
> a) Set the root surface's scissorRect to be == its clipRect which is == the viewport rect.
> b) Check if we are the root surface here, and disable scissor on that condition instead. Assume all other surfaces have valid scissorRect().
> 
> I am leaning toward b being nicer, wdyt?

If the calling code may make a more universal assumption it should be better. Otherwise we end up having the same condition in multiple places, until someone forgets it. So I'd go with (a) unless there is an efficiency issue with GL.

>> Source/WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:143
>> +    // During drawing, identifies the region outside of which nothing should be drawn.
> 
> ditto with LayerChromium.h

I'd say the knowledge of how this value is computed belongs to where it is computed. Since it's computed outside the class, that's where the comment should be. If someone changed the algorithm of scissorRect computation they will not know to go to class header file and update the comment. But that's a minor point.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:86
>> +    return layer->targetRenderSurface()->contentRect();
> 
> This could still intersect the rootDamageRect (which is the viewport rect in the main thread case). In fact I think the above function, templated, could be used for both threads.
> 
> If not, then unused parameter names should be excluded from the function's parameter list.

Can this be postponed until we have a base class for layers and surfaces? Otherwise we're not saving anything: we're blending two functions, but duplicating another one - damageInSurfaceSpace. One could argue that damageInSurfaceSpace can be a static function of CCLayerTreeHostCommon, but that's even worse - this side-steps the entire concept of data encapsulation and provides code that operates on RenderSurface's data outside the class, using getters/setters to access private variables.

The second parameter is hard to remove as the code calling this is templated, and doesn't distinguish between layer/surface types.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:103
>> +static IntRect calculateSurfaceScissorRect(LayerChromium* layer, const FloatRect& rootDamageRect)
> 
> Same here, could template the above function and use it for both threads, since CCLTH is giving a valid rootDamageRect (the viewport)?

Just noticed the clipRects are taken from different surfaces between the two functions. Which one is right? These two can be blended.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:766
>>  void CCLayerTreeHostCommon::calculateDrawTransformsAndVisibility(CCLayerImpl* layer, CCLayerImpl* rootLayer, const TransformationMatrix& parentMatrix, const TransformationMatrix& fullHierarchyMatrix, Vector<CCLayerImpl*>& renderSurfaceLayerList, Vector<CCLayerImpl*>& layerList, CCLayerSorter* layerSorter, int maxTextureSize)
> 
> This method name is a bit of a lie with this change. The "AndVisibility" refers to the visibleLayerRects.
> 
> It would be nice if we could make this method compute scissor rects as well, instead of not computing visibleLayerRects. Is that reasonable to do? Or if not, then this method name should change.

Not sure if effort justifies the benefits - sounds just moving code around. I changed the name.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:269
>> +    CCLayerTreeHostCommon::calculateVisibleAndScissorRects(renderSurfaceLayerList, damageRect);
> 
> I'm worried that we are not calculating the visibleLayerRects anymore in calculateRenderSurfaceLayerList. If someone calls that function, they would expect those variables to be computed, as it's a wrapper for calculateDrawTransformsAndVisibility.
> 
> What if we pass a rootDamageRect over to calculateRenderSurfaceLayerList, that way we can always do this computation along with calculateDrawTransforms(AndVisibility).

The question is what this expectation is based on and what is the contract of calculateRenderSurfaceLayerList. It cannot be because it's a wrapper (this may change and is transparent to the caller), but it can be because it's expected to calculate render surface list in some valid state. So if that "valid state" heavily implies presence of valid visibleRects, then we shouldn't have moved it in the first place. If it doesn't, we could say the contract is adjusted (I did update all calling code). I'm not sure what the expectation is so do not have an opinion. It is some extra work to move it back.

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:85
>> +    quadCuller.appendSurface(CCRenderSurfaceDrawQuad::create(sharedQuadState.get(), surface->contentRect(), layer, this->targetSurface()->damageInSurfaceSpace(rootDamageRect), isReplica));
> 
> Why not use the surface->scissorRect() here?

surface->scissorRect uses coordinate space of target surface of parent layer, while damageInSurfaceSpace uses our surface and contains extra logic which I'm not sure if needed.

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:211
>> +bool CCRenderSurface::calcScissorRect(bool usingPartialSwap, const FloatRect& surfaceDamageRect, IntRect& scissorRect) const
> 
> Don't abbreviate names, use calculateScissorRect. But why does this need to exist if we have CCRenderSurface::scissorRect()? Shouldn't any code that used to use this, now use that instead?

Method removed

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:484
>> +}
> 
> Like below, we should just do this directly I think with the transform. Or if this is just for CCRS::damageInSurfaceSpace(), you could use a file-static inline method, if that makes the code more clear.
> 
> Some notes about mapRect also..
> - Don't use TransformationMatrix::mapRect right now, it's buggy. Use CCMathUtil::mapClippedRect or CCMathUtil::mapQuad instead to deal with the possibility that the transformed rect is clipped by the camera, and this changes its bounds (they would become a larger bounding box).
> - Use map* to apply a transform. Use project* to apply the inverse of a transform.

I see two reasons to have separate place to implement the transformations:

1. Precisely because of the delicacies such as those you just mentioned, you don't want to make people remember these things, but rather just call a single method with a straightforward name that will do the right thing.

2. You want the calling code to read like simple English sentence that communicates the intent. And the intent here is to transform a rect into a different coordinate space. Accessing a transform, inverting it and calling CCMathUtil is just implementation detail.

That said, I'd agree that CCRenderSurface is not the best place for these methods because it's not the only class that may want to expose them. So I removed them right now but I think we should find a better place for them.

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:489
>>  }
> 
> I think this should just be done with the transform directly, we don't need to add a method for this.

Actually after tweaks to the scissorRect computation alg this method is not used anymore. Removed.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.