Bug 49998 - Allow ports finer-grain control of when to enable accelerated compositing
Summary: Allow ports finer-grain control of when to enable accelerated compositing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Vangelis Kokkevis
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-23 16:49 PST by Vangelis Kokkevis
Modified: 2010-11-30 12:42 PST (History)
8 users (show)

See Also:


Attachments
proposed patch (9.53 KB, patch)
2010-11-24 09:22 PST, Vangelis Kokkevis
simon.fraser: review-
Details | Formatted Diff | Diff
Patch adjusted per reviewer comments (12.83 KB, patch)
2010-11-29 15:42 PST, Vangelis Kokkevis
simon.fraser: review-
vangelis: commit-queue-
Details | Formatted Diff | Diff
Patch addressing recent review comments (16.89 KB, patch)
2010-11-29 18:04 PST, Vangelis Kokkevis
simon.fraser: review+
Details | Formatted Diff | Diff
Fixing Qt compile issue and style errors (16.91 KB, patch)
2010-11-29 22:17 PST, Vangelis Kokkevis
simon.fraser: review+
vangelis: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vangelis Kokkevis 2010-11-23 16:49:23 PST
Currently accelerated compositing is either on or off for everything. We need to allow ports to have more fine-grained control over which elements can trigger compositing. As an example, one port might want to trigger the compositor only in the presence of WebGL elements on the page, another might want avoid triggering compositing just for video, etc.
Comment 1 Vangelis Kokkevis 2010-11-24 09:22:44 PST
Created attachment 74768 [details]
proposed patch
Comment 2 Vangelis Kokkevis 2010-11-24 09:42:07 PST
Proposed patch uploaded.  Can you please review the general approach?  I'm not too happy with the variable/method name selection so alternative suggestions are welcome too.
Comment 3 Kenneth Russell 2010-11-24 13:14:19 PST
Comment on attachment 74768 [details]
proposed patch

This seems reasonable to me although I wonder whether it would be cleaner to have one virtual taking an enum for the content type.
Comment 4 Simon Fraser (smfr) 2010-11-26 10:40:52 PST
Comment on attachment 74768 [details]
proposed patch

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

I wonder if a bitfield could be used in the call through to ChromeClient, and for storage in RLC?

> WebCore/css/MediaQueryEvaluator.cpp:484
> -        threeDEnabled = view->compositor()->hasAcceleratedCompositing();
> +        threeDEnabled = view->compositor()->hasAcceleratedCompositing()
> +                        && frame->page()->chrome()->client()->allowsAcceleratedCompositingFor3DTransforms();

I think RenderLayerCompositor should have a canRender3DTransforms() method.

> WebCore/page/ChromeClient.h:248
> +        // Returns true if 3d transforms can trigger the accelerated compositor.
> +        virtual bool allowsAcceleratedCompositingFor3DTransforms() const { return true; }
> +        // Returns true if video can trigger the accelerated compositor.
> +        virtual bool allowsAcceleratedCompositingForVideo() const { return true; }
> +        // Returns true if canvas can trigger the accelerated compositor.
> +        virtual bool allowsAcceleratedCompositingForCanvas() const { return true; }
> +        // Returns true if plugins can trigger the accelerated compositor.
> +        virtual bool allowsAcceleratedCompositingForPlugin() const { return true; }
> +        // Returns true if css animation can trigger the accelerated compositor.
> +        virtual bool allowsAcceleratedCompositingForAnimation() const { return true; }

I agree that a single method with an enum argument would be better here.

