[Webkit-unassigned] [Bug 87167] [chromium] Cleanup scissor rect computation/use with damage
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon May 28 13:11:29 PDT 2012
https://bugs.webkit.org/show_bug.cgi?id=87167
--- Comment #8 from Dana Jansens <danakj at chromium.org> 2012-05-28 13:11:29 PST ---
(From update of attachment 143848)
View in context: https://bugs.webkit.org/attachment.cgi?id=143848&action=review
> Source/WebCore/ChangeLog:14
> + test end-to-end drawing using mock graphic context, and added more
nit: mock GraphicsContext3D
> Source/WebKit/chromium/ChangeLog:8
> + Added unit tests to CCLayerTreeHostImpl using mock graphic context
ditto nit
> Source/WebCore/platform/graphics/chromium/LayerChromium.h:312
> + // Uses layer's content space
nit: Comments in WebKit are complete sentences, including periods.
> Source/WebCore/platform/graphics/chromium/LayerChromium.h:315
> + // During drawing, identifies the region outside of which nothing should be drawn.
So this layer is never used during drawing, and currently this value is always == clipRect, right? But the method/value exists for templated code to work, and in the future this will replace clipRect entirely hopefully. I think this comment should reflect this instead.
> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:451
> +void LayerRendererChromium::drawRenderPass(const CCRenderPass* renderPass, const FloatRect& rootDamageRect)
I think we should rename this parameter to be the rootScissorRect. LRC doesn't really know/care about damage, to it this rect indicates the part of the root/surface it should be drawing.
Can we do the transform to this surface outside before calling this function, and pass in a rect on the current RenderPass instead?
> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:464
> +void LayerRendererChromium::drawQuad(const CCDrawQuad* quad, const IntRect& clipRect)
The scissorRect on the quad should be == clipRect when there is no partial swap, right? Can we just remove this clipRect parameter entirely? I think then we can remove the whole if/else dealie.
> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:475
> if (scissorRect.isEmpty())
> GLC(m_context, m_context->disable(GraphicsContext3D::SCISSOR_TEST));
This logic is probably wrong now, as I think always have a valid scissorRect on the quad to clip with now. Shouldn't the meaning of an empty quad->scissorRect be that the quad doesn't need to be drawn, regardless of partial swap enabled?
> 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?
> Source/WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:143
> + // During drawing, identifies the region outside of which nothing should be drawn.
ditto with LayerChromium.h
> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:508
> + FloatRect damageRect(FloatPoint(0, 0), viewportSize());
nit: this would be more clear as rootDamageRect
> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:86
> + if (layer->usesLayerClipping())
> + return layer->clipRect();
> + 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.
> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:92
> + CCRenderSurface* parentSurface = parentLayer->targetRenderSurface();
nit: This isn't the parentSurface, it's the target surface of our surface. Maybe just "targetSurface"?
> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:93
> + CCRenderSurface* thisSurface = layer->renderSurface();
nit: can shorten this variable just to "surface", or use some more descriptive prefix? I don't think I've seen "this" as a prefix in WebKit like this before.
> 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)?
> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:109
> + clipRect = parentSurface->contentRect();
If we keep this separate, intersect with surface->drawableContentRect() like the above function?
> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:740
> +static void walkLayersAndCalculateScissorRects(const LayerList& renderSurfaceLayerList, const FloatRect& rootDamageRect)
visibleLayerRect isn't a scissor rect, so if this function is doing both, the name should probably list both. CalculateVisibleLayerAndScissorRects?
> 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.
> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:263
> + FloatRect damageRect;
nit: rootDamageRect
> 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).
> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:309
> + if (!layerRendererCapabilities().usingPartialSwap || !it->scissorRect().isEmpty())
If we're not using partial swap, why would we still want to draw this if the scissorRect is empty? Wouldn't the frame be scissored by the entire viewport, and this layer's scissorRect still be a valid representation of what we should draw? Can't we just check "if (!isEmpty)" here?
Also, I think we should be checking the scissor rect of the layer's surface, not the layer itself? The surface's scissor can be non-empty when the owning layer's is. Can we add a unit test for this case?
> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:315
> + if (!layerRendererCapabilities().usingPartialSwap || !it->scissorRect().isEmpty())
ditto, why check partial swap here?
> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:329
> + if (!layerRendererCapabilities().usingPartialSwap || !m_rootLayerImpl->scissorRect().isEmpty())
These quads here are meant to fill the viewport, they are not scissored by the root layer's scissor rect. So I think this if() should go away.
> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:-77
> + CCRenderSurface* surface = layer->renderSurface();
> +
> // FIXME: render surface layers should be a CCLayerImpl-derived class and
> // not be handled specially here.
> CCQuadCuller quadCuller(m_quadList, layer, occlusionTracker, layer->hasDebugBorders());
>
> - CCRenderSurface* surface = layer->renderSurface();
This looks unneeded.
> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:85
> - quadCuller.appendSurface(CCRenderSurfaceDrawQuad::create(sharedQuadState.get(), surface->contentRect(), layer, surfaceDamageRect(), isReplica));
> + quadCuller.appendSurface(CCRenderSurfaceDrawQuad::create(sharedQuadState.get(), surface->contentRect(), layer, this->targetSurface()->damageInSurfaceSpace(rootDamageRect), isReplica));
Why not use the surface->scissorRect() here?
> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:96
> - quadCuller.appendReplica(CCRenderSurfaceDrawQuad::create(sharedQuadState.get(), surface->contentRect(), layer, surfaceDamageRect(), isReplica));
> + quadCuller.appendReplica(CCRenderSurfaceDrawQuad::create(sharedQuadState.get(), surface->contentRect(), layer, this->targetSurface()->damageInSurfaceSpace(rootDamageRect), isReplica));
ditto
> 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?
> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:484
> +FloatRect CCRenderSurface::screenToSurfaceSpace(const FloatRect& rect) const
> +{
> + TransformationMatrix inverseScreenSpaceTransform = m_screenSpaceTransform.inverse();
> + return inverseScreenSpaceTransform.mapRect(rect);
> +}
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.
> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:489
> +FloatRect CCRenderSurface::surfaceToScreenSpace(const FloatRect& rect) const
> +{
> + return m_screenSpaceTransform.mapRect(rect);
> }
I think this should just be done with the transform directly, we don't need to add a method for this.
> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:162
> + /**
> + * Transforms a damage rect into surface space. In some cases may decide
> + * to return the entire surface contentRect.
> + */
Use C++ style comment.
nit: "In some cases it may"
> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:206
> + // Uses the space of target surface of parent layer
"This is in the space of the surface's target surface."
I think that "target surface of parent layer" is kinda an implementation detail, the conceptual idea is it's the surface's target.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list