Bug 214234 - Improve type safety of color type clamping requirements
Summary: Improve type safety of color type clamping requirements
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-07-12 08:18 PDT by Sam Weinig
Modified: 2021-03-07 11:24 PST (History)
33 users (show)

See Also:


Attachments
Experiment (140.06 KB, patch)
2020-07-12 08:23 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Experiment (140.11 KB, patch)
2020-07-12 11:14 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2020-07-12 08:18:15 PDT
Improve type safety of color type clamping requirements
Comment 1 Sam Weinig 2020-07-12 08:20:57 PDT
Experimenting with how to do this. My first attempt replaces the primitive type template argument to the color types (and the ColorComponents type) with a struct indicating type and clamped status. So you can have:

SRGBA<UnclampedFloat>
SRGBA<ClampedFloat>
SRGBA<UnclampedByte>
SRGBA<ClampedByte>
Comment 2 Sam Weinig 2020-07-12 08:23:42 PDT
Created attachment 404097 [details]
Experiment
Comment 3 Sam Weinig 2020-07-12 08:29:16 PDT
Comment on attachment 404097 [details]
Experiment

Not fully fleshed out yet, but the basic idea is there. Conversion from SRGBA<UnclampedFloat> to SRGBA<ClampedFloat> (or the Byte variants) is done automatically here, with the idea being that users would essentially "tag" their input as either being "clamped" or "unclamped", and then functions would all actually only operate on the "clamped" types. So, if you tag your color as "unclamped" and then pass it to a function needing it "clamped", the clamping happens automatically.

For example:

void processColor(const SRGBA<ClampedFloat>&);

auto color = SRGBA<UnclampedFloat> { untrustedRed, untrustedGreen, untrustedBlue, untrustedAlpha };
processColor(color);

This works, and color is converted to SRGBA<ClampedFloat> implicitly.

There are still a few rough edges with this automatic conversion (especially around Optionals), but I think we can probably make it work if its the direction we end up going.
Comment 4 Darin Adler 2020-07-12 10:08:15 PDT
Comment on attachment 404097 [details]
Experiment

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

I love the discipline of this; makes it hard to get things wrong. Not sure that the word ClampedFloat communicates the "between 0 and 1" property. ClampedByte, on the other hand, does seem unambiguous. Not sure there would be such a thing as UnclampedByte.

Not loving the "normal language" feel of these type names. SRGBA<ClampedByte> is a far cry from Color.

> Source/WebKit/UIProcess/API/wpe/WebKitColor.cpp:78
> +    return WebCore::makeSimpleColor(WebCore::SRGBA<UnclampedFloat> { static_cast<float>(color->red), static_cast<float>(color->green), static_cast<float>(color->blue), static_cast<float>(color->alpha) });

EWS says this needs to be WebCore::UnclampedFloat.

> Source/WebKit/UIProcess/API/wpe/WebKitColor.cpp:85
> +    auto [r, g, b, a] = webCoreColor.toSRGBALossy<ClampedFloat>();

WebCore::ClampedFloat

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:341
> +                auto [red, green, blue, alpha] = color.toSRGBALossy<ClampedFloat>();

WebCore::ClampedFloat

> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCAAnimationRemote.mm:696
> +        auto [r, g, b, a] = keyframeValue.colorValue().toSRGBALossy<ClampedByte>();

WebCore::ClampedByte?
Comment 5 Sam Weinig 2020-07-12 10:54:31 PDT
(In reply to Darin Adler from comment #4)
> Comment on attachment 404097 [details]
> Experiment
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404097&action=review
> 
> I love the discipline of this; makes it hard to get things wrong. Not sure
> that the word ClampedFloat communicates the "between 0 and 1" property.

Definitely struggling with this. We continue to need a word for this concept. 

> ClampedByte, on the other hand, does seem unambiguous. Not sure there would
> be such a thing as UnclampedByte.

Indeed. An UnclampedByte in this implementation is actually an int. So, if you got a bunch of untrusted int values, say from an API we don't necessarily know is going to return values in the 0-255 range like [NSBitmapImageRep getPixel:atX:y:], the idea would be you would something like:

auto color = SRGBA<UnclampedByte> { untrustedRed, untrustedGreen, untrustedBlue, untrustedAlpha };
doSomthingWithClampedSRGBA(color);

and get automatic clamping. To reiterate the goal (though not saying it a good idea or worthwhile, because I am not sure, or well implemented) with the design: Users *tag* values that come in. Functions *declare* what type of data they want to operate on.

It could just be that we want only want the Clamped variants, and conversion from *untrusted* input to *clamped* input is explicit.

auto color = rgbaFromUntrustedValues(untrustedRed, untrustedGreen, untrustedBlue, untrustedAlpha);
doSomthingWithClampedSRGBA(color);


> 
> Not loving the "normal language" feel of these type names.
> SRGBA<ClampedByte> is a far cry from Color.

Which aspect of it is "normal language" in your mind?

I would agree that I don't love these names at all. Was hoping with iteration they would improve, but that has not panned out yet.

Any ideas? 


using SimpleColor = SRGBA<ClampedByte>; is still on the table (though I find the word Simple a bit awkward here).

Doing some bugzilla brainstorming (no bad ideas!). Maybe take some names from the GL world:

- SRGBA<ClampedByte> -> ColorType<ColorSpace::SRGB, Format::RGBA8> / SRGBA<ClampedFloat> -> ColorType<ColorSpace::SRGB, Format::RGBAF32>  

or

- SRGBA<ClampedByte> -> SRGBA8 / SRGBA<ClampedFloat> SRGBAF32

Those both seem worse. Will continue thinking.
Comment 6 Sam Weinig 2020-07-12 11:14:06 PDT
Created attachment 404110 [details]
Experiment
Comment 7 Darin Adler 2020-07-12 11:57:19 PDT
(In reply to Sam Weinig from comment #5)
> An UnclampedByte in this implementation is actually an int.

int32_t specifically?

> It could just be that we want only want the Clamped variants, and conversion
> from *untrusted* input to *clamped* input is explicit.

I think that’s right.

The only fly in the ointment is if you, the programmer, have an int, then convert to uint8_t before putting it into a ClampedByte, you have chopped it off without clamping and missed your chance to do the right thing.

> > Not loving the "normal language" feel of these type names.
> > SRGBA<ClampedByte> is a far cry from Color.
> 
> Which aspect of it is "normal language" in your mind?

I mean that "color" feels like "normal language" and "sRGBA made of clamped bytes" does not.

> Any ideas?

My favorite old technique was to talk to someone about the topic for a while and look for words they used when talking to another person and try to use those.

> using SimpleColor = SRGBA<ClampedByte>; is still on the table (though I find
> the word Simple a bit awkward here).

As I mentioned before, I took the the name "simple color" from the HTML specification <https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#simple-colour> but I added alpha so it’s not great.

Sorry, no ideas to contribute right this second.
Comment 8 Radar WebKit Bug Importer 2020-07-20 16:02:17 PDT
<rdar://problem/65852882>
Comment 9 Sam Weinig 2021-03-07 11:24:39 PST
Now that we have ExtendedSRGBA<> which is unclamped and SRGBA<> which is clamped (and variants of others as well, I think we are good here.