[webkit-reviews] review granted: [Bug 230429] ImageBitmap structured clone doesn't preserve color space : [Attachment 438637] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Cameron McCormack (:heycam)
<heycam at apple.com>'s request for review:
Bug 230429: ImageBitmap structured clone doesn't preserve color space
https://bugs.webkit.org/show_bug.cgi?id=230429

Attachment 438637: Patch

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




--- Comment #7 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 438637
  --> https://bugs.webkit.org/attachment.cgi?id=438637
Patch

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:36
> +#if PLATFORM(COCOA)
> +#include <CoreFoundation/CoreFoundation.h>
> +#endif
> +#if USE(CG)
> +#include <CoreGraphics/CoreGraphics.h>
> +#endif

I would put these under the rest of the #includes (they sort down their due to
the "<").

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1111
> -	   PixelBufferFormat format { AlphaPremultiplication::Premultiplied,
PixelFormat::RGBA8, DestinationColorSpace::SRGB() };
> +	   PixelBufferFormat format { AlphaPremultiplication::Premultiplied,
PixelFormat::RGBA8, buffer->colorSpace() };

Probably worth putting a comment about adding support for pixel format
preservation (or adding that now if it is easy enough, not sure what the
canonical representations are, perhaps the CoreVideo 4 character codes?)

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1624
> +	   auto colorSpace = destinationColorSpace.platformColorSpace();
> +	   if (!colorSpace) {
> +	       write(DestinationColorSpaceCGColorSpaceNullTag);
> +	       return;
> +	   }

I tried to make it so DestinationColorSpace never had a null CGColorSpaceRef
(and people would use std::optional<DestinationColorSpace> for a nullable one).
Do you know of any cases where this is happening?

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1667
> +#endif
> +#else
> +	   switch (destinationColorSpace.platformColorSpace()) {
> +	   case PlatformColorSpace::Name::SRGB:
> +	       write(DestinationColorSpaceSRGBTag);
> +	       break;
> +#if ENABLE(DESTINATION_COLOR_SPACE_LINEAR_SRGB)
> +	   case PlatformColorSpace::Name::LinearSRGB:
> +	       write(DestinationColorSpaceLinearSRGBTag);
> +	       break;
> +#endif
> +#if ENABLE(DESTINATION_COLOR_SPACE_DISPLAY_P3)
> +	   case PlatformColorSpace::Name::DisplayP3:
> +	       write(DestinationColorSpaceDisplayP3Tag);
> +	       break;
> +#endif

One option to make the common cases faster would be to have these special tags
for both Cocoa and non-Cocoa (but run before the color space code above). That
way, we only encode the tag in the common case, and we have better code
coverage for these on all platforms.