Bug 233756 – WC variant RemoteGraphicsContextGL::platformLayer() should be removed

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

[Kimmo Kinnunen]

WC variant RemoteGraphicsContextGL::platformLayer() should be removed

It causes problems because:
 - Increases ifdefs
 - RemoteGraphicsContextGL is not intended to have a "layer"


Currently the WC compositing logic appears to be:
In Web Process:
 a1. Figure out the id of the graphics context
 a2. Put this id in the platform layer, put the platform layer in webcore side layer tree and then to the compositor tree update
 a3. Send tree update to GPU process
In GPU Process:
 b1. Get the graphics context id from tree update
 b2. Find the graphics context by the id
 b3. Get the platform layer from the graphics context
 b4. Put the platform layer to the layer tree
 b5. Composite the layer tree

I think good approach would be to change:
a1. Return the platform layer id from the GraphicsContextGL::prepareForDisplay
a2. Put the platform layer id to webcore side layer tree, and ultimately to the compositor tree update

b2. Find the platform layer by the platform layer id


Alternative approach could be to:
a1. Make a GPU Process side mapping from RemoteGraphicsContextIdentifier to platform layer
b2. Find the platform layer by the RemoteGraphicsContextIdentifier.

[Fujii Hironori]

RemoteGraphicsContextGLProxy::platformLayer() returns an opaque PlatformLayer object.
It's set to canvas element's GraphicsLayer by using GraphicsLayer::setContentsToPlatformLayer.
This is the way binding RemoteGraphicsContextGLProxy and GraphicsLayer.

In Cocoa: https://github.com/WebKit/WebKit/blob/2115fd2a9d2e015ddebeef62ce148af0c6e80782/Source/WebKit/WebProcess/GPU/graphics/cocoa/RemoteGraphicsContextGLProxyCocoa.mm#L52
In GraphicsLayerWC: https://github.com/WebKit/WebKit/blob/2115fd2a9d2e015ddebeef62ce148af0c6e80782/Source/WebKit/WebProcess/GPU/graphics/wc/RemoteGraphicsContextGLProxyWC.cpp#L46

PlatformLayer isn't a good name for the opaque object.
platformLayer() should be renamed to layerContentsDisplayDelegate().
setContentsToPlatformLayer() should be renamed to setContentsToDisplayDelegate().
That's exactly you are doing in Bug 231009 Comment 30.

