Bug 70713 - [chromium] Implement frame rate control portions of CCScheduler
Summary: [chromium] Implement frame rate control portions of CCScheduler
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: 70712
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-24 01:44 PDT by Nat Duca
Modified: 2011-10-27 23:50 PDT (History)
5 users (show)

See Also:


Attachments
Patch (30.01 KB, patch)
2011-10-24 01:44 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Patch (33.44 KB, patch)
2011-10-25 02:41 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Patch (32.92 KB, patch)
2011-10-25 02:45 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
sigh (32.89 KB, patch)
2011-10-25 02:59 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Patch (34.96 KB, patch)
2011-10-25 14:44 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
MakeItBuild (34.92 KB, patch)
2011-10-25 16:23 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
. (34.93 KB, patch)
2011-10-25 20:08 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Cleanup (38.93 KB, patch)
2011-10-26 17:01 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-24 01:44:10 PDT
[chromium] Implement frame rate control portions of CCScheduler
Comment 1 Nat Duca 2011-10-24 01:44:32 PDT
Created attachment 112161 [details]
Patch
Comment 2 Nat Duca 2011-10-24 01:57:37 PDT
I expect that this patch is going to explode on the EWS bots because it needs CCThread::postDelayedTask: b70712.

The goal of this patch is to establish the right infrastructure for the "time" bits of the scheduler, but not implement the exact ideal algorithm. There are two parts:

* CCSmoothedTimer: ticks at an average rate given that the underlying delayed task mechanism ticks only in 1ms increments.
* CCFrameRateController: built on top of the smoothed timer, the FRC tracks pending frames and converts ticks into "draw calls" (so long as < 2 frames are in flight).

In the future:
* We can put fancier logic for minimizing clock drift into the smoothed timer. Now, I use the 16, 16, 17 technique because my quick attempts to do better ran into difficulties.

* Slowdown logic can be added to FrameRateController, e.g. "we're consistently drawing at 30Hz, so stop trying to draw at 60Hz."
Comment 3 WebKit Review Bot 2011-10-24 02:02:59 PDT
Comment on attachment 112161 [details]
Patch

Attachment 112161 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10198814
Comment 4 James Robinson 2011-10-24 13:43:58 PDT
Comment on attachment 112161 [details]
Patch

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

First round of comments, mostly nitpicks.

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:37
> +    // Dont forward the tick if we are throttled.

Dont -> Don't

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:38
> +    if (m_pendingFrameBeginTimes.size() >= 2)

This seems to be holding us to having 2 frames pending at any given point in time.  It's a bit subtle IMO that this logic is here, also this is a bit of a magic number.  Can you turn '2' into a well-named constant?

