Summary: | [chromium] Early out in a new prepareToDraw() step if checkerboarding an accelerated animation in order to skip the frame | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | vollick | ||||||||||||||
Component: | WebKit Misc. | Assignee: | Dana Jansens <danakj> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | backer, cc-bugs, danakj, enne, epenner, jamesr, nduca, piman, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 81106, 81862 | ||||||||||||||||
Bug Blocks: | 79536, 81716 | ||||||||||||||||
Attachments: |
|
Description
vollick
2012-03-16 17:54:01 PDT
Created attachment 132506 [details]
Patch
Adding dep on bug #81106 because it needs it for the unit tests to build. Created attachment 132825 [details]
Patch
rebased
nat's going to look this over first. 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. 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. :) 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 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 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 Created attachment 133101 [details]
Patch
Added //explain for bool returns.
(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? (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? > 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 on attachment 133101 [details]
Patch
Thanks for all the changes. Looks good to me.
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 Created attachment 133113 [details]
Patch
rebased
Comment on attachment 133113 [details] Patch Clearing flags on attachment: 133113 Committed r111616: <http://trac.webkit.org/changeset/111616> All reviewed patches have been landed. Closing bug. Reopening to fix scoping issue in the unit tests. Created attachment 133251 [details]
Patch
Fix mac build.
Comment on attachment 133251 [details] Patch Clearing flags on attachment: 133251 Committed r111700: <http://trac.webkit.org/changeset/111700> All reviewed patches have been landed. Closing bug. |