Bug 71267 - [chromium] Slow down commit and draw rate based on visibility and draw completion
Summary: [chromium] Slow down commit and draw rate based on visibility and draw comple...
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: Nat Duca
URL:
Keywords:
Depends on: 71269
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-31 23:40 PDT by Nat Duca
Modified: 2011-11-02 14:57 PDT (History)
2 users (show)

See Also:


Attachments
Patch (40.53 KB, patch)
2011-10-31 23:40 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Without ImmediateState (42.17 KB, patch)
2011-11-02 13:59 PDT, Nat Duca
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nat Duca 2011-10-31 23:40:30 PDT
[chromium] Slow down commit and draw rate based on visibility and draw completion
Comment 1 Nat Duca 2011-10-31 23:40:54 PDT
Created attachment 113133 [details]
Patch
Comment 2 Nat Duca 2011-11-01 00:34:42 PDT
This patch does three things:
1. Removes the setNeedsRedraw that CCThreadProxy used to force after commit [since that is now inside the scheduler]
2. Does not let a new commit begin until the previous commit makes it onscreen.
2. Does not draw when invisible [and because of #2, does not begin a new commit when invisible]

Based on measurements, this should help stabilize frame rates and reduce overlap.

This depends on changing the handling of setVisible(false) from triggering a commit to something more situation-specific.
Comment 3 James Robinson 2011-11-01 13:51:39 PDT
Comment on attachment 113133 [details]
Patch

Logic looks good but I'm not sure I understand the distinction between immediate state and member bools on CCSchedulerStateMachine. It feels a bit odd to push visible/not visible to the state machine on every nextAction() since it doesn't change very much - but I suspect there's some reason for the distinction other than frequency of change. Let's sync in person
Comment 4 James Robinson 2011-11-01 14:16:39 PDT
BTW this merge conflicts with your STREQ change
Comment 5 Nat Duca 2011-11-02 13:59:00 PDT
Created attachment 113375 [details]
Without ImmediateState
Comment 6 James Robinson 2011-11-02 14:11:29 PDT
Comment on attachment 113375 [details]
Without ImmediateState

View in context: https://bugs.webkit.org/attachment.cgi?id=113375&action=review

R=me, one comment for you to think about and do what you think is best.

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:102
> +    // Adjust the state machine's visbility state to the proxy's visibility state
> +    m_stateMachine.setVisible(m_client->visible());

For your consideration: instead of pulling visible state from the client here, could the client push the visible state down when it changes to the scheduler? It's a little easier for me to reason about interactions when state tends to flow in one direction.

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.h:32
> +// The CCSched decides how to coordinates main thread activites

looks like you accidentally part of this comment
Comment 7 Nat Duca 2011-11-02 14:50:11 PDT
(In reply to comment #6)
> For your consideration: instead of pulling visible state from the client here, could the client push the visible state down when it changes to the scheduler? It's a little easier for me to reason about interactions when state tends to flow in one direction.

Great point. Done. :)
Comment 8 Nat Duca 2011-11-02 14:57:41 PDT
Committed r99100: <http://trac.webkit.org/changeset/99100>