Bug 81437 - [chromium] Early out in a new prepareToDraw() step if checkerboarding an accelerated animation in order to skip the frame
Summary: [chromium] Early out in a new prepareToDraw() step if checkerboarding an acce...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dana Jansens
URL:
Keywords:
Depends on: 81106 81862
Blocks: 79536 81716
  Show dependency treegraph
 
Reported: 2012-03-16 17:54 PDT by vollick
Modified: 2012-03-22 08:09 PDT (History)
9 users (show)

See Also:


Attachments
Patch (39.06 KB, patch)
2012-03-18 16:22 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (39.08 KB, patch)
2012-03-20 07:55 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (58.58 KB, patch)
2012-03-21 12:54 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (58.73 KB, patch)
2012-03-21 13:02 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (58.82 KB, patch)
2012-03-21 14:44 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (58.84 KB, patch)
2012-03-22 07:00 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description vollick 2012-03-16 17:54:01 PDT
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.
Comment 1 Dana Jansens 2012-03-18 16:22:05 PDT
Created attachment 132506 [details]
Patch
Comment 2 Dana Jansens 2012-03-18 16:22:50 PDT
Adding dep on bug #81106 because it needs it for the unit tests to build.
Comment 3 Dana Jansens 2012-03-20 07:55:18 PDT
Created attachment 132825 [details]
Patch

rebased
Comment 4 Dana Jansens 2012-03-20 13:58:15 PDT
nat's going to look this over first.
Comment 5 Adrienne Walker 2012-03-20 14:33:51 PDT
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.
Comment 6 Adrienne Walker 2012-03-20 16:32:56 PDT
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.  :)
Comment 7 Dana Jansens 2012-03-21 12:54:08 PDT
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.
Comment 8 Nat Duca 2012-03-21 12:55:22 PDT
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
Comment 9 Nat Duca 2012-03-21 12:57:06 PDT
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
Comment 10 Dana Jansens 2012-03-21 13:02:36 PDT
Created attachment 133101 [details]
Patch

Added //explain for bool returns.
Comment 11 Adrienne Walker 2012-03-21 13:08:34 PDT
(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?
Comment 12 Dana Jansens 2012-03-21 13:11:04 PDT
(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?
Comment 13 Nat Duca 2012-03-21 13:17:36 PDT
> 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. ;)
Comment 14 Adrienne Walker 2012-03-21 13:27:20 PDT
Comment on attachment 133101 [details]
Patch

Thanks for all the changes.  Looks good to me.
Comment 15 WebKit Review Bot 2012-03-21 13:31:48 PDT
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
Comment 16 Dana Jansens 2012-03-21 14:44:56 PDT
Created attachment 133113 [details]
Patch

rebased
Comment 17 WebKit Review Bot 2012-03-21 15:59:55 PDT
Comment on attachment 133113 [details]
Patch

Clearing flags on attachment: 133113

Committed r111616: <http://trac.webkit.org/changeset/111616>
Comment 18 WebKit Review Bot 2012-03-21 16:00:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Dana Jansens 2012-03-21 19:01:41 PDT
Reopening to fix scoping issue in the unit tests.
Comment 20 Dana Jansens 2012-03-22 07:00:06 PDT
Created attachment 133251 [details]
Patch

Fix mac build.
Comment 21 WebKit Review Bot 2012-03-22 08:09:23 PDT
Comment on attachment 133251 [details]
Patch

Clearing flags on attachment: 133251

Committed r111700: <http://trac.webkit.org/changeset/111700>
Comment 22 WebKit Review Bot 2012-03-22 08:09:29 PDT
All reviewed patches have been landed.  Closing bug.