Bug 26651 - Add a preference to disable hardware acceleration
Summary: Add a preference to disable hardware acceleration
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Chris Marrin
URL:
Keywords: InRadar
: 23913 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-06-23 10:12 PDT by Chris Marrin
Modified: 2009-06-27 21:19 PDT (History)
1 user (show)

See Also:


Attachments
Patch (8.67 KB, patch)
2009-06-24 09:31 PDT, Chris Marrin
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (17.11 KB, patch)
2009-06-24 18:33 PDT, Chris Marrin
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (18.40 KB, patch)
2009-06-25 10:58 PDT, Chris Marrin
simon.fraser: review+
Details | Formatted Diff | Diff
Additional Patch (5.44 KB, patch)
2009-06-26 13:48 PDT, Chris Marrin
cmarrin: review-
Details | Formatted Diff | Diff
Additional Patch (3.04 KB, patch)
2009-06-26 13:53 PDT, Chris Marrin
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Marrin 2009-06-23 10:12:00 PDT
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.
Comment 1 Chris Marrin 2009-06-24 09:31:01 PDT
Created attachment 31788 [details]
Patch
Comment 2 Simon Fraser (smfr) 2009-06-24 12:05:08 PDT
<rdar://problem/6989101>
Comment 3 Simon Fraser (smfr) 2009-06-24 12:05:21 PDT
Comment on attachment 31788 [details]
Patch

Needs some more work.
Comment 4 Chris Marrin 2009-06-24 18:33:49 PDT
Created attachment 31825 [details]
Patch
Comment 5 Simon Fraser (smfr) 2009-06-24 20:49:31 PDT
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.
Comment 6 Chris Marrin 2009-06-25 10:58:33 PDT
Created attachment 31864 [details]
Patch
Comment 7 Simon Fraser (smfr) 2009-06-25 12:02:04 PDT
Comment on attachment 31864 [details]
Patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 45129)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,43 @@
> +2009-06-24  Chris Marrin  <cmarrin@apple.com>
> +
> +        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 @@
> +2009-06-24  Chris Marrin  <cmarrin@apple.com>
> +
> +        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.
Comment 8 Chris Marrin 2009-06-25 15:04:23 PDT
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.
Comment 9 Chris Marrin 2009-06-26 13:43:15 PDT
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.
Comment 10 Chris Marrin 2009-06-26 13:48:06 PDT
Created attachment 31947 [details]
Additional Patch
Comment 11 Chris Marrin 2009-06-26 13:49:46 PDT
Comment on attachment 31947 [details]
Additional Patch

File is wrong in the patch, will try again
Comment 12 Chris Marrin 2009-06-26 13:53:12 PDT
Created attachment 31948 [details]
Additional Patch
Comment 13 Chris Marrin 2009-06-26 15:59:05 PDT
Sending        WebCore/ChangeLog
Sending        WebCore/page/Settings.cpp
Sending        WebKit/mac/ChangeLog
Sending        WebKit/mac/WebView/WebPreferences.mm
Transmitting file data ....
Committed revision 45292.
Comment 14 Simon Fraser (smfr) 2009-06-27 21:19:07 PDT
*** Bug 23913 has been marked as a duplicate of this bug. ***