[chromium] Cleanup scissor rect computation/use with damage
Created attachment 143362 [details] Patch
Sorry for this sad news but apparently I was misusing clipRects in the occlusion tracker. For a RenderSurface R, its RenderSurface::clipRect should be in the space of R's target, and used to clip R. It's not in the space of R, and not used to clip children drawing into R.
(In reply to comment #2) > Sorry for this sad news but apparently I was misusing clipRects in the occlusion tracker. For a RenderSurface R, its RenderSurface::clipRect should be in the space of R's target, and used to clip R. It's not in the space of R, and not used to clip children drawing into R. For the slower of us, what does that mean for this patch? Are you describing a bug that this patch fixes, or an issue with this patch?
Oh sorry this was to Zeev. We went back and forth a bit on what a clipRect on a RenderSurface means, and we ended up treating it as being in the surface's own coordinate space. But it is actually in the target, so I think this patch is wrong as is, and we really need a unit test to demonstrate this distinction.
Created attachment 143826 [details] Patch
Created attachment 143848 [details] Patch
I re-uploaded the patch as I forgot to intersect the parentSurface->contentRect with thisSurface->drawableContentRect. No extra unit tests required as existing tests caught this change.
Comment on attachment 143848 [details] Patch 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.
Comment on attachment 143848 [details] Patch 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.
Comment on attachment 143848 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143848&action=review >>> 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. sgtm >>> 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. Yeh, I agree. Sorry to confuse, I meant "same comment as I made in LayerChromium.h for this same field." >>> 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. Ah I see your point. Since the function is using some logic with contentRect etc, this is awkward. However, I don't think it needs to do that anymore with CCMathUtil. See comment in CCRenderSurface::damageInSurfaceSpace. >>> 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. I think surface->clipRect is right (not parent). We want the scissor rect for this surface, which is its clip along with its target's damage. >>> 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. You're right, it is. My concern is with the contract. >>> 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. I'm hesitant about changing this contract. If we have a RenderSurfaceLayerList, it should be complete - all the data needed for drawing a frame should be computed for the tree together, I think. >>> 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. Oh! This argument/member should be removed from the CCRenderSurfaceDrawQuad (yay). We are not using the surfaceDamageRect anymore in LayerRendererChromium::drawRenderSurfaceQuad if we use the scissorRect instead. > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:460 > +FloatRect CCRenderSurface::damageInSurfaceSpace(const FloatRect& damageRect) const This damageRect parameter is in screen space, can the method and parameter names identify this please? > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:474 > + surfaceDamageRect = inverseScreenSpaceTransform.mapRect(damageRect); This should be using CCMathUtil::projectClippedRect. And the hasPerspective check can just go away. Does that make it more reasonable to do this externally, as it's just a project on the root rect using the surface's screen space inverse transform? Shawn can you verify what I just said here is true? >>> 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. Yeh, we've been thinking about having a place where we can methods to transform between spaces reliably for some time, but haven't done so yet..
Comment on attachment 143848 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143848&action=review Just one detail below, the rest is implemented. >> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:474 >> + surfaceDamageRect = inverseScreenSpaceTransform.mapRect(damageRect); > > This should be using CCMathUtil::projectClippedRect. And the hasPerspective check can just go away. Does that make it more reasonable to do this externally, as it's just a project on the root rect using the surface's screen space inverse transform? > > Shawn can you verify what I just said here is true? It sure would if we could remove the call to this from CCLayerTreeHostImpl::drawLayers and just use root surface's scissorRect instead. Would that make sense?
Comment on attachment 143848 [details] Patch I havent looked closely, but here's one reply you guys asked about. I'll take a closer look at the next patch whenever Zeev has time to submit it. View in context: https://bugs.webkit.org/attachment.cgi?id=143848&action=review >>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:474 >>> + surfaceDamageRect = inverseScreenSpaceTransform.mapRect(damageRect); >> >> This should be using CCMathUtil::projectClippedRect. And the hasPerspective check can just go away. Does that make it more reasonable to do this externally, as it's just a project on the root rect using the surface's screen space inverse transform? >> >> Shawn can you verify what I just said here is true? > > It sure would if we could remove the call to this from CCLayerTreeHostImpl::drawLayers and just use root surface's scissorRect instead. Would that make sense? In theory, yes, we can use CCMathUtil and remove the hasPerspective() case. However, I had wanted to double-check this before changing it. For unrelated reasons, it turns out I'll need to verify this and make the change sometime this week, so that works out. Zeev, it looks like you have basically taken this code from a different place, right? I think its OK to leave it the way it is for now, and I'll probably end up changing it in a different patch myself very soon. Lets just keep tabs on who lands their patch first, and the other person can accommodate the new code in their patch.
Created attachment 144871 [details] Patch
(In reply to comment #13) > Created an attachment (id=144871) [details] > Patch I went ahead and tested whether the CCMathUtil usage would be correct, and I verified it should work correctly. So, Zeev, if you wanted to do that in this patch, then I don't think I'll even need to submit a separate patch for it. Also, I have a separate bug on me already to add appropriate unit tests for it, and a moderate amount of test coverage exists for this same usage in other parts of code, so I think its OK for you to just make the change as you were hoping =)
Created attachment 144892 [details] Patch
Comment on attachment 144892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144892&action=review This is looking really great! A few small nits left from me. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:623 > - layer->renderSurface()->setScissorRect(this, quad->surfaceDamageRect()); > + setScissorToRect(quad->scissorRect()); > + Let's just remove this, as it's already done in drawQuad now :) > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:88 > +static IntRect calculateLayerScissorRect(LayerChromium* layer, const FloatRect& rootDamageRect) > +{ > + if (layer->usesLayerClipping()) > + return layer->clipRect(); > + return layer->targetRenderSurface()->contentRect(); > +} Could remove this and template with the above function now? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:101 > + FloatRect result = CCMathUtil::projectClippedRect(inverseScreenSpaceTransform, rootDamageRect); nit: s/result/clipAndDamage/ like above? This and calculateLayerScissorRect are more similar than they appear, maybe just reordering stuff inside one of them would make that more obvious. I like that this methods has a clipRect var, then clipAndDamage var. Can we follow this pattern in calculateLayerScissorRect also? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:738 > +static void calculateVisibleLayersAndScissorRects(const LayerList& renderSurfaceLayerList, const FloatRect& rootDamageRect) nit: calculateVisibleAndScissorRectsInternal?
Comment on attachment 144892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144892&action=review Oh I skipped a couple files somehow, oops. A few more small comments. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:317 > + if (it.representsContributingRenderSurface()) { > + if (!it->renderSurface()->scissorRect().isEmpty()) These two lines can be merged. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:324 > else if (it.representsItself() && !it->visibleLayerRect().isEmpty()) { > it->willDraw(m_layerRenderer.get()); > - pass->appendQuadsForLayer(*it, &occlusionTracker, hadMissingTiles); > + > + if (!it->scissorRect().isEmpty()) > + pass->appendQuadsForLayer(*it, &occlusionTracker, hadMissingTiles); Oh, put this scissorRect check in with the visibleLayerRect check. We must call append if we call willDraw. > Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:71 > +void CCRenderPass::appendQuadsForRenderSurfaceLayer(CCLayerImpl* layer, CCOcclusionTrackerImpl* occlusionTracker, const FloatRect& rootDamageRect) rootDamageRect isn't used in here anymore, so we can avoid adding it now?
Created attachment 144905 [details] Patch
Created attachment 144908 [details] Patch
Re-uploaded to fix the bug title in changelog; no code changes
Comment on attachment 144908 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144908&action=review Wonderful :) jamesr/enne can you please take a look? > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:467 > if (scissorRect.isEmpty()) Let's ASSERT(!scissorRect.isEmpty()) as well, since we should be preventing these quads from being added in CCLTHImpl? > Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:78 > + extra whitespace
Comment on attachment 144908 [details] Patch Looks really awesome Zeev! I did have a lot of comments... Hopefully they don't contradict Dana =) View in context: https://bugs.webkit.org/attachment.cgi?id=144908&action=review > Source/WebCore/ChangeLog:13 > + Covered by existing layout tests. Introduced more unit tests to Its worth pointing out that damage tracking and scissoring aren't actually covered by layout tests. DumpRenderTree reads back from the backbuffer, and therefore partial swap (and hence the scissoring) doesn't apply in those cases. This can possibly be fixed in the long run for "repaint" layout tests, but in general it is just the nature of layout tests that prevents it. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:74 > + FloatRect clipAndDamage = CCMathUtil::projectClippedRect(inverseScreenSpaceTransform, rootDamageRect); Since we removed that helper function, I think we should add a comment here now that makes it clear what we're doing with this transform (i.e. that we're computing the damage in local surface space, where the rootDamageRect is in root surface space.) > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:79 > + if (layer->usesLayerClipping()) > + clipAndDamage.intersect(layer->clipRect()); > + else > + clipAndDamage.intersect(renderSurface->contentRect()); I'm guessing you just copied the code the way I had written it in the past, but now I see a more readable way we could write this (ignoring my abbreviated variables): FloatRect rootSurfaceDamageInLocalSurfaceSpace = projectClippedRect(inverse, rootDamageRect) FloatRect clipAndDamageRect; if (layer->usesLayerClipping()) clipAndDamageRect = intersection(layer->clipRect(), rootSurfaceDamageInLocalSurfaceSpace); else clipAndDamageRect = intersection(renderSurface->contentRect(), rootSurfaceDamageInLocalSurfaceSpace); The advantage of this is that we don't accidentally mislead anyone by temporarily assigning the transformed damage to the clipAndDamage. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:89 > + RenderSurfaceType* targetSurface = parentLayer->targetRenderSurface(); > + RenderSurfaceType* surface = layer->renderSurface(); can we add an ASSERT(surface) and ASSERT(targetSurface) just after this? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:91 > + if (clipRect.isEmpty()) We should definitely add a comment here saying that checking isEmpty() is how (for now) we encoded whether the clipRect is supposed to be used or not, for surfaces, unlike layers which have usesLayerClipping(). > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:97 > + FloatRect clipAndDamage = CCMathUtil::projectClippedRect(inverseScreenSpaceTransform, rootDamageRect); > + > + clipAndDamage.intersect(clipRect); If you were OK with creating the more readable version in the previous function, then let's do it here, too. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:749 > + } should we put an else { assert not reached } here? I'm genuinely asking, not suggesting - maybe reviewers can say more definitively if its appropriate. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:277 > + FloatRect rootDamageRect = calculateRenderSurfaceLayerList(renderSurfaceLayerList); > > TRACE_EVENT1("webkit", "CCLayerTreeHostImpl::calculateRenderPasses", "renderSurfaceLayerList.size()", static_cast<long long unsigned>(renderSurfaceLayerList.size())); > > - if (layerRendererCapabilities().usingPartialSwap || settings().showSurfaceDamageRects) > - trackDamageForAllSurfaces(m_rootLayerImpl.get(), renderSurfaceLayerList); > - m_rootDamageRect = m_rootLayerImpl->renderSurface()->damageTracker()->currentDamageRect(); > + m_rootLayerImpl->setScissorRect(enclosingIntRect(rootDamageRect)); Personally I don't like the idea that calculateRenderSurfaceLayerList() would have any return value. It feels right as a purely imperative, procedural function that changes state of the class. It looks like the reason we needed this return value is to call setScissorRect for the rootLayerImpl. Do we need this, however? wouldn't this be correctly set by calculateVisibleAndScissorRects() ? At the very least, how about we put this inside of calculateRenderSurfaceLayerList() so that we don't need to return that local rootDamageRect? It makes sense for that line to be as closely grouped to calculateVisibleAndScissorRects as possible, if its not actually be computed inside of it. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:425 > + TransformationMatrix inverseScreenSpaceTransform = renderPass->targetSurface()->screenSpaceTransform().inverse(); > + FloatRect surfaceSwapRect = CCMathUtil::projectClippedRect(inverseScreenSpaceTransform, m_rootDamageRect); > + m_layerRenderer->drawRenderPass(renderPass, surfaceSwapRect); I'm unclear why we need to pass surfaceSwapRect here (and the name seems awkward, since surfaces don't do swapping?)... isnt the whole point of this patch that we already computed the scissor rects we want to use by this point? So this would be redundant and unnecessary, because the scissors for all surfaces and layers (and hence, quads) are already known by this time, right? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:178 > + // Exposed for testing. Returns root damage rect or full viewport if not using partial swap. > + FloatRect calculateRenderSurfaceLayerList(CCLayerList&); If you agree with my earlier comment about removing this return value, then don't forget to fix this comment, too.
Comment on attachment 144908 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144908&action=review >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:79 >> + clipAndDamage.intersect(renderSurface->contentRect()); > > I'm guessing you just copied the code the way I had written it in the past, but now I see a more readable way we could write this (ignoring my abbreviated variables): > > FloatRect rootSurfaceDamageInLocalSurfaceSpace = projectClippedRect(inverse, rootDamageRect) > FloatRect clipAndDamageRect; > if (layer->usesLayerClipping()) > clipAndDamageRect = intersection(layer->clipRect(), rootSurfaceDamageInLocalSurfaceSpace); > else > clipAndDamageRect = intersection(renderSurface->contentRect(), rootSurfaceDamageInLocalSurfaceSpace); > > The advantage of this is that we don't accidentally mislead anyone by temporarily assigning the transformed damage to the clipAndDamage. rootSurfaceDamageInLocalSurfaceSpace == surfaceSwapRect, right? Can we call it that instead? Then we're saying scissor = intersection(clip, swapRect) which is more precise yes? >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:749 >> + } > > should we put an else { assert not reached } here? I'm genuinely asking, not suggesting - maybe reviewers can say more definitively if its appropriate. It would be reached, with it.representsTargetRenderSurface(). We just don't do anything in that case, we use the contributing surface case instead. This avoids trying to set a scissor on the root surface (it's not contributing) based on its non-existent parent. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:261 > + rootDamageRect = FloatRect(FloatPoint(0, 0), viewportSize()); This should use deviceViewportSize() like the similar computation below does. >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:277 >> + m_rootLayerImpl->setScissorRect(enclosingIntRect(rootDamageRect)); > > Personally I don't like the idea that calculateRenderSurfaceLayerList() would have any return value. It feels right as a purely imperative, procedural function that changes state of the class. It looks like the reason we needed this return value is to call setScissorRect for the rootLayerImpl. Do we need this, however? wouldn't this be correctly set by calculateVisibleAndScissorRects() ? > > At the very least, how about we put this inside of calculateRenderSurfaceLayerList() so that we don't need to return that local rootDamageRect? It makes sense for that line to be as closely grouped to calculateVisibleAndScissorRects as possible, if its not actually be computed inside of it. The return value is actually the rootSwapRect right now, not the rootDamageRect I guess. This return value could also be used by the CCOcclusionTracker instead of recomputing it again below. I agree the rootLayer's scissor should be set in CCLayerTreeHostCommon::calculateVisibleAndScissorRects. Right now it's not being set on the main thread right? If it's inside there then it would be always set when the others are set. >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:425 >> + m_layerRenderer->drawRenderPass(renderPass, surfaceSwapRect); > > I'm unclear why we need to pass surfaceSwapRect here (and the name seems awkward, since surfaces don't do swapping?)... isnt the whole point of this patch that we already computed the scissor rects we want to use by this point? So this would be redundant and unnecessary, because the scissors for all surfaces and layers (and hence, quads) are already known by this time, right? (In debug) We clear everything in the surface that is going to be drawn. This area is its swap rect, not its scissor rect (which is in its target). Can also think of it as the union of all scissorRects of things that draw into it. Swap rect is the partial swap rect transformed into the surface, or the part of the surface that will be included in the swap. I got that name idea from convo with you about distinguishing damage rect vs partial swap rect. Do you think a different name would be more clear?
I think we're having a little bit of terminology confusion: Swapping doesn't really have meaning, in my opinion, for renderSurfaces. The way I see it, we should have the following definitions: damageRect: the rect that represents a region that has changed on a surface clipRect: the rect that represents a region that clips a layer/surface scissorRect: the rect that is actually given to glScissor, used to avoid redrawing stuff that hasn't changed, or to avoid drawing stuff that should actually be clipped. that means its the intersection of damage and clip (assuming both damage and clip are being used on that particular layer) swapRect: The rect that needs to be swapped from back-buffer to front buffer at the very end of our compositing algorithm. -- doesn't seem to be relevant to this patch, and maybe the word swap shouldn't even come up? If we use that terminology, I think it makes sense to *not* use swapRect. in those places. I think we really do mean damage in those cases.
(In reply to comment #23) > rootSurfaceDamageInLocalSurfaceSpace == surfaceSwapRect, right? Can we call it that instead? Then we're saying scissor = intersection(clip, swapRect) which is more precise yes? > so based on my previous comment - I think they are not the same at all... surfaceSwap in my opinion doesn't have meaning, the swap concept doesn't exist on surfaces. So I would say it is more precise to say intersection(clip, rootSurfaceDamageInLocalSurfaceSpace). > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:261 > > + rootDamageRect = FloatRect(FloatPoint(0, 0), viewportSize()); > > This should use deviceViewportSize() like the similar computation below does. > > >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:277 > >> + m_rootLayerImpl->setScissorRect(enclosingIntRect(rootDamageRect)); > > > > Personally I don't like the idea that calculateRenderSurfaceLayerList() would have any return value. It feels right as a purely imperative, procedural function that changes state of the class. It looks like the reason we needed this return value is to call setScissorRect for the rootLayerImpl. Do we need this, however? wouldn't this be correctly set by calculateVisibleAndScissorRects() ? > > > > At the very least, how about we put this inside of calculateRenderSurfaceLayerList() so that we don't need to return that local rootDamageRect? It makes sense for that line to be as closely grouped to calculateVisibleAndScissorRects as possible, if its not actually be computed inside of it. > > The return value is actually the rootSwapRect right now, not the rootDamageRect I guess. This return value could also be used by the CCOcclusionTracker instead of recomputing it again below. > > I agree the rootLayer's scissor should be set in CCLayerTreeHostCommon::calculateVisibleAndScissorRects. Right now it's not being set on the main thread right? If it's inside there then it would be always set when the others are set. > Is it possible the swap/scissor/damage definition clarification changes what you're saying here? If you still think we need this return value, lets discuss offline and then update this bug with a summary of our discussion? > >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:425 > >> + m_layerRenderer->drawRenderPass(renderPass, surfaceSwapRect); > > > > I'm unclear why we need to pass surfaceSwapRect here (and the name seems awkward, since surfaces don't do swapping?)... isnt the whole point of this patch that we already computed the scissor rects we want to use by this point? So this would be redundant and unnecessary, because the scissors for all surfaces and layers (and hence, quads) are already known by this time, right? > > (In debug) We clear everything in the surface that is going to be drawn. This area is its swap rect, not its scissor rect (which is in its target). Can also think of it as the union of all scissorRects of things that draw into it. > > Swap rect is the partial swap rect transformed into the surface, or the part of the surface that will be included in the swap. I got that name idea from convo with you about distinguishing damage rect vs partial swap rect. Do you think a different name would be more clear? Same for here - if needed lets discuss offline briefly?
(In reply to comment #25) > (In reply to comment #23) > > rootSurfaceDamageInLocalSurfaceSpace == surfaceSwapRect, right? Can we call it that instead? Then we're saying scissor = intersection(clip, swapRect) which is more precise yes? > > > > so based on my previous comment - I think they are not the same at all... surfaceSwap in my opinion doesn't have meaning, the swap concept doesn't exist on surfaces. So I would say it is more precise to say intersection(clip, rootSurfaceDamageInLocalSurfaceSpace). Shawn and I spoke offline about this naming stuff.. calling it "swap" rect is correct now, but he has plans that will make it less-so in the future. We arrived at the following naming scheme: - the rect in root space that is the aggregate of all damage is the "rootScissorRect". (this is currently called "rootDamageRect" in the code which is a misnomer.) - this same rect, transformed to a non-root surface is called "rootScissorRectInCurrentSurface" (or ...TargetSurface etc if appropriate). This indicates that its the rootScissorRect transformed to some other surface. Can we use these names throughout this patch? Sorry for the name churn :/ > > > At the very least, how about we put this inside of calculateRenderSurfaceLayerList() so that we don't need to return that local rootDamageRect? It makes sense for that line to be as closely grouped to calculateVisibleAndScissorRects as possible, if its not actually be computed inside of it. > > > > The return value is actually the rootSwapRect right now, not the rootDamageRect I guess. This return value could also be used by the CCOcclusionTracker instead of recomputing it again below. > > > > I agree the rootLayer's scissor should be set in CCLayerTreeHostCommon::calculateVisibleAndScissorRects. Right now it's not being set on the main thread right? If it's inside there then it would be always set when the others are set. > > > > Is it possible the swap/scissor/damage definition clarification changes what you're saying here? If you still think we need this return value, lets discuss offline and then update this bug with a summary of our discussion? We use the rootScissorRect as input for the CCOcclusionTracker, so it'd be nice to get that from the calculateRenderSurfaceLayerList function somehow instead of computing it again. Perhaps just using a reference parameter would feel nicer instead of a return value, as it has a name. The meaning of a FloatRect being returned from calculateRSLL is a bit vague.
Comment on attachment 144908 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144908&action=review >> Source/WebCore/ChangeLog:13 >> + Covered by existing layout tests. Introduced more unit tests to > > Its worth pointing out that damage tracking and scissoring aren't actually covered by layout tests. DumpRenderTree reads back from the backbuffer, and therefore partial swap (and hence the scissoring) doesn't apply in those cases. This can possibly be fixed in the long run for "repaint" layout tests, but in general it is just the nature of layout tests that prevents it. I meant that the code changes were not supposed to change any functionality therefore no new e2e tests are needed. Existing tests would catch if we broke something unrelated. The scissorRect computation is covered by unit tests. >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:74 >> + FloatRect clipAndDamage = CCMathUtil::projectClippedRect(inverseScreenSpaceTransform, rootDamageRect); > > Since we removed that helper function, I think we should add a comment here now that makes it clear what we're doing with this transform (i.e. that we're computing the damage in local surface space, where the rootDamageRect is in root surface space.) Let's bring back the helper function. It will be duplicated in 2 places now (because we merged the calcScissorRects, but oh well), but right now I'll need to paste the same comment in THREE places. When we merge the surface classes, one implementation will go away, and then we can move it into more appropriate place.
(In reply to comment #27) > (From update of attachment 144908 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144908&action=review > > >> Source/WebCore/ChangeLog:13 > >> + Covered by existing layout tests. Introduced more unit tests to > > > > Its worth pointing out that damage tracking and scissoring aren't actually covered by layout tests. DumpRenderTree reads back from the backbuffer, and therefore partial swap (and hence the scissoring) doesn't apply in those cases. This can possibly be fixed in the long run for "repaint" layout tests, but in general it is just the nature of layout tests that prevents it. > > I meant that the code changes were not supposed to change any functionality therefore no new e2e tests are needed. Existing tests would catch if we broke something unrelated. The scissorRect computation is covered by unit tests. > > >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:74 > >> + FloatRect clipAndDamage = CCMathUtil::projectClippedRect(inverseScreenSpaceTransform, rootDamageRect); > > > > Since we removed that helper function, I think we should add a comment here now that makes it clear what we're doing with this transform (i.e. that we're computing the damage in local surface space, where the rootDamageRect is in root surface space.) > > Let's bring back the helper function. It will be duplicated in 2 places now (because we merged the calcScissorRects, but oh well), but right now I'll need to paste the same comment in THREE places. When we merge the surface classes, one implementation will go away, and then we can move it into more appropriate place. Sure, let's call it like the variable would be, something like computeRootScissorRectInCurrentSurface(rootScissorRect)
(In reply to comment #28) > (In reply to comment #27) > > (From update of attachment 144908 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=144908&action=review > > > > >> Source/WebCore/ChangeLog:13 > > >> + Covered by existing layout tests. Introduced more unit tests to > > > > > > Its worth pointing out that damage tracking and scissoring aren't actually covered by layout tests. DumpRenderTree reads back from the backbuffer, and therefore partial swap (and hence the scissoring) doesn't apply in those cases. This can possibly be fixed in the long run for "repaint" layout tests, but in general it is just the nature of layout tests that prevents it. > > > > I meant that the code changes were not supposed to change any functionality therefore no new e2e tests are needed. Existing tests would catch if we broke something unrelated. The scissorRect computation is covered by unit tests. > > > > >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:74 > > >> + FloatRect clipAndDamage = CCMathUtil::projectClippedRect(inverseScreenSpaceTransform, rootDamageRect); > > > > > > Since we removed that helper function, I think we should add a comment here now that makes it clear what we're doing with this transform (i.e. that we're computing the damage in local surface space, where the rootDamageRect is in root surface space.) > > > > Let's bring back the helper function. It will be duplicated in 2 places now (because we merged the calcScissorRects, but oh well), but right now I'll need to paste the same comment in THREE places. When we merge the surface classes, one implementation will go away, and then we can move it into more appropriate place. > > Sure, let's call it like the variable would be, something like computeRootScissorRectInCurrentSurface(rootScissorRect) Shawn are you OK with this name?
Comment on attachment 144908 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144908&action=review >>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:277 >>> + m_rootLayerImpl->setScissorRect(enclosingIntRect(rootDamageRect)); >> >> Personally I don't like the idea that calculateRenderSurfaceLayerList() would have any return value. It feels right as a purely imperative, procedural function that changes state of the class. It looks like the reason we needed this return value is to call setScissorRect for the rootLayerImpl. Do we need this, however? wouldn't this be correctly set by calculateVisibleAndScissorRects() ? >> >> At the very least, how about we put this inside of calculateRenderSurfaceLayerList() so that we don't need to return that local rootDamageRect? It makes sense for that line to be as closely grouped to calculateVisibleAndScissorRects as possible, if its not actually be computed inside of it. > > The return value is actually the rootSwapRect right now, not the rootDamageRect I guess. This return value could also be used by the CCOcclusionTracker instead of recomputing it again below. > > I agree the rootLayer's scissor should be set in CCLayerTreeHostCommon::calculateVisibleAndScissorRects. Right now it's not being set on the main thread right? If it's inside there then it would be always set when the others are set. After discussing with Dana briefly, I'll just make m_rootDamageRect (now m_rootScissorRect) to be set to deviceViewPortSize when not using partial swap. This will remove the need for either return value or out param.
> > Sure, let's call it like the variable would be, something like computeRootScissorRectInCurrentSurface(rootScissorRect) > > Shawn are you OK with this name? Sure, I'm OK with it. =) Please forgive me if I bring this up again a little bit down the road when we have some retrospective about what better naming we can have. But there is definitely no need to block you on this naming stuff for now.
Created attachment 145316 [details] Patch
Comment on attachment 145316 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145316&action=review If Shawn's happy I think we're definitely ready for a more formal review :) > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:388 > +void LayerRendererChromium::clearRenderSurface(CCRenderSurface* renderSurface, CCRenderSurface* rootRenderSurface, const FloatRect& surfaceSwapRect) last nit: Thanks for switching the naming elsewhere, just in the LRC .cpp/.h files: s/surfaceSwapRect/rootScissorRectInCurrentSurface/
I tested this most recent patch manually on several pages with --enable-partial-swap, and it seems to work just fine =) pretty awesome, Zeev!
Comment on attachment 145316 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145316&action=review Calculating scissor rects along with visibility rather than during drawing seems great. (Although those two calculate functions really should eventually be combined...) I like the code changes here a lot, but have some questions about your tests. > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:390 > + /* empty list */ nit: Don't use C-style comments unless it's multiline. > Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1217 > + // 0 is the hardcoded return value of getUniformLocation > + // 1 is count ??? > + // Boolean cast to str?? > + // float* values > + EXPECT_CALL(*m_context, uniformMatrix4fv(0, 1, _, _)) > + .WillOnce(Return()) > + .RetiresOnSaturation(); > + > + // 0 is the hardcoded return value of getUniformLocation > + // 3 floats are color / 255 > + // 1 is Alpha > + EXPECT_CALL(*m_context, uniform4f(0, _, _, _, 1)) > + .WillOnce(Return()) > + .RetiresOnSaturation(); Do you really need to expect all of these uniforms? This seems a bit overtested. Is drawElements and useProgram(1) enough to catch this? > Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1262 > + // Set up mock graphics context I love all the comments you have in headers in this patch, but I feel like a lot of these tests are a bit over-commented. You don't need to say what you're doing unless it's not obvious. > Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1270 > + harness.mustSetScissor(0, 0, 10, 10); > + harness.mustSetScissor(0, 0, 10, 10); Why is the scissor set twice? Can we not do that or at worst make the test not have to care about that implementation detail? > Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1336 > +static PassOwnPtr<CCLayerTreeHostImpl> setupLayersForOpacity(bool partialSwap, CCLayerTreeHostImplClient* client) > +{ > + CCSettings settings; > + settings.partialSwapEnabled = true; What's the partialSwap parameter supposed to be doing? Are the tests using this wrong? > Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1440 > +TEST_F(CCLayerTreeHostImplTest, quadsToFillScreen) What does this test do that viewportCovered doesn't do? > Source/WebKit/chromium/tests/CCRenderSurfaceTest.cpp:116 > + nit: useless whitespace
Comment on attachment 145316 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145316&action=review Thanks; fixed all except where indicated >> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:390 >> + /* empty list */ > > nit: Don't use C-style comments unless it's multiline. Actually didn't need this comment at all; removed. >> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1217 >> + .RetiresOnSaturation(); > > Do you really need to expect all of these uniforms? This seems a bit overtested. Is drawElements and useProgram(1) enough to catch this? Yes that would be enough; Moved them to be ignored. >> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1262 >> + // Set up mock graphics context > > I love all the comments you have in headers in this patch, but I feel like a lot of these tests are a bit over-commented. You don't need to say what you're doing unless it's not obvious. Removed more obvious comments >> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1270 >> + harness.mustSetScissor(0, 0, 10, 10); > > Why is the scissor set twice? Can we not do that or at worst make the test not have to care about that implementation detail? First is for clearing the surface, second for drawing the quads; added comment to that effect. It seems important to distinguish this in the tests, in the sense that either 3 calls or 1 would be wrong. >> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1336 >> + settings.partialSwapEnabled = true; > > What's the partialSwap parameter supposed to be doing? Are the tests using this wrong? Good catch thanks. The parameter was not used, but it only affected test coverage, not correctness. This is because the expectations are identical in both cases. >> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1440 >> +TEST_F(CCLayerTreeHostImplTest, quadsToFillScreen) > > What does this test do that viewportCovered doesn't do? I see two differences - one that this one is using partial swap, and two that we have more surfaces. I also remember needing this to catch something, though cannot remember what right now. >> Source/WebKit/chromium/tests/CCRenderSurfaceTest.cpp:116 >> + > > nit: useless whitespace Fixed
(In reply to comment #36) > >> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1440 > >> +TEST_F(CCLayerTreeHostImplTest, quadsToFillScreen) > > > > What does this test do that viewportCovered doesn't do? > > I see two differences - one that this one is using partial swap, and two that we have more surfaces. I also remember needing this to catch something, though cannot remember what right now. Maybe it can be called viewportCoveredWithPartialSwap then? makes the reason it exists more clear
Created attachment 145424 [details] Patch
Comment on attachment 145424 [details] Patch Attachment 145424 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12861844
Created attachment 145444 [details] Patch
Rebased and uploaded again
Comment on attachment 145444 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145444&action=review Thanks for the changes. R=me, with two small changes: > Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1275 > + // One scissor for clearing surface, another for drawing the quads > + harness.mustSetScissor(0, 8, 2, 2); > + harness.mustSetScissor(0, 8, 2, 2); Hmm. I still think mustSetScissor should allow any number of redundant scissor settings. If we start shadowing state locally in LRC and not repeatedly setting scissors, then this test would needlessly fail because of that implementation detail. > Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1421 > +TEST_F(CCLayerTreeHostImplTest, quadsToFillScreen) I still don't think this really adds anything over the other test (or occlusion tests that have multiple surfaces) and should probably just get removed. If you're going to add a gutter quad test related to partial swap, then it should probably be something where the scissor is smaller than the viewport and then you verify that the quads fill the scissor.
Created attachment 145458 [details] Patch
Thanks; implemented the two changes.
Created attachment 145584 [details] Patch
Rebased and re-uploaded with reviewer information.
Comment on attachment 145584 [details] Patch Clearing flags on attachment: 145584 Committed r119401: <http://trac.webkit.org/changeset/119401>
All reviewed patches have been landed. Closing bug.