RESOLVED CONFIGURATION CHANGED214234
Improve type safety of color type clamping requirements
https://bugs.webkit.org/show_bug.cgi?id=214234
Summary Improve type safety of color type clamping requirements
Sam Weinig
Reported 2020-07-12 08:18:15 PDT
Improve type safety of color type clamping requirements
Attachments
Experiment (140.06 KB, patch)
2020-07-12 08:23 PDT, Sam Weinig
no flags
Experiment (140.11 KB, patch)
2020-07-12 11:14 PDT, Sam Weinig
no flags
Sam Weinig
Comment 1 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>
Sam Weinig
Comment 2 2020-07-12 08:23:42 PDT
Created attachment 404097 [details] Experiment
Sam Weinig
Comment 3 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.
Darin Adler
Comment 4 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?
Sam Weinig
Comment 5 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.
Sam Weinig
Comment 6 2020-07-12 11:14:06 PDT
Created attachment 404110 [details] Experiment
Darin Adler
Comment 7 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.
Radar WebKit Bug Importer
Comment 8 2020-07-20 16:02:17 PDT
Sam Weinig
Comment 9 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.
Note You need to log in before you can comment on or make changes to this bug.