Bug 204955

Created attachment 385057 [details] Patch

Created attachment 385071 [details] Patch

Created attachment 385072 [details] Patch

Comment on attachment 385071 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385071&action=review > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:654 > - colorSpace = ColorSpaceSRGB; > + colorSpace = ColorSpace::SRGB; Can you do the enum class change in a different patch? > Source/WebCore/platform/graphics/RenderingBackend.h:41 > + virtual std::pair<uint64_t, std::unique_ptr<IOSurface>> createSharedIOSurface(const IntSize&, const IntSize& contextSize, ColorSpace) = 0; > + virtual bool releaseSharedIOSurface(uint64_t surfaceID) = 0; Please use ObjectIdentifier<> rather than a bare uint64_t. > Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:86 > + auto result = IOSurfaceManager::sharedManager().createSharedIOSurface(size, contextSize, colorSpace); > + > + ASSERT(!!result.first == !!result.second); > + if (result.second) > + reply(result.first, result.second->createSendRight()); > + else > + reply(0, { }); I think you need a RenderingBackend-type proxy thing in the GPU process that does this part. > Source/WebKit/GPUProcess/IOSurfaceManager.h:36 > +class IOSurfaceManager { Can we not call it a Manager? IOSurfaceSet?

Comment on attachment 385072 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385072&action=review IOSurface is an macOS/iOS class/concept, so I don't think it should be used in these ostensibly cross platform classes. In most other cases we create an abstraction around it and use IOSurface as the implementation of that abstraction on Apple platforms. > Source/WebCore/ChangeLog:4 > + Make GPU process create/release a shared IOSurface > + https://bugs.webkit.org/show_bug.cgi?id=204955 How are you testing this? > Source/WebCore/page/Page.h:280 > +#if ENABLE(GPU_PROCESS) > + RefPtr<RenderingBackend> renderingBackend(); > +#endif Leaking the notion of a GPU process into the page seems very unfortunate. I think this would be much cleaner/clearer if all pages had a RenderingBackend, and one implementation of the backend was a GPU process. > Source/WebCore/platform/graphics/ColorSpace.h:33 > +enum class ColorSpace : uint8_t { > + SRGB, > + LinearRGB, > + DisplayP3 Can you make this change in its own change prior to this? > Source/WebKit/GPUProcess/GPUConnectionToWebProcess.messages.in:26 > + void CreateSharedIOSurface(WebCore::IntSize size, WebCore::IntSize contextSize, enum:uint8_t WebCore::ColorSpace colorSpace) -> (uint64_t surfaceID, MachSendRight sendRight) Synchronous Please do not introduce any new sync messages, they almost always cause performance issues down the line.

(In reply to Sam Weinig from comment #5) > Comment on attachment 385072 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=385072&action=review > > IOSurface is an macOS/iOS class/concept, so I don't think it should be used > in these ostensibly cross platform classes. In most other cases we create > an abstraction around it and use IOSurface as the implementation of that > abstraction on Apple platforms. Agreed, it should have a more "ImageBuffer"y name (but not that obviously). RenderingSurface? > > Source/WebCore/ChangeLog:4 > > + Make GPU process create/release a shared IOSurface > > + https://bugs.webkit.org/show_bug.cgi?id=204955 > > How are you testing this? Other folks are bringing up the testing story. > > Source/WebCore/page/Page.h:280 > > +#if ENABLE(GPU_PROCESS) > > + RefPtr<RenderingBackend> renderingBackend(); > > +#endif > > Leaking the notion of a GPU process into the page seems very unfortunate. I > think this would be much cleaner/clearer if all pages had a > RenderingBackend, and one implementation of the backend was a GPU process. This was my intent when suggesting this design, indeed. > > Source/WebKit/GPUProcess/GPUConnectionToWebProcess.messages.in:26 > > + void CreateSharedIOSurface(WebCore::IntSize size, WebCore::IntSize contextSize, enum:uint8_t WebCore::ColorSpace colorSpace) -> (uint64_t surfaceID, MachSendRight sendRight) Synchronous > > Please do not introduce any new sync messages, they almost always cause > performance issues down the line. We add sync messages /out/ from the Web Content process all the time. We do need to come up with a plan that isn't sync here, but don't know what it is yet (since currently these things happen during painting).

(In reply to Tim Horton from comment #6) > (In reply to Sam Weinig from comment #5) > > > Source/WebCore/ChangeLog:4 > > > + Make GPU process create/release a shared IOSurface > > > + https://bugs.webkit.org/show_bug.cgi?id=204955 > > > > How are you testing this? > > Other folks are bringing up the testing story. > Boo. Do it together or do it first. :) > > > > Source/WebKit/GPUProcess/GPUConnectionToWebProcess.messages.in:26 > > > + void CreateSharedIOSurface(WebCore::IntSize size, WebCore::IntSize contextSize, enum:uint8_t WebCore::ColorSpace colorSpace) -> (uint64_t surfaceID, MachSendRight sendRight) Synchronous > > > > Please do not introduce any new sync messages, they almost always cause > > performance issues down the line. > > We add sync messages /out/ from the Web Content process all the time. We do > need to come up with a plan that isn't sync here, but don't know what it is > yet (since currently these things happen during painting). Assuming painting doesn't do much/any read back of the surface (and that be a big assumption), you could buffer the painting calls until the surface has been created, and then send the batch of buffered painting calls all at once. If read back is needed, and can't be implemented otherwise, you could do the sync wait only at that point.

(In reply to Tim Horton from comment #6) > (In reply to Sam Weinig from comment #5) > > > > Source/WebKit/GPUProcess/GPUConnectionToWebProcess.messages.in:26 > > > + void CreateSharedIOSurface(WebCore::IntSize size, WebCore::IntSize contextSize, enum:uint8_t WebCore::ColorSpace colorSpace) -> (uint64_t surfaceID, MachSendRight sendRight) Synchronous > > > > Please do not introduce any new sync messages, they almost always cause > > performance issues down the line. > > We add sync messages /out/ from the Web Content process all the time. We do > need to come up with a plan that isn't sync here, but don't know what it is > yet (since currently these things happen during painting). Just because we do it already, doesn't mean it is a good pattern, or something we should add more of. For sure, it isn't as bad as blocking the main thread of the UI process with a sync call, but it still is something that we should work hard to avoid, and not build into our designs.

Created attachment 385330 [details] Patch

Created attachment 385337 [details] Patch

Comment on attachment 385071 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385071&action=review >> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:654 >> + colorSpace = ColorSpace::SRGB; > > Can you do the enum class change in a different patch? Done in r253268. >> Source/WebCore/platform/graphics/RenderingBackend.h:41 >> + virtual bool releaseSharedIOSurface(uint64_t surfaceID) = 0; > > Please use ObjectIdentifier<> rather than a bare uint64_t. Fixed. >> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:86 >> + reply(0, { }); > > I think you need a RenderingBackend-type proxy thing in the GPU process that does this part. Done. >> Source/WebKit/GPUProcess/IOSurfaceManager.h:36 >> +class IOSurfaceManager { > > Can we not call it a Manager? IOSurfaceSet? I changed the name to RenderingSurfaceSet to be platform independent. It still returns IOSurface only. But the plan is to make it return a platform independent surface e.g. 'RenderingSurface'.

Comment on attachment 385072 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385072&action=review >>> Source/WebCore/ChangeLog:4 >>> + https://bugs.webkit.org/show_bug.cgi?id=204955 >> >> How are you testing this? > > Other folks are bringing up the testing story. With this patch, we can't. But we just need to make the ImageBuffer of the CanvasRenderingContext2D be backed by a GPUProcess IOSurface. I think I can do this in the next patch through an internal setting. >>> Source/WebCore/page/Page.h:280 >>> +#endif >> >> Leaking the notion of a GPU process into the page seems very unfortunate. I think this would be much cleaner/clearer if all pages had a RenderingBackend, and one implementation of the backend was a GPU process. > > This was my intent when suggesting this design, indeed. We discussed this and Simon even suggested having a RenderingBackend per Document or Frame. The reason was in some cases, like frame animation throttling, we may want to have control over flushing the GraphicsContext for this frame. But I really don't know the best owner of RenderingBackend, so I think we can leave it in the page for now and we can change it later if we have to. >> Source/WebCore/platform/graphics/ColorSpace.h:33 >> + DisplayP3 > > Can you make this change in its own change prior to this? Done in r253268. >>> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.messages.in:26 >>> + void CreateSharedIOSurface(WebCore::IntSize size, WebCore::IntSize contextSize, enum:uint8_t WebCore::ColorSpace colorSpace) -> (uint64_t surfaceID, MachSendRight sendRight) Synchronous >> >> Please do not introduce any new sync messages, they almost always cause performance issues down the line. > > We add sync messages /out/ from the Web Content process all the time. We do need to come up with a plan that isn't sync here, but don't know what it is yet (since currently these things happen during painting). Creating an IOSurface does not happen very often. We definitely have to make the painting asynchronous but it requires very careful synchronization. If creating the IOSurface appears in the traces, which I doubt, we can make it also asynchronous message. But please let us have a simple interface for now till we start measuring the perf implication.

(In reply to Said Abou-Hallawa from comment #12) > Comment on attachment 385072 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=385072&action=review > > >>> Source/WebCore/ChangeLog:4 > >>> + https://bugs.webkit.org/show_bug.cgi?id=204955 > >> > >> How are you testing this? > > > > Other folks are bringing up the testing story. > > With this patch, we can't. But we just need to make the ImageBuffer of the > CanvasRenderingContext2D be backed by a GPUProcess IOSurface. I think I can > do this in the next patch through an internal setting. > > >>> Source/WebCore/page/Page.h:280 > >>> +#endif > >> > >> Leaking the notion of a GPU process into the page seems very unfortunate. I think this would be much cleaner/clearer if all pages had a RenderingBackend, and one implementation of the backend was a GPU process. > > > > This was my intent when suggesting this design, indeed. > > We discussed this and Simon even suggested having a RenderingBackend per > Document or Frame. The reason was in some cases, like frame animation > throttling, we may want to have control over flushing the GraphicsContext > for this frame. But I really don't know the best owner of RenderingBackend, > so I think we can leave it in the page for now and we can change it later if > we have to. I think there was a misunderstanding here. Sam's point is not that Page shouldn't own it, it's that *all* Pages should have a RenderingBackend (even ones that don't use a GPU process), with a sensible in-process implementation that is used in the GPU-process-disabled case. > >> Source/WebCore/platform/graphics/ColorSpace.h:33 > >> + DisplayP3 > > > > Can you make this change in its own change prior to this? > > Done in r253268. > > >>> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.messages.in:26 > >>> + void CreateSharedIOSurface(WebCore::IntSize size, WebCore::IntSize contextSize, enum:uint8_t WebCore::ColorSpace colorSpace) -> (uint64_t surfaceID, MachSendRight sendRight) Synchronous > >> > >> Please do not introduce any new sync messages, they almost always cause performance issues down the line. > > > > We add sync messages /out/ from the Web Content process all the time. We do need to come up with a plan that isn't sync here, but don't know what it is yet (since currently these things happen during painting). > > Creating an IOSurface does not happen very often. This is not true on pages where e.g. a layer changes size frequently, though it might be true with canvas. > We definitely have to make the painting asynchronous but it requires very careful synchronization. If > creating the IOSurface appears in the traces, It already does. > which I doubt, we can make it > also asynchronous message. But please let us have a simple interface for now > till we start measuring the perf implication. Sam is right that we should avoid it if possible. On the other hand, I understand why it's hard to do async things under painting code. BUT, since we're going to have to figure it out for painting, we might as well start thinking about it now. Maybe we should have a brainstorming session about this tomorrow. It would be /amazing/ if we could avoid adding handleSyncMessage to the GPU process entirely.

(In reply to Tim Horton from comment #13) > > It would be /amazing/ if we could avoid adding handleSyncMessage to the GPU > process entirely. It would indeed! I hope you aren't holding your breath.

(In reply to Eric Carlson from comment #14) > (In reply to Tim Horton from comment #13) > > > > It would be /amazing/ if we could avoid adding handleSyncMessage to the GPU > > process entirely. > > It would indeed! > > I hope you aren't holding your breath. Don't worry, I complained at you too

Created attachment 385359 [details] Patch Fix gtk, wpe, wincairo and win EWS

Created attachment 385670 [details] Patch

Created attachment 385673 [details] Patch

Created attachment 385682 [details] Patch

Created attachment 385684 [details] Patch

Created attachment 385689 [details] Patch

(In reply to Tim Horton from comment #13) > (In reply to Said Abou-Hallawa from comment #12) > > Comment on attachment 385072 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=385072&action=review > > > > >>> Source/WebCore/ChangeLog:4 > > >>> + https://bugs.webkit.org/show_bug.cgi?id=204955 > > >> > > >> How are you testing this? > > > > > > Other folks are bringing up the testing story. > > > > With this patch, we can't. But we just need to make the ImageBuffer of the > > CanvasRenderingContext2D be backed by a GPUProcess IOSurface. I think I can > > do this in the next patch through an internal setting. > > > > >>> Source/WebCore/page/Page.h:280 > > >>> +#endif > > >> > > >> Leaking the notion of a GPU process into the page seems very unfortunate. I think this would be much cleaner/clearer if all pages had a RenderingBackend, and one implementation of the backend was a GPU process. > > > > > > This was my intent when suggesting this design, indeed. > > > > We discussed this and Simon even suggested having a RenderingBackend per > > Document or Frame. The reason was in some cases, like frame animation > > throttling, we may want to have control over flushing the GraphicsContext > > for this frame. But I really don't know the best owner of RenderingBackend, > > so I think we can leave it in the page for now and we can change it later if > > we have to. > > I think there was a misunderstanding here. > > Sam's point is not that Page shouldn't own it, it's that *all* Pages should > have a RenderingBackend (even ones that don't use a GPU process), with a > sensible in-process implementation that is used in the GPU-process-disabled > case. > The GPU process painting through RenderingBackend is based on replaying back the DisplayList. It will be hard to make the GPU-process-disabled case rely on RenderingBackend especially if DisplayList is not used. > > >> Source/WebCore/platform/graphics/ColorSpace.h:33 > > >> + DisplayP3 > > > > > > Can you make this change in its own change prior to this? > > > > Done in r253268. > > > > >>> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.messages.in:26 > > >>> + void CreateSharedIOSurface(WebCore::IntSize size, WebCore::IntSize contextSize, enum:uint8_t WebCore::ColorSpace colorSpace) -> (uint64_t surfaceID, MachSendRight sendRight) Synchronous > > >> > > >> Please do not introduce any new sync messages, they almost always cause performance issues down the line. > > > > > > We add sync messages /out/ from the Web Content process all the time. We do need to come up with a plan that isn't sync here, but don't know what it is yet (since currently these things happen during painting). > > > > Creating an IOSurface does not happen very often. > > This is not true on pages where e.g. a layer changes size frequently, though > it might be true with canvas. > All the messages are made asynchronous. Generating the SurfaceIdentifier happens in the WebProcess. Sending the "CreateSurface" message does not need to wait for a reply from the GPUProcess. However two cases the WebProcess may be blocked later only for "sometime" to make sure everything is synchronized correctly: 1. Flushing the surface. 2. Getting the platform surface. > > We definitely have to make the painting asynchronous but it requires very careful synchronization. If > > creating the IOSurface appears in the traces, > > It already does. > > > which I doubt, we can make it > > also asynchronous message. But please let us have a simple interface for now > > till we start measuring the perf implication. > > Sam is right that we should avoid it if possible. On the other hand, I > understand why it's hard to do async things under painting code. BUT, since > we're going to have to figure it out for painting, we might as well start > thinking about it now. Maybe we should have a brainstorming session about > this tomorrow. > > It would be /amazing/ if we could avoid adding handleSyncMessage to the GPU > process entirely. Done.

Comment on attachment 385689 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385689&action=review > Source/WebCore/page/Page.h:280 > +#if ENABLE(GPU_PROCESS) > + RefPtr<RenderingBackend> renderingBackend(); > +#endif It doesn't seem like this should be guarded by ENABLE(GPU_PROCESS). The point of having an abstraction like this is so WebCore doesn't have to know that there is a GPU process or not. > Source/WebCore/platform/graphics/RenderingBackend.h:48 > + // These function send IPC async messages but they do not block the main thread. > + virtual SurfaceIdentifier createProxySurface(const IntSize&, const IntSize& contextSize, ColorSpace) = 0; Presumably this comment is only correct for an implementation of RenderingBackend that uses an external process, and should be in the concrete implementation, not the base class. The base class should only express the requirements, e.g. "Implementations of this function must not block the main thread", or whatever invariant you want upheld.

I'm really not clear on what the plan is here, so it's a bit hard to review this change. I know we don't often write out plans ahead of time for WebKit, but given the scope of this change, it might make sense to outline a plan/design, at least in broad strokes.

(In reply to Said Abou-Hallawa from comment #22) > (In reply to Tim Horton from comment #13) > > (In reply to Said Abou-Hallawa from comment #12) > > > Comment on attachment 385072 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=385072&action=review > > > > > > >>> Source/WebCore/ChangeLog:4 > > > >>> + https://bugs.webkit.org/show_bug.cgi?id=204955 > > > >> > > > >> How are you testing this? > > > > > > > > Other folks are bringing up the testing story. > > > > > > With this patch, we can't. But we just need to make the ImageBuffer of the > > > CanvasRenderingContext2D be backed by a GPUProcess IOSurface. I think I can > > > do this in the next patch through an internal setting. > > > > > > >>> Source/WebCore/page/Page.h:280 > > > >>> +#endif > > > >> > > > >> Leaking the notion of a GPU process into the page seems very unfortunate. I think this would be much cleaner/clearer if all pages had a RenderingBackend, and one implementation of the backend was a GPU process. > > > > > > > > This was my intent when suggesting this design, indeed. > > > > > > We discussed this and Simon even suggested having a RenderingBackend per > > > Document or Frame. The reason was in some cases, like frame animation > > > throttling, we may want to have control over flushing the GraphicsContext > > > for this frame. But I really don't know the best owner of RenderingBackend, > > > so I think we can leave it in the page for now and we can change it later if > > > we have to. > > > > I think there was a misunderstanding here. > > > > Sam's point is not that Page shouldn't own it, it's that *all* Pages should > > have a RenderingBackend (even ones that don't use a GPU process), with a > > sensible in-process implementation that is used in the GPU-process-disabled > > case. > > > > The GPU process painting through RenderingBackend is based on replaying back > the DisplayList. It will be hard to make the GPU-process-disabled case rely > on RenderingBackend especially if DisplayList is not used. Then it seems like this is the wrong abstraction point. Taking just the case of HTML canvas, it seems like all WebCore should need to do / know about is how to create an ImageBuffer, with that ImageBuffer exposing a GraphicsContext that makes sense for the mode it is in. If the GPU process is in use, an ImageBuffer backed by a display list recorder style GraphicsContext, a connection to the GPU Process, etc would be used. If the GPU process is not in use, the traditional ImageBuffer would be used.

(In reply to Sam Weinig from comment #25) > > Then it seems like this is the wrong abstraction point. > > Taking just the case of HTML canvas, it seems like all WebCore should need > to do / know about is how to create an ImageBuffer, with that ImageBuffer > exposing a GraphicsContext that makes sense for the mode it is in. If the > GPU process is in use, an ImageBuffer backed by a display list recorder > style GraphicsContext, a connection to the GPU Process, etc would be used. > If the GPU process is not in use, the traditional ImageBuffer would be used. I agree and I mentioned the same thing in the latest WebCore/ChangeLog. ImageBuffer will be the ideal abstraction since it encapsulates different and platform dependent backing store for the image buffer/surface. But I mentioned also this will require code refactoring for the ImageBuffer class especially it is difficult to hack.

(In reply to Said Abou-Hallawa from comment #26) > (In reply to Sam Weinig from comment #25) > > > > Then it seems like this is the wrong abstraction point. > > > > Taking just the case of HTML canvas, it seems like all WebCore should need > > to do / know about is how to create an ImageBuffer, with that ImageBuffer > > exposing a GraphicsContext that makes sense for the mode it is in. If the > > GPU process is in use, an ImageBuffer backed by a display list recorder > > style GraphicsContext, a connection to the GPU Process, etc would be used. > > If the GPU process is not in use, the traditional ImageBuffer would be used. > > I agree and I mentioned the same thing in the latest WebCore/ChangeLog. > ImageBuffer will be the ideal abstraction since it encapsulates different > and platform dependent backing store for the image buffer/surface. But I > mentioned also this will require code refactoring for the ImageBuffer class > especially it is difficult to hack. Seems like that refactoring should be done first then.

Comment on attachment 385689 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385689&action=review > Source/WebCore/platform/graphics/cg/RenderingSurfaceCG.h:41 > + std::unique_ptr<IOSurface>& platformSurface() { return m_platfromSurface; } Minor nit - m_platfromSurface => m_platformSurface.

Created attachment 386801 [details] Patch

(In reply to Sam Weinig from comment #27) > (In reply to Said Abou-Hallawa from comment #26) > > (In reply to Sam Weinig from comment #25) > > > > > > Then it seems like this is the wrong abstraction point. > > > > > > Taking just the case of HTML canvas, it seems like all WebCore should need > > > to do / know about is how to create an ImageBuffer, with that ImageBuffer > > > exposing a GraphicsContext that makes sense for the mode it is in. If the > > > GPU process is in use, an ImageBuffer backed by a display list recorder > > > style GraphicsContext, a connection to the GPU Process, etc would be used. > > > If the GPU process is not in use, the traditional ImageBuffer would be used. > > > > I agree and I mentioned the same thing in the latest WebCore/ChangeLog. > > ImageBuffer will be the ideal abstraction since it encapsulates different > > and platform dependent backing store for the image buffer/surface. But I > > mentioned also this will require code refactoring for the ImageBuffer class > > especially it is difficult to hack. > > Seems like that refactoring should be done first then. I did the refactoring but I also include all the other classes which allow remote rendering. In fact if the following statement in HTMLCanvasElement::createImageBuffer() bool usesRemoteDrawing = false; is changed to be bool usesRemoteDrawing = true; The canvas drawing will be carried out by the GPU process. I can split the patch into smaller patches once the refactoring is approved. I preferred putting things in one patch so I make the design for the old and the new requirements.

Comment on attachment 386801 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386801&action=review > Source/WebCore/platform/graphics/ImageBuffer.cpp:38 > + return RenderingBackend::sharedBackend().createImageBuffer(size, renderingMode, resolutionScale, colorSpace, hostWindow); Rather than going through yet another singleton, createImageBuffer would probably make more sense as function on HostWindow, that can call out to the ChromeClient for an implementation. > Source/WebCore/platform/graphics/RenderingBackend.cpp:55 > + return RenderingBackend::ensureSharedBackend(); Seems like this will fail if it's the first call to ensureSharedBackend(), since ensureSharedBackend() requires the page pointer to non-null to not crash in that case. > Source/WebCore/platform/graphics/RenderingBackend.h:41 > + static RenderingBackend& ensureSharedBackend(Page* = nullptr); It's a layering violation to use Page from platform code. Instead, the idiomatic way to go is to use HostWindow, which is 1-to-1 with Page.

Looking much better!

Created attachment 386813 [details] Patch

Created attachment 386816 [details] Patch

Created attachment 386929 [details] Patch

Created attachment 386979 [details] Patch

Created attachment 386993 [details] Patch

Created attachment 387006 [details] Patch

Created attachment 387023 [details] Patch

Comment on attachment 387023 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387023&action=review > Source/WebCore/ChangeLog:12 > + -- ImageBufferBackned Backned! > Source/WebCore/ChangeLog:13 > + -- DrawingContext GraphicsContext? > Source/WebCore/ChangeLog:52 > + WebPorcess: RemotePhysicalImageBuffer<ImageBufferIOSurfaceBackend> > + GPUPorcess: RemotePhysicalImageBufferProxy<ImageBufferIOSurfaceBackend> Porcess!

Created attachment 387059 [details] Patch

Created attachment 387072 [details] Patch

Created attachment 387171 [details] Patch

Created attachment 387181 [details] Patch

Created attachment 387191 [details] Patch

This patch should fix the gtk build. No promises. Not sure that all changes were correct, either. That’s why we have tests.

Also a review comment from fiddling with this: the children of the Cairo backend should also have Cairo in their names

Created attachment 387193 [details] Patch

Now with WPE too

Created attachment 387276 [details] Patch

Created attachment 387281 [details] Patch

Created attachment 387283 [details] Patch

Created attachment 387302 [details] Patch

Created attachment 387432 [details] Patch

Created attachment 387434 [details] Patch

Created attachment 387436 [details] Patch

Created attachment 387440 [details] Patch

Created attachment 387474 [details] Patch

Created attachment 387478 [details] Patch

Created attachment 387503 [details] Patch

To help seeing the sequence of calls when rendering on an ImageBuffer, remotely consider this example. It creates an ImageBuffer, fill a rectangle on it and then draws the ImageBuffer on a destination rectangle: C++ code: 1. auto buffer = ImageBuffer::create(RenderingMode::Remote, ...) 2. auto context = buffer->context() 3. context.fillRect(...) 4. buffer->draw(destContext, ...) Calling sequence: WebPorcess GPUProcess 1.1. ImageBuffer::create() 1.2. RemoteRenderingBackend::createImageBuffer() 1.3. RemotePhysicalImageBuffer::create() 1.4 IPC::MessageSender::send(RemoteRenderingBackendProxy::CreateImageBuffer, ...) 1.5. RemoteRenderingBackendProxy::createImageBuffer() 1.6. RemotePhysicalImageBufferProxy::create() 1.7. ImageBufferShareableBitmapBackend::create() 1.8 IPC::MessageSender::send(RemoteImageBuffer::CreateImageBufferBackend, ...) 1.9. RemotePhysicalImageBuffer::createImageBufferBackend() 1.10. ImageBufferShareableBitmapBackend::create() 2.1. DisplayListImageBuffer::context() 2.2 DrawingContext::context() 3.1. GraphicsContext::fillRect() 3.1. DisplayList::Recorder::fillRect() 3.2. DisplayList::DisplayList::appendItem() 4.1. PhysicalImageBuffer::draw() 4.2. RemotePhysicalImageBuffer::flushDrawingContext() 4.3. IPC::MessageSender::send(RemoteImageBufferProxy::FlushContext, ...) 4.4. RemoteImageBufferProxy::flushContext() 4.5. RemotePhysicalImageBufferProxy::localFlushContext() 4.6. DisplayList::Replayer::replay() 4.6. IPC::MessageSender::send(Messages::RemoteImageBuffer::SetFlushIdentifier, ...) 4.7. RemoteImageBuffer::setFlushIdentifier() 4.8. ImageBufferCGBackend::draw() 4.9. ImageBufferShareableBitmapBackend::copyNativeImage() 4.10. GraphicsContext::drawNativeImage()

Comment on attachment 387503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387503&action=review > Source/WebKit/ChangeLog:22 > + -- ImageBufferShareableIOSurfaceBackend is a CG only accelerated ImageBuffer > + backed by ImageBufferIOSurfaceBackend All IOSurfaces are shareable, so what's the difference? > Source/WebKit/WebProcess/GPU/RenderingBackend/PlatformImageBufferShareableBackend.h:38 > +#if USE(CG) && (USE(IOSURFACE_CANVAS_BACKING_STORE) || ENABLE(ACCELERATED_2D_CANVAS)) Probably just HAVE(IOSURFACE)

Comment on attachment 387503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387503&action=review This patch would be smaller if the enum class changes were done separately. > Source/WebCore/platform/graphics/AlphaPremultiplication.h:32 > +enum class AlphaPremultiplication : uint8_t { This could go into GraphicsTypes.h > Source/WebCore/platform/graphics/DisplayListImageBuffer.h:34 > +class DisplayListImageBuffer : public PhysicalImageBuffer<BackendType> , public DisplayList::DrawingContext { space before comma I wonder if this would be better with composition rather than inheritance. DisplayListImageBuffer could own a DrawingContext (or just own the GraphicsContext and DisplayList directly). > Source/WebCore/platform/graphics/GraphicsTypes.h:28 > +#include "RenderingMode.h" Why did you move this into its own header? > Source/WebCore/platform/graphics/PhysicalImageBuffer.h:33 > +class PhysicalImageBuffer : public ImageBuffer { I don't really like the term "physical" for this. Maybe PixelImageBuffer? Or MemoryBackedImageBuffer?

Comment on attachment 387503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387503&action=review >> Source/WebKit/ChangeLog:22 >> + backed by ImageBufferIOSurfaceBackend > > All IOSurfaces are shareable, so what's the difference? ImageBufferShareableIOSurfaceBackend can be created from an ImageBufferBackendHandle and also it can create an ImageBufferBackendHandle. This is how the IOSurface backends of the RemoteImageBuffer and RemoteImageBufferProxy are created. But the ImageBufferBackendHandle is defined in WebKit as using ImageBufferBackendHandle = Variant< #if PLATFORM(COCOA) MachSendRight, #endif ShareableBitmap::Handle >; It is defined in WebKit because ShareableBitmap is defined in WebKit. So dealing with ShareableBitmap can't be not done in WebCore.. >> Source/WebCore/platform/graphics/AlphaPremultiplication.h:32 >> +enum class AlphaPremultiplication : uint8_t { > > This could go into GraphicsTypes.h I think messages.py generates a forward declaration for every type in the messages.in file unless you add an entry in types_that_cannot_be_forward_declared. In this case, it will add an #include statement for this type. This file is not currently included in the WebKit2 derived sources but it will be needed to implement the remote getImageData() and putImageData(). >> Source/WebCore/platform/graphics/DisplayListImageBuffer.h:34 >> +class DisplayListImageBuffer : public PhysicalImageBuffer<BackendType> , public DisplayList::DrawingContext { > > space before comma > > I wonder if this would be better with composition rather than inheritance. DisplayListImageBuffer could own a DrawingContext (or just own the GraphicsContext and DisplayList directly). I copied DisplayList::DrawingContext from DisplayListDrawingContext and I was planning to replace DisplayListDrawingContext by DisplayList::DrawingContext but it seemed it would.a big change and it is not in the scope of this patch. So I kept both for now. And I think the inheritance can be replaced by composition >> Source/WebCore/platform/graphics/GraphicsTypes.h:28 >> +#include "RenderingMode.h" > > Why did you move this into its own header? Because RenderingMode is included in RemoteRenderingBackendProxy.messages.in. messages.py needs to include it otherwise it will generate a wrong forward declaration namespace WebCore { class RenderingMode }; >> Source/WebKit/WebProcess/GPU/RenderingBackend/PlatformImageBufferShareableBackend.h:38 >> +#if USE(CG) && (USE(IOSURFACE_CANVAS_BACKING_STORE) || ENABLE(ACCELERATED_2D_CANVAS)) > > Probably just HAVE(IOSURFACE) I copied it from bool CanvasRenderingContext2DBase::isAccelerated() const { #if USE(IOSURFACE_CANVAS_BACKING_STORE) || ENABLE(ACCELERATED_2D_CANVAS) auto* context = canvasBase().existingDrawingContext(); return context && context->isAcceleratedContext(); #else return false; #endif }

Comment on attachment 387503 [details] Patch Just sharing some layout test results 1) GTK Layout tests results for canvas and fast/canvas on r254446 (1 failure) https://people.igalia.com/clopez/wkbug/204955/canvas_layout_results_r254446/results.html 2) GTK Layout tests results for canvas and fast/canvas on r254446 + attachment 387503 [details] (this patch) (132 failures) https://people.igalia.com/clopez/wkbug/204955/canvas_layout_results_attachment_387503/results.html Hope this helps

Created attachment 387678 [details] Patch

Created attachment 387688 [details] Patch

(In reply to Carlos Alberto Lopez Perez from comment #65) > Comment on attachment 387503 [details] > Patch > > Just sharing some layout test results > > 1) GTK Layout tests results for canvas and fast/canvas on r254446 (1 failure) > https://people.igalia.com/clopez/wkbug/204955/canvas_layout_results_r254446/ > results.html > > 2) GTK Layout tests results for canvas and fast/canvas on r254446 + > attachment 387503 [details] (this patch) (132 failures) > https://people.igalia.com/clopez/wkbug/204955/ > canvas_layout_results_attachment_387503/results.html > > > Hope this helps Yes. This was helpful and promising as well. Can you please try the latest patch and see if most of the failures were fixed or not? This is the change I made: diff --git a/Source/WebCore/platform/graphics/cairo/ImageBufferCairoBackend.h b/Source/WebCore/platform/graphics/cairo/ImageBufferCairoBackend.h index 03ea1662017..23b919ad522 100644 --- a/Source/WebCore/platform/graphics/cairo/ImageBufferCairoBackend.h +++ b/Source/WebCore/platform/graphics/cairo/ImageBufferCairoBackend.h @@ -50,6 +50,7 @@ public: protected: using ImageBufferBackend::ImageBufferBackend; + ColorFormat backendColorFormat() const override { return ColorFormat::BGRA; } virtual void platformTransformColorSpace(const std::array<uint8_t, 256>&) { } };

(In reply to Said Abou-Hallawa from comment #68) > Yes. This was helpful and promising as well. Can you please try the latest > patch and see if most of the failures were fixed or not? Its a big patch and I'm getting rejects when trying to apply it. Can you comment in which revision you have based it so I can apply it over the same revision than you to avoid patch-apply rejects? Thanks

(In reply to Carlos Alberto Lopez Perez from comment #69) > (In reply to Said Abou-Hallawa from comment #68) > > Yes. This was helpful and promising as well. Can you please try the latest > > patch and see if most of the failures were fixed or not? > > Its a big patch and I'm getting rejects when trying to apply it. Can you > comment in which revision you have based it so I can apply it over the same > revision than you to avoid patch-apply rejects? Thanks Thanks Carlos for helping out. The revision I based this patch on is r254519. Alternatively if you can apply the previous patch, which you may still have it built, and just add the following line in ImageBufferCairoBackend.h ColorFormat backendColorFormat() const override { return ColorFormat::BGRA; }

(In reply to Said Abou-Hallawa from comment #70) > (In reply to Carlos Alberto Lopez Perez from comment #69) > > (In reply to Said Abou-Hallawa from comment #68) > > > Yes. This was helpful and promising as well. Can you please try the latest > > > patch and see if most of the failures were fixed or not? > > > > Its a big patch and I'm getting rejects when trying to apply it. Can you > > comment in which revision you have based it so I can apply it over the same > > revision than you to avoid patch-apply rejects? Thanks > > Thanks Carlos for helping out. > > The revision I based this patch on is r254519. Alternatively if you can > apply the previous patch, which you may still have it built, and just add > the following line in ImageBufferCairoBackend.h > > ColorFormat backendColorFormat() const override { return ColorFormat::BGRA; } Much better now! Only 1 new failure. The new failure is fast/canvas/canvas-putImageData-zero-alpha.html New results: 1) GTK Layout tests results for canvas and fast/canvas on r254519 (1 failure) https://people.igalia.com/clopez/wkbug/204955/canvas_layout_results_r254519/results.html 2) GTK Layout tests results for canvas and fast/canvas on r254519 + attachment 387688 [details] (2 failures) https://people.igalia.com/clopez/wkbug/204955/canvas_layout_results_attachment_387688/results.html

Created attachment 389770 [details] Patch

Created attachment 389783 [details] Patch

Created attachment 389784 [details] Patch

Created attachment 389804 [details] Patch for review

<rdar://problem/57177349>

Rename ContextualImageBuffer<> to ConcreteImageBuffer<> Drop "concrete" in the derived classes. Rename "RemoteImageBuffer[Proxy]" to something (it's not an image buffer). using UnacceleratedImageBuffer = ContextualImageBuffer<PlatformUnacceleratedImageBufferBackend>; using AcceleratedImageBuffer = ContextualImageBuffer<PlatformAcceleratedImageBufferBackend>; using UnacceleratedDisplayListImageBuffer = DisplayListImageBuffer<PlatformUnacceleratedImageBufferBackend>; using AcceleratedDisplayListImageBuffer = DisplayListImageBuffer<PlatformAcceleratedImageBufferBackend>; Can you do this? using AcceleratedDisplayListImageBuffer = ContextualImageBuffer<Accelerated, DisplayList>; RemoteImageBuffer -> RemoteImageBufferChannel?

Created attachment 391230 [details] Complete Patch

Created attachment 391238 [details] perf patch

Comment on attachment 389804 [details] Patch for review restoring r? on the final sub-patch needed to complete this task.

Comment on attachment 389804 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=389804&action=review > Source/WebCore/platform/graphics/ContextualImageBuffer.h:78 > + AffineTransform baseTransform() const override { return ensureBackend() ? ensureBackend()->baseTransform() : AffineTransform(); } This ensureBackend() ? ensureBackend() : pattern is weird. What if ensureBackend() is very expensive? Also, is the backend created lazily? Should it be? > Source/WebCore/platform/graphics/ContextualImageBuffer.h:182 > + if (auto* backend = ensureBackend()) { Can we just make the backend eagerly? > Source/WebKit/GPUProcess/RenderingBackend/RemoteContextualImageBufferProxy.h:37 > +class RemoteContextualImageBufferProxy : public WebCore::ContextualImageBuffer<BackendType>, public RemoteImageBufferProxy { Does RemoteImageBufferProxy exist now? I think we talked about changing how it works. > Source/WebKit/GPUProcess/RenderingBackend/RemoteContextualImageBufferProxy.h:39 > + using BaseContextualImageBuffer::m_backend; A bit odd. > Source/WebKit/WebProcess/GPU/RenderingBackend/RemoteImageBuffer.cpp:86 > + constexpr const unsigned MaxWaitsCount = 3; "Waits count" is unclear.

After making HostWindow be the creator of the remote ImageBuffer, there is no need for this bug. *** This bug has been marked as a duplicate of bug 207134 ***

Wrong bug.

Created attachment 392118 [details] Patch

Created attachment 392140 [details] Patch for review

Comment on attachment 392140 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=392140&action=review > Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferMessageHandler.h:58 > + // Blockers I find this comment a little odd; I'd rather have the nature of the function be explicitly in the name. > Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferMessageHandler.h:59 > + void ensureBackendCreated(); In a followup patch: waitForBackendToBeCreated waitForBackendCreation > Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackend.h:67 > + // Blockers Please remove.

Created attachment 392194 [details] Patch

Comment on attachment 392140 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=392140&action=review >> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferMessageHandler.h:59 >> + void ensureBackendCreated(); > > In a followup patch: > waitForBackendToBeCreated > waitForBackendCreation Functions' names were changed to void waitForCreateImageBufferBackend(); void waitForCommitImageBufferFlushContext(); >> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackend.h:67 >> + // Blockers > > Please remove. Comment was removed.

Created attachment 392201 [details] Patch

Comment on attachment 389804 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=389804&action=review >> Source/WebCore/platform/graphics/ContextualImageBuffer.h:78 >> + AffineTransform baseTransform() const override { return ensureBackend() ? ensureBackend()->baseTransform() : AffineTransform(); } > > This ensureBackend() ? ensureBackend() : pattern is weird. What if ensureBackend() is very expensive? Also, is the backend created lazily? Should it be? Inline pattern was removed. Instead I'm using the pattern if (auto* backend = ensureBackend()) return backend->... The backend of the RemoteImageBuffer should be "asynchronous". Here are the steps for creating it: 1. RemoteRenderingBackend creates RemoteImageBuffer. 2. RemoteImageBuffer sends the message "CreateImageBuffer" to RemoteRenderingBackendProxy via RemoteRenderingBackend. 3. RemoteRenderingBackendProxy creates RemoteImageBufferProxy 4. RemoteImageBufferProxy sends the message "CreateImageBufferBackend" to RemoteImageBuffer via RemoteRenderingBackendProxy and RemoteRenderingBackend 5. RemoteImageBuffer creates its backend using the handle sent from RemoteImageBufferProxy. Both RemoteImageBuffer and RemoteImageBufferProxy backends have to share the same memory for the backend data. If any of the functions of this class is called before the backend is created, execution will be halted till the message is received or one second period is elapsed. >> Source/WebCore/platform/graphics/ContextualImageBuffer.h:182 >> + if (auto* backend = ensureBackend()) { > > Can we just make the backend eagerly? This would require RemoteImageBuffer to send a synchronous message for CreateImageBuffer message. >> Source/WebKit/GPUProcess/RenderingBackend/RemoteContextualImageBufferProxy.h:37 >> +class RemoteContextualImageBufferProxy : public WebCore::ContextualImageBuffer<BackendType>, public RemoteImageBufferProxy { > > Does RemoteImageBufferProxy exist now? I think we talked about changing how it works. Yes it exists but it is the name of the remote ImageBuffer in GPUProcess. >> Source/WebKit/GPUProcess/RenderingBackend/RemoteContextualImageBufferProxy.h:39 >> + using BaseContextualImageBuffer::m_backend; > > A bit odd. Because the base class is a template class, I have to to include this using statement before referencing any member in it. >> Source/WebKit/WebProcess/GPU/RenderingBackend/RemoteImageBuffer.cpp:86 >> + constexpr const unsigned MaxWaitsCount = 3; > > "Waits count" is unclear. The variable and the loop below were removed.

Comment on attachment 392201 [details] Patch Clearing flags on attachment: 392201 Committed r257747: <https://trac.webkit.org/changeset/257747>

All reviewed patches have been landed. Closing bug.