Bug 228308

Created attachment 434257 [details] WIP patch

Created attachment 434501 [details] WIP patch

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.

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.

Created attachment 440578 [details] WIP patch

Created attachment 441343 [details] WIP patch

Created attachment 441642 [details] Patch

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

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

You could add the GraphicsLayer subclasses in a single patch.

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

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

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.

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?

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.

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

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

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

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.

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

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.

Created attachment 442326 [details] Patch * Addressed review feedbacks.

Comment on attachment 442326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442326&action=review > ChangeLog:1 > +2021-10-24 Fujii Hironori <Hironori.Fujii@sony.com> duplicated.

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

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.

Created attachment 442526 [details] Patch

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.

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)

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

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

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.

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.

Created attachment 442568 [details] wrong patch * Addressed review feedbacks.

Created attachment 442569 [details] Patch

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.

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.

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

<rdar://problem/84864092>