Bug 218934

Created attachment 414121 [details] Patch

Created attachment 414125 [details] Patch

Comment on attachment 414125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414125&action=review > Source/WebCore/platform/graphics/NativeImage.cpp:67 > + m_observer = &observer; > + m_platformImage = nullptr; It's weird (and very non-obvious from the API) that seeing an Observer clears the platform image. > Source/WebCore/platform/graphics/NativeImage.cpp:81 > + ASSERT(m_observer); > + m_platformImage = m_observer->platformImageOfNativeImage(m_renderingResourceIdentifier); But now we have both an observer AND a native image! > Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:49 > + m_nativeImages.add(renderingResourceIdentifier, NativeImage::create(*this, renderingResourceIdentifier)); Why don't we just create the PlatformImage here and pass it into the NativeImage constructor? > Source/WebKit/GPUProcess/graphics/RemoteResourceCache.h:51 > + WebCore::PlatformImagePtr platformImageOfNativeImage(WebCore::RenderingResourceIdentifier) override; platformImageForIdentifier()

Created attachment 414728 [details] Patch

Comment on attachment 414125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414125&action=review >> Source/WebCore/platform/graphics/NativeImage.cpp:67 >> + m_platformImage = nullptr; > > It's weird (and very non-obvious from the API) that seeing an Observer clears the platform image. I renamed NativeImage::Observer to be NativeImage::ImageSource. So setting the ImageSource of the NativeImage should clear the platform image since it will provide it when it is needed. >> Source/WebCore/platform/graphics/NativeImage.cpp:81 >> + m_platformImage = m_observer->platformImageOfNativeImage(m_renderingResourceIdentifier); > > But now we have both an observer AND a native image! Yes I did not want to break the non GPUP scenario if the same NativeImage is drawn later in WebP. This can be revisited later when all the NativeImages are drawn either in GPUP or WebP but not both. >> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:49 >> + m_nativeImages.add(renderingResourceIdentifier, NativeImage::create(*this, renderingResourceIdentifier)); > > Why don't we just create the PlatformImage here and pass it into the NativeImage constructor? Done. In fact this function RemoteResourceCache::cacheNativeImage() now takes a NativeImage. This cleans the interface and makes caching the NativeImage look very similar to caching the ImageBuffer. >> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.h:51 >> + WebCore::PlatformImagePtr platformImageOfNativeImage(WebCore::RenderingResourceIdentifier) override; > > platformImageForIdentifier() Name was changed.

<rdar://problem/71650376>

Created attachment 415383 [details] Patch

Created attachment 415384 [details] Patch

Comment on attachment 415384 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415384&action=review > Source/WebKit/Shared/ShareableBitmap.h:133 > + WebCore::PlatformImagePtr createPlatfromImage() { return makeCGImageCopy(); } Typo: platfrom. it's in a few places > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:-1103 > -void ArgumentCoder<Ref<NativeImage>>::encode(Encoder& encoder, const Ref<NativeImage>& image) I'm shocked this wasn't used for anything else, I thought for sure it predated this project.

Created attachment 415431 [details] Patch

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