Add minimal support for deep color IOSurface backing store to ImageBuffer
Created attachment 413850 [details] Patch
Created attachment 413852 [details] Patch
Created attachment 413855 [details] Patch
Comment on attachment 413855 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413855&action=review > Source/WebCore/platform/graphics/ConcreteImageBuffer.h:37 > + static RefPtr<ImageBufferType> create(const FloatSize& size, float resolutionScale, ColorSpace colorSpace, const HostWindow* hostWindow, ColorFormat colorFormat, Arguments&&... arguments) Put ColorFormat colorFormat before hostWindow. > Source/WebCore/platform/graphics/ImageBuffer.cpp:41 > +RefPtr<ImageBuffer> ImageBuffer::create(const FloatSize& size, RenderingMode renderingMode, ShouldUseDisplayList shouldUseDisplayList, RenderingPurpose purpose, float resolutionScale, ColorSpace colorSpace, const HostWindow* hostWindow, ColorFormat colorFormat) Ditto. > Source/WebCore/platform/graphics/ImageBuffer.h:46 > + WEBCORE_EXPORT static RefPtr<ImageBuffer> create(const FloatSize&, RenderingMode, float resolutionScale = 1, ColorSpace = ColorSpace::SRGB, const HostWindow* = nullptr, ColorFormat = ColorFormat::Default); Ditto. > Source/WebCore/platform/graphics/ImageBufferBackend.cpp:34 > +ImageBufferBackend::ImageBufferBackend(const FloatSize& logicalSize, const IntSize& backendSize, float resolutionScale, ColorSpace colorSpace, ColorFormat colorFormat) Should we just glob ColorSpace and ColorFormat into a struct? > Source/WebCore/platform/graphics/ImageBufferBackend.h:63 > + Default = BGRA8, Would prefer Default outside the enum (as a constexpr). > Source/WebCore/platform/graphics/cg/ImageBufferCGBitmapBackend.h:40 > + static std::unique_ptr<ImageBufferCGBitmapBackend> create(const FloatSize&, float resolutionScale, ColorSpace, CGColorSpaceRef, const HostWindow*, ColorFormat); > + static std::unique_ptr<ImageBufferCGBitmapBackend> create(const FloatSize&, float resolutionScale, ColorSpace, const HostWindow*, ColorFormat); ColorFormat before hostWindow. > Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.h:43 > + static std::unique_ptr<ImageBufferIOSurfaceBackend> create(const FloatSize&, float resolutionScale, ColorSpace, CGColorSpaceRef, const HostWindow*, ColorFormat); > + static std::unique_ptr<ImageBufferIOSurfaceBackend> create(const FloatSize&, float resolutionScale, ColorSpace, const HostWindow*, ColorFormat); etc > Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:41 > + static auto create(const FloatSize& size, float resolutionScale, ColorSpace colorSpace, const HostWindow* hostWindow, ColorFormat colorFormat) > { > - return BaseConcreteImageBuffer::template create<ImageBuffer>(size, resolutionScale, colorSpace, hostWindow, size); > + return BaseConcreteImageBuffer::template create<ImageBuffer>(size, resolutionScale, colorSpace, hostWindow, colorFormat, size); colorFormat before hostWindow > Source/WebKit/Shared/ShareableBitmap.cpp:-152 > - // Create the shared memory. lolz
Comment on attachment 413855 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413855&action=review > Source/WebCore/platform/graphics/ImageBufferBackend.cpp:216 > + // We don't currently support getting or putting pixel data with deep color buffers. > + ASSERT(srcColorFormat == ColorFormat::RGBA8 || srcColorFormat == ColorFormat::BGRA8); > + ASSERT(destColorFormat == ColorFormat::RGBA8 || destColorFormat == ColorFormat::BGRA8); > + ImageBufferBackend::copyImagePixels() is a virtual function and this is the unaccelerated code path. Should we add similar assertions in the accelerated code path in ImageBufferCGBackend::copyImagePixels()? > Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:-222 > - if (srcColorFormat == ColorFormat::RGBA) Should we assert at the be
Created attachment 413869 [details] Patch
Created attachment 413872 [details] Patch
Comment on attachment 413872 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413872&action=review > Source/WebCore/platform/graphics/ImageBufferBackend.h:57 > enum class ColorFormat : uint8_t { Rename to PixelFormat (and rename all the arguments everywhere?) > Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:82 > + auto context = adoptCF(CGBitmapContextCreate(0, logicalSize.width(), logicalSize.height(), 8, 4 * logicalSize.width(), sRGBColorSpaceRef(), kCGImageAlphaPremultipliedFirst | kCGBitmapByteOrder32Host)); kCGBitmapByteOrder32Host is always BGRA?
(In reply to Simon Fraser (smfr) from comment #8) > Comment on attachment 413872 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=413872&action=review > > > Source/WebCore/platform/graphics/ImageBufferBackend.h:57 > > enum class ColorFormat : uint8_t { > > Rename to PixelFormat (and rename all the arguments everywhere?) > > > Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:82 > > + auto context = adoptCF(CGBitmapContextCreate(0, logicalSize.width(), logicalSize.height(), 8, 4 * logicalSize.width(), sRGBColorSpaceRef(), kCGImageAlphaPremultipliedFirst | kCGBitmapByteOrder32Host)); > > kCGBitmapByteOrder32Host is always BGRA? Technically? No. But it's what we already use for BGRA IOSurface and on iOS; if we introduce a new BE arch we'll have our work cut out for us :) Perhaps separately we should change everything to be more explicit.
Created attachment 413878 [details] Patch
Tools/Scripts/svn-apply failed to apply attachment 413878 [details] to trunk. Please resolve the conflicts and upload a new patch.
Wot, how
Created attachment 413889 [details] Patch
Committed r269710: <https://trac.webkit.org/changeset/269710> All reviewed patches have been landed. Closing bug and clearing flags on attachment 413889 [details].
<rdar://problem/71307425>