Bug 56156 – Add setting to always force compositing mode

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Daniel Sievers]

Add setting to always force compositing mode

[Daniel Sievers]

Created attachment 85414 [details]
Patch

[Vangelis Kokkevis]

Comment on attachment 85414 [details]
Patch

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

> Source/WebCore/page/Settings.h:375
> +        void setAlwaysEnterCompositing(bool flag) { m_alwaysEnterCompositing = flag; }

Not a huge fan of the "alwaysEnterCompositing" name (mostly the word "enter"). Possible alternative names that come to mind: alwaysEnableCompositingMode, alwaysUseCompositing, forceCompositingMode, ...

> Source/WebCore/rendering/RenderLayerCompositor.cpp:114
> +    if (settings && settings->alwaysEnterCompositing()) {

We also need to test for m_hasAcceleratedCompositing.  That value is not available at the time the constructor gets called.  Things work fine since there's a check at the end of updateCompositingLayers but it seems somewhat inefficient to go ahead and do all the work, create the GraphicsLayer tree and then gut it all out of compositing isn't supported.

Should this logic maybe move from the constructor to the top of updateCompositingLayers() ?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:725
> +    if (!m_alwaysEnterCompositing || m_renderView->document()->frame()->tree()->parent()) {

Instead of replicating the test for child frames here (which could be fragile if other conditions get added), you could possibly only set m_alwaysEnterCompositing to true in the constructor if your're not dealing with the child frames.

[Daniel Sievers]

Created attachment 85747 [details]
Patch

[Daniel Sievers]

> 
> Not a huge fan of the "alwaysEnterCompositing" name (mostly the word "enter"). Possible alternative names that come to mind: alwaysEnableCompositingMode, alwaysUseCompositing, forceCompositingMode, ...
> 

'forceCompositingMode' it is! The 'force' naming is already used on some other options, and I find it clearer than 'always enable' (with the existing 'enableHardwareAcceleratedCompositing' and such).


> > Source/WebCore/rendering/RenderLayerCompositor.cpp:114
> > +    if (settings && settings->alwaysEnterCompositing()) {
> 
> We also need to test for m_hasAcceleratedCompositing.  That value is not available at the time the constructor gets called.  Things work fine since there's a check at the end of updateCompositingLayers but it seems somewhat inefficient to go ahead and do all the work, create the GraphicsLayer tree and then gut it all out of compositing isn't supported.


Ok, I added a check for settings->acceleratedCompositingEnabled() in the constructor...




> Should this logic maybe move from the constructor to the top of updateCompositingLayers() ?
> 


... and I thought about this, but I think it's more bullet-proof to create the root platform layer in the constructor, just for the however slight chance that we might not call updateCompositingLayer() somehow. Also note that the existance of the root platform layer is used in WebView to decide whether compositing is on (m_isAcceleratedCompositingActive).


> > Source/WebCore/rendering/RenderLayerCompositor.cpp:725
> > +    if (!m_alwaysEnterCompositing || m_renderView->document()->frame()->tree()->parent()) {
> 
> Instead of replicating the test for child frames here (which could be fragile if other conditions get added), you could possibly only set m_alwaysEnterCompositing to true in the constructor if your're not dealing with the child frames.


Good point, done.

[James Robinson]

Comment on attachment 85747 [details]
Patch

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

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

You have to take this "(OOPS!)" line or a presubmit check will fail.  Explaining why this code doesn't require new tests is fine.

[Adam Barth]

Comment on attachment 85747 [details]
Patch

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

> Tools/DumpRenderTree/chromium/TestShell.h:204
> +    bool m_forceCompositingMode;

m_forceCompositingMode => m_forceCompositingEnabled?  I don't mean to nit-pick you over names.  These other variables are called "enabled" is all.

[Vangelis Kokkevis]

> > > Source/WebCore/rendering/RenderLayerCompositor.cpp:114
> > > +    if (settings && settings->alwaysEnterCompositing()) {
> > 
> > We also need to test for m_hasAcceleratedCompositing.  That value is not available at the time the constructor gets called.  Things work fine since there's a check at the end of updateCompositingLayers but it seems somewhat inefficient to go ahead and do all the work, create the GraphicsLayer tree and then gut it all out of compositing isn't supported.
> 
> 
> Ok, I added a check for settings->acceleratedCompositingEnabled() in the constructor...
> 
> 

I don't think that test is sufficient.  There's a couple of other conditions that will turn m_hasAcceleratedCompositing off, especially bits that are set on the Client.  If you do keep this code in the constructor (which is probably fine) then there's not much point in adding that check. 

Other than that, I think the patch looks fine.

[Daniel Sievers]

Comment on attachment 85747 [details]
Patch

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

>> Tools/DumpRenderTree/chromium/TestShell.h:204
>> +    bool m_forceCompositingMode;
> 
> m_forceCompositingMode => m_forceCompositingEnabled?  I don't mean to nit-pick you over names.  These other variables are called "enabled" is all.

Actually the ones with a verb are without the 'enabled' suffix, such as 'm_allowExternalPages', 'm_dumpWhenFinished'. I can change it though if you feel strongly about it :) Otherwise, I thought I'd keep the naming consistent across all the classes (and see other force*() functions in page/Settings.h).

[Daniel Sievers]

(In reply to comment #7)
> > > > Source/WebCore/rendering/RenderLayerCompositor.cpp:114
> > > > +    if (settings && settings->alwaysEnterCompositing()) {
> > > 
> > > We also need to test for m_hasAcceleratedCompositing.  That value is not available at the time the constructor gets called.  Things work fine since there's a check at the end of updateCompositingLayers but it seems somewhat inefficient to go ahead and do all the work, create the GraphicsLayer tree and then gut it all out of compositing isn't supported.
> > 
> > 
> > Ok, I added a check for settings->acceleratedCompositingEnabled() in the constructor...
> > 
> > 
> 
> I don't think that test is sufficient.  There's a couple of other conditions that will turn m_hasAcceleratedCompositing off, especially bits that are set on the Client.  If you do keep this code in the constructor (which is probably fine) then there's not much point in adding that check. 
> 

I see, RenderLayerCompositor::m_hasAcceleratedCompositing gets turned off if all compositing triggers are off, which we could simply skip with 'force compositing' turned on.
If it wasn't for the case where the LayerRendererChromium creation fails, and Chrome decides to return zero from allowedCompositingTriggers() late in the game to handle that case and turn off compositing.

The check in the RenderLayerCompositor constructor is probably still good to handle other cases where compositing is explicitly turned off (such as through the conflicting cmdline option --disable-accelerated-compositing). 

Maybe for the other unexpected failure cases it is ok to first try to force compositing and then go roundabout and turn it off later?
I'm assuming RenderLayerCompositor might get created before we know (and might fail) creating LayerRendererChromium anyways.

[Vangelis Kokkevis]

(In reply to comment #9)
> (In reply to comment #7)
> > > > > Source/WebCore/rendering/RenderLayerCompositor.cpp:114
> > > > > +    if (settings && settings->alwaysEnterCompositing()) {
> > > > 
> > > > We also need to test for m_hasAcceleratedCompositing.  That value is not available at the time the constructor gets called.  Things work fine since there's a check at the end of updateCompositingLayers but it seems somewhat inefficient to go ahead and do all the work, create the GraphicsLayer tree and then gut it all out of compositing isn't supported.
> > > 
> > > 
> > > Ok, I added a check for settings->acceleratedCompositingEnabled() in the constructor...
> > > 
> > > 
> > 
> > I don't think that test is sufficient.  There's a couple of other conditions that will turn m_hasAcceleratedCompositing off, especially bits that are set on the Client.  If you do keep this code in the constructor (which is probably fine) then there's not much point in adding that check. 
> > 
> 
> I see, RenderLayerCompositor::m_hasAcceleratedCompositing gets turned off if all compositing triggers are off, which we could simply skip with 'force compositing' turned on.
> If it wasn't for the case where the LayerRendererChromium creation fails, and Chrome decides to return zero from allowedCompositingTriggers() late in the game to handle that case and turn off compositing.
> 
> The check in the RenderLayerCompositor constructor is probably still good to handle other cases where compositing is explicitly turned off (such as through the conflicting cmdline option --disable-accelerated-compositing). 
> 
> Maybe for the other unexpected failure cases it is ok to first try to force compositing and then go roundabout and turn it off later?
> I'm assuming RenderLayerCompositor might get created before we know (and might fail) creating LayerRendererChromium anyways.

Ok, I see what you mean.  It sounds reasonable to me to leave to code as is.  Please do test though that it still works (and correctly falls back to s/w) if the layer renderer creation fails.

[Daniel Sievers]

Created attachment 85894 [details]
Patch

[Daniel Sievers]

> 
> Ok, I see what you mean.  It sounds reasonable to me to leave to code as is.  Please do test though that it still works (and correctly falls back to s/w) if the layer renderer creation fails.

Done, tested with chrome and DumpRenderTree. Seems to behave as expected in that if I force LayerRendererChromium creation failure, it will disable compositing from the end of RenderLayerCompositor::updateCompositingLayers() and render fine in software after that.

[James Robinson]

Comment on attachment 85894 [details]
Patch

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

Looks good!

> Source/WebCore/rendering/RenderLayerCompositor.cpp:118
> +    if (settings && settings->forceCompositingMode() && settings->acceleratedCompositingEnabled()
> +        && !m_renderView->document()->frame()->tree()->parent()) {

this indentation is odd - i'd try to make it clearer that line 118 is still inside the conditional and line 119 is not, maybe line up the &&s or something

> Source/WebCore/rendering/RenderLayerCompositor.h:279
> +    bool m_forceCompositingMode;
> +

nitpick: i think it'd be better to declare this with the other bools

[Daniel Sievers]

Created attachment 85973 [details]
Patch

[Daniel Sievers]

Comment on attachment 85894 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:118
>> +        && !m_renderView->document()->frame()->tree()->parent()) {
> 
> this indentation is odd - i'd try to make it clearer that line 118 is still inside the conditional and line 119 is not, maybe line up the &&s or something

I agree that it looks odd, but I ended up leaving it that way because this seems to be done commonly that way in the WebKit code, and it actually looks a bit odd too if I align it with the &&.

>> Source/WebCore/rendering/RenderLayerCompositor.h:279
>> +
> 
> nitpick: i think it'd be better to declare this with the other bools

Done.

[James Robinson]

Vangelis isn't actually a WebKit reviewer (yet) - can you set the 'Reviewed by' lines to refer to me instead?  I think a script will complain otherwise.

[Daniel Sievers]

Created attachment 85978 [details]
Patch

[Daniel Sievers]

Created attachment 85979 [details]
Patch

[James Robinson]

Good lord bugzilla is a pain.  Should be all good now!

[WebKit Commit Bot]

Comment on attachment 85979 [details]
Patch

Clearing flags on attachment: 85979

Committed r81289: <http://trac.webkit.org/changeset/81289>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.