WIP: feat(color): Rewrite color in terms of css-color-4

[GPHemsley]

(I intend to fully explain this at a later point, but I'm filing this now to facilitate parallel development and the feedback loop. It's still a work in progress.)

@emilio @tiaanl

[emilio]

Some preliminary questions / comments.

cc @tiaanl.

Also, I believe Tiaan was planning on changing the color representation to be a bit more unified, something like:

struct ColorComponents([f32; 4]);
struct Color(ColorComponents, ColorSpace);

Which might have some advantages as you can write interpolation regardless of the color-space etc.

It'd be great if he could weigh in so we could coordinate, to avoid duplicated work.

But yeah, I'd try to avoid growing the Color type too much, and it being #[repr(C)] or so is kind of a must for us (plus it remaining small is good for performance generally so...).

[emilio]

For the record, I don't think using Option<f32> is going to work great for us in terms of representing none coordinates. First, it grows the color structs quite considerably.

Second, it's not possible to use it from C/C++, which is a problem for Firefox integration.

Can we do something like this instead?

#[repr(transparent)]
pub struct OptionalColorCoordinate(pub f32);

impl OptionalColorCoordinate {
    fn none() -> Self { Self(std::f32::NAN) }
    fn get() -> Option<f32> {
        if self.0.is_nan() { None } else { Some(self.0) }
    }
}

Or something along those lines? Same for Option<AlphaValue> etc.

[GPHemsley]

@emilio Can you go into more detail about the problems this design causes? I don't like the idea of abusing NaN to capture a concept independent of the float value.

@tiaanl Do you have any thoughts on this?

[emilio]

I outlined them above:

• Bigger structs are bad for CSS parsing performance (since they're returned by value quite a lot) and memory usage (if they're stored for long periods of time, which in the case of Firefox they are). Colors are rather common, so it'd be great not to regress parsing performance and memory usage any more than necessary. A struct of four Option<f32>s is twice as big as [f32; 4], see this, so if there's a not-too-complicated way to halve the size of a color, we should definitely do that.
• Additionally, we'd love to be able to avoid converting between different color representations between C/C++ and Rust. Option<> is not ffi-safe, in general, so we would need to convert colors after parsing to another representation in Firefox if we used it. We could do it if absolutely needed, but if we can avoid it (since it's just boilerplate and potentially duplicated code we'd have to maintain), it'd be great.

Using NaN was an idea to deal with both of thos. It allows to encode none without wasting extra bits, and it shouldn't make it particularly harder to use from Rust if we do something like what I suggested above, I think? But I'm open to other options.

In an ideal world, there'd be something like a NotNaN<T: Float> in the standard library that guarantees the right range so that Option<NotNaN<f32>> is guaranteed to be 32 bits, just like Option<Box<T>> is guaranteed to be the size of a pointer as long as T is Sized. That would address both and would be perhaps slightly more idiomatic, but given that doesn't exist, rolling our own as described above seems reasonable?

Another option would be to use something like fixed-point representation where 15 or 31 bits are used for the actual number, and one bit is used for none, but that's also a bit more code, and at that point we need to make rather arbitrary precision choices, which would be great to avoid if possible. It's one of the things we could consider doing (using f15 + a bit for none) if using f32 for all this becomes prohibitively expensive in terms of memory usage (since it could halve sizes compared to f32 representation). We use that kind of trick in some other places in Firefox like for font-style angles (see this), but again, I'd prefer to avoid it if possible.

Anyways, hopefully that clarifies my concerns with this approach?

[GPHemsley]

Ah, yes, thank you for that context. I think I understand the issue a bit better now.

I'll try my best to accommodate your request, but one of the things I intended to include in the background explanation that I have yet to write is that I'd like to consider optimizations to be out of scope as much as possible. These changes are unwieldy enough already, and I'd prefer to not add further delay while I fiddle with the idiosyncrasies of Rust; memory management is not my area of expertise. Do we have tests that would be tracking regressions of this nature? And if not, can we commit to adding them in the future?

I'd also like to make explicit one point that I think is already implicit in your comment: We want to be sure to differentiate between optimizations (and other things) that are broadly applicable and those that are Firefox-specific. The former are definitely appropriate for this repo, but the latter probably aren't.

[emilio]

Do we have tests that would be tracking regressions of this nature?

Sure, we do have CSS parsing benchmarks for Firefox like this, plus other performance tests that are monitored regularly.

That said, those obviously benchmark more than color parsing, and need a Firefox checkout plus incorporating your changes into Firefox which I don't think is something reasonable to ask of you.

But it should be rather trivial to add a few benchmarks for color parsing in src/tests.rs like we have for data: uris etc?

We want to be sure to differentiate between optimizations (and other things) that are broadly applicable and those that are Firefox-specific. The former are definitely appropriate for this repo, but the latter probably aren't.

Sure, I agree. Firefox has the extra requirements of dealing with C++ and thus needing ffi-safe types etc. I'd be ok merging a change that requires Firefox to support ffi on top of cssparser (even though I think it'd be unfortunate).

However general parsing performance should be applicable to all other rust crates that depend on cssparser. Firefox might have stricter performance requirements than some other programs, but I don't think I'd be fine merging a change with performance characteristics that don't work for Firefox.

[tiaanl]

Also, I believe Tiaan was planning on changing the color representation to be a bit more unified, something like:

TLDR: I think the calculation optimized representation ((color_space, f32, f32, f32, f32))should stay in gecko and we just convert to that from whatever the parser gives us.

Right now I take in whatever the parser gives me and transform it into the ~(color_space, f32, f32, f32, f32). The plan was to move that into the parser once i implement the color() function. I'm not 100% sure if we should do that though. Not doing so will give us more flexibility to change things for better calculations inside Gecko without changing how things are parsed. Downside is there are some computation and memory overhead for converting. Not huge structures, so should be OK, but a benchmark should be done. As a consumer of this library I would rather get as much detail from the parsed values as some flat optimized representation.

[tiaanl]

Maybe

type Degrees = f32;
struct Hue(Degrees);

Allows for better syntax like Hue(32.9), etc.

[GPHemsley]

I intentionally implemented it the way I did to serve two purposes:

• Avoid having to use .0 to get at the value.
• Make clear that the value has been normalized from what may have been originally specified (e.g. radians).

So needing to use Hue::new(32.9) was an intentional design decision.

I don't know if that changes your opinion?

[tiaanl]

Will this enum also have the other predefined color spaces? rec2020, linear-srgb, display-p3, etc.?

[GPHemsley]

I haven't looked too deeply into the other predefined color spaces to know for sure, but the idea was for each independent color space to have its own enum, with a variant for each way to map into it. (That's why there's CielabColor and OklabColor.)

So if some of the predefined color spaces map directly into sRGB, then (maybe) the SrgbColor enum would be expanded to include them. But completely independent color spaces would get their own enums, and the Color enum would get new variants to accommodate them.

But that, of course, is future work. (Note that the present PR does not include any support for the color() function.)

[GPHemsley]

Bah, tests for #312 rely on color parsing.

[bors-servo]

☔ The latest upstream changes (presumably #313) made this pull request unmergeable. Please resolve the merge conflicts.

[GPHemsley]

A number of spec changes have been made as a result of this work, but it does likely mean portions of the code will need to be changed (hopefully to simplify them).

I still have to go through and evaluate them.

[bors-servo]

☔ The latest upstream changes (presumably 7c58008) made this pull request unmergeable. Please resolve the merge conflicts.