Bug 84803 – [chromium] damage tracking needs to be more robust to early exits

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

Right now, damage tracking is potentially a sore thumb for functionality that should otherwise be considered "const" that can be invoked any number of times regardless of what is drawn to the backbuffer.  Instead, we should make damage tracking cooperate better, probably doing something like accumulating damage until things are drawn... i.e. similar to the patch in https://bugs.webkit.org/show_bug.cgi?id=80899

Note, this is different than accumulating scissor rects to determine what region to swap, which is bug https://bugs.webkit.org/show_bug.cgi?id=82011 and will probably involve creating a simple "PartialSwapTracker".

[Shawn Singh]

Patch is coming in a moment.

Nat: this is a good opportunity for us to agree on the API for the damage tracker class.  I changed it to what I feel is clean, intuitive, and well designed.  Lets discuss if you would like to see a different API

Enne: this is very similar to the old patch from https://bugs.webkit.org/show_bug.cgi?id=80899 -- however, there is one major important difference.  This patch makes damage accumulate until we know that the damaged areas have actually be drawn.  This has nothing to do with swapBuffers.

This patch: accumulate damage for each surface, until we know all existing damage has been redrawn

Next patch: (https://bugs.webkit.org/show_bug.cgi?id=82011) accumulate redrawn areas of the root surface, until we know that all redrawn stuff has been presented onto the front buffer.  This will involve creating a straightforward PartialSwapTracker class that has a similar public API to damage tracker, and much simpler internals.

Finally, one comment is still relevant from the old bug https://bugs.webkit.org/show_bug.cgi?id=80899, the second half of comment 8 on that bug.

[Shawn Singh]

Created attachment 145142 [details]
Patch

[Shawn Singh]

Comment on attachment 145142 [details]
Patch

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

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:401
> +    // FIXME THIS TEST SHOWS THAT ACCUMULATING DAMAGE IS DONE INCORRECTLY AT THE MOMENT BECAUSE EXPANDING FILTERS WILL EXPAND TWICE WHEN ACCUMULATING DAMAGE ????
> +    // EXCEPT, WHERE IS IT CALLED TWICE? SOMETHING IS NOT QUITE RIGHT.

Oops, I forgot to remove this comment.  the test works fine.

[Shawn Singh]

Created attachment 145332 [details]
Same patch only removed bad leftover comment

[Shawn Singh]

James could you please review this?  Thanks in advance.

[James Robinson]

Comment on attachment 145332 [details]
Same patch only removed bad leftover comment

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

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.h:50
> +    void notifyDamagedAreaHasBeenDrawn() { m_currentDamageRect = FloatRect(); }

I think a more idiomatic name would be "didDrawDamagedArea"

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:451
> +    // Now that we know we really did draw to the back buffer, we can clear the accumulated damage.
> +    // In the current implementation (i.e. global root-level scissor applies to all surfaces), it is
> +    // necessary to clear damage for all surfaces here, not just the root surface.
> +    notifyDamageDrawnForAllSurfaces(rootLayer());

Can we clear the damage on a given RenderSurface when we draw the pass associated with it?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:494
> +    // Recursively clear damage for any existing surface.

It seems a bit weird to walk through layers to get surfaces - we calculate a renderSurfaceLayerList as part of the frame, should we go through that instead (or fold it into the pass iteration as I suggested above)?

[Shawn Singh]

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:451
> > +    // Now that we know we really did draw to the back buffer, we can clear the accumulated damage.
> > +    // In the current implementation (i.e. global root-level scissor applies to all surfaces), it is
> > +    // necessary to clear damage for all surfaces here, not just the root surface.
> > +    notifyDamageDrawnForAllSurfaces(rootLayer());
> 
> Can we clear the damage on a given RenderSurface when we draw the pass associated with it?
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:494
> > +    // Recursively clear damage for any existing surface.
> 
> It seems a bit weird to walk through layers to get surfaces - we calculate a renderSurfaceLayerList as part of the frame, should we go through that instead (or fold it into the pass iteration as I suggested above)?

This should work, except for one issue - Dana, will we still be able to do this if RenderPasses will not have a concept of surfaces?  Of course we could move the CCDamageTracker to the RenderPass if needed, but if that's what is needed, that would be a different patch.

[Dana Jansens]

(In reply to comment #7)
> > It seems a bit weird to walk through layers to get surfaces - we calculate a renderSurfaceLayerList as part of the frame, should we go through that instead (or fold it into the pass iteration as I suggested above)?
> 
> This should work, except for one issue - Dana, will we still be able to do this if RenderPasses will not have a concept of surfaces?  Of course we could move the CCDamageTracker to the RenderPass if needed, but if that's what is needed, that would be a different patch.

It seems fine to me. This is happening at the layer/surface level (where DamageTracker lives), not at the quad/pass level, so you can use the RSLL here.

[Dana Jansens]

(In reply to comment #6)
> Can we clear the damage on a given RenderSurface when we draw the pass associated with it?

Oh, but this one won't work with ubercompiness, since the pass doesn't know about the surface. I think using the RenderSurfaceLayerList makes lots of sense though.

[James Robinson]

Stepping back a bit, logically we want to track damage until we "use" it - which means either drawing (for a subsurface) or swapping (for the root).  I think we should try to reset the tracker state as close to this point as possible.  In ubercomp world, I think we want to reset the tracker whenever we "draw" the pass into a serialized form for the next layer up to consume.

[Shawn Singh]

(In reply to comment #10)
> Stepping back a bit, logically we want to track damage until we "use" it - which means either drawing (for a subsurface) or swapping (for the root).  I think we should try to reset the tracker state as close to this point as possible.  In ubercomp world, I think we want to reset the tracker whenever we "draw" the pass into a serialized form for the next layer up to consume.

I think everything you said is correct, but just to nip the confusion in the bud:  this patch accumulates damage until we know that damaged areas have been drawn.   Separately we will have a PartialSwapTracker that accumulates the region that needs swapping until we know that area has actually be swapped.

[Shawn Singh]

(In reply to comment #11)
> (In reply to comment #10)
> > Stepping back a bit, logically we want to track damage until we "use" it - which means either drawing (for a subsurface) or swapping (for the root).  I think we should try to reset the tracker state as close to this point as possible.  In ubercomp world, I think we want to reset the tracker whenever we "draw" the pass into a serialized form for the next layer up to consume.
> 
> I think everything you said is correct, but just to nip the confusion in the bud:  this patch accumulates damage until we know that damaged areas have been drawn.   Separately we will have a PartialSwapTracker that accumulates the region that needs swapping until we know that area has actually be swapped.

"until we know that damaged areas have been drawn to the back buffer"

[Dana Jansens]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > Stepping back a bit, logically we want to track damage until we "use" it - which means either drawing (for a subsurface) or swapping (for the root).  I think we should try to reset the tracker state as close to this point as possible.  In ubercomp world, I think we want to reset the tracker whenever we "draw" the pass into a serialized form for the next layer up to consume.
> > 
> > I think everything you said is correct, but just to nip the confusion in the bud:  this patch accumulates damage until we know that damaged areas have been drawn.   Separately we will have a PartialSwapTracker that accumulates the region that needs swapping until we know that area has actually be swapped.
> 
> "until we know that damaged areas have been drawn to the back buffer"

Agree with what James said. My intuition is that in ubercomp a nested compositor probably never has a reason to not swap (send its quads), so in practice this distinction probably ends up being used in the root host compositor.

What gets a little fuzzy if we have more render passes in the host/root compositor than we have surfaces. Those other render passes are going to have some state that persists across frames (texture + damage more or less?). This is stuff we stick on the RenderSurface now, but perhaps should consider moving the DamageTracker in the future.. the same we need to move the textures.

[Shawn Singh]

OK so I wanted to just clarify the situation with clearing damage and its relationship with ubercomp.  Here is what I understand, please correct me if its wrong:

Suppose there's a "child compositor" like tab contents, and a "parent compositor" like the UI.  from the child compositor's perspective, it does not need to know *anything* at all about where it draws onto, it just draws and swaps the same as always, right?  Then on the chromium side, the swapBuffers call is handled differently knowing that it should pass the tabContents texture to the parent compositor?

So the way we should clear damage is completely unrelated to how ubercomp receives that texture, right?

And finally, on the parent compositor side, when it receives that texture in the form of a layer, its the responsibility of this "ubercomp layer" to understand that the partialSwapRect of the child compositor corresponds to the m_updateRect area of the ubercomp layer, right?

[Dana Jansens]

That sounds right to me, where "texture" means "quads".

[Dana Jansens]

(In reply to comment #15)
> That sounds right to me, where "texture" means "quads".

Well, let me raise one more point. The "texture" which in this case is a set of render passes, means that the layer may generated more than one RenderPass, and that damage isn't a single m_updateRect for the layer.

Instead, we're intending to serialize the damage for each RenderPass, so that this layer's RenderPasses know their damage.

Then for a single RenderPass, the host compositor has some texture memory for it. When it goes to draw to that memory, it should use the union of all serialized damage rects for the pass, since the last time if draw that memory.

[Shawn Singh]

Comment on attachment 145332 [details]
Same patch only removed bad leftover comment

removing r=?  I'll try to get a new patch up tomorrow morning.

[Shawn Singh]

Created attachment 146617 [details]
Addressed reviewer comments

[WebKit Review Bot]

Comment on attachment 146617 [details]
Addressed reviewer comments

Clearing flags on attachment: 146617

Committed r120094: <http://trac.webkit.org/changeset/120094>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.