Bug 70057 – [chromium] Refactor LayerRendererChromium drawing code.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

This patch (coming very soon) cleans up the core nested loop that draws layers and render surfaces.  The nested loop is split into two nested loops, and put into function calls:
   - one pass to determine what to draw (visibility rects, which layers to actually draw)
   - another pass to actually draw things.

This makes it much easier to test this code properly, and makes it cleaner to add some optimizations down the road.

[Shawn Singh]

Notes for reviewers:

(1) I'm proposing to leave the FIXME in the code for now, and we can create a bug for it.  The changes required for that should be done when testing is in place, and in a separate change.  It is too much of a side-track at the moment...

(2) I added an early-exit condition if the visibleLayerRect is empty.  Please verify.   It did not introduce any regressions in layout tests, and all existing unit tests still pass...

(3) The diff came out awkward, so here is a crisp list of changes I made to LayerRendererChromium.cpp:
    - removed the nested loop from drawLayersInternal
          - replaced it with calls to determineWhatToDraw() and drawLayersOntoRenderSurfaces()
    - removed drawLayer(...)
    - created determineWhatToDraw(...)
    - created drawLayersOntoRenderSurfaces()

[Shawn Singh]

Created attachment 110911 [details]
Patch

[Adrienne Walker]

Just FYI, it's fine to skip drawing a single layer if its visible rect is empty.  That's not enough information to skip its children, but it doesn't look like you're doing that.

I'll admit that I'm not really that keen on setting (more) ephemeral flags on layers.  Pardon me while I think out loud for a second:

It seems weird to me that we have this LayerList where we end up skipping parts of it conditionally (due to opacity, skipped flags, facing direction, etc...).  On top of that, it looks like CCLayerTreeHost and LayerRendererChromium have a lot of similarities between paintLayerContents and drawLayers.

What do you think about the idea of having CCLayerTreeHostCommon::calculateDrawTransformsAndVisibility return a layer list where everything in it needs to be drawn? In other words, do all the conditional early-outs in that function at both the layer and the surface level?

[Shawn Singh]

That's a great idea, and I'll be happy to do that.  Two concerns:

(1) Personally, I feel that calculateDrawTransformsAndVisibility is too risky to modify until we have more complete testing in place.  The special cases and semantics are very fragile.

(2) People are waiting on scissoring optimization, so this change needs to happen sooner than later.

So, I think this patch is a step in the right direction, and accomplishes what is needed for now.  Then, after it lands, I can continue adding tests and continue refactoring without other people waiting.  Does that sound like a good compromise?

[Adrienne Walker]

