Bug 26651 – Add a preference to disable hardware acceleration

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Chris Marrin]

We need to add a pref to turn off hardware accleration, for these reasons:
* Need to be able to flip a pref in order to test without hardware
* Need a kill switch for applications which may have issues with hardware.

[Chris Marrin]

Created attachment 31788 [details]
Patch

[Simon Fraser (smfr)]

<rdar://problem/6989101>

[Simon Fraser (smfr)]

Comment on attachment 31788 [details]
Patch

Needs some more work.

[Chris Marrin]

Created attachment 31825 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 31825 [details]
Patch

> Index: WebCore/page/Settings.cpp
> ===================================================================

Is m_acceleratedCompositingDisabled initialized in the constructor?

> +void Settings::setAcceleratedCompositingDisabled(bool disabled)
> +{
> +    if (m_acceleratedCompositingDisabled == disabled)
> +        return;

It's not worth doing this optimization here.

> Index: WebCore/page/Settings.h
> ===================================================================
> --- WebCore/page/Settings.h	(revision 44993)
> +++ WebCore/page/Settings.h	(working copy)
> @@ -241,6 +241,9 @@ namespace WebCore {
>          void setXSSAuditorEnabled(bool);
>          bool xssAuditorEnabled() const { return m_xssAuditorEnabled; }
>  
> +        void setAcceleratedCompositingDisabled(bool);
> +        bool acceleratedCompositingDisabled() const { return m_acceleratedCompositingDisabled; }

I think we should turn this around, and rename these as "acceleratedCompositingEnabled".
It can default to true if ACCELERATED_COMPOSITING is defined.

> +bool RenderLayer::hasAcceleratedCompositing() const
> +{
> +#if USE(ACCELERATED_COMPOSITING)
> +    return compositor()->hasAcceleratedCompositing();
> +#else
> +    return false;
> +#endif
> +}

I don't think this is ever used.

> Index: WebCore/rendering/RenderLayerCompositor.cpp
> ===================================================================

> +bool RenderLayerCompositor::hasAcceleratedCompositing(bool updateFromPref /*= false*/)
> +{
> +    if (updateFromPref) {
> +        bool accelerationDisabled = m_renderView->frameView()->frame()->page()->settings() && m_renderView->frameView()->frame()->page()->settings()->acceleratedCompositingDisabled();

m_renderView->document()->settings() is shorter.


> Index: WebCore/rendering/RenderLayerCompositor.h
> ===================================================================
> --- WebCore/rendering/RenderLayerCompositor.h	(revision 44993)
> +++ WebCore/rendering/RenderLayerCompositor.h	(working copy)
> @@ -56,6 +56,9 @@ public:
>      // This will make a compositing layer at the root automatically, and hook up to
>      // the native view/window system.
>      void enableCompositingMode(bool enable = true);
> +    
> +    // Returns true if the accelerated compositing is enabled
> +    bool hasAcceleratedCompositing(bool updateFromPref=false);

Darin hates boolean arguments, because they make the code at the call site hard to read, and I agree.
An enum would make it more readable, or just two methods: hasAcceleratedCompositing() and hasAcceleratedCompositingCached()
or something.

> Index: WebKit/mac/WebView/WebPreferenceKeysPrivate.h
> ===================================================================
> --- WebKit/mac/WebView/WebPreferenceKeysPrivate.h	(revision 44993)
> +++ WebKit/mac/WebView/WebPreferenceKeysPrivate.h	(working copy)
> @@ -82,6 +82,7 @@
>  #define WebKitOfflineWebApplicationCacheEnabledPreferenceKey @"WebKitOfflineWebApplicationCacheEnabled"
>  #define WebKitZoomsTextOnlyPreferenceKey @"WebKitZoomsTextOnly"
>  #define WebKitXSSAuditorEnabledPreferenceKey @"WebKitXSSAuditorEnabled"
> +#define WebKitAcceleratedCompositingDisabledPreferenceKey @"WebKitAcceleratedCompositingDisabled"

I'm torn about whether the key should have "enabled" or "disabled" in the name. The closest equivalent
is WebKitEnableDeferredUpdates, which is only on for SnowLeopard, so I suggest we follow that.

> Index: WebKit/mac/WebView/WebPreferences.h
> ===================================================================

> +    @method acceleratedCompositingDisabled
> +*/
> +- (BOOL)acceleratedCompositingDisabled;
> +
> +/*!
> +    @method setAcceleratedCompositingDisabled:
> +    @param size

size?

> +*/
> +- (void)setAcceleratedCompositingDisabled:(BOOL)disabled;

I also think these should use 'enabled'.

So one more round and I think we've got it.

[Chris Marrin]

Created attachment 31864 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 31864 [details]
Patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 45129)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,43 @@
> [phone]  Chris Marrin  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=26651
> +
> +        Preference is named "WebKitAcceleratedCompositingDisabled"

It's WebKitAcceleratedCompositingEnabled now.

> +        and is a boolean value. Prevents compositing layers from
> +        being created, which prevents hardware animation from running.
> +        Also forces video to do software rendering. Added a cache for
> +        the flag in RenderLayerCompositing and made it all work
> +        on-the-fly when the flag is changed while a page is loaded.
> + 
> +        * WebCore.base.exp:
> +        * page/FrameView.cpp:
> +        (WebCore::FrameView::updateCompositingLayers):
> +        * page/Settings.cpp:
> +        (WebCore::Settings::setAcceleratedCompositingDisabled):
> +        * page/Settings.h:
> +        (WebCore::Settings::acceleratedCompositingDisabled):

These method names changed, so you should run prepare-changelog again.

> Index: WebCore/page/Settings.cpp
> ===================================================================

> -static void setNeedsReapplyStylesInAllFrames(Page* page)
> +static void setNeedsReapplyStylesInAllFrames(Page* page, bool /*updateCompositingLayers*/ = false)
>  {
> -    for (Frame* frame = page->mainFrame(); frame; frame = frame->tree()->traverseNext())
> +    for (Frame* frame = page->mainFrame(); frame; frame = frame->tree()->traverseNext()) {
> +        //if (updateCompositingLayers)
> +        //    frame->view()->updateCompositingLayers(FrameView::ForcedCompositingUpdate);
>          frame->setNeedsReapplyStyles();
> +    }

Some unwanted changes here.

>  }
>  
>  #if USE(SAFARI_THEME)
> @@ -104,6 +108,7 @@ Settings::Settings(Page* page)
>      // they can't use by. Leaving enabled for now to not change existing behavior.
>      , m_downloadableBinaryFontsEnabled(true)
>      , m_xssAuditorEnabled(false)
> +    , m_acceleratedCompositingEnabled(true)

I think you should #ifdef to default to true if ACCEL_COMP is defined, false otherwise.



> +void RenderLayerCompositor::cacheAcceleratedCompositingEnabledFlag()
> +{
> +    

Extra blank line here.

> +    bool hasAcceleratedCompositing = m_renderView->document()->settings() && m_renderView->frameView()->frame()->page()->settings()->acceleratedCompositingEnabled();

The cleaner way to do this is:

  bool hasAcceleratedCompositing = false;
  if (Settings* settings = m_renderView->document()->settings())
    hasAcceleratedCompositing = settings-> acceleratedCompositingEnabled();


> Index: WebCore/rendering/RenderLayerCompositor.h
> ===================================================================
> --- WebCore/rendering/RenderLayerCompositor.h	(revision 44993)
> +++ WebCore/rendering/RenderLayerCompositor.h	(working copy)
> @@ -56,6 +56,12 @@ public:
>      // This will make a compositing layer at the root automatically, and hook up to
>      // the native view/window system.
>      void enableCompositingMode(bool enable = true);
> +    
> +    // Returns true if the accelerated compositing is enabled
> +    bool hasAcceleratedCompositing() { return m_hasAcceleratedCompositing; }

Should be const.


> Index: WebKit/mac/ChangeLog
> ===================================================================
> --- WebKit/mac/ChangeLog	(revision 45129)
> +++ WebKit/mac/ChangeLog	(working copy)
> @@ -1,3 +1,25 @@
> [phone]  Chris Marrin  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=26651
> +
> +        Preference is named "WebKitAcceleratedCompositingDisabled"
> +        and is a boolean value. Prevents compositing layers from
> +        being created, which prevents hardware animation from running.
> +        Also forces video to do software rendering. Added a cache for
> +        the flag in RenderLayerCompositing and made it all work
> +        on-the-fly when the flag is changed while a page is loaded.
> +
> +        * WebView/WebPreferenceKeysPrivate.h:
> +        * WebView/WebPreferences.h:
> +        * WebView/WebPreferences.mm:
> +        (+[WebPreferences initialize]):
> +        (-[WebPreferences acceleratedCompositingDisabled]):
> +        (-[WebPreferences setAcceleratedCompositingDisabled:]):
> +        * WebView/WebView.mm:
> +        (-[WebView _preferencesChangedNotification:]):

Need to update this changelog too.

> Index: WebKit/mac/WebView/WebPreferences.mm
> ===================================================================
> --- WebKit/mac/WebView/WebPreferences.mm	(revision 44993)
> +++ WebKit/mac/WebView/WebPreferences.mm	(working copy)
> @@ -348,6 +348,7 @@ static WebCacheModel cacheModelForMainBu
>          [NSNumber numberWithBool:NO],   WebKitOfflineWebApplicationCacheEnabledPreferenceKey,
>          [NSNumber numberWithBool:YES],  WebKitZoomsTextOnlyPreferenceKey,
>          [NSNumber numberWithBool:NO],   WebKitXSSAuditorEnabledPreferenceKey,
> +        [NSNumber numberWithBool:YES],  WebKitAcceleratedCompositingEnabledPreferenceKey,
>          nil];

I think it should only default to YES when ACCEL_COMP is defined.

> +- (BOOL)acceleratedCompositingEnabled
> +{
> +    return [self _boolValueForKey: WebKitAcceleratedCompositingEnabledPreferenceKey];
> +}
> +
> +- (void)setAcceleratedCompositingEnabled:(BOOL)enabled
> +{
> +    [self _setBoolValue: enabled forKey: WebKitAcceleratedCompositingEnabledPreferenceKey];

WebKit style is no spaces after the colons.

r=me with those cleanups.

[Chris Marrin]

Sending        WebCore/ChangeLog
Sending        WebCore/WebCore.base.exp
Sending        WebCore/page/FrameView.cpp
Sending        WebCore/page/Settings.cpp
Sending        WebCore/page/Settings.h
Sending        WebCore/rendering/RenderLayer.cpp
Sending        WebCore/rendering/RenderLayer.h
Sending        WebCore/rendering/RenderLayerBacking.cpp
Sending        WebCore/rendering/RenderLayerCompositor.cpp
Sending        WebCore/rendering/RenderLayerCompositor.h
Sending        WebCore/rendering/RenderObject.h
Sending        WebKit/mac/ChangeLog
Sending        WebKit/mac/WebView/WebPreferenceKeysPrivate.h
Sending        WebKit/mac/WebView/WebPreferences.h
Sending        WebKit/mac/WebView/WebPreferences.mm
Sending        WebKit/mac/WebView/WebView.mm
Transmitting file data ................
Committed revision 45199.

[Chris Marrin]

Simon and I talked about it and decided that this flag should always default to true, to avoid someone running on Leopard, getting the flag set to false, then running on SL and having accelerated compositing turned off. I will submit a patch for this.

[Chris Marrin]

Created attachment 31947 [details]
Additional Patch

[Chris Marrin]

Comment on attachment 31947 [details]
Additional Patch

File is wrong in the patch, will try again

[Chris Marrin]

Created attachment 31948 [details]
Additional Patch

[Chris Marrin]

Sending        WebCore/ChangeLog
Sending        WebCore/page/Settings.cpp
Sending        WebKit/mac/ChangeLog
Sending        WebKit/mac/WebView/WebPreferences.mm
Transmitting file data ....
Committed revision 45292.

[Simon Fraser (smfr)]

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