Bug 73059 – [chromium] Refactor tile drawing to be more data-driven

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Adrienne Walker]

CCLayerImpl-derived classes all currently handle drawing themselves internally, making direct GL calls.  This is problematic for several reasons.  Testing these layers is tricky because testing must be done at a mock GL interface level rather than at a higher abstract level.  It makes culling closely dependent on how each layer draws.  It also makes some optimizations like layer squashing impossible.

To fix this, CCLayerImpl-derived classes should produce a set of quads to render with some basic information in them about how they are to be drawn.  This list can then be processed and drawn in a more data-driven and abstract manner.

[Adrienne Walker]

Created attachment 116684 [details]
Patch

[Adrienne Walker]

This patch is a work in progress, and is not rebaselined against ToT.  It's rough around the edges, but it passes LayoutTests/compositing, barring a few small pixel differences on four tests (due to texture filtering, also scrollbars).

Despite trying to minimize what I touched, there's a lot of diffs in this patch so here's a brief overview.

CCRenderPass is a new class created to hold a surface and a set of quads to draw. CCLayerImpl-derived classes now have a fillRenderPass function that adds quads to be drawn to the pass.  CCLayerTreeHostImpl is now responsible for calling calculateDrawTransformsEtc and traverses the tree to generate passes.  LayerRendererChromium now only draws a render pass.  A begin/end frame function on LayerRendererChromium sets up state properly when drawing.  A future patch could abstract this into a better CCRenderer interface for testing purposes.

Most of the drawing code from CCTiledLayerImpl moved to a CCTileQuadRenderer class.  I also refactored it to have fewer conditionals, (hopefully) to be more straightforward, and to be robust to skipped or culled quads.  This class is used for tiled content.  Other "single tile" layers still call CCLayerImpl::draw, but future patches could easily disassociate quad setup from drawing code in other layer types.

I'm not really happy with CCRenderPass itself, and would happily take suggestions.  One possibility would be to make QuadInfo virtual and then have CCTileQuadRenderer become a derived TiledQuadInfo class.  Testing or drawing multiple layers at once would then require upcasting using the material property as an RTTI hint to get at the derived data.  That's not that great, but maybe that's cleaner than the struct that's there now.

The other thing that seems a little ugly to me is that there's some work we do for every layer (e.g. state setting, debug border drawing) rather than per quad.  Right now I'm detecting this when the layer pointer changes, but maybe this needs to be more explicit? I'm not sure how to easily store that in CCRenderPass.  I'll happily take suggestions here (or on any of this).

[Shawn Singh]

Can we discuss offline the direction that CCRenderPass will go?

I'm OK with intermediate versions along the way, but the design I thought we had agreed on was to hide the generation of quads to the internals of CCRenderPass.  The interface provided by CCRenderPass would have been to encapsulate the WTF::Vector layerList, and the conversion from layers to quads would happen inside CCRenderPass::drawLayersOntoTargetSurface

So I wonder if what we really want is a CCQuadList class, and that class would be the object that is passed around, ideally without ever being stored explicitly?

[W. James MacLean]

(In reply to comment #2)
> This patch is a work in progress, and is not rebaselined against ToT.  It's rough around the edges, but it passes LayoutTests/compositing, barring a few small pixel differences on four tests (due to texture filtering, also scrollbars).
> 
> Despite trying to minimize what I touched, there's a lot of diffs in this patch so here's a brief overview.
> 
> CCRenderPass is a new class created to hold a surface and a set of quads to draw. CCLayerImpl-derived classes now have a fillRenderPass function that adds quads to be drawn to the pass.  CCLayerTreeHostImpl is now responsible for calling calculateDrawTransformsEtc and traverses the tree to generate passes.  LayerRendererChromium now only draws a render pass.  A begin/end frame function on LayerRendererChromium sets up state properly when drawing.  A future patch could abstract this into a better CCRenderer interface for testing purposes.
> 
> Most of the drawing code from CCTiledLayerImpl moved to a CCTileQuadRenderer class.  I also refactored it to have fewer conditionals, (hopefully) to be more straightforward, and to be robust to skipped or culled quads.  This class is used for tiled content.  Other "single tile" layers still call CCLayerImpl::draw, but future patches could easily disassociate quad setup from drawing code in other layer types.
> 
> I'm not really happy with CCRenderPass itself, and would happily take suggestions.  One possibility would be to make QuadInfo virtual and then have CCTileQuadRenderer become a derived TiledQuadInfo class.  Testing or drawing multiple layers at once would then require upcasting using the material property as an RTTI hint to get at the derived data.  That's not that great, but maybe that's cleaner than the struct that's there now.
> 
> The other thing that seems a little ugly to me is that there's some work we do for every layer (e.g. state setting, debug border drawing) rather than per quad.  Right now I'm detecting this when the layer pointer changes, but maybe this needs to be more explicit? I'm not sure how to easily store that in CCRenderPass.  I'll happily take suggestions here (or on any of this).

A couple of thoughts:

1) I agree that the quad transform would be best referenced from the layer if possible in this architecture. It seems wasteful to carry along a copy for every quad.

2) Could the check for transform invertability could be done once during CCTileQuadRenderer::beginLayer, and used to fast-forward over all quads for that layer if it's not invertible?

3) Regarding the work done when layers change, would it be reasonable to create a special QuadInfo type (CCRenderPass::TiledContentLayer?) to mark the start of each layer? Seeing this special quad would trigger the per-layer work, and it could contain the layer's visible rect and other useful info. It might be useful to have each TiledQuad contain a pointer to this QuadInfo for quick lookup if necessary.

4) I'm guessing a "SolidQuad" and a "ColoredQuad" are the same? Can drawing solid quads be made part of CCTileQuadRenderer, would that simplify things by keeping the drawing of quads all in one place?

5) I agree that drawing custom layers as single, large tiles would be a useful simplification in future.

