[Webkit-unassigned] [Bug 67341] Scissor rect optimization for chromium compositor

[bugzilla-daemon at webkit.org]

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





--- Comment #6 from Shawn Singh <shawnsingh at chromium.org>  2011-09-01 16:06:33 PST ---
Sure, I'll remove the visualization code from this patch.

> It feels to me that the "setNeedsCommitBecauseXXX" approach is quite fragile. It seems to me that there is a fair amount of fragility pushed into the layer system with this approach, as each layer has to very-carefully specify what kind of change is happening.

After taking a step back yesterday, I realized the boolean flag is completely unnecessary, and reverting that significantly reduces the amount of changes in the LayerChromium.  This includes removing the "setNeedsCommitBecause" naming stuff.

> 
> Did you give any thought to doing this as a preprocessing step on a QuadList?
> 
> E.g. make a modified version drawLayers that walks the tree but instead of drawing, for every drawCall, puts a quad into a QuadBuffer:
> struct DrawQuadInfo {FloatQuad screenspaceQuad (with depth), bool textureChanged, FloatQuad changedRectInsideTextureProjectedToScreenspace}
> 
> Then, before drawlayers, build this quadlist. Remember the last frame's quadlist as well. Walk the list, if any quads have moved or their textures changed, use that to derive screenspace scissor rect. Then set the scissor and run the drawLayers.
> 

I think this is analogous to what the patch is already doing.  If we follow your approach, we would need to add scissor rects and transforms to this QuadBuffer structure, because that is the actual information we need to know if the layer's quad has changed.  Eventually then, it becomes two loops, like you said: (1) to determine what goes into the quad buffer, (2) to actually draw things in the quad buffer.

In this patch, those two loops already exist:

(1) LayerRendererChromium::computeCompositorScissorRect walks through all layers to determine what their actual draw rect will be.  (this is not quite true in my current patch, but it is exactly poised for that role, e.g. computeAllScissorRects)

and

(2) the existing original nested loop in drawLayersInternal, which actually issues the drawQuad commands.  Once we remove the redundancy from this loop, it does become essentially a quick walk through all layers to determine what gets drawn, and should not be any less efficient than walking a separate quad buffer.

It seems to me that if we take both approaches through to completion, after cleaning away unnecessary quad buffer storage in your approach and redundant code in my approach, we'd end up with essentially the same solution.

Thoughts?

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