Bug 71040 – [chromium] WebCompositor::setDisplayRate

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebCore Misc.

[Nat Duca]

Add a method to WebKit/chromium/public/WebCompositor to indicate the display's refresh rate.

[Iain Merrick]

I'm having trouble figuring out a nice way to do this. Adding the method is obviously trivial, but how does the display rate propagate to CCScheduler, which is where it will be needed?

Currently, WebCompositorImpl has pointers to a WebCompositorClient (implemented by CompositorThread) and a CCScrollController (implemented by CCLayerTreeHostImpl). The WebCompositor is *owned* by CCThreadProxy, but that class only accesses it via the CCInputHandler interface.

CCScheduler is owned by CCThreadProxy, so either WebCompositorImpl needs a pointer back to CCThreadProxy, or a direct pointer to CCScheduler. Either way, it's a third callback interface added to a simple class. WebCompositorImpl would be nothing more than pure plumbing, routing a few messages to different places. It feels like the OO design isn't quite right: it doesn't have a role other than "forward calls to the right place".

The plumbing would be cleaner if we added setDisplayRate to CCScrollController, and implemented that interface in CCThreadProxy instead of CCLayerTreeHostImpl. CCThreadProxy would then act as the central dispatcher, delegating to CCLayerTreeHostImpl or to CCScheduler as appropriate, which makes sense because it actually owns those objects.

Is setDisplayRate really related to a method like scrollRootLayer? Maybe so, because we need to know the display rate for smooth scrolling.

I'll give it a try, anyway.

[Iain Merrick]

Created attachment 113059 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Iain Merrick]

Not ready to submit -- I think it would make sense to wait for https://bugs.webkit.org/show_bug.cgi?id=71100 so I can do that TODO. But would appreciate feedback on whether this is a good direction.

[Darin Fisher (:fishd, Google)]

Comment on attachment 113059 [details]
Patch

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

> Source/WebKit/chromium/public/WebCompositor.h:47
> +    virtual void setDisplayRate(double framesPerSecond) { }

can this be made pure virtual too?  i think this implies implementing it
in WebCompositorImpl.{h,cpp}.

is this method only ever intended to be called on the compositor thread?

[Iain Merrick]

