Bug 230429 – ImageBitmap structured clone doesn't preserve color space

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Cameron McCormack (:heycam)]

We need a way to serialize the DestinationColorSpace when structured cloning.

[Radar WebKit Bug Importer]

<rdar://problem/83293533>

[Cameron McCormack (:heycam)]

Created attachment 438634 [details]
Patch for EWS

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Cameron McCormack (:heycam)]

Created attachment 438635 [details]
Patch for EWS

[Cameron McCormack (:heycam)]

Created attachment 438637 [details]
Patch

[Cameron McCormack (:heycam)]

I'm including the fix for the flaky WPT in https://github.com/web-platform-tests/wpt/pull/30844, which is the export from bug 229022, since without the fix that PR is blocked from merging.

[Sam Weinig]

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

[Cameron McCormack (:heycam)]

(In reply to Sam Weinig from comment #7)
> 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?

I don't, but I saw some code somewhere that was checking for it explicitly.  I'll consider it more of an error case then, and add some assertions.

[Cameron McCormack (:heycam)]

Created attachment 439426 [details]
Patch

[EWS]

Committed r283193 (242240@main): <https://commits.webkit.org/242240@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439426 [details].

[Sam Weinig]

(In reply to Cameron McCormack (:heycam) from comment #8)
> (In reply to Sam Weinig from comment #7)
> > 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?
> 
> I don't, but I saw some code somewhere that was checking for it explicitly. 

Where was that?

[Cameron McCormack (:heycam)]

(In reply to Sam Weinig from comment #11)
> (In reply to Cameron McCormack (:heycam) from comment #8)
> > (In reply to Sam Weinig from comment #7)
> > > 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?
> > 
> > I don't, but I saw some code somewhere that was checking for it explicitly. 
> 
> Where was that?

I think it was my misreading of DestinationColorSpace::decode's `if (!platformColorSpace)` as being a check for the PlatformColorSpace being null, rather than whether the decoder succeeded.

In the end I didn't add any new assertions, as I saw you already had one in the DestinationColorSpace constructor.

[Sam Weinig]

Great. Thanks.