Bug 228308 – [WinCairo] New GraphicsLayer for GPU process mode

• Component: Compositing
• Status: RESOLVED FIXED

[Fujii Hironori]

[WinCairo] New GraphicsLayer for GPU process mode

In GPU process mode, WebGL is run in GPU process. So, the
compositor (TextureMapper) should be run in GPU process.
Otherwise, the output texutre of WebGL has to be transferred from
GPU process to the compositor process (UI process or web
process).

BTW, Mac port is using IOSurface, GTK and WPE ports plan to use
DMA to transfer the texture beyond the process boundary.
https://eleni.mutantstargoat.com/git/?p=webkit_codecamp;a=blob;f=codecamp.pdf
https://eleni.mutantstargoat.com/hikiko/angle-dma/

Coordinated Graphics has been significantly changed since
r212608 (Bug 168514) removed multi-process mode of Coordinated
Graphics.

I'm going to revive the architecture of the multi-process mode of
Coordinated Graphics.

[Fujii Hironori]

Created attachment 434257 [details]
WIP patch

[Fujii Hironori]

Created attachment 434501 [details]
WIP patch

[Simon Fraser (smfr)]

Comment on attachment 434501 [details]
WIP patch

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

> Source/WebCore/svg/graphics/SVGImage.cpp:283
> +#if !USE(WC) // This code path causes incorrect SVG rendering

Please don't add platform #ifdefs in shared files. If this is wrong, we should figure out how to make it work. Perhaps `!context.hasPlatformContext()` is the wrong check.

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:146
> +#if USE(WC)

"WC" is too short an acronym to be understandable. Please choose something longer.

[Fujii Hironori]

Comment on attachment 434501 [details]
WIP patch

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

>> Source/WebCore/svg/graphics/SVGImage.cpp:283
>> +#if !USE(WC) // This code path causes incorrect SVG rendering
> 
> Please don't add platform #ifdefs in shared files. If this is wrong, we should figure out how to make it work. Perhaps `!context.hasPlatformContext()` is the wrong check.

Yup. I should debug the SVG image issue or wait Myles to fix Bug#227748.

>> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:146
>> +#if USE(WC)
> 
> "WC" is too short an acronym to be understandable. Please choose something longer.

Doesn't it match with USE(CA) and GraphicsLayerCA?
WC is a tentative name for prototyping, not a acronym of anything. OK. I should have a better name.

[Fujii Hironori]

Created attachment 440578 [details]
WIP patch

[Fujii Hironori]

Created attachment 441343 [details]
WIP patch

[Fujii Hironori]

Created attachment 441642 [details]
Patch

[Simon Fraser (smfr)]

Could you split this patch up? it's very large to review in one piece.

[Fujii Hironori]

Sorry. It's difficult to split the patch.
Could you review a part of the patch?
I will fix and ask a review again, so on..

[Simon Fraser (smfr)]

You could add the GraphicsLayer subclasses in a single patch.

[Fujii Hironori]

Only GraphicsLayerWC class?
It doesn't make sense without other parts, for example DrawingAreaWC and RemoteWCLayerTreeHost.
The core part of the patch is unsplittable.

[Simon Fraser (smfr)]

New files can always be added in their own patches (admittedly, without testing).

[Fujii Hironori]

I don't like the idea.
I think separately landing new files can't reduce the review cost.
I want to keep the patch compile-able, testable and meaningful.
If I will revise the patch for review points, I will want to/need to test the new patch.

[Don Olmstead]

Comment on attachment 441642 [details]
Patch

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

Just commenting on the WebCore specific portions. Overall seems fine and fits with what's already in the WebCore platform.

> Source/WebCore/platform/graphics/wc/WCPlatformLayerGCGL.h:38
> +    uint64_t m_graphicsContextGLIdentifier;

Any reason this isn't initialized to 0 so there's some way to check validity?

[Fujii Hironori]

Comment on attachment 441642 [details]
Patch

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

