[Webkit-unassigned] [Bug 164001] Add support for wide gamut for ShareableBitmap for image popovers

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=164001

Simon Fraser (smfr) <simon.fraser at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #292860|review?                     |review-
              Flags|                            |

--- Comment #4 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 292860
  --> https://bugs.webkit.org/attachment.cgi?id=292860
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=292860&action=review

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.h:37
>> +WEBCORE_EXPORT CGColorSpaceRef extendedsRGBColorSpaceRef();
> 
> I do not love the lowercase s. What do others think?

Yeah, I read this as "extends RGB". extendedSRGBColorSpaceRef is more readable.

>> Source/WebKit2/Platform/IPC/Decoder.cpp:249
>> +bool Decoder::decode(size_t& result)
> 
> This is a bad idea, the two sides of IPC can be different bittiness - this has bitten us before. Just use uint64_t everywhere.

By which Tim means the existing uint64_t encoder/decoder.

> Source/WebKit2/Platform/IPC/Decoder.h:84
> +    bool decode(size_t&);

Remove.

> Source/WebKit2/Platform/IPC/Encoder.cpp:243
> +void Encoder::encode(size_t n)
> +{
> +    uint8_t* buffer = grow(sizeof(n), sizeof(n));
> +    copyValueToBuffer(n, buffer);
> +}

Remove.

> Source/WebKit2/Platform/IPC/Encoder.h:99
> +    void encode(size_t);

Remove.

> Source/WebKit2/Shared/ShareableBitmap.cpp:42
> +    if (flags & ShareableBitmap::SupportsExtendedColor) {
> +        if (screenSupportsExtendedColor())
> +            return 8;

I think you need to faithfully respect the flag on all hardware, not just if screenSupportsExtendedColor(). The caller should be the one to choose whether to use a deep buffer.

It would also be bad for bytesPerPixel() to give different answers in the two processes.

> Source/WebKit2/Shared/ShareableBitmap.cpp:78
> +    m_pixelSize = bytesPerPixel(m_flags);

I would call this m_bytesPerPixel. "pixelSize" is ambiguous.

> Source/WebKit2/Shared/ShareableBitmap.h:54
> +        SupportsExtendedColor = 1 << 1,

You're actually using this flag to both make a deep buffer (what we normally call "deep color", and to set the extendedSRGB colorspace. I wonder if we should either choose a better name or tease part those two things.

> Source/WebKit2/Shared/ShareableBitmap.h:76
> +        size_t m_pixelSize;

m_bytesPerPixel

> Source/WebKit2/Shared/ShareableBitmap.h:127
> +    ShareableBitmap(const WebCore::IntSize&, Flags, size_t, void*);
> +    ShareableBitmap(const WebCore::IntSize&, Flags, size_t, RefPtr<SharedMemory>);

Please name that new parameter.

> Source/WebKit2/Shared/ShareableBitmap.h:130
> +    // FIXME: make sure this is ok, this seems to be windwos thing, maybe??

windwos?

> Source/WebKit2/Shared/ShareableBitmap.h:133
> +    static size_t numBytesForSize(const WebCore::IntSize& size, int componentSize) { return size.width() * size.height() * componentSize; }

componentSize -> bytesPerPixel (not component!)

> Source/WebKit2/Shared/ShareableBitmap.h:151
> +    size_t m_pixelSize;

m_bytesPerPixel.

> Source/WebKit2/Shared/cg/ShareableBitmapCG.cpp:42
> +    if ((flags & ShareableBitmap::SupportsExtendedColor) && screenSupportsExtendedColor()) {

I don't think the screenSupportsExtendedColor() check should be here.

> Source/WebKit2/Shared/cg/ShareableBitmapCG.cpp:67
> +    if ((m_flags & ShareableBitmap::SupportsExtendedColor) && (screenSupportsExtendedColor()))

Ditto.

> Source/WebKit2/Shared/cg/ShareableBitmapCG.cpp:68
> +        colorSpace = extendedsRGBColorSpaceRef();

On Mac we probably want to use the display colorspace. I think the caller is going to have to pass in a ColorSpace.

> Source/WebKit2/Shared/cg/ShareableBitmapCG.cpp:72
> +    RetainPtr<CGContextRef> bitmapContext = adoptCF(CGBitmapContextCreateWithData(data(), m_size.width(), m_size.height(), m_pixelSize << 1 /* multiply by 2 *8/4 = << 1 */, m_size.width() * m_pixelSize, colorSpace, bitmapInfo(m_flags), releaseBitmapContextData, this));

Please don't use << 1. It's a false optimization and makes the code harder to read. Just write out the math and trust the compiler.

> Source/WebKit2/Shared/cg/ShareableBitmapCG.cpp:110
> +    // FIXME: Make this use extended color, etc.

Do we need to fix this before landing the patch?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2360
> +                                // FIXME: Only select ExtendedColor on images known to need wide gamut

Ditto.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20161026/4ccc88fa/attachment-0001.html>