Bug 208621

Created attachment 392525 [details] WIP

Created attachment 392528 [details] WIP

<rdar://problem/60068047>

Created attachment 392531 [details] WIP

Created attachment 392540 [details] Patch

Comment on attachment 392540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392540&action=review > Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:68 > + m_drawingContext.displayList().append(PutImageData::create(inputFormat, imageData, srcRect, destPoint)); The other display list items are appended via the display list recorder, which calls into Recorder::willAppendItem(). We should probably double check that it’s okay to bypass that mechanism when appending PutImageData. > Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:1171 > + ASSERT(imageData.data()); Nit - I don’t think you need this assertion, given that we would’ve dereferenced `imageData.data()` above already when creating m_data. > Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:2538 > + Nit - extra newline.

Comment on attachment 392540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392540&action=review > Source/WebCore/ChangeLog:13 > + putImageData() is implemented just as a DisplayListItem. Conceptually, it's the > + same as a draw command. Unfortunately, it can't be implemented on top of > + GraphicsContext, and instead has to be implemented on top of ImageBuffer, so > + this patch also adds an optional ImageBuffer* argument to DisplayListItem::apply(). > + The data itself is transferred using shared memory. All DisplayListItems are recorded when the drawing commands are applied to the recorder GraphicsContext. Replaying back the DisplayListItems are applied to the destination GraphicsContext. So I am not sure why ImageBuffer::putImageData has to be injected in the middle of a DisplayList. It looks a little bit hacky to make a lot of plumbing to make this happen. > Source/WebCore/ChangeLog:16 > + Implementing this as a DisplayListItem rather than its own IPC message is superior > + because it gives us more control about when to flush the in-flight display list. Another approach is to add a new message for putImageData like this PutImageData(DisplayList, AlphaPremultiplication, ...) Upon receiving this message, RemoteImageBufferProxy will replay back the DisplayList and then put the image data. This message does not even have to return a commit message to the WebProcess. > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2106 > + DisplayList::Replayer replayer(*canvasBase().drawingContext(), m_recordingContext->displayList, canvasBase().buffer()); Why is this necessary, the in-process DisplayList::ImageBuffer does not need to record the putImageData as a DisplayList::Item. It can access the data of the backend directly. > Source/WebCore/platform/graphics/AlphaPremultiplication.h:49 > +namespace WTF { > +template<> struct EnumTraits<WebCore::AlphaPremultiplication> { > + using values = EnumValues< > + WebCore::AlphaPremultiplication, > + WebCore::AlphaPremultiplication::Premultiplied, > + WebCore::AlphaPremultiplication::Unpremultiplied > + >; > +}; I think we usually put this in WebCoreArgumentCoder.h. > Source/WebCore/platform/graphics/displaylists/DisplayList.h:42 > +template<typename BackendType> class ImageBuffer; Why this forward declaration is needed? > Source/WebCore/platform/graphics/displaylists/DisplayList.h:172 > + template<typename BackendType> friend class ImageBuffer; Why is DisplayList::ImageBuffer added as a friend? > Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:69 > + void putImageData(AlphaPremultiplication inputFormat, const ImageData& imageData, const IntRect& srcRect, const IntPoint& destPoint = { }) override > + { > + m_drawingContext.displayList().append(PutImageData::create(inputFormat, imageData, srcRect, destPoint)); > + } The putImageData() special handling should be in RemoteImageBuffer (which is a superclass of DisplayList::ImageBuffer) and RemoteImageBufferProxy. > Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:1167 > + , m_imageDataSize(imageData.size()) > + , m_data(WebCore::SharedBuffer::create(imageData.data()->data(), imageData.data()->byteLength())) // This copy is actually required to preserve the semantics of putImageData(). Why don't we add encoding/decoding code for ImageData such that we do not to split the date from the dataSize? > Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:1194 > + auto imageData = ImageData::create(m_imageDataSize, Uint8ClampedArray::create(reinterpret_cast<const uint8_t*>(m_data->data()), m_data->size())); This should be handled by the decoder of ImageData. > Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:2490 > + Optional<FloatRect> localBounds(const GraphicsContext&) const override { return FloatRect(m_destPoint, m_srcRect.size()); } I do not think this is the correct calculation for the actual destination rectangle. Please see how "destRect" is calculated in ImageBufferBackend::putImageData(). > Source/WebKit/GPUProcess/graphics/RemoteImageBufferProxy.h:60 > + WebCore::DisplayList::Replayer replayer(BaseConcreteImageBuffer::context(), displayList, this); Too much plumbing to have a hacky special case works. RemoteImageBufferProxy passes "this" to Replayer. Replayer passes the ImageBuffer to Item::apply. Only PutImageData::apply() makes use of it but it ignores the GraphicsContext. I think adding this message to RenderingBackendProxy.messages.in can solve the problem with less code change: void PutImageData(DisplayList::DisplayList displayList, AlphaPremultiplication inputFormat, ImageData data, IntRect srcRect, IntPoint destPoint) RemoteImageBufferProxy::putImageData() will replay back the DisplayList and then put the image data.

Comment on attachment 392540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392540&action=review >> Source/WebCore/platform/graphics/AlphaPremultiplication.h:49 >> +}; > > I think we usually put this in WebCoreArgumentCoder.h. We’re actually moving away from putting encoders/decoders in WebCoreArgumentCode if possible. In this case, we just need to declare EnumTraits, so this could just stay in WebCore.