(In reply to comment #3)

"Can we discuss offline the direction that CCRenderPass will go?" - For the sake of those not in MTV, I'd like to at least see a summary of any such discussions posted here :-)

[Adrienne Walker]

(In reply to comment #3)

> I'm OK with intermediate versions along the way, but the design I thought we had agreed on was to hide the generation of quads to the internals of CCRenderPass.  The interface provided by CCRenderPass would have been to encapsulate the WTF::Vector layerList, and the conversion from layers to quads would happen inside CCRenderPass::drawLayersOntoTargetSurface

Can you explain what you're proposing more? What does hiding the generation of quads inside CCRenderPass rather than CCLayerTreeHostImpl buy us?

In some previous discussions, folks mentioned a number of optimizations (culling, layer smashing, maybe there was a third?) that processed a list of quads.  Because of that, I figured that the list of quads should be mostly a bucket of data, and CCLayerTreeHostImpl should encapsulate any layer tree walking.  I think it'll be easier to test quad generation and those optimizations if the quad list is exposed and isn't smart.

> So I wonder if what we really want is a CCQuadList class, and that class would be the object that is passed around, ideally without ever being stored explicitly?

How does a CCQuadList differ from CCRenderPass::m_quads? I don't follow what you're proposing.

(In reply to comment #4)
> A couple of thoughts:
> 
> 1) I agree that the quad transform would be best referenced from the layer if possible in this architecture. It seems wasteful to carry along a copy for every quad.

Definitely.  There was no intention of that being in the final version--I just wanted to get a patch up so that we could start talking about this, and that detail was extraneous.

> 2) Could the check for transform invertability could be done once during CCTileQuadRenderer::beginLayer, and used to fast-forward over all quads for that layer if it's not invertible?

Good point.  Perhaps that check should get moved to calculateDrawEtcEtcEtc.

> 3) Regarding the work done when layers change, would it be reasonable to create a special QuadInfo type (CCRenderPass::TiledContentLayer?) to mark the start of each layer? Seeing this special quad would trigger the per-layer work, and it could contain the layer's visible rect and other useful info. It might be useful to have each TiledQuad contain a pointer to this QuadInfo for quick lookup if necessary.

I think I was trying to make culling simpler, by just letting it look at the quad list and not have to worry about layers.  Here are a couple of ways I considered making layers could be more explicit, but that complicated culling:

(1) If I had a separate array of layer info that contained offsets into the quad list for which quads came from which layer, then you'd have to update those offsets when removing quads.
(2) If you had special quads in the quad list, you'd have to remove them if you removed all the quads for that layer.
(3) If you added a flag to mark the first quad of a layer, you'd have to carefully move that flag to other quads that if you removed that quad.

I also considered adding a separate "Vector<LayerInfo>" that contained all the layer data directly, but the extra copy of layer data didn't really add anything.  I ended up just pushing a few extra properties (swizzle, antialiased edges) onto the base class instead.  If we ever make render surfaces their own layer type, they can reuse the antialiased edge flag.
 
> 4) I'm guessing a "SolidQuad" and a "ColoredQuad" are the same? Can drawing solid quads be made part of CCTileQuadRenderer, would that simplify things by keeping the drawing of quads all in one place?

Yes.  See also: rough around the edges.

I don't feel strongly about where the drawing code should go.  Maybe CCTileQuadRenderer => CCQuadRenderer and we keep all the quad drawing code there? I made CCTileQuadRenderer its own class because it needs to calculate data per layer (like device transforms and inflated quads for antialiased edges) and I wanted to encapsulate that complexity.

> 5) I agree that drawing custom layers as single, large tiles would be a useful simplification in future.

Yeah.  Other than maybe YUV video, I would suspect that we could probably combine a number of shaders in the future.  Or, at least draw different layers with the same set of shaders.

