Bug 218816 – Add minimal support for deep color IOSurface backing store to ImageBuffer

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Add minimal support for deep color IOSurface backing store to ImageBuffer

[Tim Horton]

Created attachment 413850 [details]
Patch

[Tim Horton]

Created attachment 413852 [details]
Patch

[Tim Horton]

Created attachment 413855 [details]
Patch

[Simon Fraser (smfr)]

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

[Said Abou-Hallawa]

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

[Tim Horton]

Created attachment 413869 [details]
Patch

[Tim Horton]

Created attachment 413872 [details]
Patch

[Simon Fraser (smfr)]

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?

[Tim Horton]

(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.

[Tim Horton]

Created attachment 413878 [details]
Patch

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 413878 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Tim Horton]

Wot, how

[Tim Horton]

Created attachment 413889 [details]
Patch

[EWS]

Committed r269710: <https://trac.webkit.org/changeset/269710>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413889 [details].

[Radar WebKit Bug Importer]

<rdar://problem/71307425>