[webkit-reviews] review granted: [Bug 225841] Add support for creating/accessing/setting non-sRGB ImageData via canvas : [Attachment 428751] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 225841: Add support for creating/accessing/setting non-sRGB ImageData via
canvas
https://bugs.webkit.org/show_bug.cgi?id=225841

Attachment 428751: Patch

https://bugs.webkit.org/attachment.cgi?id=428751&action=review




--- Comment #12 from Darin Adler <darin at apple.com> ---
Comment on attachment 428751
  --> https://bugs.webkit.org/attachment.cgi?id=428751
Patch

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3148
> +	   auto buffer =
ImageBitmap::createImageBuffer(*scriptExecutionContextFromExecState(m_lexicalGl
obalObject), logicalSize, RenderingMode::Unaccelerated, resolutionScale);

This function is still named scriptExecutionContextFromExecState! No longer
seems quite such a good name given we don’t have something named ExecState any
more.

> Source/WebCore/html/ImageData.cpp:50
> +    if (settings && settings->colorSpace)
> +	   return *settings->colorSpace;
> +    return defaultColorSpace;

We don’t have valueSquaredOr, more’s the pity.

> Source/WebCore/html/ImageData.cpp:57
> +    auto colorSpace =
toPredefinedColorSpace(pixelBuffer.format().colorSpace);
> +    RELEASE_ASSERT(colorSpace);
> +    return adoptRef(*new ImageData(pixelBuffer.size(),
pixelBuffer.takeData(), *colorSpace));

The Optional::operator*() function already includes a RELEASE_ASSERT, so we
don’t need to add one. (I wonder if that will be an impediment to our moving
from WTF::Optional to std::optional.)

> Source/WebCore/html/ImageData.cpp:96
> +	   // FIXME: Does this need to be a "real" out of memory error with
setOutOfMemoryError called on it?

How much longer are we going to move this comment around from file to file
without asking around and fixing it?

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2158
> +    // FIXME: Add support for initializing the ImageData when settings.alpha
is false, requiring
> +    // every 4th byte to be 0xFF.

This seems a peculiar FIXME. It’s about a problem that will exist once we add a
feature, and the comment is only needed if we don’t test the feature!

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2171
> +    if (newImageData.hasException())
> +	   return newImageData.releaseException();
> +
> +    initializeEmptyImageData(newImageData.returnValue());
> +
> +    return newImageData;

Can we write this instead?

    if (!newImageData.hasException())
	initializeEmptyImageData(newImageData.returnValue());
    return newImageData;

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2185
> +    auto imageData = ImageData::createUninitialized(std::abs(sw),
std::abs(sh), m_settings.colorSpace, settings);
> +    if (imageData.hasException())
> +	   return imageData.releaseException();
> +
> +    initializeEmptyImageData(imageData.returnValue());
> +
> +    return imageData;

Ditto.

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2218
> +	   auto imageData =
ImageData::createUninitialized(imageDataRect.width(), imageDataRect.height(),
m_settings.colorSpace, settings);
> +	   if (imageData.hasException())
> +	       return imageData.releaseException();
> +
> +	   initializeEmptyImageData(imageData.returnValue());
> +	   
> +	   return imageData;

Ditto.

> Source/WebCore/html/canvas/PredefinedColorSpace.h:29
> +#include <wtf/Optional.h>

I think Forward.h is sufficient. Maybe trouble if some automatically generated
code tries to use the return value of toPredefinedColorSpace, but otherwise
should not be a problem.

> Source/WebCore/platform/graphics/ImageBufferBackend.cpp:120
> +    // FIXME: Add support for non 8-bit pixel formats.
> +    ASSERT(destinationFormat.pixelFormat == PixelFormat::RGBA8 ||
destinationFormat.pixelFormat == PixelFormat::BGRA8);

What makes it safe to assert here without returning nullopt?

> Source/WebCore/platform/graphics/ImageBufferBackend.cpp:157
> +    // FIXME: Add support for non 8-bit pixel formats.

I’d write non-8-bit. But also given how this is written it’s supporting only
two specific formats. The comment implies these are the only two 8-bit ones,
which might be accurate now, but will it be forever?

> Source/WebCore/platform/graphics/PixelBuffer.cpp:72
> +    // NOTE: Only 8-bit formats are currently supported.
> +    ASSERT(format.pixelFormat == PixelFormat::RGBA8 || format.pixelFormat ==
PixelFormat::BGRA8);

I wish there was a way that all these assertions shared a single comment, and a
single "list of 8-bit formats".

Also, I am not clear exactly what level protects us and makes this safe to
assert rather than runtime check and runtime fail (as I asked above).

> Source/WebCore/testing/Internals.cpp:6206
> +	       auto imageData =
ImageData::create(static_cast<unsigned>(image->width()),
static_cast<unsigned>(image->height()), { { PredefinedColorSpace::SRGB } });

Better to use static_cast than function-call-style cast, but are these type
casts both needed and safe?