> WebCore/rendering/RenderLayer.cpp:249
>  bool RenderLayer::hasAcceleratedCompositing() const
>  {
>  #if USE(ACCELERATED_COMPOSITING)
> -    return compositor()->hasAcceleratedCompositing();
> +    return compositor()->hasAcceleratedCompositing() && compositor()->inCompositingMode();
>  #else
>      return false;
>  #endif

I think this method should be removed, and callers should use compositor()->canRender3DTransforms().
Comment 5 Vangelis Kokkevis 2010-11-29 15:42:29 PST
Created attachment 75074 [details]
Patch adjusted per reviewer comments

Thanks for the review! I modified the patch to address the comments. Specifically:
* Added an enum to the ChromeClient per Ken's and Simon's suggestion and replaced individual calls by a single call.
* Added a canRender3dTransforms() method to the RenderLayerCompositor.  Media queries as well as RenderLayer code that needs to know whether 3d transforms are supported now call this method. I did leave the old hasAcceleratedCompositing() method intact as it has a different, legitimate, use.

Please take another look.
Comment 6 Simon Fraser (smfr) 2010-11-29 16:18:30 PST
Comment on attachment 75074 [details]
Patch adjusted per reviewer comments

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

> WebCore/page/ChromeClient.h:250
> +        enum CompositorTrigger {
> +            ThreeDTransformTrigger,
> +            VideoTrigger,
> +            PluginTrigger,
> +            CanvasTrigger,
> +            AnimationTrigger,
> +            CompositorTriggerLast
> +        };
> +        // Returns true if the accelerated compositor should be triggered in the
> +        // presence of a given element in the page.
> +        virtual bool doAcceleratedCompositingFor(CompositorTrigger) const { return true; }

I think this would be better named as allowAcceleratedCompositingFor().

I also think that it would be better for it to simply return a bitfield (or WTF::Bitmap), so that only one call is required. Since there are no other uses of WTF::BitMap in WebCore that I can find, maybe an old-schoold bitmap is more appropriate. In that case, I'd declare the enum like:

enum CompositingTrigger {
  ThreeDTransformTrigger = 1 << 0,
  VideoTrigger = 1 << 1,
  ...
  AllTriggers = 0xFFFFFFFF;
};
typedef unsigned CompositingTriggerFlags;

virtual CompositingTriggerFlags allowedCompositingTriggers() const;

If we do this, then we can eliminate allowsAcceleratedCompositing() too.

You should probably have separate enum values for 2D canvas and WebGL.

> WebCore/rendering/RenderLayer.h:292
> +    bool canRender3dTransforms() const;

We normally use 3DTransforms, not 3dTransforms

> WebCore/rendering/RenderLayerCompositor.cpp:154
> +            for (unsigned i = 0; i < ChromeClient::CompositorTriggerLast; ++i) {
> +                if (chromeClient->doAcceleratedCompositingFor(static_cast<ChromeClient::CompositorTrigger>(i)))
> +                    compositorTriggers.set(i);

If we change ChromeClient to return a bitfield, then there's no need to loop here.

> WebCore/rendering/RenderLayerCompositor.cpp:163
> -    if (hasAcceleratedCompositing != m_hasAcceleratedCompositing || showDebugBorders != m_showDebugBorders || showRepaintCounter != m_showRepaintCounter)
> +    if (hasAcceleratedCompositing != m_hasAcceleratedCompositing || showDebugBorders != m_showDebugBorders || showRepaintCounter != m_showRepaintCounter || compositorTriggersChanged)
>          setCompositingLayersNeedRebuild();

I don't like the fact that ChromeClient can return different flags (hence the need for compositorTriggersChanged). I think the contract should be that the flags can never change. If they can change, then there needs to be a new entry point that gets called when they do. If the flags would only ever change for developers toggling some debug settings, then this is OK.

> WebCore/rendering/RenderLayerCompositor.h:240
> +    typedef WTF::Bitmap<ChromeClient::CompositorTriggerLast> CompositorTriggerBitfield;
> +    CompositorTriggerBitfield m_compositorTriggers;

This could just be the CompositingTriggerFlags returned from ChromeClient, though it's a bit weird to reuse ChromeClient's enum everywhere.
Comment 7 Vangelis Kokkevis 2010-11-29 18:04:19 PST
Created attachment 75095 [details]
Patch addressing recent review comments
Comment 8 Vangelis Kokkevis 2010-11-29 18:07:17 PST
(In reply to comment #6)
> (From update of attachment 75074 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75074&action=review
> 
> > WebCore/page/ChromeClient.h:250
> > +        enum CompositorTrigger {
> > +            ThreeDTransformTrigger,
> > +            VideoTrigger,
> > +            PluginTrigger,
> > +            CanvasTrigger,
> > +            AnimationTrigger,
> > +            CompositorTriggerLast
> > +        };
> > +        // Returns true if the accelerated compositor should be triggered in the
> > +        // presence of a given element in the page.
> > +        virtual bool doAcceleratedCompositingFor(CompositorTrigger) const { return true; }
> 
> I think this would be better named as allowAcceleratedCompositingFor().
> 
> I also think that it would be better for it to simply return a bitfield (or WTF::Bitmap), so that only one call is required. Since there are no other uses of WTF::BitMap in WebCore that I can find, maybe an old-schoold bitmap is more appropriate. In that case, I'd declare the enum like:
> 
> enum CompositingTrigger {
>   ThreeDTransformTrigger = 1 << 0,
>   VideoTrigger = 1 << 1,
>   ...
>   AllTriggers = 0xFFFFFFFF;
> };
> typedef unsigned CompositingTriggerFlags;
> 
> virtual CompositingTriggerFlags allowedCompositingTriggers() const;
> 
> If we do this, then we can eliminate allowsAcceleratedCompositing() too.

I like that approach.  Done!

> 
> You should probably have separate enum values for 2D canvas and WebGL.
> 
> > WebCore/rendering/RenderLayer.h:292
> > +    bool canRender3dTransforms() const;
> 
> We normally use 3DTransforms, not 3dTransforms

Done.

> 
> > WebCore/rendering/RenderLayerCompositor.cpp:154
> > +            for (unsigned i = 0; i < ChromeClient::CompositorTriggerLast; ++i) {
> > +                if (chromeClient->doAcceleratedCompositingFor(static_cast<ChromeClient::CompositorTrigger>(i)))
> > +                    compositorTriggers.set(i);
> 
> If we change ChromeClient to return a bitfield, then there's no need to loop here.

Done.

> 
> > WebCore/rendering/RenderLayerCompositor.cpp:163
> > -    if (hasAcceleratedCompositing != m_hasAcceleratedCompositing || showDebugBorders != m_showDebugBorders || showRepaintCounter != m_showRepaintCounter)
> > +    if (hasAcceleratedCompositing != m_hasAcceleratedCompositing || showDebugBorders != m_showDebugBorders || showRepaintCounter != m_showRepaintCounter || compositorTriggersChanged)
> >          setCompositingLayersNeedRebuild();
> 
> I don't like the fact that ChromeClient can return different flags (hence the need for compositorTriggersChanged). I think the contract should be that the flags can never change. If they can change, then there needs to be a new entry point that gets called when they do. If the flags would only ever change for developers toggling some debug settings, then this is OK.
> 
> > WebCore/rendering/RenderLayerCompositor.h:240
> > +    typedef WTF::Bitmap<ChromeClient::CompositorTriggerLast> CompositorTriggerBitfield;
> > +    CompositorTriggerBitfield m_compositorTriggers;
> 
> This could just be the CompositingTriggerFlags returned from ChromeClient, though it's a bit weird to reuse ChromeClient's enum everywhere.

Done.
Comment 9 Simon Fraser (smfr) 2010-11-29 18:14:41 PST
Comment on attachment 75095 [details]
Patch addressing recent review comments

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

> WebCore/rendering/RenderLayerCompositor.cpp:97
> +    , m_compositingTriggers(0)

I think this should default to AllTriggers.
Comment 10 Early Warning System Bot 2010-11-29 18:18:26 PST
Attachment 75095 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6399113
Comment 11 Vangelis Kokkevis 2010-11-29 22:17:34 PST
Created attachment 75104 [details]
Fixing Qt compile issue and style errors
Comment 12 Vangelis Kokkevis 2010-11-29 22:18:09 PST
(In reply to comment #9)
> (From update of attachment 75095 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75095&action=review
> 
> > WebCore/rendering/RenderLayerCompositor.cpp:97
> > +    , m_compositingTriggers(0)
> 
> I think this should default to AllTriggers.

Done.  Thanks!
Comment 13 Build Bot 2010-11-30 03:09:52 PST
Attachment 75104 [details] did not build on win:
Build output: http://queues.webkit.org/results/6400112
Comment 14 Vangelis Kokkevis 2010-11-30 12:42:30 PST
Committed r72954: <http://trac.webkit.org/changeset/72954>