Bug 218529

Created attachment 413092 [details] Patch

Created attachment 413100 [details] Patch

Created attachment 413108 [details] Patch

Comment on attachment 413108 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413108&action=review > Source/WebCore/platform/graphics/GraphicsContext.cpp:816 > + if (m_impl && image.renderingResourceIdentifier()) { > + m_impl->drawImageBuffer(image, destination, source, options); I think it's wrong to rely on the presence of a image.renderingResourceIdentifier to control behavior. What's the real question you're asking? > Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:191 > + m_displayList.registerImageBuffer(makeRef(imageBuffer)); This seems like a use for willAppendItemOfType(). > Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:192 > appendItemAndUpdateExtent(DrawImageBuffer::create(imageBuffer.renderingResourceIdentifier(), destRect, srcRect, options)); Should probably ASSERT(imageBuffer.renderingResourceIdentifier()) here.

Created attachment 413361 [details] Patch

Comment on attachment 413108 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413108&action=review >> Source/WebCore/platform/graphics/GraphicsContext.cpp:816 >> + m_impl->drawImageBuffer(image, destination, source, options); > > I think it's wrong to rely on the presence of a image.renderingResourceIdentifier to control behavior. What's the real question you're asking? I changed the patch such that this question means what is written. The question here is "can we inline the DrawImageBuffer item?". The DisplayList will keep a HashSet which will be used to control the life cycle of the source ImageBuffers. And it can be used also to resolve the RenderingResourceIdentifier to an ImageBuffer if the display list is replayed back in WebP. if it is replayed In GPUP, we are going to pass the HashSet which is maintained by RemoteResourceCache. >> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:191 >> + m_displayList.registerImageBuffer(makeRef(imageBuffer)); > > This seems like a use for willAppendItemOfType(). I might be confused by your comment here. But I think willAppendItemOfType() work on the destination ImageBuffer but we want to cache the source ImageBuffer. >> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:192 >> appendItemAndUpdateExtent(DrawImageBuffer::create(imageBuffer.renderingResourceIdentifier(), destRect, srcRect, options)); > > Should probably ASSERT(imageBuffer.renderingResourceIdentifier()) here. Done.

Comment on attachment 413361 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413361&action=review > Source/WebCore/platform/graphics/GraphicsContext.cpp:815 > + if (m_impl && image.renderingResourceIdentifier()) { image.renderingResourceIdentifier should not change behavior.

Created attachment 413391 [details] Patch

Comment on attachment 413361 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413361&action=review >> Source/WebCore/platform/graphics/GraphicsContext.cpp:815 >> + if (m_impl && image.renderingResourceIdentifier()) { > > image.renderingResourceIdentifier should not change behavior. Fixed. The new patch records an inline DrawImageBuffer item for both DL and GPUP cases.

Comment on attachment 413391 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413391&action=review > Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:191 > + m_displayList.cacheImageBuffer(makeRef(imageBuffer)); Rather than this, you should the work in willAppendItemOfType(), because there are going to be other drawing commands that use ImageBuffers (e.g. clipToImageBuffer() which you did not fix in this patch). > Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:54 > + if (item.type() == ItemType::DrawImageBuffer) { > + auto& drawItem = static_cast<DrawImageBuffer&>(item); > + if (auto* imageBuffer = m_imageBuffers.get(drawItem.renderingResourceIdentifier())) > + drawItem.apply(m_context, *imageBuffer); > + return; > + } Also needs to deal with clipToImageBuffer(). > Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:57 > + if (m_delegate && m_delegate->apply(item, m_context)) > + return; It's unclear how someone reading this code would know whether it's correct for this to be after the ImageBuffer special-casing.

Created attachment 413406 [details] Patch

Comment on attachment 413391 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413391&action=review >> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:191 >> + m_displayList.cacheImageBuffer(makeRef(imageBuffer)); > > Rather than this, you should the work in willAppendItemOfType(), because there are going to be other drawing commands that use ImageBuffers (e.g. clipToImageBuffer() which you did not fix in this patch). I will address fixing clipToImageBuffer in a separate patch. >> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:57 >> + return; > > It's unclear how someone reading this code would know whether it's correct for this to be after the ImageBuffer special-casing. I cleaned this function a little. I moved the call to delegate first, then I added DrawImageBuffer special-casing second.

Committed r269503: <https://trac.webkit.org/changeset/269503> All reviewed patches have been landed. Closing bug and clearing flags on attachment 413406 [details].

<rdar://problem/71109948>