Bug 81437 – [chromium] Early out in a new prepareToDraw() step if checkerboarding an accelerated animation in order to skip the frame

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

During an accelerated css animation, if we find that we are about to draw a frame with missing textures, this should not make it to the screen. If this happens at time t, and it takes n seconds before we get the textures, the textures should be positioned as they would be at ~time t + n in the animation, not t.

[Dana Jansens]

Created attachment 132506 [details]
Patch

[Dana Jansens]

Adding dep on bug #81106 because it needs it for the unit tests to build.

[Dana Jansens]

Created attachment 132825 [details]
Patch

rebased

[Dana Jansens]

nat's going to look this over first.

[Adrienne Walker]

Comment on attachment 132825 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:153
> +                bool layerHasAnimatingTransform = screenSpaceTransformIsAnimating() || drawTransformIsAnimating();

I'd suggest moving the check for animation up to calculateRenderPasses and instead just return if any checkerboards were needed for that layer.  It'd allow us to potentially change the logic to "on desktop, don't show any checkerboards" more easily in the future.

[Adrienne Walker]

I had a conversation with Nat, which I'll paraphrase here.

Essentially, when the scheduler says "draw a frame" it really expects to draw a frame, and you can't just abort.

Ultimately, to make this happen scheduledActionDrawAndSwap needs to become scheduledActionPrepareToDraw (animate, calculateRenderPasses) and scheduledActionDrawAndSwap (actually draw, if you can).  Changing the scheduler and the proxies can be done in bug 81716.

Before that happens, CCLTHI::drawLayers also needs to be split out into a CCLTHI::prepareToDraw that generates the render passes (and stores them somewhere) and returns a boolean whether not it can draw.  That boolean will get ignored until the scheduler and proxy changes happen.

You could split that out in this bug or in a separate one, if that's easier.  :)

[Dana Jansens]

Created attachment 133100 [details]
Patch

Split CCLTHI::drawLayers() into:

bool prepareToDraw(FrameData&);
void drawLayers(const FrameData&);

I felt dirty sticking state on the LayerTreeHost, so I made a struct that the proxy owns and passes in.

Proxys call both of these but assume that prepareToDraw is always true for now. Follow up CL will fix that.

Great call on passing up "usedCheckboard" and doing the animation check in calcRenderPasses.

[Nat Duca]

Comment on attachment 133100 [details]
Patch

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

Me gusta.

> Source/WebCore/ChangeLog:3
> +        [chromium] During accelerated css animations, don't draw frames with missing textures

bug name and description of what this does out of date

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:289
> +    bool unableToDrawAnimation = false;

unableToDrawDueToAnimationCausedCheckerboard?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:-165
> -    void calculateRenderPasses(CCRenderPassList&, CCLayerList& renderSurfaceLayerList);

// explaining return value and what it means

> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.h:41
> +    virtual bool append(PassOwnPtr<CCDrawQuad> passDrawQuad);

// explaining bool ret

[Nat Duca]

Comment on attachment 133100 [details]
Patch

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

This one's even better. :)

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:289
>>      typedef CCLayerIterator<CCLayerImpl, Vector<CCLayerImpl*>, CCRenderSurface, CCLayerIteratorActions::FrontToBack> CCLayerIteratorType;
> 
> unableToDrawDueToAnimationCausedCheckerboard?

OH, nvm

[Dana Jansens]

Created attachment 133101 [details]
Patch

Added //explain for bool returns.

[Adrienne Walker]

(In reply to comment #10)
> Created an attachment (id=133101) [details]
> Patch
> 
> Added //explain for bool returns.

Just to make sure this is line with future plans, when bug 81716 gets addressed, is the proxy going to end up owning the FrameData as a member variable? Or, how is that going to get transferred between two separate scheduler actions?

[Dana Jansens]

(In reply to comment #11)
> (In reply to comment #10)
> > Created an attachment (id=133101) [details] [details]
> > Patch
> > 
> > Added //explain for bool returns.
> 
> Just to make sure this is line with future plans, when bug 81716 gets addressed, is the proxy going to end up owning the FrameData as a member variable? Or, how is that going to get transferred between two separate scheduler actions?

TBH I didn't think in those terms. But I think I'd rather the proxy own it than the LayerTreeHost. Do you agree?

[Nat Duca]

> TBH I didn't think in those terms. But I think I'd rather the proxy own it than the LayerTreeHost. Do you agree?

Seems fair. We'll see what the scheduler is in the mood to do. Its a moody best, the scheduler. ;)

[Adrienne Walker]

Comment on attachment 133101 [details]
Patch

Thanks for all the changes.  Looks good to me.

[WebKit Review Bot]

Comment on attachment 133101 [details]
Patch

Rejecting attachment 133101 [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:
patching file Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp
patching file Source/WebKit/chromium/tests/CCQuadCullerTest.cpp
patching file Source/WebKit/chromium/tests/CCSolidColorLayerImplTest.cpp
patching file Source/WebKit/chromium/tests/CCTiledLayerImplTest.cpp
patching file Source/WebKit/chromium/tests/MockCCQuadCuller.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adrienne W..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

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

[Dana Jansens]

Created attachment 133113 [details]
Patch

rebased

[WebKit Review Bot]

Comment on attachment 133113 [details]
Patch

Clearing flags on attachment: 133113

Committed r111616: <http://trac.webkit.org/changeset/111616>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dana Jansens]

Reopening to fix scoping issue in the unit tests.

[Dana Jansens]

Created attachment 133251 [details]
Patch

Fix mac build.

[WebKit Review Bot]

Comment on attachment 133251 [details]
Patch

Clearing flags on attachment: 133251

Committed r111700: <http://trac.webkit.org/changeset/111700>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.