(In reply to comment #5)
> (From update of attachment 113059 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113059&action=review
> 
> > Source/WebKit/chromium/public/WebCompositor.h:47
> > +    virtual void setDisplayRate(double framesPerSecond) { }
> 
> can this be made pure virtual too?  i think this implies implementing it
> in WebCompositorImpl.{h,cpp}.

Oops! Yes it can. I'll fix that now.

> is this method only ever intended to be called on the compositor thread?

Yes, because that's where the CCScheduler lives.

[Iain Merrick]

Created attachment 113062 [details]
Patch

[Nat Duca]

Antoine, Darin, can you advise the right place to put this?

I would ordinarily have put this on WebWidget, but I was hoping that our new WebLayerTreeView and WebCompositor abstractions would be of help. However, I don't think they are.

Would it help if WebWidget had a WebLayerTreeView* layerTreeView() accessor? Then we wouldn't pollute the WebWidget with compositor properties...

Also, I'm very confused why we have WebLayerTreeView instead of WebLayerTreeHost... or we don't rename CCLayerTreeHost* to CCLayerTreeView. Opinions?

[Iain Merrick]

As a straw man, I'll try adding this to WebWidget instead of WebCompositor, and plumbing that through to CCScheduler, so we can see how that looks.

[Iain Merrick]

Created attachment 113068 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 113068 [details]
Patch

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

> Source/WebKit/chromium/public/WebWidget.h:163
> +    // Called to inform the WebWidget of the ideal refresh rate for smooth

I'm probably missing one other piece of information.  Why is the ideal refresh rate
a per-widget setting?  Shouldn't it be a global setting?

[James Robinson]

I think there's nothing here that is tied to a specific widget or compositor instance. This function is meant to inform WebKit about the characteristics of the display.

Will this value change at runtime?

[Darin Fisher (:fishd, Google)]

It sounds like a static setter would be better.

[Iain Merrick]

I think it actually could change at runtime if the user changes the monitor settings, and there could be multiple monitors with different refresh rates. But I'm happy to make it a static if you think that isn't going to bite us later.

[Darin Fisher (:fishd, Google)]

Should it be a method on WebCore::PlatformScreen?

[Iain Merrick]

(In reply to comment #15)
> Should it be a method on WebCore::PlatformScreen?

Aha, yes! That looks like the right place. I'll update the patch appropriately.

[Nat Duca]

*** Bug 71039 has been marked as a duplicate of this bug. ***

[Iain Merrick]

Created attachment 113498 [details]
Patch

[Iain Merrick]

Thanks for the pointers. Take 3:

WebViewImpl reads framesPerSecond via PlatformScreen -> PlatformSupport -> WebScreenInfo, and passes it to CCThreadProxy via CCSettings.

[Darin Fisher (:fishd, Google)]

Comment on attachment 113498 [details]
Patch

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

> Source/WebCore/platform/PlatformScreen.h:59
> +    double screenFramesPerSecond(Widget*);

perhaps "RefreshRate" would be a more canonical name for this property?  how about naming this method screenRefreshRate?  you even say "refresh rate" in the comment.

[Iain Merrick]

I figured "framesPerSecond" is more self-explanatory as it includes the units (and called it "refresh rate" in the comment by way of clarification).

[Iain Merrick]

Created attachment 113510 [details]
Patch

[Iain Merrick]

Flipped it around to screenRefreshRate, units clarified in comments.

[Nat Duca]

Comment on attachment 113498 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:48
> +const double CCThreadProxy::kDefaultFramesPerSecond = 60.0;

Can we put the defaulting up in the creation of the settings object in WebViewImpl rather than inside the proxy?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:511
> +    const double fps = (settings.framesPerSecond > 0) ? settings.framesPerSecond : kDefaultFramesPerSecond;

/me wonders if webkit's implicit bool testing fits here... settings.fps ? xxx : yyy

[Iain Merrick]

(In reply to comment #24)
> (From update of attachment 113498 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113498&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:48
> > +const double CCThreadProxy::kDefaultFramesPerSecond = 60.0;
> 
> Can we put the defaulting up in the creation of the settings object in WebViewImpl rather than inside the proxy?

Good call, I'll do that.

> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:511
> > +    const double fps = (settings.framesPerSecond > 0) ? settings.framesPerSecond : kDefaultFramesPerSecond;
> 
> /me wonders if webkit's implicit bool testing fits here... settings.fps ? xxx : yyy

I wondered that too but I don't know if it's totally kosher for doubles. I guess it probably is? And we can test it (although I'm leaving CCThreadProxyTest for another patch).

[Iain Merrick]

Created attachment 113532 [details]
Patch

[Nat Duca]

Comment on attachment 113532 [details]
Patch

/me likes. I'll leave it to Darin/JamesR to bless. :)

[James Robinson]

Comment on attachment 113532 [details]
Patch

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

Looks reasonable to me except for a few nits. Darin?

> Source/WebKit/chromium/src/WebViewImpl.cpp:236
> +const double WebViewImpl::kDefaultCompositorRefreshRate = 60.0;

we don't use the k prefix for constants in WebKit

> Source/WebKit/chromium/src/WebViewImpl.h:575
> +    static const double kDefaultCompositorRefreshRate;

why is this class static instead of file static in the .cpp?

[Iain Merrick]

(In reply to comment #28)
> (From update of attachment 113532 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113532&action=review
> 
> Looks reasonable to me except for a few nits. Darin?
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:236
> > +const double WebViewImpl::kDefaultCompositorRefreshRate = 60.0;
> 
> we don't use the k prefix for constants in WebKit
> 
> > Source/WebKit/chromium/src/WebViewImpl.h:575
> > +    static const double kDefaultCompositorRefreshRate;
> 
> why is this class static instead of file static in the .cpp?

I had copied the style of surrounding code in WebCore/graphics/chromium/cc, but then moved the constant over to this class. I'll check the surrounding code again and make it consistent with that.

[Iain Merrick]

Created attachment 113646 [details]
Patch

[Iain Merrick]

I've addressed James' comments. Darin, could you take another look at this, please?

[WebKit Review Bot]

Comment on attachment 113646 [details]
Patch

Rejecting attachment 113646 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
p.rej
patching file Source/WebKit/chromium/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/chromium/public/WebScreenInfo.h
patching file Source/WebKit/chromium/src/PlatformSupport.cpp
Hunk #1 succeeded at 1056 (offset 7 lines).
patching file Source/WebKit/chromium/src/WebViewImpl.cpp
Hunk #2 succeeded at 2729 with fuzz 1 (offset 111 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Darin Fisher', u'--for..." exit_code: 1

Full output: http://queues.webkit.org/results/10483689

[Iain Merrick]

Oops, commit bot didn't take the patch cleanly. I'll rebase and try again.

[Iain Merrick]

Created attachment 115356 [details]
Rebased

[Tony Gentilcore]

Comment on attachment 115356 [details]
Rebased

Carrying forward fishd's r+ after rebase.

[WebKit Review Bot]

Comment on attachment 115356 [details]
Rebased

Clearing flags on attachment: 115356

Committed r100438: <http://trac.webkit.org/changeset/100438>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

This broke the unit tests in debug mode, example:

[----------] 2 tests from CCLayerTreeHostTestShortlived1
[ RUN      ] CCLayerTreeHostTestShortlived1.runSingleThread
[       OK ] CCLayerTreeHostTestShortlived1.runSingleThread (0 ms)
[ RUN      ] CCLayerTreeHostTestShortlived1.runMultiThread
ASSERTION FAILED: m_layerTreeHostImpl->settings().refreshRate > 0
Backtrace:

Reverting

[James Robinson]

Reverted: http://trac.webkit.org/changeset/100495

[Iain Merrick]

Created attachment 115568 [details]
Fixed CCLayerTreeHostTest

[James Robinson]

Comment on attachment 115568 [details]
Fixed CCLayerTreeHostTest

R=me

[WebKit Review Bot]

Comment on attachment 115568 [details]
Fixed CCLayerTreeHostTest

Rejecting attachment 115568 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
n/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/10510445

[James Robinson]

Comment on attachment 115568 [details]
Fixed CCLayerTreeHostTest

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

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

this is causing the commit to fail

[Iain Merrick]

Comment on attachment 115568 [details]
Fixed CCLayerTreeHostTest

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

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (OOPS!)
> 
> this is causing the commit to fail

Dang, didn't spot that the clever ChangeLog script had added this. We're covered by CCLayerTreeHostTest as the assert attests. Will fix.

[Iain Merrick]

Created attachment 115777 [details]
Fixed ChangeLog

[Tony Gentilcore]

Comment on attachment 115777 [details]
Fixed ChangeLog

Carring forward jamesr's r+ after fix to ChangeLog description.

[WebKit Review Bot]

Comment on attachment 115777 [details]
Fixed ChangeLog

Clearing flags on attachment: 115777

Committed r100751: <http://trac.webkit.org/changeset/100751>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

Mostly rolled out:
http://trac.webkit.org/changeset/105961

See discussion here: http://code.google.com/p/chromium/issues/detail?id=111404

Apologies for the lack of notice.  Unfortunately this regression is too high impact to leave in the tree this close to a branch point.

I think that refresh rate plumbing should be asynchronous when we wire it up. It's possible that screenInfo is the wrong API to use, or that we need to change how it works.

[Iain Merrick]

No worries. It's true that most platforms don't care about this and just use the default 60hz, so maybe we should go back to a setDisplayRate() method and have the platform code push in the right value, instead of having the compositor pull it.

[James Robinson]

I think we're moving to a push model for screenInfo in general, which might end up being the same thing. I think a push model for this makes sense, though, especially if we want to start supporting things like dragging a window between monitors that have different refresh rates or doing browser-side framerate throttling.