> (In reply to comment #3)
> 
> "Can we discuss offline the direction that CCRenderPass will go?" - For the sake of those not in MTV, I'd like to at least see a summary of any such discussions posted here :-)

Definitely.

[W. James MacLean]

(In reply to comment #5)
> > 3) Regarding the work done when layers change, would it be reasonable to create a special QuadInfo type (CCRenderPass::TiledContentLayer?) to mark the start of each layer? Seeing this special quad would trigger the per-layer work, and it could contain the layer's visible rect and other useful info. It might be useful to have each TiledQuad contain a pointer to this QuadInfo for quick lookup if necessary.
> 
> I think I was trying to make culling simpler, by just letting it look at the quad list and not have to worry about layers.  Here are a couple of ways I considered making layers could be more explicit, but that complicated culling:
> 
> (1) If I had a separate array of layer info that contained offsets into the quad list for which quads came from which layer, then you'd have to update those offsets when removing quads.
> (2) If you had special quads in the quad list, you'd have to remove them if you removed all the quads for that layer.

Good point. Although, if you left them in, e.g. two special layer quads side by side, it would still work fine (sort of a no-op).

> (3) If you added a flag to mark the first quad of a layer, you'd have to carefully move that flag to other quads that if you removed that quad.

Yes, I specifically didn't want to propose a "first tile" flag for this reason.

> I also considered adding a separate "Vector<LayerInfo>" that contained all the layer data directly, but the extra copy of layer data didn't really add anything.  I ended up just pushing a few extra properties (swizzle, antialiased edges) onto the base class instead.  If we ever make render surfaces their own layer type, they can reuse the antialiased edge flag.
> 

For culling, at least in the first iterations, it will be useful to compare tiles (quads) against entire layers, as this simplifies testing - one large rect instead of the union of a bunch of rects.

This could be done by using the layer pointer in QuadInfo's, but it would be inefficient as you'd check the same layer multiple times.

In a more efficient (aka later) version of the culler, layers might not be needed (just build up the coverage rect from quads in reverse draw order, although this won't always be representable by a single rect).

[Adrienne Walker]

(In reply to comment #6)
> (In reply to comment #5)

> > (2) If you had special quads in the quad list, you'd have to remove them if you removed all the quads for that layer.
> 
> Good point. Although, if you left them in, e.g. two special layer quads side by side, it would still work fine (sort of a no-op).

There's work that gets done per layer (like turning on states and drawing debug borders), so it's not entirely a no-op even if that work is fairly negligible.

Also, any quad list processor would have to be aware of those special quads, e.g. the culler would have to be sure not to cull them.

> For culling, at least in the first iterations, it will be useful to compare tiles (quads) against entire layers, as this simplifies testing - one large rect instead of the union of a bunch of rects.
> 
> This could be done by using the layer pointer in QuadInfo's, but it would be inefficient as you'd check the same layer multiple times.

In the first iteration, you could rebuild a list of layers from the quad list, or just skip the layer if you've already checked it.  The quad list is necessarily sorted by layer, so it'd be an easy check.  Is that plausible?

> In a more efficient (aka later) version of the culler, layers might not be needed (just build up the coverage rect from quads in reverse draw order, although this won't always be representable by a single rect).

I'm definitely hoping that you can take an eye to this patch with where culling is in the short term and will be in the long term, and sanity check that it's in line with your plans.

[Dana Jansens]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> 
> > > (2) If you had special quads in the quad list, you'd have to remove them if you removed all the quads for that layer.
> > 
> > Good point. Although, if you left them in, e.g. two special layer quads side by side, it would still work fine (sort of a no-op).
> 
> There's work that gets done per layer (like turning on states and drawing debug borders), so it's not entirely a no-op even if that work is fairly negligible.

Can you check if the next one is also a layer and do nothing when true?

> Also, any quad list processor would have to be aware of those special quads, e.g. the culler would have to be sure not to cull them.

Can this be done in an iterator? So you'd have a quad iterator and a quad+layer iterator.

[W. James MacLean]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> 
> > > (2) If you had special quads in the quad list, you'd have to remove them if you removed all the quads for that layer.
> > 
> > Good point. Although, if you left them in, e.g. two special layer quads side by side, it would still work fine (sort of a no-op).
> 
> There's work that gets done per layer (like turning on states and drawing debug borders), so it's not entirely a no-op even if that work is fairly negligible.
> 
> Also, any quad list processor would have to be aware of those special quads, e.g. the culler would have to be sure not to cull them.
> 
> > For culling, at least in the first iterations, it will be useful to compare tiles (quads) against entire layers, as this simplifies testing - one large rect instead of the union of a bunch of rects.
> > 
> > This could be done by using the layer pointer in QuadInfo's, but it would be inefficient as you'd check the same layer multiple times.
> 
> In the first iteration, you could rebuild a list of layers from the quad list, or just skip the layer if you've already checked it.  The quad list is necessarily sorted by layer, so it'd be an easy check.  Is that plausible?

Certainly :-)

> > In a more efficient (aka later) version of the culler, layers might not be needed (just build up the coverage rect from quads in reverse draw order, although this won't always be representable by a single rect).
> 
> I'm definitely hoping that you can take an eye to this patch with where culling is in the short term and will be in the long term, and sanity check that it's in line with your plans.

Indeed ... there's nothing here at present that seems like a show-stopper for culling.

[W. James MacLean]

(In reply to comment #8)
> 
> Can this be done in an iterator? So you'd have a quad iterator and a quad+layer iterator.

Actually, I like the iterator idea: if each QuadInfo carried an iterator into the layerList, it would be easy for a quad to do layer-based coverage operations on the subsequent layers (or, if we're walking the quad list backwards for culling, know efficiently when to add the next layer to the coverage map).

If Quads themselves are based on iterators, it would be nice to have bi-directional ones.

[Adrienne Walker]

Created attachment 118333 [details]
Round 2

[Adrienne Walker]

Sorry to take so long to get back to this.

This patch got a lot larger after some design brainstorming with Nat, Shawn, and James about how to store data in CCRenderPass.  After some back and forth, we decided that we should create a bit of shared layer state (which I named CCSharedLayerState).  This seems better than having pointers to layers directly and it keeps quads divorced from the layers that generate them.  It also seems better than making a "fat" quad that contains all of that data directly.

Nat made the argument that optimizing for state setting is silly, so rather than detecting when layers change, we just set up state on a per-quad basis.  So, rather than creating an iterator that's smart about detecting layer changes, we just have a single ordered quad list, and that's it.  I think that's much simpler for everyone.

And, rather than having each quad contain all the data for all quad types, there's a derived quad type to hold additional pieces of data (e.g. texture data for tile quads).  Each different material gets its own CCDrawQuad-derived class, but these classes contain no virtual functions and are just a bag of data.  The CCDrawQuad class is essentially just a glorified union.  In LayerRendererChromium, the material enum on a quad is a roll-your-own-RTTI mechanism, and we upcast to the derived type using that.  Oh, C++ boilerplate.

Debug borders are now specified as quads themselves; I forgot who made that suggestion, but it's a great idea.

In the short term, it was easier to have one quad type that had a hacky backpointer to its layer in order to draw than moving all drawing functions to LayerRendererChromium.  These layer types are all "one quad" layers, so culling can proceed right after this patch without needing that refactoring.

If this patch is too big, I think I could probably separate out moving code out of LayerRendererChromium and into CCLayerTreeHostImpl into a different patch.

There are a few pixel differences in three layout tests, largely due to the fact that I'm not using the AA shader on non-edge quads, but tests otherwise pass.  I do need to mark those as failing still.

[Vangelis Kokkevis]

Comment on attachment 118333 [details]
Round 2

I like where this patch is going!
Have you considered converting the RenderPass into just another type of CCDrawQuad which itself contains a CCQuadList?  That way, the entire scene could be rendered by traversing the top level CCQuadLIst instead of having to loop through RenderPasses.  Conceptually, what you call a RenderPass is really just another DrawQuad with a special rule on how it draws its contents. One of the advantages is that we won't need to duplicate the loop over RenderPasses every time we need to traverse all the DrawQuads.  For example, if we wanted to implement culling of completely occluded Quads, we could simply traverse the top level CCQuadList backwards.  Any RenderSurface that's completely occluded can be marked as skipped and we won't have to ever draw its contents into an intermediate texture.

[WebKit Review Bot]

Comment on attachment 118333 [details]
Round 2

Attachment 118333 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10790229

New failing tests:
compositing/geometry/vertical-scroll-composited.html
compositing/color-matching/image-color-matching.html
compositing/scaling/tiled-layer-recursion.html

[Adrienne Walker]

(In reply to comment #13)
> (From update of attachment 118333 [details])
> Have you considered converting the RenderPass into just another type of CCDrawQuad which itself contains a CCQuadList?  That way, the entire scene could be rendered by traversing the top level CCQuadList instead of having to loop through RenderPasses.  Conceptually, what you call a RenderPass is really just another DrawQuad with a special rule on how it draws its contents. One of the advantages is that we won't need to duplicate the loop over RenderPasses every time we need to traverse all the DrawQuads.  For example, if we wanted to implement culling of completely occluded Quads, we could simply traverse the top level CCQuadList backwards.  Any RenderSurface that's completely occluded can be marked as skipped and we won't have to ever draw its contents into an intermediate texture.

I'm not convinced that iteration becomes simpler if we go to a full tree structure rather than a vector of vector of leaf nodes.  I think it moves complexity around, because occlusion and drawing orders are so different.

A (root RS) - B - C (RS) - D
              |
              + - E - F

In the above tree, A and C have render surfaces.  E is a sibling of C and draws after C.  From the perspective of efficient drawing, we want to first bind C as a target, draw D, then bind A, draw B, C as a source, E, and F.  From the perspective of occlusion, we could do what you say and walk the tree backwards.  F could occlude E.  E could occlude C and D.

I think iteration to draw would be complicated in that scheme.  You'd have to walk through the tree backwards to find render surfaces, and then forwards through each of those render surfaces.  And, you'd have to treat C differently in each case, kind of like we do now with layers with surfaces that act as target and source?

It would be helpful if I had any sense of where drawing occlusion is going to be going in the future.  Could we just build up a set of occluders during a pre-draw tree walk and then look up in some efficient data structure if a given layer is going to be occluded right before we draw it? If so, then I think the current scheme is conceptually simpler for both culling and drawing.

It's an excellent point that we need some mechanism to inform surfaces-as-targets that their surfaces-as-sources have been occluded.  If we had such a pre-built occlusion data structure, we could simply store a pointer or copy of the CCRenderSurfaceDrawQuad that corresponds to the CCRenderPass in the CCRenderPass itself and do an occlusion check before we bind and draw into it.

[James Robinson]

Comment on attachment 118333 [details]
Round 2

View in context: https://bugs.webkit.org/attachment.cgi?id=118333&action=review

> Source/WebCore/platform/graphics/chromium/NonCompositedContentHost.cpp:47
> +    m_graphicsLayer->setOpacity(1);
> +    m_graphicsLayer->setContentsOpaque(true);

i think this should be (or possibly already is?) a separate patch. for one it'll conflict with my moving of NCCH.cpp

> Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.cpp:47
> +    return reinterpret_cast<const CCDebugBorderDrawQuad*>(this);

static_cast<> is better than reinterpret_cast<> here, I think

> Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.h:78
> +    RefPtr<CCSharedQuadState> m_sharedQuadState;

I would strongly prefer that this not be reference counted. Instead, the owner of the set of quads that share the data should have single ownership of the shared quad state. This could live inside CCRenderPass or QuadList itself could be promoted to a class to own both the list of quads and a list of shared state used by those quads. Or, if we want to share this state across multiple passes, define a class that owns the set of CCRenderPass or QuadLists used for a certain frame and have it own the list of CCSharedQuadStates.

We don't need these objects to live across frames, since we regenerate transforms and stuff every frame, so they definitely can have a bounded lifetime.

[Dana Jansens]

(In reply to comment #16)
> (From update of attachment 118333 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118333&action=review
> 
> > Source/WebCore/platform/graphics/chromium/NonCompositedContentHost.cpp:47
> > +    m_graphicsLayer->setOpacity(1);
> > +    m_graphicsLayer->setContentsOpaque(true);
> 
> i think this should be (or possibly already is?) a separate patch. for one it'll conflict with my moving of NCCH.cpp

Looks a lot like what https://bugs.webkit.org/show_bug.cgi?id=70564 was doing?

[Adrienne Walker]

(In reply to comment #16)
> (From update of attachment 118333 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118333&action=review
> 
> > Source/WebCore/platform/graphics/chromium/NonCompositedContentHost.cpp:47
> > +    m_graphicsLayer->setOpacity(1);
> > +    m_graphicsLayer->setContentsOpaque(true);
> 
> i think this should be (or possibly already is?) a separate patch. for one it'll conflict with my moving of NCCH.cpp

Oops.  I meant to remove that after Dana's opacity patch landed.  Fixed.

> > Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.cpp:47
> > +    return reinterpret_cast<const CCDebugBorderDrawQuad*>(this);
> 
> static_cast<> is better than reinterpret_cast<> here, I think

Not sure why I did that.  Fixed.

> > Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.h:78
> > +    RefPtr<CCSharedQuadState> m_sharedQuadState;
> 
> I would strongly prefer that this not be reference counted. Instead, the owner of the set of quads that share the data should have single ownership of the shared quad state. This could live inside CCRenderPass or QuadList itself could be promoted to a class to own both the list of quads and a list of shared state used by those quads. Or, if we want to share this state across multiple passes, define a class that owns the set of CCRenderPass or QuadLists used for a certain frame and have it own the list of CCSharedQuadStates.
> 
> We don't need these objects to live across frames, since we regenerate transforms and stuff every frame, so they definitely can have a bounded lifetime.

I started down that path, but using a RefPtr seemed less prone to failure, because a layer creates this information, the CCRenderPass has to store it, and then we hand it off to quads as a raw pointer, with the hopes that it's actually owned.  Maybe I can structure functions in such a way where this isn't possible.  I'll give it a shot.

[Adrienne Walker]

Created attachment 118477 [details]
Remove RefPtr, rebase, etc

[Shawn Singh]

Comment on attachment 118477 [details]
Remove RefPtr, rebase, etc


This patch seems really great to me. I have only a few minor comments.

FYI I did not look closely at the tiler / shader portions of the patch.  I focused primarily on the CCLayerTreeHost, LayerRendererChromium, the CCDrawQuad classes.


View in context: https://bugs.webkit.org/attachment.cgi?id=118477&action=review

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:171
> +    void drawDebugBorderQuad(const CCDebugBorderDrawQuad*);
> +    void drawRenderSurfaceQuad(const CCRenderSurfaceDrawQuad*);
> +    void drawSolidColorQuad(const CCSolidColorDrawQuad*);
> +    void drawTileQuad(const CCTileDrawQuad*);
> +    void drawCustomLayerQuad(const CCCustomLayerDrawQuad*);

Perhaps we could make these static functions, and we pass the context as an arg to them?  My reasoning:  (1) making them static would clearly indicate that they are stateless helper functions.  (2) would help separate them from drawQuad, which should not be static, and lives in a different level of abstraction.  But if there's a reason to keep them non-static, thats OK too.

> Source/WebCore/platform/graphics/chromium/cc/CCCustomLayerDrawQuad.h:36
> +class CCCustomLayerDrawQuad : public CCDrawQuad {

Could we add a FIXME comment reminding ourselves that this class is only going to be in the code for a while, as we migrate/refactor?

> Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.h:39
> +// CCDrawQuad is a bag of data used for drawing a quad. Because different

The word "Draw" could make things confusing down the road, because it is technically verb and we're using it as part of a noun phrase, "draw quad".  Instead, could we just call it CCQuad? or, if we really need, could we say CCDrawableQuad?   (I prefer CCQuad)

> Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.h:43
> +class CCDrawQuad {

I think we should plan in advance to make this object serializable, so that we can use lists of quads as an output (or input) for integration tests.  That might mean we should consider using virtual inheritance and make the CCDrawQuad a pure virtual interface?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:171
> +    renderSurfaceLayerList.append(rootLayer());
> +
> +    if (!rootLayer()->renderSurface())
> +        rootLayer()->createRenderSurface();
> +    rootLayer()->renderSurface()->clearLayerList();
> +    rootLayer()->renderSurface()->setContentRect(IntRect(IntPoint(), viewportSize()));
> +
> +    rootLayer()->setClipRect(IntRect(IntPoint(), viewportSize()));

Maybe in a future patch, we could make this "special root initialization" disappear, fold it into calculateDrawTransformsAndVisibility.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:204
> +        FloatRect surfaceDamageRect;
> +        if (layerRendererCapabilities().usingPartialSwap) {
> +            // For now, we conservatively use the root damage as the damage for
> +            // all surfaces, except perspective transforms.
> +            TransformationMatrix screenSpaceTransform = computeScreenSpaceTransformForSurface(renderSurfaceLayer);
> +            if (screenSpaceTransform.hasPerspective()) {
> +                // Perspective projections do not play nice with mapRect of
> +                // inverse transforms. In this uncommon case, its simpler to
> +                // just redraw the entire surface.
> +                // FIXME: use calculateVisibleRect to handle projections.
> +                surfaceDamageRect = renderSurface->contentRect();
> +            } else {
> +                TransformationMatrix inverseScreenSpaceTransform = screenSpaceTransform.inverse();
> +                surfaceDamageRect = inverseScreenSpaceTransform.mapRect(rootDamageRect);
> +            }
> +        }

In retrospect, I think this is good code to make into a separate function computeDamageInSurfaceSpace() - would it be OK with you to do that in this patch please?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:213
> +            if (layer->renderSurface() && layer->renderSurface() != renderSurface) {

We should probably use the helper function, renderSurfaceContributesToTarget(), defined in CCLayerTreeHostCommon.h instead of this.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:225
>  void CCLayerTreeHostImpl::drawLayers()

I REALLY like how this function turned out.  I think you found very satisfying abstractions to provide in the layerRenderer object. =)

[James Robinson]

I really like the CCQuad idea as well, FWIW

[Adrienne Walker]

Comment on attachment 118477 [details]
Remove RefPtr, rebase, etc

View in context: https://bugs.webkit.org/attachment.cgi?id=118477&action=review

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:171
>> +    void drawCustomLayerQuad(const CCCustomLayerDrawQuad*);
> 
> Perhaps we could make these static functions, and we pass the context as an arg to them?  My reasoning:  (1) making them static would clearly indicate that they are stateless helper functions.  (2) would help separate them from drawQuad, which should not be static, and lives in a different level of abstraction.  But if there's a reason to keep them non-static, thats OK too.

I need a bunch of extra information from the layer renderer, like projection matrix and window matrix and all of the shader programs.  I could make them all const, if that would help.

>> Source/WebCore/platform/graphics/chromium/cc/CCCustomLayerDrawQuad.h:36
>> +class CCCustomLayerDrawQuad : public CCDrawQuad {
> 
> Could we add a FIXME comment reminding ourselves that this class is only going to be in the code for a while, as we migrate/refactor?

Will do.

>> Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.h:39
>> +// CCDrawQuad is a bag of data used for drawing a quad. Because different
> 
> The word "Draw" could make things confusing down the road, because it is technically verb and we're using it as part of a noun phrase, "draw quad".  Instead, could we just call it CCQuad? or, if we really need, could we say CCDrawableQuad?   (I prefer CCQuad)

I thought about CCQuad, but decided to call it CCDrawQuad to differentiate from CCLayerQuad or FloatQuad, which are all very different things.  I can make it CCQuad if that's what other people want and they don't think it'd be confused with CCLayerQuad.

>> Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.h:43
>> +class CCDrawQuad {
> 
> I think we should plan in advance to make this object serializable, so that we can use lists of quads as an output (or input) for integration tests.  That might mean we should consider using virtual inheritance and make the CCDrawQuad a pure virtual interface?

I don't follow.  Why does this object need to be serialized? How would a pure virtual interface help with that? If necessary, can it wait until a later patch?

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:171
>> +    rootLayer()->setClipRect(IntRect(IntPoint(), viewportSize()));
> 
> Maybe in a future patch, we could make this "special root initialization" disappear, fold it into calculateDrawTransformsAndVisibility.

Definitely.  I feel like the root layer / root surface needs to be less special in a number of ways.  This is one.  Clear color is another.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:204
>> +        }
> 
> In retrospect, I think this is good code to make into a separate function computeDamageInSurfaceSpace() - would it be OK with you to do that in this patch please?

Will do.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:213
>> +            if (layer->renderSurface() && layer->renderSurface() != renderSurface) {
> 
> We should probably use the helper function, renderSurfaceContributesToTarget(), defined in CCLayerTreeHostCommon.h instead of this.

Excellent point.  I'm not sure where I picked this code up from.  I'll fix it.  (Hopefully it can go away soon.)

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:225
>>  void CCLayerTreeHostImpl::drawLayers()
> 
> I REALLY like how this function turned out.  I think you found very satisfying abstractions to provide in the layerRenderer object. =)

I'm glad you feel that way.  I know it was a point of contention before.  :)

[James Robinson]

Gah, forgot about CCLayerQuad.  CCDrawQuad sounds OK....

[Adrienne Walker]

Created attachment 118624 [details]
Address Shawn's comments

[Shawn Singh]

I was imagining that we need to serialize (as opposed to programmatically verify conditions with gtest) so that we have an output that we can also visualize when trying to debug.  If we do want to serialize, it seemed cleaner to virtualize the function, so that we don't end up with several different wrapper functions like drawQuad that have a switch statement.

Its OK with me to deal with this in a separate patch.

[Adrienne Walker]

(In reply to comment #24)
> Created an attachment (id=118624) [details]
> Address Shawn's comments

I couldn't make the draw functions const, because they're changing stuff on the graphics context.  So, I left that as-is.

I added the fixme, the use of that templated contributes to surface function, and moved the surface damage rect to its own function.

[Dana Jansens]

*** Bug 72965 has been marked as a duplicate of this bug. ***

[Nat Duca]

Comment on attachment 118624 [details]
Address Shawn's comments

Did you give any thought to moving functions that you're adding to LayerRendererChromium to cc/CCRendererChromiumGL or something and make that new class inherit from LRC temporarily? That way we can start (slowly but surely) moving this file to its correct home.

[James Robinson]

(In reply to comment #28)
> (From update of attachment 118624 [details])
> Did you give any thought to moving functions that you're adding to LayerRendererChromium to cc/CCRendererChromiumGL or something and make that new class inherit from LRC temporarily? That way we can start (slowly but surely) moving this file to its correct home.

I think what we'll actually do is once we've moved all of the logic that isn't GL-specific out of LRC then we'll just rename LRC->CCRendererGL or something like that. With that as a migration path, adding new GL-specific functionality to LRC is fine.  By that logic drawRenderPass() could conceivable go outside of LRC but the rest looks pretty GL-specific.

[James Robinson]

Comment on attachment 118624 [details]
Address Shawn's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=118624&action=review

> Source/WebCore/platform/graphics/chromium/ShaderChromium.h:150
> +    int alphaLocation() const { return -1; }

are these supposed to be virtual? I see that, for instance, FragmentTexAlphaBinding shadows this function but doesn't override it. since we normally use templates to dispatch callsites will statically resolve to the right function, but then i don't understand what the purpose if this base class is or why it has member functions

[Adrienne Walker]

(In reply to comment #30)
> (From update of attachment 118624 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118624&action=review
> 
> > Source/WebCore/platform/graphics/chromium/ShaderChromium.h:150
> > +    int alphaLocation() const { return -1; }
> 
> are these supposed to be virtual? I see that, for instance, FragmentTexAlphaBinding shadows this function but doesn't override it. since we normally use templates to dispatch callsites will statically resolve to the right function, but then i don't understand what the purpose if this base class is or why it has member functions

Hmm. They should probably be virtual.  It's not like we ever call anything from a base class pointer, but overriding is better than shadowing.

The reason is to simplify all tile shader programs to have the same set of uniform getters, regardless of whether they have antialiased edges or are opaque.  It let me write a single "collect uniforms" templated function in LayerRendererChromium and then we can avoid templatizing anything else from then on out, rather than having the whole drawTiles() function be templatized, like it used to be.  I thought the resulting code ended up simpler.

[Adrienne Walker]

Created attachment 118816 [details]
Clean up quad opacity/opaque

[Adrienne Walker]

(In reply to comment #32)
> Created an attachment (id=118816) [details]
> Clean up quad opacity/opaque

Dana mentioned that CCDrawQuad::isOpaque should be called something different if it's going to combine opacity=1 and contents opaque.  So, I renamed it drawsOpaque() and cleaned up how derived classes flagged that they were transparent.

[Adrienne Walker]

Created attachment 119118 [details]
Fix ToT merge conflicts

[James Robinson]

(In reply to comment #31)
> (In reply to comment #30)
> > (From update of attachment 118624 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=118624&action=review
> > 
> > > Source/WebCore/platform/graphics/chromium/ShaderChromium.h:150
> > > +    int alphaLocation() const { return -1; }
> > 
> > are these supposed to be virtual? I see that, for instance, FragmentTexAlphaBinding shadows this function but doesn't override it. since we normally use templates to dispatch callsites will statically resolve to the right function, but then i don't understand what the purpose if this base class is or why it has member functions
> 
> Hmm. They should probably be virtual.  It's not like we ever call anything from a base class pointer, but overriding is better than shadowing.
> 
> The reason is to simplify all tile shader programs to have the same set of uniform getters, regardless of whether they have antialiased edges or are opaque.  It let me write a single "collect uniforms" templated function in LayerRendererChromium and then we can avoid templatizing anything else from then on out, rather than having the whole drawTiles() function be templatized, like it used to be.  I thought the resulting code ended up simpler.

I don't see any code that actually uses the FragmentTileBindingBase class directly. Does it just exist to make sure that there's a definition for each of the *Location() getters so the template instantiation succeeds for each program type? I'm nervous because this depends on getting the _static_ type of the Program* passed to tileUniformLocation() correct. If someone gets the static type wrong by doing something innocuous looking like implicitly converting a pointer to a pointer to the base class and then passes it to this function everything will compile fine and run without complaint but produce the wrong result.

I think I'd prefer adding getters that return -1 to FragmentTexAlphaBinding and FragmentTexOpaqueBinding. It means adding 5 getters instead of 4, but it cuts down one one class, so I think in terms of LoC it'll be a minor win.

[James Robinson]

Comment on attachment 119118 [details]
Fix ToT merge conflicts

View in context: https://bugs.webkit.org/attachment.cgi?id=119118&action=review

This looks awesome. R=me with some small comments.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:443
> +    renderMatrix.scale3d(layerRect.width(), layerRect.height(), 1);

nit: scaleNonUniform(layerRect.width(), layerRect.height()) would be strictly better, this is doing an unnecessary set of calculations on the 3rd column of the matrix. it doesn't matter much here since this isn't performance sensitive code but it's a good habit to get into.

> Source/WebCore/platform/graphics/chromium/cc/CCDebugBorderDrawQuad.cpp:43
> +    if (m_color.alpha() != 1)
> +        m_quadOpaque = false;

i'm pretty sure all debug border quads are non-opaque since we just draw a line loop around the edges. the middle is always empty

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:74
> +    PassOwnPtr<CCSharedQuadState> sharedQuadState() const;

nit: i think this should be called createSharedQuadState(). every call does some calculations and returns a new value so it shouldn't look like a simple getter

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:56
> +    CCRenderPass(CCRenderSurface*);

explicit

[Adrienne Walker]

(In reply to comment #36)

Thanks for the review.  Will fix all nits as requested.  I'll add a comment to the uniform getting function in case anybody adds a tile shader and hits a compile error there.

> > Source/WebCore/platform/graphics/chromium/cc/CCDebugBorderDrawQuad.cpp:43
> > +    if (m_color.alpha() != 1)
> > +        m_quadOpaque = false;
> 
> i'm pretty sure all debug border quads are non-opaque since we just draw a line loop around the edges. the middle is always empty

Hrm.  Do we need to differentiate "opaque for culling" vs. "opaque for drawing"? From a drawing sense, this "quad" is opaque and we should disable blending.  From a culling sense, this quad doesn't fill its entire contents.

On the other hand, these are *debug* quads though, so it'd probably be most simple to just combine the two opaque use cases, turning on blending for debug quads even though it's not needed.  I'll add a comment that it refers to both culling and drawing.

[W. James MacLean]

(In reply to comment #37)
> (In reply to comment #36)
> 
> Thanks for the review.  Will fix all nits as requested.  I'll add a comment to the uniform getting function in case anybody adds a tile shader and hits a compile error there.
> 
> > > Source/WebCore/platform/graphics/chromium/cc/CCDebugBorderDrawQuad.cpp:43
> > > +    if (m_color.alpha() != 1)
> > > +        m_quadOpaque = false;
> > 
> > i'm pretty sure all debug border quads are non-opaque since we just draw a line loop around the edges. the middle is always empty
> 
> Hrm.  Do we need to differentiate "opaque for culling" vs. "opaque for drawing"? From a drawing sense, this "quad" is opaque and we should disable blending.  From a culling sense, this quad doesn't fill its entire contents.
> 
> On the other hand, these are *debug* quads though, so it'd probably be most simple to just combine the two opaque use cases, turning on blending for debug quads even though it's not needed.  I'll add a comment that it refers to both culling and drawing.

It's probably not a huge deal either way. We can always turn off debug borders when doing performance measurements.

[James Robinson]

(In reply to comment #37)
> (In reply to comment #36)
> 
> Thanks for the review.  Will fix all nits as requested.  I'll add a comment to the uniform getting function in case anybody adds a tile shader and hits a compile error there.
> 
> > > Source/WebCore/platform/graphics/chromium/cc/CCDebugBorderDrawQuad.cpp:43
> > > +    if (m_color.alpha() != 1)
> > > +        m_quadOpaque = false;
> > 
> > i'm pretty sure all debug border quads are non-opaque since we just draw a line loop around the edges. the middle is always empty
> 
> Hrm.  Do we need to differentiate "opaque for culling" vs. "opaque for drawing"? From a drawing sense, this "quad" is opaque and we should disable blending.  From a culling sense, this quad doesn't fill its entire contents.
> 
> On the other hand, these are *debug* quads though, so it'd probably be most simple to just combine the two opaque use cases, turning on blending for debug quads even though it's not needed.  I'll add a comment that it refers to both culling and drawing.

That's a good point. I didn't think that there was a distinction when I saw this and assumed that 'opaque' meant 'this quad covers up every pixel within its bounds'. Maybe needs blending is a different bit?

[Dana Jansens]

(In reply to comment #39)
> That's a good point. I didn't think that there was a distinction when I saw this and assumed that 'opaque' meant 'this quad covers up every pixel within its bounds'. Maybe needs blending is a different bit?

Needs blending is basically the quad drawsOpaque() function right now, as it considers 'covers all pixels' but also things like translations. Should this be setting something that controls the outcome of that function rather than 'opaque'?

[Adrienne Walker]

(In reply to comment #40)
> (In reply to comment #39)
> > That's a good point. I didn't think that there was a distinction when I saw this and assumed that 'opaque' meant 'this quad covers up every pixel within its bounds'. Maybe needs blending is a different bit?
> 
> Needs blending is basically the quad drawsOpaque() function right now, as it considers 'covers all pixels' but also things like translations. Should this be setting something that controls the outcome of that function rather than 'opaque'?

That's a good point.  Quads with antialiased edges but opaque contents still should cull what's behind their transformed quad rect (insert hand waving about how to handle edge pixels and floating point inconsistencies).

[Adrienne Walker]

Created attachment 119304 [details]
Review comments, opaque/blending changes

[Adrienne Walker]

(In reply to comment #42)
> Created an attachment (id=119304) [details]
> Review comments, opaque/blending changes

I added an explicit needsBlending() function to CCDrawQuad.  drawsOpaque is now unused, but should be used for culling.  I think that's more reasonable.

jamesr, would you mind taking an additional look at the opaque/blending changes and R+'ing if that looks good to you?

[James Robinson]

Comment on attachment 119304 [details]
Review comments, opaque/blending changes

View in context: https://bugs.webkit.org/attachment.cgi?id=119304&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.cpp:43
> +    , m_quadOpaque(true)

i think this is the wrong default, at least for CCCustomLayerDrawQuads - I think they have to be treated as potentially non-opaque unless the layer provides information to the contrary.

> Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.cpp:44
> +    , m_needsBlending(false)

Same here, custom quads probably do need blending by default

although I guess you could also say that custom layers can/will enable blending and set the blend func as they need to

> Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.h:55
> +    bool needsBlending() const { return !m_sharedQuadState->isOpaque() || m_needsBlending || opacity() != 1; }
> +    bool isLayerAxisAlignedIntRect() const { return m_sharedQuadState->isLayerAxisAlignedIntRect(); }

you need blending on if you aren't an axis aligned IntRect too, regardless of opacity etc - right? i'm thinking of a rotated unscaled solid color layer or render surface, for instance - we'd want to use linear filtering and blending to get the edges to look OK. linear filtering w/out blending will look funky

> Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.h:88
> +    // By default, the shared quad state determines whether or not this quad is
> +    // opaque or needs blending. Derived classes can override with these
> +    // variables.
> +    bool m_quadOpaque;
> +    bool m_needsBlending;

these don't completely override what the shared quad state says, they're an additional input. is the idea here that we will use this to do things like enable blending only for the exterior tiles on a tiled layer that has edge AA?

[James Robinson]

Comment on attachment 119304 [details]
Review comments, opaque/blending changes

I think this does what we want, so R=me. I kind of hope that we can simplify the quad/shared/material bits a bit. I think we're in a much better place to do that with this patch than without.

[Adrienne Walker]

(In reply to comment #44)

> > Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.h:55
> > +    bool needsBlending() const { return !m_sharedQuadState->isOpaque() || m_needsBlending || opacity() != 1; }
> > +    bool isLayerAxisAlignedIntRect() const { return m_sharedQuadState->isLayerAxisAlignedIntRect(); }
> 
> you need blending on if you aren't an axis aligned IntRect too, regardless of opacity etc - right? i'm thinking of a rotated unscaled solid color layer or render surface, for instance - we'd want to use linear filtering and blending to get the edges to look OK. linear filtering w/out blending will look funky

Nope.  That's not how blending works.

The graphics pipeline is vertex shader => rasterization (determine which pixels are covered) => fragment shader => blending => final pixel.  If you want to blend, you have to output something in the alpha channel from the fragment shader.  You always get aliased edges on primitives regardless of blend state, unless you're using some form of AA that takes multiple samples per final pixel and combines them.

> > Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.h:88
> > +    // By default, the shared quad state determines whether or not this quad is
> > +    // opaque or needs blending. Derived classes can override with these
> > +    // variables.
> > +    bool m_quadOpaque;
> > +    bool m_needsBlending;
> 
> these don't completely override what the shared quad state says, they're an additional input. is the idea here that we will use this to do things like enable blending only for the exterior tiles on a tiled layer that has edge AA?

Yes, that's exactly it.

[Adrienne Walker]

Committed r102838: <http://trac.webkit.org/changeset/102838>

[David Reveman]

(In reply to comment #46)
> (In reply to comment #44)
> 
> > > Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.h:55
> > > +    bool needsBlending() const { return !m_sharedQuadState->isOpaque() || m_needsBlending || opacity() != 1; }
> > > +    bool isLayerAxisAlignedIntRect() const { return m_sharedQuadState->isLayerAxisAlignedIntRect(); }
> > 
> > you need blending on if you aren't an axis aligned IntRect too, regardless of opacity etc - right? i'm thinking of a rotated unscaled solid color layer or render surface, for instance - we'd want to use linear filtering and blending to get the edges to look OK. linear filtering w/out blending will look funky
> 
> Nope.  That's not how blending works.
> 
> The graphics pipeline is vertex shader => rasterization (determine which pixels are covered) => fragment shader => blending => final pixel.  If you want to blend, you have to output something in the alpha channel from the fragment shader.  You always get aliased edges on primitives regardless of blend state, unless you're using some form of AA that takes multiple samples per final pixel and combines them.

Hm, the AA method we use for tiled layers require blending as coverage and alpha channel value is computed in the fragment shader.

[Adrienne Walker]

(In reply to comment #48)

> Hm, the AA method we use for tiled layers require blending as coverage and alpha channel value is computed in the fragment shader.

Yeah, this patch addresses that.  Sorry if my comment was confusing.  I was trying to say that the blending decision is not simply:

    any non-axis aligned quad => needs blending

...and that it's instead the following chain of logic:

   non-axis aligned non-root tile quad => edge antialiasing => needs blending