(In reply to comment #4)
> That's a great idea, and I'll be happy to do that.  Two concerns:
> 
> (1) Personally, I feel that calculateDrawTransformsAndVisibility is too risky to modify until we have more complete testing in place.  The special cases and semantics are very fragile.
> 
> (2) People are waiting on scissoring optimization, so this change needs to happen sooner than later.

What part of this patch is needed for the scissoring optimization? If you're wary about doing refactoring before we have better testing, then maybe it'd better to have an even smaller, more directed patch to enable scissoring before doing this work.

[Shawn Singh]

Scissoring needs a preliminary loop through all layers that determines exactly what will be drawn for each layer (so that we can union that layer's intended draw rect to an overall compositor scissor).  So I think essentially this entire patch is a prerequisite for scissoring.

The only additional step that I took in this patch is to avoid redundant code by removing it from the original loop.

[Nat Duca]

Comment on attachment 110911 [details]
Patch

I agree with Enne's point that putting empemeral state on layers that have to do with "the next frame's state" violates layering.

As far as the "too risky to try" stuff, I disagree. We have pretty good coverage of this via layout tests. If they don't pass, then something's broken.

One thing you could try is to create a RenderingSchedule object:
  RenderingSchedule {
    void skip(CCLayerImpl* impl) { m_skipped.add(impl); }
    bool shouldSkip(CCLayerImpl*) { return impl in m_skipped; }
  protected:
    set<CCLayerImpl*> m_skipped;
  }
Pass this into determineWhatToDraw, as well as the drawing functions. That way at least you're not adding empemeral state onto the layers.

[Vangelis Kokkevis]

(In reply to comment #4)
> That's a great idea, and I'll be happy to do that.  Two concerns:
> 
> (1) Personally, I feel that calculateDrawTransformsAndVisibility is too risky to modify until we have more complete testing in place.  The special cases and semantics are very fragile.

It's probably not that bad. I agree that changing the logic there is scary but modifying what it returns should leave most of the existing code intact.  

An added benefit would be that we don't have to worry about having to keep the various layer skipping conditions in sync across different parts of the code (updating transforms, painting layers, drawing)


> 
> (2) People are waiting on scissoring optimization, so this change needs to happen sooner than later.
> 
> So, I think this patch is a step in the right direction, and accomplishes what is needed for now.  Then, after it lands, I can continue adding tests and continue refactoring without other people waiting.  Does that sound like a good compromise?

[Shawn Singh]

OK, I'll dig into how to put it into calculateDrawTransformsAndVisibility, and submit a new patch soon (ETA 1 day) =)

~Shawn

[James Robinson]

Comment on attachment 110911 [details]
Patch

What if instead of manipulating state on the layers you manipulated the layer lists (removed layers that aren't drawn from their LayerList, remove surfaces that are entirely skipped from the renderSurfaceLayerList).  Would that make things cleaner?

[Shawn Singh]

(In reply to comment #10)
> (From update of attachment 110911 [details])
> What if instead of manipulating state on the layers you manipulated the layer lists (removed layers that aren't drawn from their LayerList, remove surfaces that are entirely skipped from the renderSurfaceLayerList).  Would that make things cleaner?

Yeah, I think that's what Enne and Vangelis were suggesting, too.  Next patch should work this way.

[Shawn Singh]

So, I dug into it, and here's what I found:

The "right" way to do this is to never add layers that wouldn't be drawn.  (the "hack" way would be to remove them after we realize they can be skipped).  To do it the right way, we'll have to change the ordering in which layers are appended to the list.  This means we also have to change the layer sorter so that it orders things back-to-front instead of front-to-back.

Of course I prefer the "right" way, but I want to make sure you all are OK with reversing the ordering of layers in the layer lists (and in the sorter).   As I understand, the best way to reverse the sorting order is around line 421 of CCLayerSorter; instead of popping from the back, we can iterate starting from the beginning and clear the entire list afterwards.

should we be CC'ing Iain about this, too?  I couldn't find his email.

[James Robinson]

I'm not sure from your description exactly what the proposal is. Could you elaborate a bit?

[Shawn Singh]

sure...  the proposal is as follows:

(1) create a bool layerNeedsToBeDrawn().  all early exit conditions can be placed there.

(2) add a conditional check before appending the layer to the layer list:

    if (layerNeedsToBeDrawn(layer))
        descendants.append(layer)

(3) Currently, this append occurs *before* the recursion of the subtree.  However, layerNeedsToBeDrawn() requires knowing some information *after* recursion of the subtree.  So there are some options:
    (3a) Move the conditional append to after the recursion.  this effectively reverses the order of the layer list for each render surface.  And we'll have to make sure the sorter reverse its ordering, too.
    (3b) We can leave the recursion alone, and just remove the layer afterwards.  Not ideal, but at least the code is still pretty clean.

(3c) Technically the scissor is the only condition that needs to happen after recursion.  We could separate layerNeedsToBeDrawn into two pieces.  But I think that's not a very clean design.

[Adrienne Walker]

(In reply to comment #14)

>     (3b) We can leave the recursion alone, and just remove the layer afterwards.  Not ideal, but at least the code is still pretty clean.

I think I prefer this solution, rather than reversing the order of lists everywhere.

If a layer's scissor rect is dependent on its children and that layer ends up with an empty scissor rect, then (theoretically) it should be the case that it has no children that need to get drawn.  Removing it should just be popping the last layer off the list.  Or, maybe I'm missing something?

[Nat Duca]

I'm with Enne. 3b plz. I doubt there's much performance impact in deleting the layer after the fact... I'm assuming in the common case, the layer will still be at-or-near the end of the vector anyway.

[Shawn Singh]

OK, I'll do 3b  =)  Makes things easier.

However, I don't think it will be the common case that we're removing from the very end of the list. All the other conditions, not just the scissor, will also have a chance to remove a layer, too, while all its children should still be drawn.  But its still possible it will be negligible... I guess we'd have to measure its impact after implementing it.

[Adrienne Walker]

(In reply to comment #17)
> OK, I'll do 3b  =)  Makes things easier.
> 
> However, I don't think it will be the common case that we're removing from the very end of the list. All the other conditions, not just the scissor, will also have a chance to remove a layer, too, while all its children should still be drawn.  But its still possible it will be negligible... I guess we'd have to measure its impact after implementing it.

Oops, I misread what you wrote, then.  I think I'm arguing for 3c.  In other words, check for everything but scissoring ahead of time and don't add the layer if it's not drawn, then do a second check just for scissoring and remove it if it fails that check.  I think this is the simplest solution.

[Nat Duca]

Agreed.

Note that "not drawing a layer when it is outside the scissor rect" is not going to give a win. Yes, we can do it "because we can," but emphatically, it will not give us any savings. Lets keep our eye on the key  win: setting the scissor. Even if we drew every single layer, that will be a win.

[Shawn Singh]

Summary:
After going into detail and almost completing it, I found that we really must have a separate pass through all layers, to skip layers whose content rects and visible rects are empty.  I think the best choice is to request everyone to look again at my original patch.

Explanation:
This is because we need to know the contentRect of the target render surface.  this information is not known until the recursion is well past the context of that layer.  So no matter what we put in calculateDrawTransformsAndVisibility we'll still need a separate walk like the original one I had.

Furthermore, it is additionally messy because calculateDrawTransforms is a templatized function.  I took a brief glance, and it seems like early exit conditions are different for main thread and Impl thread.  It would still be a great refactoring opportunity, but that is a sidetrack is definitely not necessary for scissoring.

If there is a really good reason to still do it, I can still put half of the early exit conditions in the recursive function; I've already implemented it anyway.  But I just don't think its as "clean" as we had hoped.

Otherwise, can we re-visit the original patch and see if its acceptable?

We could, as jamesr suggested, replace the "ephemeral state" with removing layers from the list.  Its not clear to me that this would be a good idea though.  It could be negligible for many pages, but it will not scale well with number of layers, and will not scale well with other optimizations like culling away occluded layers.

[Vangelis Kokkevis]

(In reply to comment #20)
> Summary:
> After going into detail and almost completing it, I found that we really must have a separate pass through all layers, to skip layers whose content rects and visible rects are empty.  I think the best choice is to request everyone to look again at my original patch.
> 
> Explanation:
> This is because we need to know the contentRect of the target render surface.  this information is not known until the recursion is well past the context of that layer.  So no matter what we put in calculateDrawTransformsAndVisibility we'll still need a separate walk like the original one I had.
> 

The logic is probably more complex than it should be but I don't think you have to worry about RS that have empty drawableContentRects and descandants with non-empty rects.  The drawableContectRect of the RS is calculated as the union of all the drawableContectRects of its descendants.

Looking at the early out conditions in LayerRendererChromium::drawLayersInternal we have:

1. RS with empty layer lists. Don't have to worry about removed layers from the list for those as the list is already empty
2. RS with zero opacity. We should add a check for that in calculateDrawTransformsAndVisibilityInternal (cDTAVI)and bail out on layers with zero opacity (not even create RS for them)
3. layer->drawsContent() is false : we can test that in cDTAVI and don't add the layer in the list
4. layer->opacity == 0 : same as above.
5. layer->bounds is empty : we can also check that in cDTAVI. 

So, I think as far as the early out conditions, we should be able to take care of them directly in cDTAVI and avoid adding layers that don't draw to lists. Is there something that I'm overlooking here? 

 


> Furthermore, it is additionally messy because calculateDrawTransforms is a templatized function.  I took a brief glance, and it seems like early exit conditions are different for main thread and Impl thread.  It would still be a great refactoring opportunity, but that is a sidetrack is definitely not necessary for scissoring.
> 
> If there is a really good reason to still do it, I can still put half of the early exit conditions in the recursive function; I've already implemented it anyway.  But I just don't think its as "clean" as we had hoped.
> 
> Otherwise, can we re-visit the original patch and see if its acceptable?
> 
> We could, as jamesr suggested, replace the "ephemeral state" with removing layers from the list.  Its not clear to me that this would be a good idea though.  It could be negligible for many pages, but it will not scale well with number of layers, and will not scale well with other optimizations like culling away occluded layers.

[Shawn Singh]

Yes, those conditions I can put in calculateDrawTransformsAndVisibility...  (except that drawsContent is a virtual and some implementations might be dependent on those rects, too.)

The right place for computing the scissor is after all early exits.  Can you please explain what you mean by "don't worry about" the other rect early exits?  We should also separate them from the draw loop as well, so that we can compute the scissor after (or while) those additional layers are being skipped.

[Vangelis Kokkevis]

(In reply to comment #22)
> Yes, those conditions I can put in calculateDrawTransformsAndVisibility...  (except that drawsContent is a virtual and some implementations might be dependent on those rects, too.)
> 
> The right place for computing the scissor is after all early exits.  Can you please explain what you mean by "don't worry about" the other rect early exits?  We should also separate them from the draw loop as well, so that we can compute the scissor after (or while) those additional layers are being skipped.

cDTAV can prune the layer lists by removing any layers that won't participate in the draw. From that point on, we don't have to worry about checking early out conditions when painting their contents or drawing them. 

If the scissor rect optimization requires one more pass through the RS layer lists that would be fine.

[Shawn Singh]

OK consider it done =)

[Adrienne Walker]

(In reply to comment #20)

> I took a brief glance, and it seems like early exit conditions are different for main thread and Impl thread.

If there were differences, that would mean that we had painted a layer that wasn't drawn or that we were drawing a layer that wasn't painted.  There may be differences that exist right now because the code is in two different places, but I don't think they should be there.

[Shawn Singh]

Thanks - I was able to templatize it, and its a bit cleaner.

New patch coming right now.  I dont think we came to consensus on how to deal with the "shouldDraw()" ephemeral state.   My proposal is to keep it, and we will definitely need a separate bug to merge it with skipsDraw.  I think its unnecessarily inefficient to remove layers at that point. =)

[Shawn Singh]

Created attachment 111649 [details]
moved some early exits sooner so that layers are simply not added to layer lists

[James Robinson]

I think the 'shouldDraw' stuff is bad - I think everybody was in favor of removing layers from lists except for you.  We should favor clarity unless there's evidence of a performance issue.

[Shawn Singh]

Created attachment 111670 [details]
removing layers to skip them instead of keeping additional layer state

[Adrienne Walker]

Comment on attachment 111670 [details]
removing layers to skip them instead of keeping additional layer state

I guess my refactoring hope had been that the determineWhatToDraw function could get merged entirely into calculateDrawTransformsAndVisibility.  Then, the renderSurfaceLayerList that you get back from calculateDrawTransformsAndVisibility would just be the list of surfaces and layers to draw, rather than a list of things that maybe you should be drawing.

I do like this a lot better than the shouldDraw patch.

[Shawn Singh]

Yeah, I agree it would have been nice.  I went in that direction all the way to the point where I could say with concrete proof that we need a separate pass after calculateDrawTransformsAndVisibility.   There is information that needs to propagate back up the recursion before layers can know whether to be skipped.

If its any consolation, the flow of logic that I'm proposing is:

drawLayersInternal(...)
   1. calculateDrawTransformsAndVisibility
   2. determineWhatToDraw
   3. actually draw stuff.

So, if you want me to associate #1 and #2 into some larger umbrella function, we could do that.  for example:

calculateRenderPasses(...)
   1. calculateDrawTransformsAndVisibility(...)  // recursive
   2. determineWhatToDraw(...)   // post-process that prunes layers further, computes layer visibility and overall scissor.

drawLayersInternal(...)
   1. calculateRenderPasses(...)
   2. for each render pass(...)
        (a) drawRenderPass.

Doing this is just a matter of moving chunks of code into different places, based on the existing patch.  Would this feel cleaner?

[Vangelis Kokkevis]

(In reply to comment #31)
> Yeah, I agree it would have been nice.  I went in that direction all the way to the point where I could say with concrete proof that we need a separate pass after calculateDrawTransformsAndVisibility.   There is information that needs to propagate back up the recursion before layers can know whether to be skipped.

like?

[Shawn Singh]

(In reply to comment #32)
> (In reply to comment #31)
> > Yeah, I agree it would have been nice.  I went in that direction all the way to the point where I could say with concrete proof that we need a separate pass after calculateDrawTransformsAndVisibility.   There is information that needs to propagate back up the recursion before layers can know whether to be skipped.
> 
> like?

I referred to it in #20...  but I should be more clear =)

The render surface's content rect depends on each layer's drawableContentRect.   A layer could be skipped if the surface's *total* content rect doesn't intersect the layer's scissor.   This *total* content rect is not known until the recursion is done (for that render surface's subtree), at which point layers that fit this condition are no longer in scope and need to be found again in another loop.

We cannot compute this condition with a partial content rect, otherwise we could be skipping the layer when it would have intersected the total content rect and actually needed to draw something.

[Shawn Singh]

By the way...

the third condition in the post-process is drawsContent().  I believe this can eventually be moved into the recursive part where its ideal, but I'm still working to figure out why it doesn't work as expected.  I'll keep going, but please feel free to suggest that we can address that in a separate patch =)

[Vangelis Kokkevis]

(In reply to comment #33)
> (In reply to comment #32)
> > (In reply to comment #31)
> > > Yeah, I agree it would have been nice.  I went in that direction all the way to the point where I could say with concrete proof that we need a separate pass after calculateDrawTransformsAndVisibility.   There is information that needs to propagate back up the recursion before layers can know whether to be skipped.
> > 
> > like?
> 
> I referred to it in #20...  but I should be more clear =)
> 
> The render surface's content rect depends on each layer's drawableContentRect.   A layer could be skipped if the surface's *total* content rect doesn't intersect the layer's scissor.   This *total* content rect is not known until the recursion is done (for that render surface's subtree), at which point layers that fit this condition are no longer in scope and need to be found again in another loop.
> 

Is this an existing early-out condition?  I'm not sure I'm seeing it in the code that handles drawing of the layers. 

> We cannot compute this condition with a partial content rect, otherwise we could be skipping the layer when it would have intersected the total content rect and actually needed to draw something.

[Shawn Singh]

The target surface intersect layer scissor existed in LayerRendererChromium::drawLayer().   It seems to be necessary for correctness, not just optimization, since layout tests fail without it.

The visibility rect existed in only a few specific layer types, and Enne suggested it can be generalized to all layer.

There is one final early exit which I havent touched, but we could eventually.  It is m_skipsDraw that is also only exists for a few specific layer types.

[Shawn Singh]

Created attachment 111881 [details]
Cleaner and closer to ideal.

[Shawn Singh]

(In reply to comment #37)
> Created an attachment (id=111881) [details]
> Cleaner and closer to ideal.

Hi all, thanks for your patience on this patch.

please note:

I was wrong, it turns out the rect condition is not needed for correctness.  For now that means we can avoid the intermediate loop.   I think that loop may still be necessary for the scissor patch, though.  We'll deal with it at that time.

Thanks to Enne for discussions and suggestions to debug the drawsContent/mask layer stuff.   If we want to use drawsContent() as an culling condition, then masks needed to be painted and updated with their associated render surface, rather than with their layer.

[Adrienne Walker]

Comment on attachment 111881 [details]
Cleaner and closer to ideal.

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

This is looking really good.  I have one small nit.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:316
> +            if (!renderSurfaceLayer->drawsContent())
> +                paintContentsIfDirty(renderSurfaceLayer->maskLayer(), IntRect(IntPoint(), renderSurfaceLayer->contentBounds()));

This conditional seems a bit dodgy.  Are there cases other than !drawsContent() where a masked layer might not draw (opacity), but the render surface that it creates does draw?

I think it'd be safer to always paint masks when we handle their render surface and never paint them when we're looking at a layer.  (And, same with updateCompositorResources.)

[Shawn Singh]

Created attachment 112043 [details]
removed dodgy conditions

[Vangelis Kokkevis]

Comment on attachment 112043 [details]
removed dodgy conditions

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

Looking nice and clean!

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:300
> +        renderSurface->setSkipsDraw(true);

Do we use the skipsDraw bit anywhere? From a quick look around the code it doesn't look like we do.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:314
>              continue;

Maybe for a subsequent patch but if we want the set of conditions that could make us skip a RS (no layers, opacity == 0) to stay in sync between painting and drawing, maybe we could do these tests in  calculateDrawTransformsAndVisibilityInternal() and avoid adding the RS layer into the renderSurfaceLayerList.

[Shawn Singh]

(In reply to comment #41)
> (From update of attachment 112043 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112043&action=review
> 
> Looking nice and clean!
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:300
> > +        renderSurface->setSkipsDraw(true);
> 
> Do we use the skipsDraw bit anywhere? From a quick look around the code it doesn't look like we do.

Off the top of my memory, skipsDraw is used from within the render surface drawing itself.  similar skipsDraw are hidden inside specific layer types, so for now I did not pull them out.


> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:314
> >              continue;
> 
> Maybe for a subsequent patch but if we want the set of conditions that could make us skip a RS (no layers, opacity == 0) to stay in sync between painting and drawing, maybe we could do these tests in  calculateDrawTransformsAndVisibilityInternal() and avoid adding the RS layer into the renderSurfaceLayerList.


Certainly agreed!  I'll spur a separate discussion on additional refactoring soon.

[Adrienne Walker]

Comment on attachment 112043 [details]
removed dodgy conditions

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:381
> +        // The same is true for the render surface's replica layer as well.

Can you explain the inconsistency between paint and upload here, i.e. handle only masks early during painting but masks and replica masks early during upload.

(Should we be handling all masks, replicas, and replica masks when we handle the layer associated with the surface?)

[Shawn Singh]

(In reply to comment #43)
> (From update of attachment 112043 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112043&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:381
> > +        // The same is true for the render surface's replica layer as well.
> 
> Can you explain the inconsistency between paint and upload here, i.e. handle only masks early during painting but masks and replica masks early during upload.
> 
> (Should we be handling all masks, replicas, and replica masks when we handle the layer associated with the surface?)

The inconsistency probably does not need to exist.  Should be straightforward to fix.  Please note that this will probably warrant restructuring a few functions, too, so I'll go ahead and do that, too. =)

[Shawn Singh]

Created attachment 112068 [details]
cleaned up a lot more

[Shawn Singh]

(In reply to comment #45)
> Created an attachment (id=112068) [details]
> cleaned up a lot more

So this time I went ahead and scrubbed and polished all the code that this patch was touching.
Here is a summary of the patch:

- same LayerRendererChromium refactoring as before

- there was some existing redundant code that I would have needed to duplicate yet again, with "targetSurfaceRect" and "visibleLayerRect".  So I folded all that into the CCLayerTreeHostCommon::computeVisibleLayerRect function.

- in the CCLayerTreeHost, isolated mask and replica painting/updating into their own clean functions.

- was now able to remove some of the early exit conditions from the LayerChromium side, since they're now in the common place.

I'm happy to adjust anything that's already done.  If there is any additional cleanup we should do related to this refactor, then could we please create a new bug and assign it to me instead?

FYI, this patch seemed to increase flakiness of "compositing/reflections/nested-reflection-animated.html".  However that test was already labeled explicitly as being flaky, so I still feel confident about the changes in this patch.

[Vangelis Kokkevis]

Comment on attachment 112068 [details]
cleaned up a lot more

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:367
> +void CCLayerTreeHost::updateMaskResourcesForRenderSurface(LayerChromium* renderSurfaceLayer, GraphicsContext3D* context, TextureAllocator* allocator)

I would recommend folding this method into updateCompositorResources. I don't see much of a use to be separate.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:68
> +    IntRect targetSurfaceRect = layer->targetRenderSurface()->contentRect();

paintLayerContents had some logic about using the defaultContentRect instead of the targetRenderSurface rect. Why is it no longer necessary?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:93
> +IntRect CCLayerTreeHostCommon::calculateVisibleLayerRect(LayerChromium* layer)

Can you use a template to define both these methods?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:113
> +bool layerShouldBeAppended(LayerType* layer)

nit: Not a big fan of the name for this method as "appended" makes only sense in the context of the current caller.

[Vangelis Kokkevis]

> FYI, this patch seemed to increase flakiness of "compositing/reflections/nested-reflection-animated.html".  However that test was already labeled explicitly as being flaky, so I still feel confident about the changes in this patch.

Is the output of this test correct when you run it in the browser?

[Adrienne Walker]

(In reply to comment #48)
> > FYI, this patch seemed to increase flakiness of "compositing/reflections/nested-reflection-animated.html".  However that test was already labeled explicitly as being flaky, so I still feel confident about the changes in this patch.
> 
> Is the output of this test correct when you run it in the browser?

This test is pretty flaky, especially in debug mode.  If this is the only test that's failing, I'm not worried about correctness issues.  I think the replica code that it's testing is also tested in a number of other tests.

[Shawn Singh]

Yes the animation of that test looks the same as a stable build of chrome.
 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:68
> > +    IntRect targetSurfaceRect = layer->targetRenderSurface()->contentRect();
> 
> paintLayerContents had some logic about using the defaultContentRect instead of the targetRenderSurface rect. Why is it no longer necessary?

calculateDrawTransformsAndVisibility was always setting the targetRenderSurface for all layers.  The "default" case would have never been hit (this is verified by that assert statement).


> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:113
> > +bool layerShouldBeAppended(LayerType* layer)
> 
> nit: Not a big fan of the name for this method as "appended" makes only sense in the context of the current caller.

I would prefer not to say "layerShouldBeDrawn", because not all layers that return true will actually be drawn.

How about layerCanBeSkipped and I'll invert the boolean logic?

[Shawn Singh]

Created attachment 112219 [details]
Patch

[Adrienne Walker]

Comment on attachment 112219 [details]
Patch

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

So close.  Thank you again for taking the time to clean up the surrounding code with this patch.  :)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:382
> +        if (renderSurfaceLayer->maskLayer())
> +            updateCompositorResources(renderSurfaceLayer->maskLayer(), context, allocator);
> +        
> +        if (renderSurfaceLayer->replicaLayer() && renderSurfaceLayer->replicaLayer()->maskLayer())
> +            updateCompositorResources(renderSurfaceLayer->replicaLayer()->maskLayer(), context, allocator);
> +        

Unless I'm misreading, I think you're missing updateCompositorResources(renderSurfaceLayer->replicaLayer(), context, allocator).  updateCompositorResources used to be recursive before this change and so would hit this case.

(Maybe we don't have a test for a replica layer where the replica layer itself doesn't draw?)

[Shawn Singh]

(In reply to comment #52)
> (From update of attachment 112219 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112219&action=review
> 
> So close.  Thank you again for taking the time to clean up the surrounding code with this patch.  :)
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:382
> > +        if (renderSurfaceLayer->maskLayer())
> > +            updateCompositorResources(renderSurfaceLayer->maskLayer(), context, allocator);
> > +        
> > +        if (renderSurfaceLayer->replicaLayer() && renderSurfaceLayer->replicaLayer()->maskLayer())
> > +            updateCompositorResources(renderSurfaceLayer->replicaLayer()->maskLayer(), context, allocator);
> > +        
> 
> Unless I'm misreading, I think you're missing updateCompositorResources(renderSurfaceLayer->replicaLayer(), context, allocator).  updateCompositorResources used to be recursive before this change and so would hit this case.
> 
> (Maybe we don't have a test for a replica layer where the replica layer itself doesn't draw?)

(1) Actually the existing code before this patch also did not have that call.

(2) As I understand, a CCRenderSurface, when it draws, will also draw the replicaLayer with the exact same texture, just with a different transform.  So, as of now there are no "resources" for a replica to update.

(3) FYI there is a related FIXME in CCRenderSurface::draw, that is related to your question.

So would it be OK if we addressed this awkwardness in a different bug?

[Shawn Singh]

foot in mouth.

I'll fix it =)

Its worth noting that this did not fail layout tests.

[Shawn Singh]

Created attachment 112244 [details]
added updateCompositorResources for replica

[Adrienne Walker]

Thanks.  That last patch unofficially looks good to me.

[James Robinson]

Comment on attachment 112244 [details]
added updateCompositorResources for replica

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

R=me

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:395
> +        TRACE_EVENT("LayerRendererChromium::drawLayersInternal::drawLayersOntoRenderSurfaces", this, 0);

nit: since this is now a helper function you can just move the trace into the function body and avoid having to make this extra scope

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:402
> +    // For masks and repliacs however, we may still need to check them here.

typo 'repliacs' -> 'replicas'

[Shawn Singh]

Created attachment 112256 [details]
fixed last two suggestions

[Shawn Singh]

Created attachment 112257 [details]
fixed last two suggestions

[James Robinson]

Comment on attachment 112257 [details]
fixed last two suggestions

Rockin'

[WebKit Review Bot]

Comment on attachment 112257 [details]
fixed last two suggestions

Rejecting attachment 112257 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
, check),
  File "/mnt/git/webkit-commit-queue/Source/WebKit/chromium/tools/gyp/pylib/gyp/input.py", line 218, in LoadOneBuildFile
    None)
  File "WebKit.gypi", line 103
    'tests/ScrollAnimatorNoneTest.cpp',
                                      ^
SyntaxError: invalid syntax
Error: 'depot_tools/gclient sync' failed 3 tries and returned 512 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.

Full output: http://queues.webkit.org/results/10211026

[Julien Chaffraix]

Comment on attachment 112257 [details]
fixed last two suggestions

Sorry for the mess, the cq should be back on track now.

[WebKit Review Bot]

Comment on attachment 112257 [details]
fixed last two suggestions

Clearing flags on attachment: 112257

Committed r98313: <http://trac.webkit.org/changeset/98313>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.