<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:simon.fraser@apple.com" title="Simon Fraser (smfr) <simon.fraser@apple.com>"> <span class="fn">Simon Fraser (smfr)</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Add support for wide gamut for ShareableBitmap for image popovers"
href="https://bugs.webkit.org/show_bug.cgi?id=164001">bug 164001</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #292860 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Add support for wide gamut for ShareableBitmap for image popovers"
href="https://bugs.webkit.org/show_bug.cgi?id=164001#c4">Comment # 4</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Add support for wide gamut for ShareableBitmap for image popovers"
href="https://bugs.webkit.org/show_bug.cgi?id=164001">bug 164001</a>
from <span class="vcard"><a class="email" href="mailto:simon.fraser@apple.com" title="Simon Fraser (smfr) <simon.fraser@apple.com>"> <span class="fn">Simon Fraser (smfr)</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=292860&action=diff" name="attach_292860" title="Patch">attachment 292860</a> <a href="attachment.cgi?id=292860&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=292860&action=review">https://bugs.webkit.org/attachment.cgi?id=292860&action=review</a>
<span class="quote">>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.h:37
>> +WEBCORE_EXPORT CGColorSpaceRef extendedsRGBColorSpaceRef();
>
> I do not love the lowercase s. What do others think?</span >
Yeah, I read this as "extends RGB". extendedSRGBColorSpaceRef is more readable.
<span class="quote">>> 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.</span >
By which Tim means the existing uint64_t encoder/decoder.
<span class="quote">> Source/WebKit2/Platform/IPC/Decoder.h:84
> + bool decode(size_t&);</span >
Remove.
<span class="quote">> Source/WebKit2/Platform/IPC/Encoder.cpp:243
> +void Encoder::encode(size_t n)
> +{
> + uint8_t* buffer = grow(sizeof(n), sizeof(n));
> + copyValueToBuffer(n, buffer);
> +}</span >
Remove.
<span class="quote">> Source/WebKit2/Platform/IPC/Encoder.h:99
> + void encode(size_t);</span >
Remove.
<span class="quote">> Source/WebKit2/Shared/ShareableBitmap.cpp:42
> + if (flags & ShareableBitmap::SupportsExtendedColor) {
> + if (screenSupportsExtendedColor())
> + return 8;</span >
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.
<span class="quote">> Source/WebKit2/Shared/ShareableBitmap.cpp:78
> + m_pixelSize = bytesPerPixel(m_flags);</span >
I would call this m_bytesPerPixel. "pixelSize" is ambiguous.
<span class="quote">> Source/WebKit2/Shared/ShareableBitmap.h:54
> + SupportsExtendedColor = 1 << 1,</span >
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.
<span class="quote">> Source/WebKit2/Shared/ShareableBitmap.h:76
> + size_t m_pixelSize;</span >
m_bytesPerPixel
<span class="quote">> Source/WebKit2/Shared/ShareableBitmap.h:127
> + ShareableBitmap(const WebCore::IntSize&, Flags, size_t, void*);
> + ShareableBitmap(const WebCore::IntSize&, Flags, size_t, RefPtr<SharedMemory>);</span >
Please name that new parameter.
<span class="quote">> Source/WebKit2/Shared/ShareableBitmap.h:130
> + // FIXME: make sure this is ok, this seems to be windwos thing, maybe??</span >
windwos?
<span class="quote">> Source/WebKit2/Shared/ShareableBitmap.h:133
> + static size_t numBytesForSize(const WebCore::IntSize& size, int componentSize) { return size.width() * size.height() * componentSize; }</span >
componentSize -> bytesPerPixel (not component!)
<span class="quote">> Source/WebKit2/Shared/ShareableBitmap.h:151
> + size_t m_pixelSize;</span >
m_bytesPerPixel.
<span class="quote">> Source/WebKit2/Shared/cg/ShareableBitmapCG.cpp:42
> + if ((flags & ShareableBitmap::SupportsExtendedColor) && screenSupportsExtendedColor()) {</span >
I don't think the screenSupportsExtendedColor() check should be here.
<span class="quote">> Source/WebKit2/Shared/cg/ShareableBitmapCG.cpp:67
> + if ((m_flags & ShareableBitmap::SupportsExtendedColor) && (screenSupportsExtendedColor()))</span >
Ditto.
<span class="quote">> Source/WebKit2/Shared/cg/ShareableBitmapCG.cpp:68
> + colorSpace = extendedsRGBColorSpaceRef();</span >
On Mac we probably want to use the display colorspace. I think the caller is going to have to pass in a ColorSpace.
<span class="quote">> 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));</span >
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.
<span class="quote">> Source/WebKit2/Shared/cg/ShareableBitmapCG.cpp:110
> + // FIXME: Make this use extended color, etc.</span >
Do we need to fix this before landing the patch?
<span class="quote">> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2360
> + // FIXME: Only select ExtendedColor on images known to need wide gamut</span >
Ditto.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>