Comment on attachment 392540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392540&action=review >> Source/WebCore/ChangeLog:13 >> + The data itself is transferred using shared memory. > > All DisplayListItems are recorded when the drawing commands are applied to the recorder GraphicsContext. Replaying back the DisplayListItems are applied to the destination GraphicsContext. So I am not sure why ImageBuffer::putImageData has to be injected in the middle of a DisplayList. It looks a little bit hacky to make a lot of plumbing to make this happen. The reason is to avoid unnecessary flush()es. If we added a new message, then whenever Javascript just happened to call putImageData, that would require a flush. I'm worried about Javascript like the following: context.fillRect(...); context.putImageData(...); context.fillRect(...); context.putImageData(...); context.fillRect(...); context.putImageData(...); context.fillRect(...); context.putImageData(...); If we added a new message for PutImageData, the above code would cause 4 flushes. If we make it a display list item, there's only one flush. This is important because the schedule we flush is an implementation detail. We want to be as simple or as sophisticated as we want when deciding when to flush. And, we want that schedule to be as resilient as possible to the original Javascript. >> Source/WebCore/platform/graphics/displaylists/DisplayList.h:42 >> +template<typename BackendType> class ImageBuffer; > > Why this forward declaration is needed? Because of the "friend" line below. >> Source/WebCore/platform/graphics/displaylists/DisplayList.h:172 >> + template<typename BackendType> friend class ImageBuffer; > > Why is DisplayList::ImageBuffer added as a friend? So the ImageBuffer can call DisplayList::append(PutImageData::create(...)). append() is a private function. >> Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:68 >> + m_drawingContext.displayList().append(PutImageData::create(inputFormat, imageData, srcRect, destPoint)); > > The other display list items are appended via the display list recorder, which calls into Recorder::willAppendItem(). We should probably double check that it’s okay to bypass that mechanism when appending PutImageData. Good catch! >> Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:69 >> + } > > The putImageData() special handling should be in RemoteImageBuffer (which is a superclass of DisplayList::ImageBuffer) and RemoteImageBufferProxy. Yeah, I intentionally put this in DisplayListImageBuffer so that this new "PutImageData is a DisplayListItem" codepath is used even for in-process canvases. However, if you think this new codepath should only be used for GPU process canvases, I'm happy to try to move it. >> Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:1194 >> + auto imageData = ImageData::create(m_imageDataSize, Uint8ClampedArray::create(reinterpret_cast<const uint8_t*>(m_data->data()), m_data->size())); > > This should be handled by the decoder of ImageData. I did this intentionally because of the "FIXME" on the above line - I'm trying to set myself up to avoid a copy in a later patch. Passing the SharedBuffer into the implementation of putImageData() seems more natural than having ImageData optionally own a SharedBuffer. I'll try the latter, and see if it's workable, and if so, I'll do that. I'd like to do this in a follow-up patch, though, which removes the copy here. >> Source/WebKit/GPUProcess/graphics/RemoteImageBufferProxy.h:60 >> + WebCore::DisplayList::Replayer replayer(BaseConcreteImageBuffer::context(), displayList, this); > > Too much plumbing to have a hacky special case works. RemoteImageBufferProxy passes "this" to Replayer. Replayer passes the ImageBuffer to Item::apply. Only PutImageData::apply() makes use of it but it ignores the GraphicsContext. I think adding this message to RenderingBackendProxy.messages.in can solve the problem with less code change: > > void PutImageData(DisplayList::DisplayList displayList, AlphaPremultiplication inputFormat, ImageData data, IntRect srcRect, IntPoint destPoint) > > RemoteImageBufferProxy::putImageData() will replay back the DisplayList and then put the image data. Right - less code change, but worse performance. Performance is more important than code changes, and I don't think this patch is beyond the limit of amount of acceptable plumbing.

