Bug 231009 – Cocoa WebGL should support UI side compositing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kimmo Kinnunen]

Cocoa WebGL support UI side compositing

Currently WebGL needs IOKit in WP

[Kimmo Kinnunen]

<rdar://83437059>

[Kimmo Kinnunen]

Created attachment 441214 [details]
Patch for feedback

[Kimmo Kinnunen]

Created attachment 441215 [details]
Patch for feedback

[Simon Fraser (smfr)]

I like the idea of a "content provider". Can it be a function on GraphicsLayerClient?

[Kimmo Kinnunen]

> I like the idea of a "content provider". Can it be a function on GraphicsLayerClient?

In a way "yes", considering that current GraphicsLayerClient can provide contents via painting via "draws content".

However, in practice "no", as the GraphicsLayerClient is general purpose and closed, the "content provider" is platform-specific and open-ended.

I'd view it the other way, in that eventually GraphicsLayerClient::paintContents could be expressed as a "content provider", and RenderLayerBacking (or RenderLayers?) could be the instance that returns "paintContents content provider".

RenderLayerBacking does now: m_layer->setDrawsContent(true)
Instead it would do: m_layer->setContentFromProvider(this);

RenderLayerBacking does now: m_layer->setDrawsContent(false);
Instead it would do: m_layer->setContentFromProvider(nullptr); (or ->setNoContents());

However, it's probably not within my expertise to implement that part, and it'd be a refactor without much benefit..

[Kimmo Kinnunen]

Created attachment 444701 [details]
Patch

[Fujii Hironori]

Comment on attachment 444701 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsLayer.h:525
>      virtual void setContentsToPlatformLayer(PlatformLayer*, ContentsLayerPurpose) { }

Do you have a plan to remove setContentsToPlatformLayer?

> Source/WebCore/platform/graphics/GraphicsLayer.h:526
> +    virtual void setContentsFromDisplayDelegate(RefPtr<GraphicsLayerContentsDisplayDelegate>&&, ContentsLayerPurpose);

This name sounds inconsistent to me.
Other methods setting the contents layer are named setContentsTo*.
I prefer setContentsToDisplayDelegate.

[Fujii Hironori]

(In reply to Kimmo Kinnunen from comment #5)
> RenderLayerBacking does now: m_layer->setDrawsContent(true)
> Instead it would do: m_layer->setContentFromProvider(this);

setDrawsContent makes the primary layer paintable, not the content layer.
I think it should be setPrimaryProvider or setPrimaryToProvider for the consistency.

[Kimmo Kinnunen]

Created attachment 444791 [details]
Patch

[Kimmo Kinnunen]

Created attachment 444804 [details]
Patch

[Kimmo Kinnunen]

Created attachment 444807 [details]
Patch

[Kimmo Kinnunen]

Created attachment 444961 [details]
Patch

[Kimmo Kinnunen]

Created attachment 444976 [details]
Patch

[Kimmo Kinnunen]

Created attachment 445007 [details]
Patch

[Kimmo Kinnunen]

Created attachment 445014 [details]
Patch

[Kimmo Kinnunen]

Created attachment 445021 [details]
Patch

[Kimmo Kinnunen]

Created attachment 445067 [details]
Patch

[Kimmo Kinnunen]

Created attachment 445089 [details]
Patch

[Kimmo Kinnunen]

Created attachment 445537 [details]
Patch

[Kimmo Kinnunen]

Created attachment 445550 [details]
Patch

[Kimmo Kinnunen]

Created attachment 445561 [details]
Patch

[Kimmo Kinnunen]

Created attachment 445569 [details]
Patch

[Kimmo Kinnunen]

Created attachment 445576 [details]
Patch

[Kimmo Kinnunen]

Created attachment 445594 [details]
Patch

[Kimmo Kinnunen]

Created attachment 445686 [details]
Patch

[Kimmo Kinnunen]

Created attachment 445695 [details]
Patch

[Kimmo Kinnunen]

Created attachment 445702 [details]
Patch

[Kimmo Kinnunen]

Created attachment 445713 [details]
Patch

[Kimmo Kinnunen]

Created attachment 445716 [details]
Patch

[Kimmo Kinnunen]

Created attachment 445717 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 445717 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsLayer.h:526
> +    virtual void setContentsToDisplayDelegate(RefPtr<GraphicsLayerContentsDisplayDelegate>&&, ContentsLayerPurpose);

The name is a bit odd. Maybe setContentsDisplayDelegate?

Does calling setContentsToSolidColor() or setContentsToPlatformLayer() remove the delegate?

> Source/WebCore/platform/graphics/GraphicsLayerContentsDisplayDelegate.h:47
> +    virtual void willDelegateDisplay(PlatformCALayer&);

This name is ambiguous. On first reading I expect it to return a bool. Does it mean "prepare to render a frame"?

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1363
> +        m_contentsLayer = createPlatformCALayer(PlatformCALayer::LayerTypeContentsProvidedLayer, this);

Naively I would expect that the GraphicsLayerContentsDisplayDelegate would do the layer creation?

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1369
> +        m_contentsLayer->setBackingStoreAttached(true);
> +        m_contentsLayer->setAcceleratesDrawing(true);

I feel like these should be under delegate control too?

> Source/WebCore/platform/graphics/cocoa/WebProcessGraphicsContextGLCocoa.mm:49
> +    }
> +    void display(PlatformCALayer& layer) final

