Bug 164001 – Add support for wide gamut for ShareableBitmap for image popovers

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Add support for wide gamut for ShareableBitmap for image popovers

[Megan Gardner]

Created attachment 292860 [details]
Patch

[Tim Horton]

Comment on attachment 292860 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:101
> +//    // Out-of-date CG installations will not honor kCGColorSpaceSRGB. This logic avoids

I think it is likely that if not all Windowses know about kCGColorSpaceSRGB, they won't know about kCGColorSpaceExtendedSRGB either, so probably just un-comment this out.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.h:37
> +WEBCORE_EXPORT CGColorSpaceRef extendedsRGBColorSpaceRef();

I do not love the lowercase s. What do others think?

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

[Tim Horton]

Comment on attachment 292860 [details]
Patch

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

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

I'm not 100% sure we want this to depend on the screen. I believe images that bounce through ShareableBitmap can end up getting transmitted to other devices. Maybe we want callers to decide whether they set SupportsExtendedColor based on screenSupportsExtendedColor instead, since they will know whether there's a chance of it going elsewhere or not?

> 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));

I think it's really OK to multiply by 2 instead of shifting here.

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

I don't think this is optional? You're pointing at extended color data, won't that cause trouble if you make a non-extended-sRGB image from it? Or maybe it's OK because of the semicompatible nature of extended sRGB? What happens to out-of-range values?

[Simon Fraser (smfr)]

Comment on attachment 292860 [details]
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.

[Megan Gardner]

Created attachment 292867 [details]
Patch

[Tim Horton]

Comment on attachment 292867 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:101
> +    // Don't worry about extended colors on windows

This is not a "don't worry", this is a "if it doesn't have support, fall back to device". I also think it should probably fall back to sRGB, not device (if we have sRGB support but not extended, this one should use sRGB, not "device").

> Source/WebKit2/Shared/ShareableBitmap.cpp:41
> +        if (screenSupportsExtendedColor())

What Simon said before: this shouldn't depend on the screen. If wants extended color, it gets extended color.

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

Get rid of the comment and solve it one way or the other :) Take a peek at ShareableBitmapCairo:

return Checked<unsigned, RecordOverflow>(cairo_format_stride_for_width(cairoFormat, size.width())) * size.height();

So I think you're OK.

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

No need for the parens around the function call at the end.

[Megan Gardner]

Created attachment 292870 [details]
Patch

[Tim Horton]

Comment on attachment 292870 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:103
> +    // if there is no support on device, fall back to sRGB

Capital I, end with a period! (https://webkit.org/code-style-guidelines/#comments-sentences)

> Source/WebKit2/Shared/ShareableBitmap.cpp:54
> +    encoder << m_bytesPerPixel;

Why do we encode it if it's computable given m_flags, which we already encode?

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

Get rid of the comment.

[Megan Gardner]

Created attachment 292872 [details]
Patch

[Megan Gardner]

Created attachment 292873 [details]
Patch

[Beth Dakin]

Comment on attachment 292873 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:97
> +CGColorSpaceRef extendedSRGBColorSpaceRef()

I think you are missing a ChangeLog in WebCore.

[Simon Fraser (smfr)]

Comment on attachment 292873 [details]
Patch

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

> Source/WebKit2/Shared/ShareableBitmap.cpp:37
> +static uint calculateBytesPerPixel(ShareableBitmap::Flags flags)

uint -> unsigned.

> Source/WebKit2/Shared/ShareableBitmap.cpp:40
> +        return 8;

You should add a comment here saying which format we use for SupportsExtendedColor, so why 8 is the right number.

> Source/WebKit2/Shared/ShareableBitmap.cpp:78
> +    uint bytesPerPixel = calculateBytesPerPixel(flags);

unsigned. Or just remove this variable.

> Source/WebKit2/Shared/ShareableBitmap.cpp:101
> +    return adoptRef(new ShareableBitmap(size, flags, bytesPerPixel, sharedMemory));

I'm a bit confused about why bytesPerPixel needs to be passed in. Can't ShareableBitmap compute it from flags?

[Simon Fraser (smfr)]

How are we going to test this?

[Megan Gardner]

Created attachment 293069 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 293069 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Added extendedColorSpace creator.

This doesn't match the actual function name. Would be better to say "Add a function to return a CGColorSpaceRef for extended sRGB".

> Source/WebCore/ChangeLog:10
> +        This is currently untestable, due to simulator limitations, so no tests added.

What about Mac, since this is shared code?

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:103
> +    // If there is no support on device, fall back to sRGB.

"on device" is confusing in code that's shared with Mac.

> Source/WebKit2/ChangeLog:12
> +        This also adds using wide gamut images for image popovers in mobileSafari.

"This also adds using wide gamut images for image popovers in mobileSafari." -> "This makes it possible to show wide gamut images in image popovers on iOS".

> Source/WebKit2/ChangeLog:13
> +        This is currently untestable, due to simulator limitations, so no tests added.

But this code also compiles on Mac, so you need an excuse to not test on Mac too :)

> Source/WebKit2/Shared/ShareableBitmap.h:76
> +        uint64_t m_bytesPerPixel;

Still uint64_t.

[Megan Gardner]

Created attachment 293084 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 293084 [details]
Patch

Clearing flags on attachment: 293084

Committed r208020: <http://trac.webkit.org/changeset/208020>