Bug 217596

Created attachment 411086 [details] Patch

Created attachment 411096 [details] Patch

Created attachment 411098 [details] Patch

Created attachment 411125 [details] Patch for review

<rdar://problem/70424954>

Created attachment 412064 [details] Patch

Created attachment 412070 [details] Patch for review

Created attachment 412586 [details] Patch

Comment on attachment 412586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412586&action=review > Source/WebCore/platform/graphics/BitmapImage.cpp:214 > +static inline void drawNativeImage(BitmapImage& image, GraphicsContext& context, const NativeImagePtr& nativeImage, const FloatRect& destRect, const FloatRect& srcRect, const IntSize& srcSize, const ImagePaintingOptions& options) It's really confusing for this to take both a BitmapImage and a NativeImage. Can the latter be derived from the former? > Source/WebCore/platform/graphics/GraphicsContext.cpp:734 > +void GraphicsContext::drawNativeImage(const NativeImagePtr& image, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, const ImagePaintingOptions& options, CachedImage* cachedImage) This is a layering violation. You can't use CachedImage in platform code. > Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1736 > + WEBCORE_EXPORT DrawNativeImage(const NativeImageType&, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, const ImagePaintingOptions&); I don't think it makes sense to encode draw native image. If we're drawing an image with GPUP, can't it always be represented as an ImageBuffer? > Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:190 > +void Recorder::drawNativeImage(const NativeImagePtr& image, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, const ImagePaintingOptions& options, CachedImage* cachedImage) layering violation.

The life cycle of the PlatformImage will be controlled by a new called called NativeImage, see bug 218427.

Created attachment 412945 [details] Patch

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Created attachment 412947 [details] Patch for review

Comment on attachment 412947 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=412947&action=review > Source/WebCore/platform/graphics/GraphicsContext.cpp:761 > + if (m_impl && image.isPDFDocumentImage()) Why should we need to check PDF document images here? This seems to break encapsulation. > Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:179 > +void RemoteRenderingBackend::cachePlatformImage(const PlatformImagePtr& image, RenderingResourceIdentifier renderingResourceIdentifier) I don't see the type PlatformImagePtr defined anywhere. > Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:84 > +bool RemoteResourceCacheProxy::lockRemoteNativeImageForRemoteClient(NativeImage& image, RenderingResourceIdentifier remoteClientIdentifier) I don't see the NativeImage type defined anywhere.

Comment on attachment 412947 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=412947&action=review > Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:51 > +PlatformImagePtr RemoteResourceCache::cachedPlatformImage(RenderingResourceIdentifier renderingResourceIdentifier) Nit: It's not really a "cache" if there is no automatic eviction. This seems more like a "repository" or something like that.

Comment on attachment 412947 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=412947&action=review This looks like a good direction. > Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.h:59 > + void nativeImageWillBeDeleted(WebCore::RenderingResourceIdentifier) override; final?

Comment on attachment 412947 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=412947&action=review >> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:84 >> +bool RemoteResourceCacheProxy::lockRemoteNativeImageForRemoteClient(NativeImage& image, RenderingResourceIdentifier remoteClientIdentifier) > > I don't see the NativeImage type defined anywhere. Oh, it's in https://bugs.webkit.org/show_bug.cgi?id=218427

Created attachment 413740 [details] Patch