If we only support a fixed number of pending frames using a Deque<> seems like massive overkill. Why not just use a double[]?

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:53
> +    /* FIXME: do something with the the delta between now and frameBeginTime.

we don't typically use c-style comments

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:36
> +class CCFrameRateControllerClient {

protected d'tor here please, so people don't try to delete through a CCFrameRateControllerClient*

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:41
> +class CCFrameRateController : public CCTimerClient {

this has virtual functions but no virtual d'tor - i sense danger

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:55
> +    /* Use the following methods to adjust target frame rate.

another c-style comment

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:66
> +    // CCTimerClient implementation. Do not call directly.

if it shouldn't be called directly does it need to be public?

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:67
> +    void onTimerTick();

virtual keyword, please

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:34
> +class CCTimerClient {

please declare a protected virtual empty d'tor - right now if someone tries to delete a CCTimerClient through a CCTimerClient* they will leak

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:39
> +/*

we don't normally use c-style comments

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:48
> +    CCSmoothedTimer(int goalRate, CCThread* thread)

'goalRate' isn't super clear. What's the unit of 'rate'?  Is this target FPS? Target delay between frames in millis?

Is 'int' precise enough? If this is target FPS some displays might have non-integral framerates

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:58
> +    void getGoalRate(int goalRate) { m_goalRate = goalRate; }

this looks like a setter, not a getter

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:62
> +    virtual double getCurrentTime() const { return monotonicallyIncreasingTime(); }

we don't use 'get' prefix for getters

> Source/WebKit/chromium/tests/CCFrameRateControllerTest.cpp:44
> +    void onVSyncTick() { m_tickCalled = true; }

virtual

> Source/WebKit/chromium/tests/CCSchedulerTestCommon.h:42
> +    void onTimerTick() { m_tickCalled = true; }

virtual keyword please
Comment 5 Adrienne Walker 2011-10-24 14:53:36 PDT
Comment on attachment 112161 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:58
> +    } else if (m_goalRate == 30) {
> +        if (!m_tickStep)
> +            delay = 33;
> +        else
> +            delay = 32;

Er, don't you mean 34 and 33, rather than 33 and 32?

(All this logic seems a bit dodgy.  Wouldn't you start drifting if task posting timing is not perfect?)
Comment 6 Nat Duca 2011-10-24 16:03:52 PDT
Ah, makes sense, thanks for sorting me out.

Two high level things:
1. Lets hold off making this webcore wide. Amongst other things, we might need to go from category strings to something more like LogChannels so it makes more sense to other ports.
2. You'll need two types for every type you introduce in your solution: the WebCore type, and the type you add to WebKit/public/ that you use to get from WebKit back to Chromium. That reality might influence your API design a bit...
Comment 7 Nat Duca 2011-10-24 16:05:28 PDT
(In reply to comment #6)

@*%&@)(* wrong friggin bug. @)@&)*@)$#
Comment 8 Nat Duca 2011-10-24 16:59:40 PDT
> If we only support a fixed number of pending frames using a Deque<> seems like massive overkill. Why not just use a double[]?

Yes it is, but its not performance critical code, and to do [], I need more error prone code. Can we keep it pretty please?

> Frame rates

I would like to start with integral frame rates, actually. If there is something that doesn't use that, we can pass in a ratio instead. E.g. 89.285714hz is 480000 / 5376. The reason for this is I'm trying to avoid numerical precison errors.


> The voodoo with 16 17 17
Stand by for a better patch, with a cooler algorithm.
Comment 9 Nat Duca 2011-10-25 02:41:53 PDT
Created attachment 112309 [details]
Patch
Comment 10 Nat Duca 2011-10-25 02:45:00 PDT
Created attachment 112310 [details]
Patch
Comment 11 Nat Duca 2011-10-25 02:46:03 PDT
(In reply to comment #10)
> Created an attachment (id=112310) [details]
> Patch

Note, I gave up on the idea of using integer math for this. It was a stupid idea. :)
Comment 12 WebKit Review Bot 2011-10-25 02:50:00 PDT
Comment on attachment 112310 [details]
Patch

Attachment 112310 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10209201
Comment 13 Nat Duca 2011-10-25 02:59:02 PDT
Created attachment 112312 [details]
sigh
Comment 14 Vangelis Kokkevis 2011-10-25 13:15:31 PDT
Comment on attachment 112312 [details]
sigh

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

Looks good!

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:61
> +    double now = currentTime();

nit: you could move this up and use now in line 57 instead of calling currentTime() twice

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:66
> +    double amountLate = now - m_nextTickTime;

Should you be worried about timer wrap-around?

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:70
> +        // is our tolerance factor for "rasonable" delays due to the thread being busy.

typo: rasonable
Comment 15 James Robinson 2011-10-25 13:42:50 PDT
Comment on attachment 112312 [details]
sigh

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

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:48
> +    CCFrameRateController(PassOwnPtr<CCSmoothedTimer>);

explicit

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:31
> +#include <math.h>

are we using this include?

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:62
> +    const float maxLateBeforeDroppingFrame = 2.5;

why float? we're about to compare it to a double

could be static

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:65
> +    bool tick;

Could you initialize this here? Or just drop it if you drop the 'else if' clause below?

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:68
> +    if (amountLate >= -1.0 && amountLate <= maxLateBeforeDroppingFrame) {
> +        // Timer was (mostly) on time. We penalize to -1.0 fixed on the early side because

Wouldn't this be a lot easier to reason about if we floored ourselves?

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:76
> +    } else if (amountLate < -1.0) {
> +        // Timer came back really early. Unlikely but better to handle then explode.
> +        newNextTickTime = m_nextTickTime;
> +        tick = false;

This should be impossible now that the timers are all base::TimeTicks based - the code in render_widget was to deal with base::Time / base::TimeTicks mismatches. I think you want to ASSERT() at a minimum here if you want to write code at all

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:59
> +    void setInterval(double intervalMs)

Is anything else supposed to happen when the interval changes? What if the interval is 50ms, the last tick was 30ms ago, and the caller sets the interval to 20? Should they get a callback sooner?

I don't see any callers of this in this patch.

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:67
> +    virtual double currentTime() const { return monotonicallyIncreasingTime(); }

This is super confusing - there's a function in the global namespace from wtf/CurrentTime.h called 'currentTime()' which happens to have really bad behavior for what we want here. Either match the name of the function you're wrapping or come up with another name
Comment 16 Nat Duca 2011-10-25 14:41:20 PDT
Comment on attachment 112312 [details]
sigh

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

>> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:66
>> +    double amountLate = now - m_nextTickTime;
> 
> Should you be worried about timer wrap-around?

currentTime() would be toast by that point, right?

>> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:68
>> +        // Timer was (mostly) on time. We penalize to -1.0 fixed on the early side because
> 
> Wouldn't this be a lot easier to reason about if we floored ourselves?

Not sure I follow.
Comment 17 Nat Duca 2011-10-25 14:44:28 PDT
Created attachment 112408 [details]
Patch
Comment 18 WebKit Review Bot 2011-10-25 14:56:11 PDT
Comment on attachment 112408 [details]
Patch

Attachment 112408 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10208490
Comment 19 Nat Duca 2011-10-25 16:23:47 PDT
Created attachment 112426 [details]
MakeItBuild
Comment 20 WebKit Review Bot 2011-10-25 16:43:28 PDT
Comment on attachment 112426 [details]
MakeItBuild

Attachment 112426 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10213362
Comment 21 James Robinson 2011-10-25 17:43:30 PDT
Comment on attachment 112426 [details]
MakeItBuild

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

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:91
> +    if (tick && m_client)

there is no tick

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:62
> +    virtual double monotonicallyIncreasingTime() const { return monotonicallyIncreasingTime(); }

this is a bit of a trip. can you say "return ::monotonicallyIncreasingTime()" or "return WTF::monotonicallyIncreasingTime()" to make it a bit more human-parseable?
Comment 22 Nat Duca 2011-10-25 18:58:31 PDT
(In reply to comment #21)
> there is no tick

But fortunately, there is also no spoon.

Sorry, couldn't resist.
Comment 23 Nat Duca 2011-10-25 20:08:57 PDT
Created attachment 112442 [details]
.
Comment 24 James Robinson 2011-10-25 22:47:52 PDT
Comment on attachment 112442 [details]
.

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

All nits although OwnPtr<T*> is pretty strange looking. R=me but please check that carefully.

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:31
> +#include <math.h>

if you aren't using this #include remove it

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:46
> +            : m_frameRateController(frameRateController) { }

this indentation is odd - would expect 4-space from prev line

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:75
> +    OwnPtr<CCSmoothedTimer*> m_timer;

why is this an OwnPtr<> to a pointer? is this a typo? How does it compile?

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:58
> +    double now = monotonicallyIncreasingTime();

nit: move this below the early return please

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:73
> +        // Timer was (mostly) on time. We penalize to -1.0 fixed on the early side because

I think I've finally grokked this but it took me a while :)  Please let me know if my understanding here is correct: The idea here is if the timer callback rate is 'close enough' to the specified interval rate, then we just keep ticking using the same timebase. We rely on the floor behavior to never drift too early, and if we start drifting too late we reset the timebase rather than pass excessively short intervals.  I think the 'penalize' wording here made it hard to see what's going on.
Maybe you could provide a brief example here of how this code would behave in some simple situations?  For example with an interval of 16.67, infinitely fast frames, a completely uncontested thread, and perfectly accurate timers this would go something like:

STATE_STARTING, now=0: m_nextTickTime = 0, m_state -> STATE_ACTIVE
amountLate = 0
newNextTimeTick = 16.67
postTickTask(floor(16.67 - 0) = 16)

STATE_ACTIVE, now=16:
amountLate = -0.67
newNextTimeTick = 33.34
postTickTask(floor(33.34 - 16) = 17)

STATE_ACTIVE, now=33:
amountLate = -0.34
newNextTimeTick = 50.0
postTickTask(floor(50.0 - 33) = 17)

etc.

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:50
> +            : m_client(0)

odd indent here too, would normally be 4-space from previous line

can you put this in the .cpp?

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:300
> -        webkit_support::PostDelayedTask(CCLayerTreeHostTest::onBeginTest, static_cast<void*>(this), 0);
> +         webkit_support::PostDelayedTask(CCLayerTreeHostTest::onBeginTest, static_cast<void*>(this), 0);

bizarre space here

> Source/WebKit/chromium/tests/CCSchedulerTestCommon.h:31
> +#include <wtf/CurrentTime.h>

don't think you need this include either

> Source/WebKit/chromium/tests/CCSchedulerTestCommon.h:88
> +    FakeCCSmoothedTimer(double i, WebCore::CCThread* t)
> +        : CCSmoothedTimer(i, t)

might as well expand these variable names
i->intervalMs
t->thread
Comment 25 James Robinson 2011-10-25 22:49:12 PDT
Also I still don't really see what the Deque is gonna be for.
Comment 26 Nat Duca 2011-10-26 03:15:32 PDT
Great feedback overall, I'll add some explanation based (heavily) on what you wrote [nice job!], and get a new patch up. :)


(In reply to comment #25)
> Also I still don't really see what the Deque is gonna be for.

Maybe you missed my earlier comment amongst my flailing. The operation we are using is a queue: we push frames in, we pop them out. We also use the queue size to make decisions about dropping a tick. 
We use a dequeue in the rate limiting implementation too, gpu/client/gles2_implemenation.cc as well as in the GpuScheduler for the mac swapbuffer rate limiter.

I'm using WTF::Dequeue because I wasn't sure if it was safe to use stl queue directly. I don't need double-endedness.

I could also use a vector, frankly, and push_back/pop_front it. Its gonna be 2-long so NBD.

Are you wondering about dequeue vs another data structure? Or any stl/wtf data structure versus hand-coding it? I'm confused...
Comment 27 Iain Merrick 2011-10-26 06:35:16 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=112442&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:36
> +class CCFrameRateControllerClient {

Maybe make this a nested class? i.e. CCFrameRateController::Client

I'm not sure if the style guide prefers it one way or the other, but a nested class would make the code a lot shorter in some places, with little impact elsewhere.

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:48
> +    explicit CCFrameRateController(PassOwnPtr<CCSmoothedTimer>);

I think this is where my idea of smoothing out a jittery SwapBuffers callback would fit in.

Instead of taking a CCSmoothedTimer argument, CCFrameRateController could just expose a public onTimerTick() method. Then the caller can choose to drive that from a CCSmoothedTimer, from a SwapBuffers callback, or something else.

And maybe the method names should be switched around:

// Call this once per screen refresh
void CCFrameRateController::onVSyncTick()

// CCFrameRateController calls this when we should draw a new frame.
// It will be synced with screen refresh, maybe throttled by an integral factor.
virtual void CCFrameRateControllerClient::onAnimationFrameTick()

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:79
> +        newNextTickTime = now + m_intervalMs;

If the thread is overworked, the timer will end up running at a slightly lower rate, but wouldn't it be better to drop to exactly 50% frame rate? Or is that CCFrameRateController's job?

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:81
> +        // Timer came back impossibly early. Sanity check only.

Does this really need its own case? Could just DCHECK(amountLate >= -1.0) on line 73.

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:42
> +// This timer implements a time source that achieves the specified interval

Unless I'm missing something, WebCore/platform/Timer.h does this too. It has an m_nextFireTime which is incremented by m_repeatInterval each time it runs. The key difference here isn't really the smoothing, but the fact that we use CCThread rather than being tied to the main thread.

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:49
> +    CCSmoothedTimer(double intervalMs, CCThread* thread)

For consistency with the rest of WebKit and Chrome, I'd expect a double to be seconds, and a long to be milliseconds (or ticks). Since you're working in doubles here, maybe just use seconds everywhere?

> Source/WebKit/chromium/tests/CCSchedulerTestCommon.h:91
> +    void setMonotonicallyIncreasingTime(double time) { m_monotonicallyIncreasingTime = time; }

Would be convenient in these tests to have an incrementTime() method.
Comment 28 Iain Merrick 2011-10-26 07:53:09 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=112442&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:62
> +    virtual double monotonicallyIncreasingTime() const { return WTF::monotonicallyIncreasingTime(); }

In both tests and production code, this is closely coupled with CCThread::postDelayedTask.

Maybe this should actually be a virtual method on CCThread. The real CCThread can use the real clock, the fake CCThread a fake clock, and everyone's happy.
Comment 29 Adrienne Walker 2011-10-26 09:26:09 PDT
Comment on attachment 112442 [details]
.

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

>> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:75
>> +    OwnPtr<CCSmoothedTimer*> m_timer;
> 
> why is this an OwnPtr<> to a pointer? is this a typo? How does it compile?

OwnPtr<T*> is identical* to OwnPtr<T> in what it stores, what gets deleted, and how it interacts with PassOwnPtr through the magic of templates.  It's just bad style, not incorrect.

* For nearly all types. HBITMAP (i.e. void*) and some other Windows-only types have specific cleanup functions that get invoked.
Comment 30 James Robinson 2011-10-26 11:04:41 PDT
(In reply to comment #27)
> View in context: https://bugs.webkit.org/attachment.cgi?id=112442&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:36
> > +class CCFrameRateControllerClient {
> 
> Maybe make this a nested class? i.e. CCFrameRateController::Client
> 
> I'm not sure if the style guide prefers it one way or the other, but a nested class would make the code a lot shorter in some places, with little impact elsewhere.
> 

You can't forward declare an inner class, which is why we typically don't use them for cases like this.
Comment 31 James Robinson 2011-10-26 11:15:08 PDT
> (In reply to comment #25)
> > Also I still don't really see what the Deque is gonna be for.
> 
> Maybe you missed my earlier comment amongst my flailing. The operation we are using is a queue: we push frames in, we pop them out. We also use the queue size to make decisions about dropping a tick. 
> We use a dequeue in the rate limiting implementation too, gpu/client/gles2_implemenation.cc as well as in the GpuScheduler for the mac swapbuffer rate limiter.
> 
> I'm using WTF::Dequeue because I wasn't sure if it was safe to use stl queue directly. I don't need double-endedness.
> 
> I could also use a vector, frankly, and push_back/pop_front it. Its gonna be 2-long so NBD.
> 
> Are you wondering about dequeue vs another data structure? Or any stl/wtf data structure versus hand-coding it? I'm confused...

It looks like this isn't input into the current scheme and it's unclear exactly how it will interact with everything else. Could you just add the support when you add the logic using it (and the relevant tests)?
Comment 32 Nat Duca 2011-10-26 14:55:07 PDT
(In reply to comment #27)
> View in context: https://bugs.webkit.org/attachment.cgi?id=112442&action=review
> 
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:48
> > +    explicit CCFrameRateController(PassOwnPtr<CCSmoothedTimer>);
> 
> I think this is where my idea of smoothing out a jittery SwapBuffers callback would fit in.

I added a CCTimeSource. Something based on a swapbuffers or vsync would be a subclass of a timesource. The thing formerly called CCSmoothedTimer is now called CCDelayBasedTimeSource.

> And maybe the method names should be switched around:
I did some changes here. I think you'll like it.

> If the thread is overworked, the timer will end up running at a slightly lower rate, but wouldn't it be better to drop to exactly 50% frame rate?

That's the job of CCFrameRateController's job. The timesource gives ticks, the framerate controller decides to ignore them. :)

A *key* problem is deciding when the thread (or the GPU) is overworked. We lack metrics to determine this at runtime.

> > Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:81
> > +        // Timer came back impossibly early. Sanity check only.
> 
> Does this really need its own case? Could just DCHECK(amountLate >= -1.0) on line 73.
Doh. Thanks man, this is SO much cleaner now.

> > Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:42
> > +// This timer implements a time source that achieves the specified interval
> 
> Unless I'm missing something, WebCore/platform/Timer.h does this too.

Its really that this is CCThread-coupled, platform-coupled. That has a few non-obvious consequences around the timebase [monotonic time vs webkit seconds, which are not the same iirw].

> > Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:49
> > +    CCSmoothedTimer(double intervalMs, CCThread* thread)
> For consistency with the rest of WebKit and Chrome, I'd expect a double to be seconds, and a long to be milliseconds (or ticks).
Again, the disconnect is that currentTime() uses a wall-time timebase. The monotonic time uses a special timebase that we guarantee to not move backward, even when the clock changes. It also doesn't count from the epoch, which gives increased numeric precision down in the micro/nano range --- epoch based doubles can't count below 0.3microsecnds.


> CCThread::currentTime()
I buy it. But we should make this work for CCMainThread, too. I think that can be done by having CCMainThread implement CCThread as well. I'm open to a patch to do this, as I agree it is cleaner.
Comment 33 Nat Duca 2011-10-26 17:01:48 PDT
Created attachment 112621 [details]
Cleanup
Comment 34 James Robinson 2011-10-26 23:10:55 PDT
Comment on attachment 112621 [details]
Cleanup

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

R still =me. I like the explanation a lot.

> Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.cpp:84
> +// We treat delays in tasks differently depending on the amount of delay we encouter. Suppose we

typo 'encouter'->'encounter'. 'encouter' sounds like a completely different sort of thing entirely

> Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.h:40
> +// NOTE: Deleting CCDelayBasedTimeSource when a tick is pending will lead to a crash.
> +//      To fix this, we need to generalize CCScopedMainThreadProxy.

instead of just saying that this crashes, i think you can fix this by making this class refcounted and calling ref() in postTickTask() and deref() before returning from onTick(). it's not much code and one less headache to deal with later
Comment 35 Nat Duca 2011-10-27 23:50:39 PDT
Committed r98700: <http://trac.webkit.org/changeset/98700>