Bug 93328 – [Chromium] Add a CCFrameRateController state machine

• Component: Platform
• Status: RESOLVED WONTFIX

[Brian Anderson]

Add a CCFrameRateController state machine

[Brian Anderson]

Created attachment 156838 [details]
Patch

[Brian Anderson]

Created attachment 156984 [details]
Patch

[Brian Anderson]

Comment on attachment 156984 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:189
> +        updateAverageTicksPerFrame(ticksSinceLastFrame);

Right now, this is essentially the same as HIGH_FREQUENCY_1_FRAME_QUEUE, so we might be able to get rid of it.  Keeping it around in case we find it useful.

[WebKit Review Bot]

Attachment 156984 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:165:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:178:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:227:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:228:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:231:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:231:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:88:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:89:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:90:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:91:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 10 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dana Jansens]

Comment on attachment 156984 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:41
> +// If we receive a swap ack after our previous tick pretend like it came before
> +// if it arrived within the following threshold in seconds.

i think this sentence is confused?

[Brian Anderson]

Comment on attachment 156984 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:41
>> +// if it arrived within the following threshold in seconds.
> 
> i think this sentence is confused?

Yes.  Will reword to the following:

// If we receive a swap ack after our previous tick and it arrived within the
// following threshold, we assume the ack was just late, so we retroactively tick.
const double retroactiveTickThreshold = 0.002; // seconds

[John Bates]

Comment on attachment 156984 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:303
> +        double delta = m_lastVsyncTrigger - WTF::monotonicallyIncreasingTime();

WTF::monotonicallyIncreasingTime should be replaced with WebKit::Platform::current()->highResolutionMonotonicallyIncreasingTime() once https://bugs.webkit.org/show_bug.cgi?id=93316 lands.

[Brian Anderson]

Created attachment 157089 [details]
Patch

[Brian Anderson]

Who can I talk with to understand the CCScheduler better?

The CCScheduler and CCFrameRateController aren't interacting the way I thought they interacted.  We'll need to change how they talk to each other so the CCFrameRateController can have better control over the frame rate.  Currently, each m_client->vsyncTick() doesn't necessarily cause a frame to draw.  I think I'd need to add an argument to vsyncTick() that requests a draw, and have it return whether or not the draw actually occurred.  In the case that a draw isn't requested, the CCScheduler can execute other actions like uploading textures.

Also, the patch as-is implements several concepts which may or may not be in the final version of this patch.  They are there primarily for testing and discussion.

[Nat Duca]

What problem are you trying to solve here?

CCScheduler is my code, happy to discuss with you.