(In reply to Said Abou-Hallawa from comment #18) > Created attachment 413740 [details] > Patch Three notes about this patch: 1. PDFDocumentImage will be broken but the rest of images should be fine with this patch. Once it is landed I am going to file a bug for PDFDocumentImage. 2. The following DisplayList items should be removed in a follow-up patch since they are used with this patch: DrawImage, DrawTiledImage, DrawTiledScaledImage, 3. Drawing a canvas into another canvas is blocked by bug 218773.

Created attachment 413742 [details] Patch

(In reply to Said Abou-Hallawa from comment #19) > (In reply to Said Abou-Hallawa from comment #18) > > Created attachment 413740 [details] > > Patch > > Three notes about this patch: > > 1. PDFDocumentImage will be broken but the rest of images should be fine > with this patch. Once it is landed I am going to file a bug for > PDFDocumentImage. > > 2. The following DisplayList items should be removed in a follow-up patch > since they are used with this patch: > > DrawImage, > DrawTiledImage, > DrawTiledScaledImage, > > 3. Drawing a canvas into another canvas is blocked by bug 218773. A fourth comment 4. I think the class ImageDataReference can be replaced by just using ImageDataReference = RefPtr<ImageData>;

Comment on attachment 413742 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413742&action=review I think this patch looks pretty good! A couple of minor comments: > Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1280 > + DrawNativeImage(RenderingResourceIdentifier renderingResourceIdentifier, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, const ImagePaintingOptions& options) Nit - I think we might actually be able to remove this and just use the default constructor instead? > Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:52 > +bool RemoteResourceCacheProxy::cacheNativeImage(NativeImage& image) Nit - it wasn't obvious at first what the bool argument here represents; could we use an enum instead, like `enum class AddedNewEntry : bool { No, Yes };`?

Created attachment 413754 [details] Patch

Comment on attachment 413742 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413742&action=review >> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1280 >> + DrawNativeImage(RenderingResourceIdentifier renderingResourceIdentifier, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, const ImagePaintingOptions& options) > > Nit - I think we might actually be able to remove this and just use the default constructor instead? What do you mean by the default constructor in this context? The default constructor does not expect any parameters and all the members of this class are private. >> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:52 >> +bool RemoteResourceCacheProxy::cacheNativeImage(NativeImage& image) > > Nit - it wasn't obvious at first what the bool argument here represents; could we use an enum instead, like `enum class AddedNewEntry : bool { No, Yes };`? I changed the code such that we do not need to return a bool from this function. I made sending the CacheNativeImage message initiated by this function. This will correspond to what we do in RemoteResourceCacheProxy::nativeImageWillBeDeleted() where we initiate sending the ReleaseRemoteResource message.

Comment on attachment 413742 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413742&action=review >>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1280 >>> + DrawNativeImage(RenderingResourceIdentifier renderingResourceIdentifier, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, const ImagePaintingOptions& options) >> >> Nit - I think we might actually be able to remove this and just use the default constructor instead? > > What do you mean by the default constructor in this context? The default constructor does not expect any parameters and all the members of this class are private. My bad — that's correct, we need this constructor because the members are private. >>> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:52 >>> +bool RemoteResourceCacheProxy::cacheNativeImage(NativeImage& image) >> >> Nit - it wasn't obvious at first what the bool argument here represents; could we use an enum instead, like `enum class AddedNewEntry : bool { No, Yes };`? > > I changed the code such that we do not need to return a bool from this function. I made sending the CacheNativeImage message initiated by this function. This will correspond to what we do in RemoteResourceCacheProxy::nativeImageWillBeDeleted() where we initiate sending the ReleaseRemoteResource message. 👍🏻

(In reply to Said Abou-Hallawa from comment #21) > (In reply to Said Abou-Hallawa from comment #19) > > (In reply to Said Abou-Hallawa from comment #18) > > > Created attachment 413740 [details] > > > Patch > > > > Three notes about this patch: > > > > 1. PDFDocumentImage will be broken but the rest of images should be fine > > with this patch. Once it is landed I am going to file a bug for > > PDFDocumentImage. > > > > 2. The following DisplayList items should be removed in a follow-up patch > > since they are used with this patch: > > > > DrawImage, > > DrawTiledImage, > > DrawTiledScaledImage, > > > > 3. Drawing a canvas into another canvas is blocked by bug 218773. > > A fourth comment > > 4. I think the class ImageDataReference can be replaced by just > > using ImageDataReference = RefPtr<ImageData>; 5. Maybe it is time to add something like DisplayList::ResourceCache and have both RemoteResourceCacheProxy and RemoteResourceCache inherit from it. namespace DisplayList { class ResourceCache { public: virtual void cacheImageBuffer(Ref<ImageBuffer>&&); virtual void releaseImageBuffer(WebCore::RenderingResourceIdentifier); ImageBuffer* cachedImageBuffer(RenderingResourceIdentifier); virtual void cacheNativeImage(WebCore::NativeImage&); virtual void releaseNativeImage(WebCore::RenderingResourceIdentifier); const ImageBufferHashMap& imageBuffers() const { return m_imageBuffers; } const NativeImageHashMap& nativeImages() const { return m_nativeImages; } private: ImageBufferHashMap m_imageBuffers; NativeImageHashMap m_nativeImages; }; } And a pointer to ResourceCache can be passed to DisplayList::Replayer.

Created attachment 413761 [details] Patch

Comment on attachment 413761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413761&action=review > Source/WebCore/platform/graphics/NativeImage.cpp:48 > + if (m_observer) > + m_observer->releaseNativeImage(m_renderingResourceIdentifier); The observer is holding a ref to the NativeImage, so this code will never get hit. I think you're leaking all the images. > Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.messages.in:30 > + CacheNativeImage(WebCore::NativeImageReference nativeImage) I think it's deceptive to state that we can encode NativeImages like this. We can't losslessly encode PDF images, SVG Images etc. I think this should take ShareableBitmap and RenderingRsourceIdentifier arguments. > Source/WebKit/GPUProcess/graphics/RemoteResourceCache.h:55 > WebCore::ImageBufferHashMap m_imageBuffers; > + WebCore::NativeImageHashMap m_nativeImages; At some point m_imageBuffers will go away, and m_nativeImages will be SharableBitmaps, right?

Created attachment 413786 [details] Patch

The failure on iOS is happening because custom painting uses DisplayList for drawing. Because I removed recording the DrawImage, calling BitmapImage() is now allowed while the GraphicsContext is backed by a DisplayList::Recorder. And because allowSubsampling is allowed on iOS only, we end up calling the function BitmapImage::subsamplingLevelForScaleFactor() which expects a GraphicsContext with a valid platformContext(). I think the fix for this function is to check context.hasPlatformContext() before using context.platformContext().

Created attachment 413842 [details] Patch

Created attachment 413847 [details] Patch

Comment on attachment 413761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413761&action=review >> Source/WebCore/platform/graphics/NativeImage.cpp:48 >> + m_observer->releaseNativeImage(m_renderingResourceIdentifier); > > The observer is holding a ref to the NativeImage, so this code will never get hit. I think you're leaking all the images. Yes you right. I also mistakenly replaced the type of RemoteResourceCacheProxy::m_imageBuffers from using RemoteImageBufferProxyHashMap = HashMap<WebCore::RenderingResourceIdentifier, WebCore::ImageBuffer*> to NativeImageHashMap which is using ImageBufferHashMap = HashMap<RenderingResourceIdentifier, Ref<ImageBuffer>>; So the NativeImage and the ImageBuffers were leaking when GPU rendering for canvas is enabled. I made RemoteResourceCacheProxy holds raw pointer for ImageBuffer and NativeImage and I made sure the destructor of NativeImage is being called when the decoded frame is destroyed. >> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.messages.in:30 >> + CacheNativeImage(WebCore::NativeImageReference nativeImage) > > I think it's deceptive to state that we can encode NativeImages like this. We can't losslessly encode PDF images, SVG Images etc. I think this should take ShareableBitmap and RenderingRsourceIdentifier arguments. I do not fully understand this comment. I think: 1. We need to decode the PlatformImagePtr into a SharableBitmap inside the NativeImage. So encoding/decoding the NativeImage will be done by sending a Handle from WebP to GPUP. 2. We need to have CachePDFDocumentImage([no sure what should be here but maybe a SharedBuffer]) 3. For the reset of the images they are already fixed by removing the recording from the GraphicsContext functions. For example, SVGImage should work if DOM rendering is enabled. >> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.h:55 >> + WebCore::NativeImageHashMap m_nativeImages; > > At some point m_imageBuffers will go away, and m_nativeImages will be SharableBitmaps, right? m_imageBuffers have to stay since this is where the RemoteImageBuffers in the GPU side can live. The m_nativeImages have to stay as well but the underlying data will be a SharableBitmap instead of PlatformImagePtr. These two HashMaps are used to 1. Keep the resources alive in the GPU side 2. Resolve the renderingResourceIdentifier in DrawImageBuffer and DrawNativeImage to a resource when replaying back the DisplayList.

Comment on attachment 413847 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413847&action=review > Source/WebCore/platform/graphics/NativeImage.h:61 > + void setRenderingResourceIdentifier(RenderingResourceIdentifier renderingResourceIdentifier) { m_renderingResourceIdentifier = renderingResourceIdentifier; } It's a bit weird to have m_renderingResourceIdentifier be initialized to the generated value, then set here. Should RenderingResourceIdentifier be passed into the constructor as an Optional<>? > Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1300 > + RenderingResourceIdentifier m_renderingResourceIdentifier; We can all this m_imageIdentifier here > Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:201 > + if (!m_delegate) > + m_displayList.cacheNativeImage(image); > + else > + m_delegate->cacheNativeImage(image); This is a bit confusing. It assumes knowledge of delegate behavior. > Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:65 > if (!m_remoteRenderingBackendProxy) > return; > flushDrawingContext(); > + m_remoteRenderingBackendProxy->remoteResourceCacheProxy().releaseImageBuffer(m_renderingResourceIdentifier); > m_remoteRenderingBackendProxy->releaseRemoteResource(m_renderingResourceIdentifier); Please move to the .cpp file. > Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:89 > { > ASSERT(m_remoteRenderingBackendProxy); > + m_remoteRenderingBackendProxy->remoteResourceCacheProxy().cacheImageBuffer(*this); > + > m_drawingContext.displayList().setItemBufferClient(this); > m_drawingContext.displayList().setTracksDrawingItemExtents(false); > } Move to the cpp file. > Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:191 > + void cacheNativeImage(WebCore::NativeImage& image) override > + { > + if (m_remoteRenderingBackendProxy) > + m_remoteRenderingBackendProxy->remoteResourceCacheProxy().cacheNativeImage(image); > + } Put in the cpp file. > Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.h:54 > + using ImageBufferHashMap = HashMap<WebCore::RenderingResourceIdentifier, WebCore::ImageBuffer*>; > + using NativeImageHashMap = HashMap<WebCore::RenderingResourceIdentifier, WebCore::NativeImage*>; Can we use WeakPtr here?

Created attachment 413875 [details] Patch

Comment on attachment 413847 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413847&action=review >> Source/WebCore/platform/graphics/NativeImage.h:61 >> + void setRenderingResourceIdentifier(RenderingResourceIdentifier renderingResourceIdentifier) { m_renderingResourceIdentifier = renderingResourceIdentifier; } > > It's a bit weird to have m_renderingResourceIdentifier be initialized to the generated value, then set here. Should RenderingResourceIdentifier be passed into the constructor as an Optional<>? This function was removed. Instead a new create and a new constructor were added to take PlatformImagePtr and RenderingResourceIdentifier. >> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1300 >> + RenderingResourceIdentifier m_renderingResourceIdentifier; > > We can all this m_imageIdentifier here The name of this member was changed to m_imageIdentifier. >> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:201 >> + m_delegate->cacheNativeImage(image); > > This is a bit confusing. It assumes knowledge of delegate behavior. Yes this is right. I was trying to save a call to DisplayList::cacheNativeImage() but calling it unconditionally is cleaner and can make the DisplayList re-playable by itself. >> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:191 >> + } > > Put in the cpp file. There is no cpp file for this header file. >> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.h:54 >> + using NativeImageHashMap = HashMap<WebCore::RenderingResourceIdentifier, WebCore::NativeImage*>; > > Can we use WeakPtr here? Yes we can. Done.

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