Bug 226143 – Convert DestinationColorSpace from an enum to class wrapping a platform color space (CGColorSpaceRef for CG ports, etc.)

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Convert DestinationColorSpace from an enum to class wrapping a platform color space (CGColorSpaceRef for CG ports, etc.)

[Sam Weinig]

Created attachment 429418 [details]
Patch

[Sam Weinig]

Created attachment 429419 [details]
Patch

[Sam Weinig]

Created attachment 429420 [details]
Patch

[Sam Weinig]

Created attachment 429421 [details]
Patch

[Sam Weinig]

Created attachment 429426 [details]
Patch

[Sam Weinig]

Created attachment 429427 [details]
Patch

[Sam Weinig]

Created attachment 429428 [details]
Patch

[Sam Weinig]

Created attachment 429431 [details]
Patch

[Sam Weinig]

Created attachment 429444 [details]
Patch

[Sam Weinig]

Created attachment 429450 [details]
Patch

[Sam Weinig]

Created attachment 429466 [details]
Patch

[Sam Weinig]

Created attachment 429467 [details]
Patch

[Sam Weinig]

Created attachment 429470 [details]
Patch

[Sam Weinig]

Ultimately, DestinationColorSpace should be renamed to ColorSpace, and the existing ColorSpace enum should find a new name (perhaps we can push it out of platform and into the Style layer and call it StyleColorSpace or CSSExposedColorSpace.

[Darin Adler]

(In reply to Sam Weinig from comment #14)
> Ultimately, DestinationColorSpace should be renamed to ColorSpace, and the
> existing ColorSpace enum should find a new name (perhaps we can push it out
> of platform and into the Style layer and call it StyleColorSpace or
> CSSExposedColorSpace.

Usually the best practice is to rename the short one to a longer name sooner. Then we can claim the ColorSpace name after it’s free and clear.

[Darin Adler]

Comment on attachment 429470 [details]
Patch

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

Is it OK to make this code less optimal? Would love to stay super-efficient for cases where these are the most common color spaces.

> Source/WebCore/Headers.cmake:1297
> +    platform/graphics/PlatformColorSpace.h

Not a huge fan of the word "platform". Always confused about what exactly it means.

> Source/WebCore/platform/graphics/DestinationColorSpace.h:36
> +    WEBCORE_EXPORT static DestinationColorSpace SRGB();

Sure would be nice if functional ike this could be super-efficient, like constexpr. But instead it’s a function that calls another function to fetch the value of a global variable. Still not too bad, but for an idiom where we are constantly calling this, seems like it would be nice if this was something that could compile down to nothing.

> Source/WebCore/platform/graphics/DestinationColorSpace.h:44
> +    WEBCORE_EXPORT explicit DestinationColorSpace(PlatformColorSpace);

Doesn’t seem like this needs to be explicit. It’s a lossless conversion.

But maybe the agenda is to make this class contain more over time, and eventually the explicit will make sense?

> Source/WebCore/platform/graphics/DestinationColorSpace.h:45
> +    PlatformColorSpaceValue platformColorSpace() const { return m_platformColorSpace.get(); }

Seems unfortunate that this function name has to repeat the words "color space". But we don’t have a great naming scheme for this sort of thing: "underlying platform-specific representation of this platform-independent wrapper".

> Source/WebCore/platform/graphics/PlatformColorSpace.h:41
> +using PlatformColorSpace = RetainPtr<CGColorSpaceRef>;
> +using PlatformColorSpaceValue = CGColorSpaceRef;

Wish we had a better idiom to relate these pairs of types. The "main type" and the "peek type", to use our hash table value parlance.

> Source/WebCore/platform/graphics/PlatformColorSpace.h:47
> +    enum class Name {

Maybe ": uint8_t"?

> Source/WebCore/platform/graphics/PlatformColorSpace.h:85
> +    return PlatformColorSpace { *name };

Could also use:

    return { { *name } };

> Source/WebCore/platform/graphics/RemoteVideoSample.cpp:59
> +    auto ioSurface =  IOSurface::create(size, DestinationColorSpace::SRGB(), IOSurface::Format::BGRA);

Extra space here after "="?

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:890
> +    BeginClipToDrawingCommands(const FloatRect& destination, const DestinationColorSpace& colorSpace)
>          : m_destination(destination)
>          , m_colorSpace(colorSpace)
>      {
>      }

Do we need this? it’s just convenience because the caller can copy and then call the rvalue reference version of the constructor. In fact, can also write this:

    BeginClipToDrawingCommands(const FloatRect& destination, const DestinationColorSpace& colorSpace)
        : BeginClipToDrawingCommands { destination, DestinationColorSpace { colorSpace } }
    {
    }

To avoid repeating anything at all. Might put the rvalue reference version first and treat it as the "main" version.

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1985
> +    GetPixelBuffer(const PixelBufferFormat& outputFormat, const IntRect& srcRect)
>          : m_srcRect(srcRect)
>          , m_outputFormat(outputFormat)
>      {
>      }

Same here.

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1996
> +    ~GetPixelBuffer() { }

= default instead? Or does that not force a non-trivial destructor? Surprised that this qualifies as non-trivial.

[Sam Weinig]

(In reply to Darin Adler from comment #15)
> (In reply to Sam Weinig from comment #14)
> > Ultimately, DestinationColorSpace should be renamed to ColorSpace, and the
> > existing ColorSpace enum should find a new name (perhaps we can push it out
> > of platform and into the Style layer and call it StyleColorSpace or
> > CSSExposedColorSpace.
> 
> Usually the best practice is to rename the short one to a longer name
> sooner. Then we can claim the ColorSpace name after it’s free and clear.

Totally agree, just bogged down on not having a good name for it that I like.

[Sam Weinig]

(In reply to Darin Adler from comment #16)
> Comment on attachment 429470 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429470&action=review
> 
> Is it OK to make this code less optimal? Would love to stay super-efficient
> for cases where these are the most common color spaces.

I'm pretty confident that none of this is in hot code. The biggest downside here is that a few places get an additional CFRetain/CFRelease, and I have a good way to remove the majority of those by making the constant accessors, DestinationColorSpace::SRGB() and friends, return a const reference singleton instead.

> 
> > Source/WebCore/Headers.cmake:1297
> > +    platform/graphics/PlatformColorSpace.h
> 
> Not a huge fan of the word "platform". Always confused about what exactly it
> means.

I'm not super into it either, but it seems pretty idiomatic at this point for type that is meant to wrap the port's builtin version of that concept:

PlatformImage -> RetainPtr<CGImageRef>
PlatformColorSpace -> RetainPtr<CGColorSpaceRef>
PlatformLayer -> CALayer

Admittedly, this isn't as idiomatic as I thought when I started writing this response and there are some strong counter examples :). Any thoughts on a good naming convention for the "wraps an underlying libraries object"? In this case, I could probably avoid it entirely, but it made DestinationColorSpace much cleaner if I could remove much of the port specific gunk elsewhere.

> 
> > Source/WebCore/platform/graphics/DestinationColorSpace.h:36
> > +    WEBCORE_EXPORT static DestinationColorSpace SRGB();
> 
> Sure would be nice if functional ike this could be super-efficient, like
> constexpr. But instead it’s a function that calls another function to fetch
> the value of a global variable. Still not too bad, but for an idiom where we
> are constantly calling this, seems like it would be nice if this was
> something that could compile down to nothing.

Getting it to nothing might be tricky, but I have a few ideas about how to make this better.

1) Make it return a "const DestinationColorSpace&" that is backed by a constant global. e.g.:

const DestinationColorSpace& DestinationColorSpace::SRGB()
{
#if USE(CG)
    static NeverDestroyed<DestinationColorSpace> colorSpace;
    static std::once_flag onceFlag;
    std::call_once(onceFlag, [] {
        colorSpace.get() = DestinationColorSpace(sRGBColorSpaceRef());
    });
    return colorSpace.get();
#else
    static NeverDestroyed<DestinationColorSpace> colorSpace(PlatformColorSpace::Name::SRGB);
    return colorSpace.get();
#endif
}

2) I don't think I can safely use a tagged pointer (since we don't allocate the CGColorSpaceRef, we really shouldn't), but if I could, we could a tagged pointer for the 3 or 4 common color spaces we reference. Alternatively, we could look at getting CG to use a tagged pointer for us, they already do this for common CGColorRefs.

> 
> > Source/WebCore/platform/graphics/DestinationColorSpace.h:44
> > +    WEBCORE_EXPORT explicit DestinationColorSpace(PlatformColorSpace);
> 
> Doesn’t seem like this needs to be explicit. It’s a lossless conversion.
> 
> But maybe the agenda is to make this class contain more over time, and
> eventually the explicit will make sense?

I'd like to keep it explicit for a little bit to help me identify where conversions are happening. Once this get a bit cleaner, it might make sense to remove the explicit.

> 
> > Source/WebCore/platform/graphics/DestinationColorSpace.h:45
> > +    PlatformColorSpaceValue platformColorSpace() const { return m_platformColorSpace.get(); }
> 
> Seems unfortunate that this function name has to repeat the words "color
> space". But we don’t have a great naming scheme for this sort of thing:
> "underlying platform-specific representation of this platform-independent
> wrapper".

Yeah. maybe just platform() could work (like kit() and core() before it), but I am not sure how much I like "platform" in general now.

> 
> > Source/WebCore/platform/graphics/PlatformColorSpace.h:41
> > +using PlatformColorSpace = RetainPtr<CGColorSpaceRef>;
> > +using PlatformColorSpaceValue = CGColorSpaceRef;
> 
> Wish we had a better idiom to relate these pairs of types. The "main type"
> and the "peek type", to use our hash table value parlance.
> 
> > Source/WebCore/platform/graphics/PlatformColorSpace.h:47
> > +    enum class Name {
> 
> Maybe ": uint8_t"?

Fixed.

> 
> > Source/WebCore/platform/graphics/PlatformColorSpace.h:85
> > +    return PlatformColorSpace { *name };
> 
> Could also use:
> 
>     return { { *name } };

Fixed.

> 
> > Source/WebCore/platform/graphics/RemoteVideoSample.cpp:59
> > +    auto ioSurface =  IOSurface::create(size, DestinationColorSpace::SRGB(), IOSurface::Format::BGRA);
> 
> Extra space here after "="?

Fixed.

> 
> > Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:890
> > +    BeginClipToDrawingCommands(const FloatRect& destination, const DestinationColorSpace& colorSpace)
> >          : m_destination(destination)
> >          , m_colorSpace(colorSpace)
> >      {
> >      }
> 
> Do we need this? it’s just convenience because the caller can copy and then
> call the rvalue reference version of the constructor. In fact, can also
> write this:
> 
>     BeginClipToDrawingCommands(const FloatRect& destination, const
> DestinationColorSpace& colorSpace)
>         : BeginClipToDrawingCommands { destination, DestinationColorSpace {
> colorSpace } }
>     {
>     }

I think I can have just a single one, since the const reference one is really always a copy:

    BeginClipToDrawingCommands(const FloatRect& destination, DestinationColorSpace colorSpace)
        : m_destination(destination)
        , m_colorSpace(WTFMove(colorSpace))
    {
    }

Not sure why I added both.


> 
> > Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1996
> > +    ~GetPixelBuffer() { }
> 
> = default instead? Or does that not force a non-trivial destructor?
> Surprised that this qualifies as non-trivial.

Surprisingly "= default" keeps it trivial: https://en.cppreference.com/w/cpp/language/destructor#Trivial_destructor

Thanks for the review.

[Sam Weinig]

Created attachment 429480 [details]
Patch

[EWS]

ChangeLog entry in Source/WebCore/PAL/ChangeLog contains OOPS!.

[Sam Weinig]

Created attachment 429485 [details]
Patch

[EWS]

Committed r277940 (238067@main): <https://commits.webkit.org/238067@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/78376248>

[Sam Weinig]

(In reply to Sam Weinig from comment #18)
> (In reply to Darin Adler from comment #16)
> > Comment on attachment 429470 [details]
> > Patch
> > 
> > Sure would be nice if functional ike this could be super-efficient, like
> > constexpr. But instead it’s a function that calls another function to fetch
> > the value of a global variable. Still not too bad, but for an idiom where we
> > are constantly calling this, seems like it would be nice if this was
> > something that could compile down to nothing.
> 
> Getting it to nothing might be tricky, but I have a few ideas about how to
> make this better.
> 
> 1) Make it return a "const DestinationColorSpace&" that is backed by a
> constant global. e.g.:
> 
> const DestinationColorSpace& DestinationColorSpace::SRGB()
> {
> #if USE(CG)
>     static NeverDestroyed<DestinationColorSpace> colorSpace;
>     static std::once_flag onceFlag;
>     std::call_once(onceFlag, [] {
>         colorSpace.get() = DestinationColorSpace(sRGBColorSpaceRef());
>     });
>     return colorSpace.get();
> #else
>     static NeverDestroyed<DestinationColorSpace>
> colorSpace(PlatformColorSpace::Name::SRGB);
>     return colorSpace.get();
> #endif
> }
> 
> 2) I don't think I can safely use a tagged pointer (since we don't allocate
> the CGColorSpaceRef, we really shouldn't), but if I could, we could a tagged
> pointer for the 3 or 4 common color spaces we reference. Alternatively, we
> could look at getting CG to use a tagged pointer for us, they already do
> this for common CGColorRefs.
> 

I did #1 with https://bugs.webkit.org/show_bug.cgi?id=226160.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 226182

[Sam Weinig]

Created attachment 429600 [details]
Patch

[Sam Weinig]

(In reply to Sam Weinig from comment #26)
> Created attachment 429600 [details]
> Patch

Update patch with fix for nil color space in WebViewImpl.

[EWS]

Committed r277986 (238099@main): <https://commits.webkit.org/238099@main>

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