Created attachment 392657 [details] Patch

Comment on attachment 392657 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392657&action=review > Source/WebCore/platform/graphics/displaylists/DisplayList.h:110 > + virtual void apply(GraphicsContext&) const { }; No semicolon. > Source/WebCore/platform/graphics/displaylists/DisplayList.h:114 > + }; No semicolon. > Source/WebCore/platform/graphics/displaylists/DisplayListDrawingContext.cpp:40 > + auto recorder = makeUnique<Recorder>(displayListContext, m_displayList, GraphicsContextState(), FloatRect({ }, logicalSize), AffineTransform()); > + m_recorder = recorder.get(); > + return recorder; This is icky. You keep a copy of the raw pointer then return the unique_ptr<>? You've lost any lifetime guarantees about how long m_recorder is valid. If the context stores the recorder, why can't you just dig it out later? > Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:2497 > + AlphaPremultiplication m_inputFormat; > + IntSize m_imageDataSize; 3 bytes of padding here. > Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.h:42 > + WEBCORE_EXPORT Replayer(GraphicsContext&, const DisplayList&, ImageBuffer* = nullptr); Would be a little nicer if Replayer had a delegate which could supply the ImageBuffer on demand. > Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.h:50 > + ImageBuffer* m_imageBuffer; { nullptr }. Vague ownership model.

Comment on attachment 392657 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392657&action=review >> Source/WebCore/platform/graphics/displaylists/DisplayListDrawingContext.cpp:40 >> + return recorder; > > This is icky. You keep a copy of the raw pointer then return the unique_ptr<>? You've lost any lifetime guarantees about how long m_recorder is valid. > > If the context stores the recorder, why can't you just dig it out later? And have a method in GraphicsContext that only one subclass implements? Or, alternatively, have GraphicsContext return its impl, and have DisplayListDrawingContext downcast it to a Recorder? What do you think about making GraphicsContext::impl a RefPtr instead of a UniquePtr? That way we would get the ownership guarantees? >> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.h:50 >> + ImageBuffer* m_imageBuffer; > > { nullptr }. > > Vague ownership model. It's the same as m_displayList and m_context

