Bug 56156

Summary: Add setting to always force compositing mode
Product: WebKit Reporter: Daniel Sievers <sievers>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andreip, ap, commit-queue, husky, jamesr, klobag, sievers, simon.fraser, vangelis
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Daniel Sievers 2011-03-10 17:32:11 PST
Add setting to always force compositing mode
Comment 1 Daniel Sievers 2011-03-10 17:43:15 PST
Created attachment 85414 [details]
Patch
Comment 2 Vangelis Kokkevis 2011-03-11 11:31:00 PST
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.
Comment 3 Daniel Sievers 2011-03-14 17:40:13 PDT
Created attachment 85747 [details]
Patch
Comment 4 Daniel Sievers 2011-03-14 17:47:28 PDT
> 
> 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.
Comment 5 James Robinson 2011-03-14 17:52:30 PDT
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.
Comment 6 Adam Barth 2011-03-15 02:10:37 PDT
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.
Comment 7 Vangelis Kokkevis 2011-03-15 10:56:50 PDT
> > > 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.
Comment 8 Daniel Sievers 2011-03-15 11:01:05 PDT
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).
Comment 9 Daniel Sievers 2011-03-15 11:37:35 PDT
(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.
Comment 10 Vangelis Kokkevis 2011-03-15 14:22:34 PDT
(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.
Comment 11 Daniel Sievers 2011-03-15 18:43:48 PDT
Created attachment 85894 [details]
Patch
Comment 12 Daniel Sievers 2011-03-15 18:46:30 PDT
> 
> 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.
Comment 13 James Robinson 2011-03-16 11:05:59 PDT
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
Comment 14 Daniel Sievers 2011-03-16 14:10:57 PDT
Created attachment 85973 [details]
Patch
Comment 15 Daniel Sievers 2011-03-16 14:13:49 PDT
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.
Comment 16 James Robinson 2011-03-16 14:34:57 PDT
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.
Comment 17 Daniel Sievers 2011-03-16 14:37:10 PDT
Created attachment 85978 [details]
Patch
Comment 18 Daniel Sievers 2011-03-16 14:38:08 PDT
Created attachment 85979 [details]
Patch
Comment 19 James Robinson 2011-03-16 14:39:14 PDT
Good lord bugzilla is a pain.  Should be all good now!
Comment 20 WebKit Commit Bot 2011-03-16 16:38:08 PDT
Comment on attachment 85979 [details]
Patch

Clearing flags on attachment: 85979

Committed r81289: <http://trac.webkit.org/changeset/81289>
Comment 21 WebKit Commit Bot 2011-03-16 16:38:12 PDT
All reviewed patches have been landed.  Closing bug.