[chromium] Implement frame rate control portions of CCScheduler
Created attachment 112161 [details] Patch
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 on attachment 112161 [details] Patch Attachment 112161 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10198814
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 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?)
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...
(In reply to comment #6) @*%&@)(* wrong friggin bug. @)@&)*@)$#
> 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.
Created attachment 112309 [details] Patch
Created attachment 112310 [details] Patch
(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 on attachment 112310 [details] Patch Attachment 112310 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10209201
Created attachment 112312 [details] sigh
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 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 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.
Created attachment 112408 [details] Patch
Comment on attachment 112408 [details] Patch Attachment 112408 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10208490
Created attachment 112426 [details] MakeItBuild
Comment on attachment 112426 [details] MakeItBuild Attachment 112426 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10213362
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?
(In reply to comment #21) > there is no tick But fortunately, there is also no spoon. Sorry, couldn't resist.
Created attachment 112442 [details] .
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
Also I still don't really see what the Deque is gonna be for.
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...
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.
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 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.
(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.
> (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)?
(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.
Created attachment 112621 [details] Cleanup
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
Committed r98700: <http://trac.webkit.org/changeset/98700>