Comment on attachment 392540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392540&action=review >>> Source/WebCore/ChangeLog:13 >>> + The data itself is transferred using shared memory. >> >> All DisplayListItems are recorded when the drawing commands are applied to the recorder GraphicsContext. Replaying back the DisplayListItems are applied to the destination GraphicsContext. So I am not sure why ImageBuffer::putImageData has to be injected in the middle of a DisplayList. It looks a little bit hacky to make a lot of plumbing to make this happen. > > The reason is to avoid unnecessary flush()es. If we added a new message, then whenever Javascript just happened to call putImageData, that would require a flush. > > I'm worried about Javascript like the following: > > context.fillRect(...); > context.putImageData(...); > context.fillRect(...); > context.putImageData(...); > context.fillRect(...); > context.putImageData(...); > context.fillRect(...); > context.putImageData(...); > > If we added a new message for PutImageData, the above code would cause 4 flushes. If we make it a display list item, there's only one flush. > > This is important because the schedule we flush is an implementation detail. We want to be as simple or as sophisticated as we want when deciding when to flush. And, we want that schedule to be as resilient as possible to the original Javascript. Okay this is fine although I am not convinced what you wrote above is a real user scenario. Usually putImageData() is preceded by getImageData() or at least this is what we do in the MotionMark Images test. But if I wrote this code and I know I am going to call putImageData with the same data, I would convert the data first to an image and use drawImage() instead since drawImage() is more efficient than putImageData(). The solution is still a little bit hacky. Please find a better way plumbing the ImageBuffer into the DisplayList::Replayer. There are other instances of not implemented DisplayList::Items for which the drawing destination is not the GraphicsContext, for example the form controls. So it does not make sense to keep adding these destinations if we want to support these items. An approach is to make RemoteImageBuffer an observer to the DisplayList::Replayer. The replayer can ask its observer whether it needs to apply the item to itself or not. RemoteImageBuffer will override the observer virtual function applyItem() and in it will check the item. If it sees it is PutImageData, it gets the data and calls putImageData. Please see https://bugs.webkit.org/show_bug.cgi?id=208597 where I made RemoteImageBuffer an observer to the recorder.

Comment on attachment 392657 [details] Patch r- based on feedback

Comment on attachment 392540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392540&action=review >>>> Source/WebCore/ChangeLog:13 >>>> + The data itself is transferred using shared memory. >>> >>> All DisplayListItems are recorded when the drawing commands are applied to the recorder GraphicsContext. Replaying back the DisplayListItems are applied to the destination GraphicsContext. So I am not sure why ImageBuffer::putImageData has to be injected in the middle of a DisplayList. It looks a little bit hacky to make a lot of plumbing to make this happen. >> >> The reason is to avoid unnecessary flush()es. If we added a new message, then whenever Javascript just happened to call putImageData, that would require a flush. >> >> I'm worried about Javascript like the following: >> >> context.fillRect(...); >> context.putImageData(...); >> context.fillRect(...); >> context.putImageData(...); >> context.fillRect(...); >> context.putImageData(...); >> context.fillRect(...); >> context.putImageData(...); >> >> If we added a new message for PutImageData, the above code would cause 4 flushes. If we make it a display list item, there's only one flush. >> >> This is important because the schedule we flush is an implementation detail. We want to be as simple or as sophisticated as we want when deciding when to flush. And, we want that schedule to be as resilient as possible to the original Javascript. > > Okay this is fine although I am not convinced what you wrote above is a real user scenario. Usually putImageData() is preceded by getImageData() or at least this is what we do in the MotionMark Images test. But if I wrote this code and I know I am going to call putImageData with the same data, I would convert the data first to an image and use drawImage() instead since drawImage() is more efficient than putImageData(). > > The solution is still a little bit hacky. Please find a better way plumbing the ImageBuffer into the DisplayList::Replayer. There are other instances of not implemented DisplayList::Items for which the drawing destination is not the GraphicsContext, for example the form controls. So it does not make sense to keep adding these destinations if we want to support these items. > > An approach is to make RemoteImageBuffer an observer to the DisplayList::Replayer. The replayer can ask its observer whether it needs to apply the item to itself or not. RemoteImageBuffer will override the observer virtual function applyItem() and in it will check the item. If it sees it is PutImageData, it gets the data and calls putImageData. Please see https://bugs.webkit.org/show_bug.cgi?id=208597 where I made RemoteImageBuffer an observer to the recorder. Sorry I meant RemoteImageBufferProxy to be an observer of the DisplayList::Replayer.

