Summary: | [chromium] Prevent compositor ticking if it can't draw | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brian Anderson <brianderson> | ||||||||||
Component: | New Bugs | Assignee: | Brian Anderson <brianderson> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | cc-bugs, dglazkov, jamesr, jbates, nduca, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Brian Anderson
2012-08-29 16:38:28 PDT
Created attachment 161377 [details]
Patch
Comment on attachment 161377 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161377&action=review > Source/WebCore/ChangeLog:9 > + Background extensions had an always ticking compositor even though > + they couldn't draw. This patch disables the ticks when canDraw is false Why are they visible? Can we figure that out first before patching the compositor to deal with this state? Comment on attachment 161377 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161377&action=review This fixes the background extension ticking for chromium bug http://code.google.com/p/chromium/issues/detail?id=142061. There were some issues with live locking and test / assertion failures, but those have been addressed. The tests have been updated for the new interfaces, but I haven't yet added a regression test. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:191 > +void CCLayerTreeHostImpl::notifyIfCanDrawChanged() This is the new function that should be called whenever canDraw might have changed. > Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:-164 > - m_stateMachine.setCanDraw(m_client->canDraw()); Removed this function because we are notified of canDraw state changes and don't need to poll for here anymore. > Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:223 > + m_hasMoreResourceUpdates || m_stateMachine.vsyncCallbackNeeded()); Without the m_hasMoreResourceUpdates check, we will not perform multiple frame uploads since candDraw will not become true until all textures have been uploaded. > Source/WebCore/platform/graphics/chromium/cc/CCScheduler.h:-55 > - virtual bool canDraw() = 0; The client will now notify us of canDraw changes instead of having to poll for them. > Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:49 > + , m_canDraw(false) This must be default false to match the initial state of the CCLayerTreeImpl, otherwise the scheduler can attempt to tick before everything is initialized. Tests must explicitly setCanDraw(true) now. > Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:56 > +String CCSchedulerStateMachine::toString() This function has been added for convenience. It isn't used anywhere in this patch. > Why are they visible? Can we figure that out first before patching the compositor to deal with this state?
I attempted to figure out why they were visible the past two days, but didn't make much progress. I agree we should still try to figure that out, but after talking with Nat this morning I decided to shift gears back to this patch. Notifying of canDraw changes instead of polling for it is something that needs to be fixed anyway and happens to fix this bug too.
Comment on attachment 161377 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161377&action=review I like this overall, good work. Lets file a new crbug about the visibility being wrong of extensions and see if we can assign it to an extensions team person. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:193 > + bool newCanDraw = canDraw(); I think its safe to over-call m_client->onCanDrawStateChanged... that removes a bit of complexity here -- the setter can just call m_cleint->blahblah. All thats going to do is ping the scheduler, which is cheap. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:165 > + void resetContentsTexturesPurged() { m_contentsTexturesPurged = false; notifyIfCanDrawChanged(); } probably move to the cpp now that its not trivial >> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:223 >> + m_hasMoreResourceUpdates || m_stateMachine.vsyncCallbackNeeded()); > > Without the m_hasMoreResourceUpdates check, we will not perform multiple frame uploads since candDraw will not become true until all textures have been uploaded. Are you sure about this? This feels like its another change? Why is canDraw tied to textures being uploaded? >> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:56 >> +String CCSchedulerStateMachine::toString() > > This function has been added for convenience. It isn't used anywhere in this patch. <3 This is awesome. Comment on attachment 161377 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161377&action=review Will file a separate bug for the visibility issue. >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:193 >> + bool newCanDraw = canDraw(); > > I think its safe to over-call m_client->onCanDrawStateChanged... that removes a bit of complexity here -- the setter can just call m_cleint->blahblah. All thats going to do is ping the scheduler, which is cheap. Ok. >>> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:223 >>> + m_hasMoreResourceUpdates || m_stateMachine.vsyncCallbackNeeded()); >> >> Without the m_hasMoreResourceUpdates check, we will not perform multiple frame uploads since candDraw will not become true until all textures have been uploaded. > > Are you sure about this? This feels like its another change? Why is canDraw tied to textures being uploaded? There was live-lock without this, so we need this or we need a way to upload textures when we aren't ticking (which we might need anyway for http://code.google.com/p/chromium/issues/detail?id=145825, but I don't think it is trival). Note: I am moving the m_hasMoreResourceUpdates check into the vsyncCallbackNeeded function. The information required is also available in the state machine and it makes writing the regression tests easier. > Note: I am moving the m_hasMoreResourceUpdates check into the vsyncCallbackNeeded function. The information required is also available in the state machine and it makes writing the regression tests easier.
Ah, that sounds much cleaner. Perfect.
Created attachment 161751 [details]
Patch
Comment on attachment 161751 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161751&action=review > Source/WebKit/chromium/tests/CCSchedulerStateMachineTest.cpp:537 > +TEST(CCSchedulerStateMachineTest, TestVsyncCallbackNeededOnCanDrawAndResourceUpdates) This is the regression test that makes sure the state machine wont request ticking if we can't draw. It also tests to make sure we wont disable vsync ticsk if we need to upload textures, which results in live lock with the current logic. Comment on attachment 161751 [details]
Patch
This is real nice. It looks like you have some merging to do, however.
Comment on attachment 161751 [details] Patch Will rebase once https://bugs.webkit.org/show_bug.cgi?id=94254 lands. Created attachment 162103 [details]
Patch
Comment on attachment 162103 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162103&action=review > Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:159 > + // Avoid nested calls to this function. The top-level call of this > + // function will automatically process the next action. > + if (m_insideProcessScheduledActions) this looks new - what's the reason for this change? Comment on attachment 162103 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162103&action=review >> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:159 >> + if (m_insideProcessScheduledActions) > > this looks new - what's the reason for this change? Was pointing this out to Nat earlier. The reasoning is that during a commit, the canDraw state would change, triggering a frame to draw in the middle of the commit. Then, when the commit completed, another frame would draw. Tests were seeing more commits/frames than expected. This change warrants it's own patch, so I'm attempting to move it out. There's a challenge in making the two patches independent though. Created attachment 162117 [details]
Patch
Comment on attachment 162117 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162117&action=review > Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:67 > + m_frameRateController->setActive(m_stateMachine.vsyncCallbackNeeded()); This is the workaround to prevent recursion and extra frames/commits. Interestingly, everything still seem to work even without setting the frame rate controller active but I'm keeping this here just in case. Will make processScheduledActions a separate patch to land after this one. Comment on attachment 162117 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162117&action=review I like this, but file a bug and put a link about the bug to fix this right where your workaround text goes. >> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:67 >> + m_frameRateController->setActive(m_stateMachine.vsyncCallbackNeeded()); > > This is the workaround to prevent recursion and extra frames/commits. Interestingly, everything still seem to work even without setting the frame rate controller active but I'm keeping this here just in case. > > Will make processScheduledActions a separate patch to land after this one. Lets file a crbug and assign to you to make the scheduler non reentrant as a fix for this. Comment on attachment 162117 [details]
Patch
R=me
Comment on attachment 162117 [details] Patch Attachment 162117 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13746685 New failing tests: fast/innerHTML/innerHTML-iframe.html Comment on attachment 162117 [details]
Patch
Need some help parsing the results. The fast/innerHTML/innerHTML-iframe.html was failing before this patch on the bots. I cannot reproduce the error locally on either a Debug or Release build, even with --repeat-each=100.
Comment on attachment 162117 [details]
Patch
Pretty confident that can't be you - that test doesn't invoke the compositor and definitely not the threaded path.
Comment on attachment 162117 [details] Patch Clearing flags on attachment: 162117 Committed r127556: <http://trac.webkit.org/changeset/127556> All reviewed patches have been landed. Closing bug. w00t! |