>> Source/WebCore/platform/graphics/wc/WCPlatformLayerGCGL.h:38
>> +    uint64_t m_graphicsContextGLIdentifier;
> 
> Any reason this isn't initialized to 0 so there's some way to check validity?

It is set by setGraphicsContextGLIdentifier immediately after constructed. But, it'a good idea. Will fix.

[Simon Fraser (smfr)]

Can m_graphicsContextGLIdentifier be an ObjectIdentifer<>? We're trying hard not to add any new bare uint64_t identifier types now.

[Fujii Hironori]

(In reply to Simon Fraser (smfr) from comment #16)
> Can m_graphicsContextGLIdentifier be an ObjectIdentifer<>? We're trying hard
> not to add any new bare uint64_t identifier types now.

Good point. I have to confess it's a slightly layer violation.
GraphicsContextGLIdentifier is in WebKit layer, but RemoteGraphicsContextGLProxyBase is in WebCore layer.
I think it can be solved by add a new virutal method to RemoteGraphicsContextGLProxyBase to create a WebGL platfrom layer.

#if USE(GRAPHICS_LAYER_WC)
   virutal createPlatformLayer() = 0;
#endif

Will try.

[Don Olmstead]

(In reply to Fujii Hironori from comment #17)
> (In reply to Simon Fraser (smfr) from comment #16)
> > Can m_graphicsContextGLIdentifier be an ObjectIdentifer<>? We're trying hard
> > not to add any new bare uint64_t identifier types now.
> 
> Good point. I have to confess it's a slightly layer violation.
> GraphicsContextGLIdentifier is in WebKit layer, but
> RemoteGraphicsContextGLProxyBase is in WebCore layer.
> I think it can be solved by add a new virutal method to
> RemoteGraphicsContextGLProxyBase to create a WebGL platfrom layer.
> 
> #if USE(GRAPHICS_LAYER_WC)
>    virutal createPlatformLayer() = 0;
> #endif
> 
> Will try.

The Cocoa port does have the concept of a WebGL layer. Not sure if it would be beneficial to turn it more of a cross-platform construct.

[Don Olmstead]

Comment on attachment 441642 [details]
Patch

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

Will continue reviewing tomorrow. Overall happy just want to be thorough with it since its a huge change.

> Source/WebKit/GPUProcess/graphics/wc/RemoteWCLayerTreeHost.cpp:59
> +            glContextHolder->context = GLContext::createContextForWindow(reinterpret_cast<HWND>(nativeWindow));

This should be WebCore::GLNativeWindowType to be cross platform. On Windows it should end up with HWND but other platforms seems like void* is the actual.

> Source/WebKit/GPUProcess/graphics/wc/RemoteWCLayerTreeHost.h:67
> +    RefPtr<WCSharedGLContextHolder::Holder> m_sharedGLContextHolder;

I know that the Holder class is a wrapper around EGL in terms of the implementation but I don't think EGL Context should be in the name.

> Source/WebKit/GPUProcess/graphics/wc/WCSharedGLContextHolder.h:41
> +class WCSharedGLContextHolder {

Maybe this should be named around the Scene since its the one using it? Maybe WCSharedSceneContext?

The GL is an implementation detail, and may change over time, so tying the method name to it isn't great.

Also not sure if this needs to be available publicly or if it can live inside the WCScene.

[Fujii Hironori]

(In reply to Don Olmstead from comment #18)
> The Cocoa port does have the concept of a WebGL layer. Not sure if it would
> be beneficial to turn it more of a cross-platform construct.

WebGLLayer is Cocoa-spefic. I don't think we can share.

[Fujii Hironori]

Comment on attachment 441642 [details]
Patch

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

>> Source/WebKit/GPUProcess/graphics/wc/RemoteWCLayerTreeHost.cpp:59
>> +            glContextHolder->context = GLContext::createContextForWindow(reinterpret_cast<HWND>(nativeWindow));
> 
> This should be WebCore::GLNativeWindowType to be cross platform. On Windows it should end up with HWND but other platforms seems like void* is the actual.

Will fix.

>> Source/WebKit/GPUProcess/graphics/wc/WCSharedGLContextHolder.h:41
>> +class WCSharedGLContextHolder {
> 
> Maybe this should be named around the Scene since its the one using it? Maybe WCSharedSceneContext?
> 
> The GL is an implementation detail, and may change over time, so tying the method name to it isn't great.
> 
> Also not sure if this needs to be available publicly or if it can live inside the WCScene.

I think we can change the code after someone will actually create TextureMapperDawn or TextureMapperD3D.
OK, I'm going to create WCSceneContext and WCSharedSceneContextHolder.

I added a comment above to describe why this class is needed. Does it make sense?
That's the reason it can't live inside the WCScene.

[Fujii Hironori]

Created attachment 442326 [details]
Patch

* Addressed review feedbacks.

[Fujii Hironori]

Comment on attachment 442326 [details]
Patch

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

> ChangeLog:1
> [phone]  Fujii Hironori  <[e-mail]>

duplicated.

[Fujii Hironori]

Future Tasks

* sub-texture upload
  Implement GraphicsLayerWC::setNeedsDisplayInRect
* tiling scroll
* headless mode (Bug215041)
  composting in GPU process and reading back to shared memory and send to UI process
* async compositing
  Do not wait for finish compositing before starting painting the next frame
  Needs WebGL double buffering
* image layer
  Implement GraphicsLayerWC::setContentsToImage
* ASYNC_SCROLLING support
* video decoding in GPU process
* painting theme, scroll bar and controls in DisplayList mode

[Fujii Hironori]

Comment on attachment 442326 [details]
Patch

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

> Source/WebCore/platform/graphics/wc/RemoteGraphicsContextGLProxyBaseWC.cpp:37
> +    m_platformLayer = createPlatformLayer();

This code is crashing. It's too early to call the virtual method createPlatformLayer while constructing a base class.
I should add setPlatformLayer, and call it after the base class constructed.

[Fujii Hironori]

Created attachment 442526 [details]
Patch

[Darin Adler]

Comment on attachment 442526 [details]
Patch

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

> Source/WebCore/platform/graphics/RemoteGraphicsContextGLProxyBase.cpp:37
> +#include "TextureMapperPlatformLayer.h" // ~RemoteGraphicsContextGLProxyBase needs it

Not sure we should add comments like this one. No guarantee it stays accurate over time.

> Source/WebCore/platform/graphics/wc/RemoteGraphicsContextGLProxyBaseWC.cpp:50
> +} // namespace WebCore
> +#endif

Blank line

> Source/WebCore/platform/graphics/win/GraphicsContextWin.cpp:57
> +        return 0;

nullptr

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:343
> +    ASSERT_UNUSED(addResult, addResult.isNewEntry);

If we are asserting this, then I think we could use add instead of ensure above. The reason we use ensure is to optimize the case where the hash table already has a value at this key, but if that case represents an error, add is likely slightly more efficient in the non-error case and also more straightforward.

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:357
> +    auto iter = m_remoteGraphicsContextGLMap.find(identifier);
> +    if (iter == m_remoteGraphicsContextGLMap.end())
> +        return nullptr;
> +    return iter->value.get();

This can be written like this:

    return m_remoteGraphicsContextGLMap.get(identifier);

> Source/WebKit/GPUProcess/graphics/wc/RemoteWCLayerTreeHost.cpp:41
> +using namespace WebCore;

This is an anti-pattern. Please don’t add it in new code, and use WebCore prefix as necessary.

> Source/WebKit/GPUProcess/graphics/wc/RemoteWCLayerTreeHost.cpp:43
> +IPC::StreamConnectionWorkQueue& remoteGraphicsContextGLStreamWorkQueue();

Why is this being declared here rather than in a header?

> Source/WebKit/GPUProcess/graphics/wc/RemoteWCLayerTreeHost.cpp:57
> +    remoteGraphicsContextGLStreamWorkQueue().dispatch([scene = m_scene.get(), glContextHolder = m_sharedSceneContextHolder.get(), nativeWindow] {

This looks a little risky; what guarantees the lifetime of these captured values?

> Source/WebKit/GPUProcess/graphics/wc/WCScene.cpp:39
> +using namespace WebCore;

Please don’t use this.

> Source/WebKit/GPUProcess/graphics/wc/WCScene.h:32
> +#include <wtf/RefPtr.h>

This include should not be needed. Forward.h should be enough to use RefPtr in just an argument type name.

> Source/WebKit/GPUProcess/graphics/wc/WCScene.h:33
> +#include <wtf/Vector.h>

This include should not be needed. Forward.h should be enough to use Vector in just an argument type name.

> Source/WebKit/GPUProcess/graphics/wc/WCScene.h:58
> +    typedef HashMap<uint64_t, std::unique_ptr<Layer>> LayerMap;

In new code, please use "using" instead of "typedef".

> Source/WebKit/GPUProcess/graphics/wc/WCSceneContext.cpp:35
> +using namespace WebCore;

Please don’t use this.

> Source/WebKit/GPUProcess/graphics/wc/WCSceneContext.h:42
> +    WTF_MAKE_NONCOPYABLE(WCSceneContext);

Don’t really need this. Happens automatically if we have a std::unique_ptr data member without doing anything explicit.

> Source/WebKit/GPUProcess/graphics/wc/WCSceneContext.h:44
> +    WCSceneContext(uint64_t nativeWindow);

explicit

> Source/WebKit/UIProcess/wc/DrawingAreaProxyWC.cpp:39
> +using namespace WebCore;

Please don’t add.

> Source/WebKit/WebProcess/WebPage/wc/DrawingAreaWC.cpp:41
> +using namespace WebCore;

Please no.

> Source/WebKit/WebProcess/WebPage/wc/DrawingAreaWC.cpp:239
> +    const size_t rectThreshold = 10;
> +    const double wastedSpaceThreshold = 0.75;

Consider constexpr for these kinds of things in new code.

[Fujii Hironori]

Comment on attachment 442526 [details]
Patch

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

>> Source/WebKit/GPUProcess/graphics/wc/RemoteWCLayerTreeHost.cpp:41
>> +using namespace WebCore;
> 
> This is an anti-pattern. Please don’t add it in new code, and use WebCore prefix as necessary.

Really!? I didn't know that.
"using namespace WebCore" in the WebKit namespace is a pretty widely adopted common idiom in WebKit layer.
On the other hand, doing that in the global scope is the anti-pattern. (Bug 192449)

[Darin Adler]

(In reply to Fujii Hironori from comment #28)
> "using namespace WebCore" in the WebKit namespace is a pretty widely adopted
> common idiom in WebKit layer.

Yes, one we are trying to get rid of; I remove them as often as possible. And I require not adding new ones.

> On the other hand, doing that in the global scope is the anti-pattern. (Bug
> 192449)

Yes, that one is even worse, a disaster for unified compilation, to be avoided at all costs.

[Darin Adler]

(In reply to Darin Adler from comment #29)
> And I require not adding new ones.

*request*

[Fujii Hironori]

Comment on attachment 442526 [details]
Patch

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

>> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:357
>> +    return iter->value.get();
> 
> This can be written like this:
> 
>     return m_remoteGraphicsContextGLMap.get(identifier);

It can't compile.
https://gist.github.com/fujii/1c3fc5fd0e8f67aa494370fd5adb02d9
It seems a problem of ScopedActiveMessageReceiveQueue or GenericHashTraits::peek.
This patch is too big. I will fix it in another patch later.

[Fujii Hironori]

Comment on attachment 442526 [details]
Patch

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

>> Source/WebKit/GPUProcess/graphics/wc/RemoteWCLayerTreeHost.cpp:57
>> +    remoteGraphicsContextGLStreamWorkQueue().dispatch([scene = m_scene.get(), glContextHolder = m_sharedSceneContextHolder.get(), nativeWindow] {
> 
> This looks a little risky; what guarantees the lifetime of these captured values?

Good point.
Because ANGLE isn't thread-safe, they should be used only in the WebGL thread.
So, I designed WCScene and WCSceneContext classes completely thread-agnostic.
So, I don't want to make them ThreadSafeRefCounted.
That's the reason I pass the raw pointers of them here.
It actually looks risky code. However, lifetime of them are simple.
WCScene and WCSceneContext are destroyed in WebGL thread dipatched by RemoteWCLayerTreeHost destructor.

[Fujii Hironori]

Created attachment 442568 [details]
wrong patch

* Addressed review feedbacks.

[Fujii Hironori]

Created attachment 442569 [details]
Patch

[Don Olmstead]

Comment on attachment 442569 [details]
Patch

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

r=me with nits

> Source/WebCore/PlatformWinCairo.cmake:28
> +    platform/graphics/wc/RemoteGraphicsContextGLProxyBaseWC.cpp

Since this is agnostic and there is a reliance on TextureMapper maybe put these into the WebCore/platform/TextureMapper.cmake instead in an if block.

> Source/WebKit/GPUProcess/graphics/wc/WCScene.cpp:85
> +    for (auto& layerUpdate : update.changedLayers) {

To confirm this is meant to be the same logic as GraphicsLayerTextureMapper::commitLayerChanges() correct?

Just wondering if there's a better way so you don't have to keep them in sync.

> Source/WebKit/UIProcess/wc/DrawingAreaProxyWC.cpp:66
> +    send(Messages::DrawingArea::DidUpdate());

Just double checking this but the expectation is that an update is always fired.

[Fujii Hironori]

Comment on attachment 442569 [details]
Patch

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

Thank you very much for the review.

>> Source/WebCore/PlatformWinCairo.cmake:28
>> +    platform/graphics/wc/RemoteGraphicsContextGLProxyBaseWC.cpp
> 
> Since this is agnostic and there is a reliance on TextureMapper maybe put these into the WebCore/platform/TextureMapper.cmake instead in an if block.

I plan to add WC.cmake when PlayStation port adopts GraphicsLayerWC.
Becuase  are also using TextureMapper, but I think they won't adopt GraphicsLayerWC.
I think TextureMapper.cmake isn't a good place to add.
It'd be wonderful if AppleWin WK2, GTK and WPE will adopt GraphicsLayerWC in the future.

>> Source/WebKit/GPUProcess/graphics/wc/WCScene.cpp:85
>> +    for (auto& layerUpdate : update.changedLayers) {
> 
> To confirm this is meant to be the same logic as GraphicsLayerTextureMapper::commitLayerChanges() correct?
> 
> Just wondering if there's a better way so you don't have to keep them in sync.

Other GraphicsLayer implementations (GraphicsLayerCARemote, CoordinatedGraphicsLayer) also have the similar logic (RemoteLayerTreePropertyApplier::applyPropertiesToLayer, CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly).
But, I think it's difficult to sync them because they vary a lot.

WinCairo WK2 is still using GraphicsLayerTextureMapper in non-GPU process mode yet.
But, I plan to always enable GPU process mode for WinCairo WK2, and to remove WinCairo WK2 GraphicsLayerTextureMapper code because GPU process mode is promising and works very nice.
And, I'd like to remove GraphicsLayerTextureMapper if we wil be able to remove WinCairo WK1.

>> Source/WebKit/UIProcess/wc/DrawingAreaProxyWC.cpp:66
>> +    send(Messages::DrawingArea::DidUpdate());
> 
> Just double checking this but the expectation is that an update is always fired.

Good question.
DrawingAreaWC sends DrawingAreaProxy::Update message to DrawingAreaWCProxy only when it want to update the screen in non-AC mode.
In AC mode, DrawingAreaWC needs to send DrawingAreaProxy::EnterAcceleratedCompositingMode message. But, I forgot to do it.
Will fix.

[Fujii Hironori]

Committed r285099 (243741@main): <https://commits.webkit.org/243741@main>

[Radar WebKit Bug Importer]

<rdar://problem/84864092>