Allow ports finer-grain control of when to enable accelerated compositing

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Vangelis Kokkevis]

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.

[Vangelis Kokkevis]

Created attachment 74768 [details]
proposed patch

[Vangelis Kokkevis]

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.

[Kenneth Russell]

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.

[Simon Fraser (smfr)]

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().

[Vangelis Kokkevis]

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.

[Simon Fraser (smfr)]

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.

[Vangelis Kokkevis]

Created attachment 75095 [details]
Patch addressing recent review comments

[Vangelis Kokkevis]

(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.

[Simon Fraser (smfr)]

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.

[Early Warning System Bot]

Attachment 75095 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6399113

[Vangelis Kokkevis]

Created attachment 75104 [details]
Fixing Qt compile issue and style errors

[Vangelis Kokkevis]

(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!

[Build Bot]

Attachment 75104 [details] did not build on win:
Build output: http://queues.webkit.org/results/6400112

[Vangelis Kokkevis]

Committed r72954: <http://trac.webkit.org/changeset/72954>