Blank lines between functions please.

> Source/WebCore/platform/graphics/mac/WebLayer.mm:86
> +    if (!layer->owner()->platformCALayerDrawsContent() && !layer->owner()->platformCALayerDelegatesDisplay(layer.get()))

Maybe we should add a function that does both these checks.

> Source/WebKit/WebProcess/GPU/graphics/cocoa/RemoteGraphicsContextGLProxyCocoa.mm:56
> +    }
> +    void display(WebCore::PlatformCALayer& layer) final

Blank lines please

[Fujii Hironori]

Comment on attachment 445717 [details]
Patch

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

>> Source/WebCore/platform/graphics/GraphicsLayer.h:526
>> +    virtual void setContentsToDisplayDelegate(RefPtr<GraphicsLayerContentsDisplayDelegate>&&, ContentsLayerPurpose);
> 
> The name is a bit odd. Maybe setContentsDisplayDelegate?
> 
> Does calling setContentsToSolidColor() or setContentsToPlatformLayer() remove the delegate?

Just a idea. What about using overloading for the methods?

    virtual void setContents(Image*) { }
    virtual void setContents(const Color&) { }
    virtual void setContents(RefPtr<GraphicsLayerContentsDisplayDelegate>&&, ContentsLayerPurpose);
    virtual void setContents(RefPtr<Model>&&) { }

> Source/WebKit/WebProcess/GPU/graphics/wc/RemoteGraphicsContextGLProxyWC.cpp:73
> +        , m_layerContentsDisplayDelegate(PlatformLayerDisplayDelegate::create(makeUnique<WCPlatformLayerGCGL>(m_graphicsContextGLIdentifier)))

We no longer need WCPlatformLayerGCGL. I will remove it in follow-up patch.

[Kimmo Kinnunen]

Created attachment 445846 [details]
Patch

[Kimmo Kinnunen]

Created attachment 445872 [details]
Patch

[Fujii Hironori]

Created attachment 445990 [details]
Patch

* Fixed WinCairo build

[Kimmo Kinnunen]

Thanks Fujii!

[Kimmo Kinnunen]

Created attachment 446150 [details]
Patch for landing

[Kimmo Kinnunen]

Created attachment 446151 [details]
Patch for landing

[EWS]

Committed r286596 (?): <https://commits.webkit.org/r286596>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446151 [details].

[Truitt Savell]

I believe the changes in https://trac.webkit.org/changeset/286596/webkit

Caused these two tests to timeout on iOS:
compositing/tiling/tiled-reflection-inwindow.html
fast/forms/ios/focus-select-and-switch-tabs.html

I don't think EWS caught this because it was considered flaky during the run. current history shows a very clear regression point though

History:
https://results.webkit.org/?suite=layout-tests&suite=layout-tests&test=editing%2Fpasteboard%2Fdom-paste%2Fdom-paste-requires-user-gesture.html&test=fast%2Fforms%2Fios%2Ffocus-select-and-switch-tabs.html&platform=ios

Tracking in: https://bugs.webkit.org/show_bug.cgi?id=234032

[Truitt Savell]

Reverted r286596 for reason:

broke 2 tests on iOS

Committed r286709 (244986@trunk): <https://commits.webkit.org/244986@trunk>

[Kimmo Kinnunen]

Created attachment 446713 [details]
Patch

[Kimmo Kinnunen]

Created attachment 446723 [details]
Patch

[Kimmo Kinnunen]

Created attachment 446726 [details]
Patch

[Kimmo Kinnunen]

Created attachment 446730 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 446730 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/WebProcessGraphicsContextGLCocoa.mm:89
> +    const bool m_isOpaque;
> +    const float m_contentsScale;
> +    std::unique_ptr<IOSurface> m_displayBuffer;

Doesn't really matter but I would flip the ordering to avoid padding.

> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.h:140
> +    // FIXME: This should be removed and m_bufferHandle should be used to ref the buffer once ShareableBitmap::Handle
> +    // can be encoded multiple times.

File a bug to track?

> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:117
> +    // FIXME: For simplicity this should be moved to the end of display() once the buffer handles can be created once
> +    // and stored in m_bufferHandle.

Reference the same bug.

> Source/WebKit/WebProcess/GPU/graphics/cocoa/RemoteGraphicsContextGLProxyCocoa.mm:91
> +    const bool m_isOpaque;
> +    const float m_contentsScale;
> +    MachSendRight m_displayBuffer;

Ditto.

[Kimmo Kinnunen]

Created attachment 446789 [details]
Patch for landing

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 446789 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Kimmo Kinnunen]

Created attachment 446976 [details]
Patch for landing

[EWS]

Committed r286943 (245168@main): <https://commits.webkit.org/245168@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446976 [details].

[Simon Fraser (smfr)]

Comment on attachment 446976 [details]
Patch for landing

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:33104
> +				7B582DD82716F55B004B92D0 /* (null) in Headers */,

What?