Bug 80899 – [chromium] Make damage tracker accumulate damage until explicitly cleared

• Component: Layout and Rendering
• Status: RESOLVED WONTFIX

[Shawn Singh]

There may be some reasons (to be determined) that this is necessary soon.  Otherwise, it is probably a good idea to do this anyway down the road.

Instead of assuming that every update on damage tracker is guaranteed to be presented to the front buffer, we should accumulate damage rects, and only explicitly reset the damage tracker when we know it has be presented (i.e. where swapBuffers is finally performed).

[Nat Duca]

Looks like aura flashes aren't due to this swap. So this isn't on the hot-list any more.

Lets do this though, its a cleaner architecture.

[Shawn Singh]

Created attachment 131787 [details]
This should be the right fix, but no unit test yet

Let me know if we need to push this and I'll write a unit test to land it asap

[Nat Duca]

(In reply to comment #2)
> Let me know if we need to push this and I'll write a unit test to land it asap

@piman, wdyt? Aura-blocker?

[Antoine Labour]

This in itself is not a blocker. It may hide/workaround some problems, but I don't feel strongly about this specifically to rush it in.

[Shawn Singh]

I'm getting some hints that we should push this for m19, so that threaded animation will work?  If that's true, here's the patch (coming in just a moment).

It should be noted that eventually we should explicitly separate the concepts of scissor rect and partial swap rect, but this patch is for anyone's immediate needs.

For reviewers: to understand my changes on the unit test, note that there is a strong pattern in most the tests:
   1. clearDamageForAllSurfaces() // this is what is new for this patch
   2. Apply the changes that are to be tested for that frame
   3. emulateDrawingOneFrame()
   4. verify the resulting damage rect.

Hopefully that will make it easier to understand what's going on at a glance.

[Shawn Singh]

Created attachment 133359 [details]
Patch with updated unit tests

[Adrienne Walker]

Comment on attachment 133359 [details]
Patch with updated unit tests

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

> Source/WebCore/ChangeLog:15
> +        Before this patch, the damage tracker requires that every time the
> +        backbuffer is drawn to, that change must be presented with
> +        swapBuffers or postSubBuffer. This behavior is undesired; there
> +        are reasons we don't want to present anything to the front buffer,
> +        even though we may draw to the back buffer (for example, readback
> +        for the thumbnailer.)

We call CCLTHI::swapBuffers() during readback for the thumbnailer, and thus will clear the damage rect there.  Is that intended, given this comment?

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:242
> +    clearDamageForAllSurfaces(root.get());
>      child->setUpdateRect(FloatRect(10, 11, 12, 13));
>      emulateDrawingOneFrame(root.get());

Can you just wrap clearDamageForAllSurfaces into emulateDrawingOneFrame and have some emulatePrepareOneFrameWithoutDrawing for the one corner case that you don't need it?

[Shawn Singh]

(In reply to comment #7)
> (From update of attachment 133359 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133359&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        Before this patch, the damage tracker requires that every time the
> > +        backbuffer is drawn to, that change must be presented with
> > +        swapBuffers or postSubBuffer. This behavior is undesired; there
> > +        are reasons we don't want to present anything to the front buffer,
> > +        even though we may draw to the back buffer (for example, readback
> > +        for the thumbnailer.)
> 
> We call CCLTHI::swapBuffers() during readback for the thumbnailer, and thus will clear the damage rect there.  Is that intended, given this comment?

Sure, I can remove that.  or, I could modify this to say "single-threaded readback" and remove swapBuffers from the compositeAndReadback single-threaded path, since it will no longer be necessary after this bug.

> 
> > Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:242
> > +    clearDamageForAllSurfaces(root.get());
> >      child->setUpdateRect(FloatRect(10, 11, 12, 13));
> >      emulateDrawingOneFrame(root.get());
> 
> Can you just wrap clearDamageForAllSurfaces into emulateDrawingOneFrame and have some emulatePrepareOneFrameWithoutDrawing for the one corner case that you don't need it?

Actually, no, that won't work.  emulateDrawingOneFrame is what will cause damage, and we need to access that damage and verify it in the tests before we clear it.    I did try making a different wrapper, though, getCurrentDamageAndClearDamage().  However, I found that was (1) more error-prone because we may not need to access all surfaces for the test (and thus forgetting to clear them, and (2) cause some messy logic to clear damage rects at the beginning of every test case.  It turned out to be more uniform and less error-prone to make the clearDamageForAllSurfaces wrapper this way.

Hope that's OK?

[Shawn Singh]

(In reply to comment #5)
> I'm getting some hints that we should push this for m19, so that threaded animation will work?  If that's true, here's the patch (coming in just a moment).

Spoke with Dana recently, it looks like the reason this might have been needed for m19 is no longer an issue.   Let's wait until next week after m19 branches

[Shawn Singh]

This approach is obsolete - instead, we will take the approach in https://bugs.webkit.org/show_bug.cgi?id=82011 which does the same thing but will be a more intuitive and testable solution.