Bug 70955 – Add CCSmoothedCallback class for smoothing out jittery vsync

• Status: CLOSED
• Resolution: WONTFIX
• Component: New Bugs

[Iain Merrick]

Add FakeCCThread that supports multiple in-flight tasks

[Iain Merrick]

Created attachment 112570 [details]
Patch

[Iain Merrick]

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.

[Iain Merrick]

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.

[Nat Duca]

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?

[Iain Merrick]

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.

[Iain Merrick]

Created attachment 112712 [details]
Patch

[Iain Merrick]

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)

[Nat Duca]

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.

[Iain Merrick]

Created attachment 112877 [details]
Patch

[Iain Merrick]

Updated after conversation with Nat.

This is now a pure adapter: it takes a CCTimedSource, wraps it, and implements CCTimedSource.

[Iain Merrick]

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.

[James Robinson]

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.

[Iain Merrick]

(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.

[Iain Merrick]

Splitting this patch up to make it easier to digest.

I have moved FakeCCThread into https://bugs.webkit.org/show_bug.cgi?id=71479

[Adam Barth]

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).

[Nat Duca]

Iain, lets close this WontFix?

[Iain Merrick]

Yep, done. Sorry, hadn't realised that Resolved and Closed are different states.