(In reply to Said Abou-Hallawa from comment #13) > Comment on attachment 392540 [details] > An approach is to make RemoteImageBuffer an observer to the > DisplayList::Replayer. The replayer can ask its observer whether it needs to > apply the item to itself or not. RemoteImageBuffer will override the > observer virtual function applyItem() and in it will check the item. If it > sees it is PutImageData, it gets the data and calls putImageData. Please see > https://bugs.webkit.org/show_bug.cgi?id=208597 where I made > RemoteImageBuffer an observer to the recorder. Okay, I can do this. However, I would like some guidance from you or Simon about which situations would call for delegates, and which are okay to just pass a pointer around. I'd like some clarity around why a delegate which can optionally override the execution of display list items is a better solution than injecting all the dependencies the replayer needs to execute all the DisplayListItems.

(In reply to Myles C. Maxfield from comment #16) > (In reply to Said Abou-Hallawa from comment #13) > > Comment on attachment 392540 [details] > > An approach is to make RemoteImageBuffer an observer to the > > DisplayList::Replayer. The replayer can ask its observer whether it needs to > > apply the item to itself or not. RemoteImageBuffer will override the > > observer virtual function applyItem() and in it will check the item. If it > > sees it is PutImageData, it gets the data and calls putImageData. Please see > > https://bugs.webkit.org/show_bug.cgi?id=208597 where I made > > RemoteImageBuffer an observer to the recorder. > > Okay, I can do this. However, I would like some guidance from you or Simon > about which situations would call for delegates, and which are okay to just > pass a pointer around. I'd like some clarity around why a delegate which can > optionally override the execution of display list items is a better solution > than injecting all the dependencies the replayer needs to execute all the > DisplayListItems. A direct follow-up question: If the delegate is going to supply the ImageBuffer, why not have the delegate supply the GraphicsContext too? What is the distinction between the two?

(In reply to Myles C. Maxfield from comment #17) > (In reply to Myles C. Maxfield from comment #16) > > (In reply to Said Abou-Hallawa from comment #13) > > > Comment on attachment 392540 [details] > > > An approach is to make RemoteImageBuffer an observer to the > > > DisplayList::Replayer. The replayer can ask its observer whether it needs to > > > apply the item to itself or not. RemoteImageBuffer will override the > > > observer virtual function applyItem() and in it will check the item. If it > > > sees it is PutImageData, it gets the data and calls putImageData. Please see > > > https://bugs.webkit.org/show_bug.cgi?id=208597 where I made > > > RemoteImageBuffer an observer to the recorder. > > > > Okay, I can do this. However, I would like some guidance from you or Simon > > about which situations would call for delegates, and which are okay to just > > pass a pointer around. I'd like some clarity around why a delegate which can > > optionally override the execution of display list items is a better solution > > than injecting all the dependencies the replayer needs to execute all the > > DisplayListItems. > > A direct follow-up question: If the delegate is going to supply the > ImageBuffer, why not have the delegate supply the GraphicsContext too? What > is the distinction between the two? The distinction is obvious all the DisplayList items (so far) implement the functions of GraphicsContextImpl. These functions have correspondent in GraphicsContext. And you are adding a new one which does not have similar call in GraphicsContext. Its distention is the pixels of an ImageBuffer. So instead of making a very special case for your case PutImageData, my suggestion is to make it more generic: -- If the function is GraphicsContext based function, it is going to be recorded and replayed back through GraphicsContext. And this is the common case. -- All special cases will go through the DipslayList "Observer" which can be anything and not necessarily and ImageBuffer.

Created attachment 392820 [details] Patch

Comment on attachment 392820 [details] Patch Clearing flags on attachment: 392820 Committed r258051: <https://trac.webkit.org/changeset/258051>

All reviewed patches have been landed. Closing bug.

(In reply to WebKit Commit Bot from comment #20) > Comment on attachment 392820 [details] > Patch > > Clearing flags on attachment: 392820 > > Committed r258051: <https://trac.webkit.org/changeset/258051> Attempt to fix build failures on WinCairo/WPE/GTK: Committed r258060: <https://trac.webkit.org/changeset/258060>

Thanks, David!