(In reply to Kimmo Kinnunen from comment #0)
> a1. Return the platform layer id from the
> GraphicsContextGL::prepareForDisplay

The return type of GraphicsContextGL::prepareForDisplay void.
How can I return the platform layer id?

> a2. Put the platform layer id to webcore side layer tree, and ultimately to
> the compositor tree update

How does it set to canvas element's GraphicsLayer?

> Alternative approach could be to:
> a1. Make a GPU Process side mapping from RemoteGraphicsContextIdentifier to
> platform layer

How can I know which platform layer is bound to the RemoteGraphicsContextIdentifier?

[Kimmo Kinnunen]

(In reply to Fujii Hironori from comment #1)
> RemoteGraphicsContextGLProxy::platformLayer() returns an opaque
> PlatformLayer object.

No, here we are talking about the GPUP side code having problem, e.g.

WC variant RemoteGraphicsContextGL::platformLayer() should be removed

 
> (In reply to Kimmo Kinnunen from comment #0)
> > a1. Return the platform layer id from the
> > GraphicsContextGL::prepareForDisplay
> 
> The return type of GraphicsContextGL::prepareForDisplay void.
> How can I return the platform layer id?

WebCore calls GraphicsContextGL::prepareForDisplay.
In that, your RemoteGraphicsContextGLProxy::prepareForDisplay will call GPUP RemoteGraphicsContextGL::prepareForDisplay(). This should return the 'id' for the new display buffer.
This id should be set to the 'current display buffer' property of the RemoteGraphicsContextGLProxy.

Ideally, in your case, this should go to the "platform layer" that stores the id.

When GraphicsLayer::setContentsToPlatformLayer() / GraphicsLayer::setContentsDisplayDelegate() gets called, you associate the "platform layer" to the GraphicsLayer.

When your WebCore side compositor commits, you go and fetch the id from the platform layer and store it in the tree update.


> 
> > a2. Put the platform layer id to webcore side layer tree, and ultimately to
> > the compositor tree update
> 
> How does it set to canvas element's GraphicsLayer?
> 
> > Alternative approach could be to:
> > a1. Make a GPU Process side mapping from RemoteGraphicsContextIdentifier to
> > platform layer
> 
> How can I know which platform layer is bound to the
> RemoteGraphicsContextIdentifier?

You can use a hash map. E.g. when the platform layer is created, you add its id to a global hash map (in thread-safe manner).

The reason this is a bad idea is that your compositor frames should contain references to the pictures rendered by the context (e.g. the id of the display buffer), not references to the rendering context. Currently your compositor is not capable of doing that, but anyway you should emulate the correct approach by storing the ids of GPUP side platform layers, not rendering context id.


After UI-side compositing commit is landed, it is easier to do:

Result of RemoteGraphicsContextGL::prepareForDisplay() is:
Mac:  IOSurface mach port (id)
WC: RGPUP PlatformLayer id (it's in HashMap<id,PlatformLayer> in gpup) 

When WebCore-side layer tree does display for GraphicsLayer representing the canvas:
Mac: DisplayDelegate puts the Mach port to PlatformCALayer contents property
WC: DisplayDelegate puts the id to <somewhere>

When Webcore-side layer tree commits:
Mac: the IOSurface mach port is put to the tree update.
WC: the id is put to the tree update (WCLayerUpateInfo, [sic]) WCLayerUpateInfo::contentsLayerId instead of WCLayerUpateInfo::graphicsContextGLIdentifier)

When the GPUP-side layer tree is applied in RemoteWCLayerTreeHost::update:
WCLayerUpateInfo::contentsLayerId will tell which platform layer to use.

To store the platform layers to a map and share the platform layers between processes, you need some sort of refcount scheme. E.g. when you share a layer from GPUP to WP, you need to increment the refcount (e.g. WP owns one ref). 
At WP side, every thing that uses the id will need to hold a ref, and then when WP drops the last ref, you need to let the GPUP know this.

Maybe there's some alternatives, but I think this should be consistent at least..

[Fujii Hironori]

Oh, I misunderstood. Thank you very much for the explanation. Will try it the next week.

[Fujii Hironori]

Created attachment 446003 [details]
WIP patch

[Fujii Hironori]

Created attachment 446303 [details]
WIP patch

[Fujii Hironori]

Created attachment 446315 [details]
WIP patch

[Kimmo Kinnunen]

Looks like a good start. I know it's a WIP, just some ideas.

I think you still need some sort of ref-counted object that represents the display buffer contents.
Because the compositor thread and the RemoteGraphicsContextGL thread are different, the graphics context might be destroyed already when the compositor will still show the contents of the last displayed buffer.

> Source/WebKit/GPUProcess/graphics/wc/RemoteWCLayerTreeHost.cpp:87
> +    remoteGraphicsContextGLStreamWorkQueue().dispatch([this, weakThis = WeakPtr(*this), scene = m_scene.get(), update = WTFMove(update)]() mutable {

Submitting non-webgl related tasks to remoteGraphicsContextGLStreamWorkQueue is a bit of a problem since they do not  really belong there.
It is also a problem since the work queue might contain massive amount of drawing calls, so your compositor would jank without need.  
It's better to have the WCDisplayBufferManager use a lock internally.

> Source/WebKit/GPUProcess/graphics/wc/WCDisplayBufferManager.h:50
> +    HashMap<WebCore::ProcessIdentifier, HashSet<RefPtr<WebCore::GraphicsLayerContentsDisplayDelegate>>> m_displayBufferMap;

It's not a good idea to hold the GraphicsLayerContentsDisplayDelegate, as that is not intended to be used this way.
It, again, will make changing GraphicsLayerContentsDisplayDelegate harder if it's used in a way that's not what it's used for.

Instead, you could use the layer type, as you already then navigate to the platform type via GraphicsLayerContentsDisplayDelegate::platformLayer().

This class also could be threadsafe using a lock, so you would not need to submit non-webgl tasks to remotegraphicscontextgl work queue.

> Source/WebKit/GPUProcess/graphics/wc/WCScene.cpp:73
> +void WCScene::update(WCUpateInfo&& update, Vector<RefPtr<WebCore::GraphicsLayerContentsDisplayDelegate>>&& displayDelegates)

Maybe it's not super-useful to pass the display delegates, nor that they would be passed here. 
Instead, below..

> Source/WebKit/GPUProcess/graphics/wc/WCScene.cpp:169
> +                auto platformLayer = (*displayDelegatesIterator)->platformLayer();

..this could be instead

PlatformLayer* platformLayer = nullptr;
if (layerUpdate.displayBufferIdentifier)
  platformLayer = WCDisplayBufferManager::singleton().getPlatformLayer(layerUpdate.displayBufferIdentifier);

> Source/WebKit/Shared/wc/WCDisplayBufferIdentifier.h:42
> +class WCDisplayBufferIdentifier {

The identifier most likely should be a similar construct as other cross-process identifiers, e.g. take a look how GraphicsContextGLIdentifier is done:

using GraphicsContextGLIdentifier = ObjectIdentifier<GraphicsContextGLIdentifierType>;

It's not a good idea to store a ptr value, as that has the potential to be used in insecure manner.

> Source/WebKit/WebProcess/GPU/graphics/wc/WCPlatformLayerGCGL.h:40
>      GraphicsContextGLIdentifier& graphicsContextGLIdentifier() { return m_graphicsContextGLIdentifier; }

I think in general there should not be a reason to store the context identifier?

> Source/WebKit/WebProcess/GPU/graphics/wc/WCPlatformLayerGCGL.h:42
> +    WCDisplayBufferIdentifier takeDisplayBufferIdentifier()

The assertions could be the other way around.
E.g. it's error to take out the identifier if there is nothing to be taken.

> Source/WebKit/WebProcess/GPU/graphics/wc/WCPlatformLayerGCGL.h:48
> +    {

In general it should be ok to overwrite a display buffer, probably also with WC compositor.

This would mean that graphics context has submitted a display buffer for the compositor, but the compositor did not have time to composite. So when the frame is overwritten, it means that we skip a frame.

> Source/WebKit/WebProcess/WebPage/wc/WCUpateInfo.h:78
> +    bool platformLayer;

probably this could be called hasContentsLayer?
(platformLayer would indicate a property that stores the  platform layer)
It's unclear if you ever have a valid case where "hasContentsLayer == true" but then "contentsLayerIdentifier" is empty.. So it might be that this bool is not needed.

> Source/WebKit/WebProcess/WebPage/wc/WCUpateInfo.h:93
> +    WCDisplayBufferIdentifier displayBufferIdentifier;

This could be called "contentsLayerIdentifier".

[Fujii Hironori]

Thank you very much for the review.

(In reply to Kimmo Kinnunen from comment #7)
> Looks like a good start. I know it's a WIP, just some ideas.
> 
> I think you still need some sort of ref-counted object that represents the
> display buffer contents.
> Because the compositor thread and the RemoteGraphicsContextGL thread are
> different, the graphics context might be destroyed already when the
> compositor will still show the contents of the last displayed buffer.
>
> > Source/WebKit/GPUProcess/graphics/wc/RemoteWCLayerTreeHost.cpp:87
> > +    remoteGraphicsContextGLStreamWorkQueue().dispatch([this, weakThis = WeakPtr(*this), scene = m_scene.get(), update = WTFMove(update)]() mutable {
> 
> Submitting non-webgl related tasks to remoteGraphicsContextGLStreamWorkQueue
> is a bit of a problem since they do not  really belong there.
> It is also a problem since the work queue might contain massive amount of
> drawing calls, so your compositor would jank without need.  
> It's better to have the WCDisplayBufferManager use a lock internally.

WinCairo is using ANGLE for the compositor and WebGL.
Because ANGLE D3D backend isn't thread-safe, the compositor should be run in the WebGL thread of GPU process.

However, this patch is crashing due to a lifetime issue.
That's because PlatformLayerDisplayDelegate doesn't hold the lifetime of m_platformLayer.

> > Source/WebKit/GPUProcess/graphics/wc/WCDisplayBufferManager.h:50
> > +    HashMap<WebCore::ProcessIdentifier, HashSet<RefPtr<WebCore::GraphicsLayerContentsDisplayDelegate>>> m_displayBufferMap;
> 
> It's not a good idea to hold the GraphicsLayerContentsDisplayDelegate, as
> that is not intended to be used this way.
> It, again, will make changing GraphicsLayerContentsDisplayDelegate harder if
> it's used in a way that's not what it's used for.
> 
> Instead, you could use the layer type, as you already then navigate to the
> platform type via GraphicsLayerContentsDisplayDelegate::platformLayer().

Because WinCairo's PlatformLayer isn't ref-counted, I use GraphicsLayerContentsDisplayDelegate.
I will try to make WinCairo's PlatformLayer ref-counted.

> This class also could be threadsafe using a lock, so you would not need to
> submit non-webgl tasks to remotegraphicscontextgl work queue.

ANGLE D3D backend isn't thread-safe.

> > Source/WebKit/GPUProcess/graphics/wc/WCScene.cpp:73
> > +void WCScene::update(WCUpateInfo&& update, Vector<RefPtr<WebCore::GraphicsLayerContentsDisplayDelegate>>&& displayDelegates)
> 
> Maybe it's not super-useful to pass the display delegates, nor that they
> would be passed here. 
> Instead, below..

You are right. Will fix.
The reason I did so is I had a plan to reuse WCScene when I will create a threaded-compositor mode of WC.

> > Source/WebKit/GPUProcess/graphics/wc/WCScene.cpp:169
> > +                auto platformLayer = (*displayDelegatesIterator)->platformLayer();
> 
> ..this could be instead
> 
> PlatformLayer* platformLayer = nullptr;
> if (layerUpdate.displayBufferIdentifier)
>   platformLayer =
> WCDisplayBufferManager::singleton().getPlatformLayer(layerUpdate.
> displayBufferIdentifier);

I think "get" isn't allowed for the method. https://webkit.org/code-style-guidelines/#names-if-exists

getPlatformLayer should take m_webProcessIdentifier as an argument for a security reason.

The current WinCairo's WebGL implementation is using a single output texture for WebGL.
A WebGL PlatformLayer has a texture.
This is the reason why WinCairo isn't using "buffer ID" for WebGL as of now.
But, I have a plan to do double-buffering for async-compositing (Bug 228308 comment 24).
I will need "buffer ID".
That's the reason I don't name this method like "getPlatformLayer".

> > Source/WebKit/Shared/wc/WCDisplayBufferIdentifier.h:42
> > +class WCDisplayBufferIdentifier {
> 
> The identifier most likely should be a similar construct as other
> cross-process identifiers, e.g. take a look how GraphicsContextGLIdentifier
> is done:
> 
> using GraphicsContextGLIdentifier =
> ObjectIdentifier<GraphicsContextGLIdentifierType>;
> 
> It's not a good idea to store a ptr value, as that has the potential to be
> used in insecure manner.

Using pointer is the most efficient way to generate valid identifier because all objects already has own address.
ObjectIdentifier assumes it never overflow, it doesn't check unused identifier.
This is less problematic for web process becuase it is re-spawned frequently for Process Swap On Navigation.
However, I think it's problematic for GPU process.
For the security concern, I use HashSet for validate the identifier.

> > Source/WebKit/WebProcess/GPU/graphics/wc/WCPlatformLayerGCGL.h:40
> >      GraphicsContextGLIdentifier& graphicsContextGLIdentifier() { return m_graphicsContextGLIdentifier; }
> 
> I think in general there should not be a reason to store the context
> identifier?

Will fix.

> > Source/WebKit/WebProcess/GPU/graphics/wc/WCPlatformLayerGCGL.h:42
> > +    WCDisplayBufferIdentifier takeDisplayBufferIdentifier()
> 
> The assertions could be the other way around.
> E.g. it's error to take out the identifier if there is nothing to be taken.

The current WinCairo's WebGL implementation is using a single output texture for WebGL.
That's the reason takeDisplayBufferIdentifier() returns a single WCDisplayBufferIdentifier .
If WinCairo will use more buffer for WebGL, the return type should be Vector<WCDisplayBufferIdentifier>.

And, I think it's a valid case that takeDisplayBufferIdentifier returns no WCDisplayBufferIdentifier if no buffer is available yet.

> > Source/WebKit/WebProcess/GPU/graphics/wc/WCPlatformLayerGCGL.h:48
> > +    {
> 
> In general it should be ok to overwrite a display buffer, probably also with
> WC compositor.
> 
> This would mean that graphics context has submitted a display buffer for the
> compositor, but the compositor did not have time to composite. So when the
> frame is overwritten, it means that we skip a frame.

All WCDisplayBufferIdentifier should be passed back to GPU process to release.
Otherwise, WCDisplayBufferIdentifier will leak.

> > Source/WebKit/WebProcess/WebPage/wc/WCUpateInfo.h:78
> > +    bool platformLayer;
> 
> probably this could be called hasContentsLayer?
> (platformLayer would indicate a property that stores the  platform layer)
> It's unclear if you ever have a valid case where "hasContentsLayer == true"
> but then "contentsLayerIdentifier" is empty.. So it might be that this bool
> is not needed.
> 
> > Source/WebKit/WebProcess/WebPage/wc/WCUpateInfo.h:93
> > +    WCDisplayBufferIdentifier displayBufferIdentifier;
> 
> This could be called "contentsLayerIdentifier".

I don't want to call it "layer" because I have a plan to use more buffers for WebGL.
If it's just a "layer identifier", I don't need this complex architecture change.
I want to call it "buffer" or "surface".
How about "contentBufferIdentifier"?
I will change the type WCDisplayBufferIdentifier to Vector<WCDisplayBufferIdentifier> to release all buffers in the future.
Then, it will be renamed to "contentBufferIdentifiers".

[Fujii Hironori]

> > Source/WebKit/WebProcess/WebPage/wc/WCUpateInfo.h:78
> > +    bool platformLayer;
> 
> probably this could be called hasContentsLayer?
> (platformLayer would indicate a property that stores the  platform layer)

Will fix.

> It's unclear if you ever have a valid case where "hasContentsLayer == true"
> but then "contentsLayerIdentifier" is empty.. So it might be that this bool
> is not needed.

In this patch, the maxnum number of buffer identifier is one for a single WebGL context.
If prepareForDisplay is called twice, the second buffer identifier will be empty.
That's the case "hasContentsLayer == true" and "contentsLayerIdentifier" is empty.
In this case, WebGL platfrom layer should keep displaying the previous buffer.

[Radar WebKit Bug Importer]

<rdar://problem/86261919>

[Fujii Hironori]

(In reply to Fujii Hironori from comment #9)
> > > Source/WebKit/WebProcess/WebPage/wc/WCUpateInfo.h:78
> > > +    bool platformLayer;
> > 
> > probably this could be called hasContentsLayer?
> > (platformLayer would indicate a property that stores the  platform layer)
> 
> Will fix.

No. We shouldn't call it "contents layer".
GraphicsLayer has usesContentsLayer() method which should return true if setContentsTo* or setContentsDisplayDelegate is used.
So, for example, if setContentsToImage is used, the GraphicsLayer has a "contents layer".
However, this bool parameter is used only for "platform layer" that is set by setContentsToPlatformLayer.
"hasPlatformLayer" seemd a good name.

[Fujii Hironori]

Created attachment 447206 [details]
WIP patch

[Fujii Hironori]

Created attachment 447326 [details]
Patch

[Fujii Hironori]

Created attachment 447329 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 447329 [details]
Patch

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

Great, I think this paves the way for having the correct drawing buffer, display buffer management for WC port!

> Source/WebKit/GPUProcess/graphics/wc/WCContentBuffer.h:74
> +    void platformLayerWillBeDestroyed() override

FWIW: So this signifies an discrepancy or a slight error:
It does not make sense that the "image" referenced by the wccontentbuffer can be "destroyed". WCContentBuffer *is* the image.
In future work, if you spend time on this, you can change it so that instead of storing the platform layer into the wccontentbuffer, you store ANGLE texture id.
This way your compositor works correctly in the case about the display buffer and drawing buffer.

> Source/WebKit/Shared/wc/WCContentBufferIdentifier.h:38
> +class WCContentBufferIdentifier {

I still think this is a bit bad to have structure like:
- Everything in WebKit uses ObjectIdentifier
- Except WCContentBufferIdentifier
There's very much a mental overhead of looking at the code and undestanding "this should be an identifier, why is it not a normal identifier".
Also, you implement custom code for the "process qualified identifier" that we also have.

Also, the security idea is that it is good to avoid exposing addresses of controllable allocations.


So if there's a real concern about overflowing 64-bit integers: 
If my calculations are correct, you can animate 60fps webgl for 584942417355 years until an overflow.
That is 584942 years showing million canvas elements simultaneously 60fps.

[Fujii Hironori]

Comment on attachment 447329 [details]
Patch

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

Thank you very much for the review.

>> Source/WebKit/GPUProcess/graphics/wc/WCContentBuffer.h:74
>> +    void platformLayerWillBeDestroyed() override
> 
> FWIW: So this signifies an discrepancy or a slight error:
> It does not make sense that the "image" referenced by the wccontentbuffer can be "destroyed". WCContentBuffer *is* the image.
> In future work, if you spend time on this, you can change it so that instead of storing the platform layer into the wccontentbuffer, you store ANGLE texture id.
> This way your compositor works correctly in the case about the display buffer and drawing buffer.

Yes. That's what I'm planning to do in the future.

>> Source/WebKit/Shared/wc/WCContentBufferIdentifier.h:38
>> +class WCContentBufferIdentifier {
> 
> I still think this is a bit bad to have structure like:
> - Everything in WebKit uses ObjectIdentifier
> - Except WCContentBufferIdentifier
> There's very much a mental overhead of looking at the code and undestanding "this should be an identifier, why is it not a normal identifier".
> Also, you implement custom code for the "process qualified identifier" that we also have.
> 
> Also, the security idea is that it is good to avoid exposing addresses of controllable allocations.
> 
> 
> So if there's a real concern about overflowing 64-bit integers: 
> If my calculations are correct, you can animate 60fps webgl for 584942417355 years until an overflow.
> That is 584942 years showing million canvas elements simultaneously 60fps.

OK. Will fix.

[Fujii Hironori]

Created attachment 447660 [details]
Patch for landing

[Fujii Hironori]

Comment on attachment 447660 [details]
Patch for landing

Clearing flags on attachment: 447660

Committed r287288 (245440@trunk): <https://commits.webkit.org/245440@trunk>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.