Bug 70955 - Add CCSmoothedCallback class for smoothing out jittery vsync
Summary: Add CCSmoothedCallback class for smoothing out jittery vsync
Status: CLOSED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 71479
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-26 11:51 PDT by Iain Merrick
Modified: 2012-07-31 01:32 PDT (History)
4 users (show)

See Also:


Attachments
Patch (13.79 KB, patch)
2011-10-26 11:53 PDT, Iain Merrick
no flags Details | Formatted Diff | Diff
Patch (32.24 KB, patch)
2011-10-27 11:11 PDT, Iain Merrick
no flags Details | Formatted Diff | Diff
Patch (53.27 KB, patch)
2011-10-28 09:38 PDT, Iain Merrick
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Iain Merrick 2011-10-26 11:51:55 PDT
Add FakeCCThread that supports multiple in-flight tasks
Comment 1 Iain Merrick 2011-10-26 11:53:11 PDT
Created attachment 112570 [details]
Patch
Comment 2 Iain Merrick 2011-10-26 11:57:53 PDT
For context, this is related to https://bugs.webkit.org/show_bug.cgi?id=70713

I have started working on a jitter-smoothing Vsync callback handler that looks like this:

----

class CCSmoothedCallbackClient {
public:
    virtual void onTick();
};

// Smooths out a series of irregular callbacks while maintaining the same phase.
// We assume that the callbacks arrive at roughly the specified period, but with
// random delays of up to half a cycle. We attempt to remove the delays.
// TODO(husky): We could also infer the interval from the incoming ticks.
class CCSmoothedCallback {
public:
    CCSmoothedCallback(CCSmoothedCallbackClient* client, double interval, CCThread* thread);

    void tick();
};

----

I copied FakeCCThread from the other patch, but found I wanted to be able to post more than one task at a time. For example, if interval=10, the first call to tick() should result in:

postTask(client, &onTick)
postDelayedTask(client, &onTick, 10)

And the next onTick would be scheduled when the next tick() comes in.
Comment 3 Iain Merrick 2011-10-26 12:02:24 PDT
The "half a cycle" thing is a pretty random requirement, might want to think a bit bigger. We could have platforms where our 60fps "vsync" callbacks come in bursts of three every 50ms, say.
Comment 4 Nat Duca 2011-10-26 17:50:45 PDT
Comment on attachment 112570 [details]
Patch

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

Neat point Iain. Does the CCMainThread aspect of this appeal to you at all? Its not fun, but I think it'd move us in a nice direction overall.

> Source/WebKit/chromium/tests/FakeCCThread.h:39
> +    long long getTime() const;

We use double elsewhere, so we can get precision. The postTask takes a long long because it only understand that, but queuing could introduce 

I support it being on ccthread, but we would need to be able to write code saying "GetTheCurrentTime" that would work on main thread too. We need that anyway because I'd like the same CCDelayBasedTimeSource code to work regardless of running on mainthread or ccthread. I think a good way to do this is do away with CCMainThread class and instead have have a global somewhere in the compositor which is s_mainThread. The embeddor of the compositor, e.g WebKit/src/chromium could set up CCMainThread just like it does for the impl thread.

> Source/WebKit/chromium/tests/FakeCCThread.h:45
> +    void incrementTime(long long);

advanceTime

> Source/WebKit/chromium/tests/FakeCCThread.h:49
> +    bool runNextTask(long long);

also a runall?
Comment 5 Iain Merrick 2011-10-27 04:07:18 PDT
Comment on attachment 112570 [details]
Patch

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

>> Source/WebKit/chromium/tests/FakeCCThread.h:39
>> +    long long getTime() const;
> 
> We use double elsewhere, so we can get precision. The postTask takes a long long because it only understand that, but queuing could introduce 
> 
> I support it being on ccthread, but we would need to be able to write code saying "GetTheCurrentTime" that would work on main thread too. We need that anyway because I'd like the same CCDelayBasedTimeSource code to work regardless of running on mainthread or ccthread. I think a good way to do this is do away with CCMainThread class and instead have have a global somewhere in the compositor which is s_mainThread. The embeddor of the compositor, e.g WebKit/src/chromium could set up CCMainThread just like it does for the impl thread.

Hmm, there are a bunch of issues here, let's discuss it in person.

I'm suspicious of new globals, but I need to think it through. I like the idea of the main thread and the cc thread just being instances of some common Thread class, not different types. We'd be introducing more Chrome-style threading into WebKit, which feels like the right direction.

>> Source/WebKit/chromium/tests/FakeCCThread.h:45
>> +    void incrementTime(long long);
> 
> advanceTime

Done

>> Source/WebKit/chromium/tests/FakeCCThread.h:49
>> +    bool runNextTask(long long);
> 
> also a runall?

