Plumb vsync-enabled flag up to compositor thread and support disable-vsync
Created attachment 154500 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 154500 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154500&action=review > Source/Platform/chromium/public/WebLayerTreeViewClient.h:91 > + virtual bool isRenderingVSynced() const { return true; } Since this is only queried at startup, wouldn't it be easier to expose on WebLayerTreeView::Settings / CCLayerTreeSettings instead? As is it looks like the client could change it dynamically (but it can't).
(In reply to comment #3) > (From update of attachment 154500 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154500&action=review > > > Source/Platform/chromium/public/WebLayerTreeViewClient.h:91 > > + virtual bool isRenderingVSynced() const { return true; } > > Since this is only queried at startup, wouldn't it be easier to expose on WebLayerTreeView::Settings / CCLayerTreeSettings instead? As is it looks like the client could change it dynamically (but it can't). Yeah, I'll try that instead.
Lets see what the settings path looks like.
Created attachment 154767 [details] Patch
LGTM
Comment on attachment 154767 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154767&action=review > Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:57 > + // Disable framerate throttling by TimeSource, and instead render frames as > + // fast as possible. > + void disableTimeSourceThrottling(CCThread*); This feels a bit off - what about having CCThreadProxy simply create a different CCTimeSource and passing that in to the framerate controller? It could either make a delay based time source with an interval of 0 or another simpler implementation that just did postTask
Comment on attachment 154767 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154767&action=review >> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:57 >> + void disableTimeSourceThrottling(CCThread*); > > This feels a bit off - what about having CCThreadProxy simply create a different CCTimeSource and passing that in to the framerate controller? It could either make a delay based time source with an interval of 0 or another simpler implementation that just did postTask Using CCTimeSource would require some slightly ugly changes elsewhere: - CCFrameRateController still needs about the same code minus m_manualTicker. - CCTimeSourceInfinite would have the m_manualTicker code. - CCTimeSource would need a new virtual like "pollForManualTick" or "forceTick", etc. Not sure if the vsync-disabled path should add that much complexity. Another option is to virtualize CCFrameRateController and have a vsync-disabled version... That would have the complexity of sharing the m_numFramesPending logic while virtualizing didBeginFrame and didFinishFrame, but might be worth it. Wdyt?
Created attachment 155068 [details] Patch
Comment on attachment 155068 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155068&action=review Could you update CCFrameRateControllerTest.cpp with at least some basic tests of this class? > Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:69 > + // TimeSource ticks are not used in this mode, but we still use it to track > + // active state, so make one that ticks slowly. I don't know what this means - it looks like we post a task whenever the "active" state changes, so I can't figure out why we need another timer. Is this just because the m_timeSource stores the "active" bool? We could keep that as a member on CCFrameRateController if we want
Comment on attachment 155068 [details] Patch I think we need at least a bit of test coverage for the new CCFrameRateController path before landing.
Created attachment 155093 [details] Patch
Comment on attachment 155068 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155068&action=review Added a test for unthrottled behavior. >> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:69 >> + // active state, so make one that ticks slowly. > > I don't know what this means - it looks like we post a task whenever the "active" state changes, so I can't figure out why we need another timer. Is this just because the m_timeSource stores the "active" bool? We could keep that as a member on CCFrameRateController if we want Done.
Comment on attachment 155093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155093&action=review R=me, just one question for you before setting cq? > Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:91 > + postManualTick(); do we need to post a tick if we aren't active?
Comment on attachment 155093 [details] Patch Nice job!
Comment on attachment 155093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155093&action=review >> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:91 >> + postManualTick(); > > do we need to post a tick if we aren't active? no, but postManualTick early-outs if we aren't active so call-sites don't need the check. otoh, this would probably look clearer if it checked before calling. I'll post a new CL.
Created attachment 155321 [details] Patch
Comment on attachment 155321 [details] Patch Clearing flags on attachment: 155321 Committed r124097: <http://trac.webkit.org/changeset/124097>
All reviewed patches have been landed. Closing bug.