Bug 95399 - [chromium] Prevent compositor ticking if it can't draw
Summary: [chromium] Prevent compositor ticking if it can't draw
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brian Anderson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-29 16:38 PDT by Brian Anderson
Modified: 2012-09-05 03:36 PDT (History)
6 users (show)

See Also:


Attachments
Patch (30.37 KB, patch)
2012-08-29 18:09 PDT, Brian Anderson
no flags Details | Formatted Diff | Diff
Patch (34.72 KB, patch)
2012-08-31 12:23 PDT, Brian Anderson
no flags Details | Formatted Diff | Diff
Patch (36.03 KB, patch)
2012-09-04 14:56 PDT, Brian Anderson
no flags Details | Formatted Diff | Diff
Patch (34.45 KB, patch)
2012-09-04 16:10 PDT, Brian Anderson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Anderson 2012-08-29 16:38:28 PDT
[chromium] Prevent compositor ticking if it can't draw
Comment 1 Brian Anderson 2012-08-29 18:09:30 PDT
Created attachment 161377 [details]
Patch
Comment 2 James Robinson 2012-08-29 18:17:56 PDT
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 3 Brian Anderson 2012-08-29 18:24:14 PDT
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.
Comment 4 Brian Anderson 2012-08-29 18:30:24 PDT
> 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 5 Nat Duca 2012-08-30 14:45:24 PDT
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 6 Brian Anderson 2012-08-30 14:57:01 PDT
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.
Comment 7 Nat Duca 2012-08-30 15:32:53 PDT
> 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.
Comment 8 Brian Anderson 2012-08-31 12:23:15 PDT
Created attachment 161751 [details]
Patch
Comment 9 Brian Anderson 2012-08-31 12:27:47 PDT
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 10 James Robinson 2012-09-04 11:14:55 PDT
Comment on attachment 161751 [details]
Patch

This is real nice. It looks like you have some merging to do, however.
Comment 11 Brian Anderson 2012-09-04 11:21:23 PDT
Comment on attachment 161751 [details]
Patch

Will rebase once https://bugs.webkit.org/show_bug.cgi?id=94254 lands.
Comment 12 Brian Anderson 2012-09-04 14:56:43 PDT
Created attachment 162103 [details]
Patch
Comment 13 James Robinson 2012-09-04 15:02:03 PDT
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 14 Brian Anderson 2012-09-04 15:31:51 PDT
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.
Comment 15 Brian Anderson 2012-09-04 16:10:49 PDT
Created attachment 162117 [details]
Patch
Comment 16 Brian Anderson 2012-09-04 16:14:11 PDT
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 17 Nat Duca 2012-09-04 18:18:46 PDT
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 18 James Robinson 2012-09-04 18:21:08 PDT
Comment on attachment 162117 [details]
Patch

R=me
Comment 19 WebKit Review Bot 2012-09-04 18:46:07 PDT
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 20 Brian Anderson 2012-09-04 19:00:17 PDT
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 21 James Robinson 2012-09-04 19:39:51 PDT
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 22 WebKit Review Bot 2012-09-04 23:06:55 PDT
Comment on attachment 162117 [details]
Patch

Clearing flags on attachment: 162117

Committed r127556: <http://trac.webkit.org/changeset/127556>
Comment 23 WebKit Review Bot 2012-09-04 23:07:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Nat Duca 2012-09-05 03:36:46 PDT
w00t!