Bug 95399 – [chromium] Prevent compositor ticking if it can't draw

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Brian Anderson]

[chromium] Prevent compositor ticking if it can't draw

[Brian Anderson]

Created attachment 161377 [details]
Patch

[James Robinson]

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?

[Brian Anderson]

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.

[Brian Anderson]

> 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.

[Nat Duca]

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.

[Brian Anderson]

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.

[Nat Duca]

> 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.

[Brian Anderson]

Created attachment 161751 [details]
Patch

[Brian Anderson]

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.

[James Robinson]

Comment on attachment 161751 [details]
Patch

This is real nice. It looks like you have some merging to do, however.

[Brian Anderson]

Comment on attachment 161751 [details]
Patch

Will rebase once https://bugs.webkit.org/show_bug.cgi?id=94254 lands.

[Brian Anderson]

Created attachment 162103 [details]
Patch

[James Robinson]

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?

[Brian Anderson]

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.

[Brian Anderson]

Created attachment 162117 [details]
Patch

[Brian Anderson]

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.

[Nat Duca]

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.

[James Robinson]

Comment on attachment 162117 [details]
Patch

R=me

[WebKit Review Bot]

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

[Brian Anderson]

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.

[James Robinson]

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.

[WebKit Review Bot]

Comment on attachment 162117 [details]
Patch

Clearing flags on attachment: 162117

Committed r127556: <http://trac.webkit.org/changeset/127556>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nat Duca]

w00t!