Good idea, done.
Comment 6 Iain Merrick 2011-10-27 11:11:20 PDT
Created attachment 112712 [details]
Patch
Comment 7 Iain Merrick 2011-10-27 11:14:08 PDT
Also includes FakeCCThread, which could be submitted separately.

CCTimeSource.h copied from https://bugs.webkit.org/show_bug.cgi?id=70713 (which should go in first)
Comment 8 Nat Duca 2011-10-28 02:45:11 PDT
Comment on attachment 112712 [details]
Patch

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

I like FakeCCThread, thats pretty cool. Lets peel it off to another patch, regardless. Even if you just shove it up on a webkit bug and assign me owner so we don't lose it, that'd be cool. Long term, I think it belongs with the CCMainThread->CCThread patch... 

I think this algorithm is fascinating. But, there's a lot of code duplication between DelayBasedTimeSource and this, basically because they'e both using a delayed task to build the signal.

Can we rephrase the algorithm as a mode on the CCDelayBasedTimSource, or move the existing algorithm plus this to another subclass?

I'd be fine with setMode(...) and have a few fields on DBTS that aren't used in one mode or the other.

One thing I've been meaning to do is make a variant of the DBTS "achieves60Hz" test that adds noise to the delays. Maybe you can expand on that? I suspect that delay errors are gamma-distributed.

> Source/WebCore/WebCore.gypi:3564
> +            'platform/graphics/chromium/cc/CCSmoothedCallback.cpp',

Lets give this a name that brings it close to CCTimeSource. CCSwapCallbackTimesource?

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedCallback.cpp:36
> +CCSmoothedCallback::CCSmoothedCallback(double interval, CCThread* thread)

This is based on swapbuffers ack right? /me wonders if there is a clean abstraction so that the graphics context owner (LayerRendererChromium) can create this kind of timesource but then hand it off to the frame rate controller? Its fine if such an abstraction doesn't exist, but this patch in particular pokes at the awkwardness of the plumbing from LayerRendererChromium back to CCFrameRateController.
Comment 9 Iain Merrick 2011-10-28 09:38:20 PDT
Created attachment 112877 [details]
Patch
Comment 10 Iain Merrick 2011-10-28 09:40:22 PDT
Updated after conversation with Nat.

This is now a pure adapter: it takes a CCTimedSource, wraps it, and implements CCTimedSource.
Comment 11 Iain Merrick 2011-11-01 04:42:58 PDT
Ping?

To address Nat's comment above, yes, the idea is that this would be driven from the SwapBuffers ack and somehow injected into the CCScheduler. I haven't figured out exactly how that would work but it would be a separate patch anyway.
Comment 12 James Robinson 2011-11-01 14:03:37 PDT
Comment on attachment 112877 [details]
Patch

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

Have you verified if this behaves significantly better/differently from the timer based source?

> Source/WebCore/platform/graphics/chromium/cc/CCThread.h:51
> +    // Get this thread's monotonically-increasing millisecond clock.

This doesn't make sense to me. the time is not a property of the thread.
Comment 13 Iain Merrick 2011-11-02 04:28:58 PDT
(In reply to comment #12)
> (From update of attachment 112877 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112877&action=review
> 
> Have you verified if this behaves significantly better/differently from the timer based source?

Well, it locks phase with an external signal? CCDelayBasedTimeSource has no way of staying in phase.

Maybe CCSmoothedTimeSource should actually *use* CCDelayBasedTimeSource, or subclass it. But I'm not sure that would make the code simpler.

> > Source/WebCore/platform/graphics/chromium/cc/CCThread.h:51
> > +    // Get this thread's monotonically-increasing millisecond clock.
> 
> This doesn't make sense to me. the time is not a property of the thread.

It has to be a virtual somewhere for testability, and it's significantly simpler to have it here than elsewhere. For example, if it's a virtual in each class that calls monotonicallyIncreasingTime(), each of those classes needs its own subclass for testing. If it's an overridable global function, tests will interfere with each other.

I agree it looks a bit odd here, but I think it's somewhat justifiable, as the thread already has postDelayedTask() which uses the same units. postDelayedTask() and monotonicallyIncreasingTime() are closely related and frequently used together.
Comment 14 Iain Merrick 2011-11-03 09:41:08 PDT
Splitting this patch up to make it easier to digest.

I have moved FakeCCThread into https://bugs.webkit.org/show_bug.cgi?id=71479
Comment 15 Adam Barth 2012-07-27 01:09:14 PDT
Comment on attachment 112877 [details]
Patch

Cleared review? from attachment 112877 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).
Comment 16 Nat Duca 2012-07-30 10:38:33 PDT
Iain, lets close this WontFix?
Comment 17 Iain Merrick 2012-07-31 01:32:20 PDT
Yep, done. Sorry, hadn't realised that Resolved and Closed are different states.