Bug 71100 - [chromium] Connect CCThreadProxy to FrameRateController and SchedulerStateMachine via CCScheduler
Summary: [chromium] Connect CCThreadProxy to FrameRateController and SchedulerStateMac...
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:
Blocks:
 
Reported: 2011-10-28 02:20 PDT by Nat Duca
Modified: 2011-10-31 16:58 PDT (History)
4 users (show)

See Also:


Attachments
Patch (28.01 KB, patch)
2011-10-28 02:20 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Patch (50.70 KB, patch)
2011-10-28 17:55 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
nits and tests (73.86 KB, patch)
2011-10-30 18:24 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-28 02:20:22 PDT
[chromium] Connect CCThreadProxy to FrameRateController and SchedulerStateMachine via CCScheduler
Comment 1 Nat Duca 2011-10-28 02:20:56 PDT
Created attachment 112836 [details]
Patch
Comment 2 Nat Duca 2011-10-28 02:21:38 PDT
Initial patch. Still debugging, but the shape should be good enough for a structural "omg this is terrible" review. :)
Comment 3 Nat Duca 2011-10-28 17:55:48 PDT
Created attachment 112952 [details]
Patch
Comment 4 James Robinson 2011-10-28 19:16:10 PDT
Comment on attachment 112952 [details]
Patch

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

have not grokked patch yet, just dumping some initial comments

> Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.h:50
> +    virtual double monotonicallyIncreasingTime() const { return WTF::monotonicallyIncreasingTime() * 1000.0; }

I get what you are doing here but I think it's way too confusing to have this share the name with a function in the global namespace and do something different. If you want to have this return a different unit from the normal WebKit time functions (which return a double in seconds) then pick a different function name. IMO the best thing to do here is to abandon the WTF naming scheme completely and pick our own, which this function will happen to implement on top of WTF::monotonicallyIncreasingTime. The WebKit conventions here are pretty effed up

Additionally it's not too useful to put the definition of a virtual in the .h, even if it's short, because the compiler is unlikely to be able to inline anyway. Just stick the impl in the .cpp.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:60
> +    virtual void didCommitAndDrawFrame(int frameNumber) { }

why's this the only client function with an empty impl? shouldn't it be pure virtual too?

if you do plan to keep it pure virtual comment out the parameter name like this:

virtual void didCommitAndDrawFrame(int /* frameNumber */) { }

to avoid stupid unused variable warnings on the parameter

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:407
> +

extra newline

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:422
> +    m_currentTextureUpdaterOnImplThread->update(m_layerTreeHostImpl->context(), 16);

16's a magic number here. can you make it a named constant?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:127
> +    // Set whent he main thread is waiitng on a readback

typos here "whent he" -> "when the", "waiitng" -> "waiting"

please end comments that are sentences (or fragments) with a period

> Source/WebKit/chromium/tests/CCSchedulerTest.cpp:36
> +// FIXME: do we need tests of the scheduler? Its mostly a passthrough.

i was expecting this to become more than just a passthrough when the scheduler itself gained logic
Comment 5 James Robinson 2011-10-28 23:37:39 PDT
Comment on attachment 112952 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:41
> +    virtual void beginFrame() { m_scheduler->onBeginFrame(); }

does this adapter exist just because CCFrameControllerClient and CCScheduler can't agree on whether this is called beginFrame() or onBeginFrame()? can we just agree on a name and not need this guy?

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:116
> +    // Early out so we don't spam TRACE_EVENTS with useless processScheduledActions

we have traces for all the m_client calls, right? do we really need another trace for processScheduledActions() then?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:479
> +    // Tell the main thread the frame draw, if it was a real main thread frame

i can't quite understand what this comment is trying to say

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:482
> +        m_mainThreadProxy->postTask(createMainThreadTask(this, &CCThreadProxy::didCommitAndDrawFrame, m_layerTreeHostImpl->sourceFrameNumber()));

what do we need to inform the main thread about after we draw? can't it just wait until the next BFAC for any state we need to push?
Comment 6 James Robinson 2011-10-29 15:28:43 PDT
Comment on attachment 112952 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:114
> +void CCScheduler::processScheduledActions(bool insideVSyncTick)

this function takes a bool parameter but all of the callsites pass in a literal value. this makes the callers much harder to read. please use an enum here
Comment 7 Nat Duca 2011-10-30 17:03:57 PDT
> we have traces for all the m_client calls, right? do we really need another trace for processScheduledActions() then?
PSA can do several things in a single cycle, and it is useful when tracing to be able to distingiush one psa group from different ones.

> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:482
> what do we need to inform the main thread about after we draw? can't it just wait until the next BFAC for any state we need to push?

Great question. This is based on a discussion with Antoine and @bauman: we need to tell the RenderWidget to didFlushPaint after a commit has made in order for plugins to work properly with threaded compositing. We dont want to create a new frame in this case --- we just need to ack to the main thread that the frame was drawn.  Two specific use cases:
1. UpdateRect should be sent on this
2. didFlushPaint should be sent on this

The frame number is passed to resolve the race condition where a plugin asks for a commit right after a commit(from previous damage) - in that case, a didCommitAndDraw will arrive for that previous frame. By paying attention to CCLTH::frameNumber and the ack frame number, the client can tell when the frame it requested has actually been submitted to the GPU.
Comment 8 Nat Duca 2011-10-30 18:24:08 PDT
Created attachment 113007 [details]
nits and tests
Comment 9 James Robinson 2011-10-31 13:29:02 PDT
Comment on attachment 113007 [details]
nits and tests

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

R=me, some nits (nearly all spelling)

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:122
> +        default:
> +            ASSERT_NOT_REACHED();

It's better to leave the default case off when switching on an enum if you have a case: for each value. If you leave out a case the compiler will yell at you, instead of it falling through to the default: and being a runtime error.

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:126
> +        // If we were just told to udpate resources, but there

udpate->update

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:127
> +        // are no more pending, then clear

This sentence sort of trails off - clear what?

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.h:58
> +    ~CCScheduler();

virtual

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.h:73
> +    void beginFrame();

virtual

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:372
> +        // point of view, but asynchronously performd on the impl thread,

performd -> performed

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:457
> +    // Check for a pending compositeAndReadback

should we still swap on a compositeAndReadback() ?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:127
> +    // Set whent the main thread is waiing on a readback.

waiing->waiting

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:140
> +    enum { UpdatesPerFrame = 16 };

We typically do static const (numerictype) in the .cpp instead of enum values in the header for constants, especially since this one isn't needed outside ccthreadproxy.cpp. you could even make it a function static in scheduledActionUpdateMoreResources() so it's close to the use

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:142
> +

extra newline

> Source/WebKit/chromium/tests/CCSchedulerTest.cpp:68
> +    // SetNedsCommit should begin the frame.

SetNedsCommit -> SetNeedsCommit
Comment 10 Nat Duca 2011-10-31 16:58:04 PDT
Committed r98915: <http://trac.webkit.org/changeset/98915>