Closed Bug 1340484 Opened 7 years ago Closed 7 years ago

stylo: border-collapse-bevels-1a.html fails due to different rgba() rounding behaviour

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: heycam, Assigned: manishearth)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Failed test cases
3.07 KB, text/plain
Details
Plot showing that floor is fairer
15.45 KB, image/png
Details
Bug 1340484 - Round RGB values when obtaining from HSL instead of flooring;
59 bytes, text/x-review-board-request
Details 
The comparison of border-collapse-bevels-1a.html (with stylo) to itself (without stylo) fails due to different rounding of the rgba(..., ..., ..., 0.3) values that Servo and Gecko does.  From some quick code inspection I can't see where this difference is coming in.  Xidorn, since you've been looking at color thing recently, can you check to see whether this difference is reasonable, or whether we should tweak the way we write nscolor values from Servo?  (I will add fuzzy-if() annotations for now.)

https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/Q6ahpJXPRpaoFZzG8FX12A/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1
Flags: needinfo?(xidorn+moz)
So the only difference between gecko and servo in this regard now is that servo converts from floating point to a byte representation using this[1]:  (val * 256.).floor().max(0.).min(255.) as u8

While gecko rounds then clamps I think.

[1]: https://github.com/servo/rust-cssparser/blob/master/src/color.rs#L384
Ah, well found.  Would be interesting to see what other browsers do.
Sorry for the delay, was on a flight when I looked at this so couldn't comment.

Blink (and WebKit I suppose) does rounding instead of flooring[1].

[1]: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp?l=461&rcl=7a4e3a4e6fa17894c3a4cdc0f03ed3380fe468f9
I have no idea. Probably as what emilio said, but I see no difference between min(floor(x * 256), 255) and round(x * 255) when x is any value from n / 255.0.

Also note that there is a precision loss for interpolation of currentcolor. It is probably unrelated here, though.
Flags: needinfo?(xidorn+moz)
I explain below my reasoning for the new behavior of color rounding in rust-cssparser. I think it makes more sense, but if it causes web-compatibility issues we should absolutely change it to match other engines. (Which I suppose means breaking the "equal parts" property described below.)

If there is no real compat issue but this makes our testing significantly harder, I’ll also concede (do whatever Gecko does).

If we can change or make fuzzy a small number of test with not much negative consequences, I’d prefer that.

-----

We want to convert each color channel between f32 values nominally in the 0.0 to 1.0 inclusive range, and u8 bytes from 0 to 255. Some properties nice to have are:

* 0 maps to 0.0
* 255 maps to 1.0
* Values in between map linearly

* 0.0 (and anything under) maps to 0
* 1.0 (and anything over) maps to 255
* Equal parts of the 0.0 to 1.0 range map to each integer

* A u8 -> f32 -> u8 round-trip gives the initial value

To exaggerate, let’s say replace f32 with real numbers and reduce bytes to only two bits, so there are four possible values: 0, 1, 2, 3. The first set of properties make them map to 0.0, 0.333…, 0.666…, and 1.0. So the mapping from integers divides by 3 (2^bits - 1), not 4 (2^bits).

For the reverse mapping, it’s tempting to multiply by 3 for symmetry. After that, we need to approximate that scaled real number with an integer. We can:

* Round toward zero or -infinity. This makes 1.0 the only real in the nominal range that maps to 3 (the maximum integer)
* Round toward +infinity. Same for 0.0 and 0
* Round to the nearest. N-0.5 to N+0.5 maps to the integer N, except for 0 and 1 where only half of that is in the nominal range.

To preserve the "equal parts" property, we want the mapping to be:

* 0.0 to 0.25 (also -inf to 0.0) => 0
* 0.25 to 0.5 => 1
* 0.5 to 0.75 => 2
* 0.75 to 1.0 (also 1.0 to +inf) => 3

One way to implement this conversion is to multiply by 4 (the number of available integer values), then round toward 0 or -inf, then clamp.

If we translate all this back to 8-bit bytes, this is why cssparser divides by 255 but multiplies by 256.

(We also see that 0.333… is in 0.25 to 0.5, and 0.666… is in 0.5 to 0.75, so the round-trip property is preserved. I’ve verified this is also the case with 8 bits.)
I would probably prefer Gecko's way since it could produce more predictable and intuitive result when combining two values.

Taking your two-bit number as an example, what would you expect from (0 * 30% + 1 * 70%)? People would expect it to be 0.7 and round to 1 again. But with your converting approach, the result would be (0 * 30% + 0.333... * 70%) => 0.2333... => 0, which would confuse people.

Actually, (1/256) is ~99.61% of (1/255), which means, (0 * 1% + 1 * 99%) would become 0 with your approach in 8-bit color. This also means small rounding error can lead to failure of round-trip in some cases.

It probably wouldn't cause serious webcompat issue, since color with difference by 1 is probably subtle enough for a normal human beings to recognize. The story of test is probably different. I know there are both our internal test and csswg test do exact color comparison. Making all those tests fuzzy probably needs a nontrivial effort.

Given these, I guess it is probably better using what Gecko does.
Priority: -- → P2
Attached file Failing test cases (obsolete) — 
These are the test cases affected by the rounding issue.
Attached file Failed test cases — 
Update list of failed test cases. Add all test cases under color4.
Attachment #8853226 - Attachment is obsolete: true
FWIW, blink does round when converting alphas, but it floors the RGB components.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp?rcl=7a4e3a4e6fa17894c3a4cdc0f03ed3380fe468f9&l=426

There's a todo comment about fixing this, but I think this behavior is wrong.

In the attached image, `floor(4*x + 0.5)` is basically `round(4*x)`, being used as a proxy for `round(255*x)` (on a scale of 0-4 instead of 0-255) which is too minute in its features to inspect. `floor(5*x)` is a proxy for `floor(256*x)`. The `floor(256*x)` one is evenly distributed wrt the domain, but the `round(255*x)` one has the values 0 and 255 getting a shorter domain, and all other values getting a slightly larger domain.

With floor(256*x) you will have 1.0 mapping to the invalid value 256, but you can clamp that value out. Servo does.

This means that `rgb(10%, 10%, 10%)` is `rgb(25,25,25)` in servo and blink, but `rgb(26, 26,26)` in gecko. The alpha values get interpreted differently, too -- Servo interprets them correctly IMO (interpreting them the same fair way rgb is interpreted), whereas Gecko and Blink both map them incorrectly. I don't know why Blink has both behaviors.
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Although floor seems to be fairer (which Simon has explained in comment 5), as I stated in comment 6, dividing by 255 but multiple by 256 could cause non-intuitive result for interpolation, and lead to failure of round-trip with small rounding error during calculation.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=acf307af3ccfe958207573807606cb4d5c4898a7

For some reason this does not make layout/reftests/writing-mode/1124636-1-fieldset-max-height.html  pass. I tried it out locally, and while the stored integer value of the alpha is now consistent according to getComputedStyle, the rendering has not changed.

Going to have to look closer into this.
> failure of round-trip with small rounding error during calculation.

I’ve tested that all 256 u8 values round-trip correctly with this the conversions currently in rust-cssparser.
(In reply to Simon Sapin (:SimonSapin) from comment #15)
> > failure of round-trip with small rounding error during calculation.
> 
> I’ve tested that all 256 u8 values round-trip correctly with this the
> conversions currently in rust-cssparser.

I didn't say they would not be round-trip by themselves. They will, but how about precision loss during calculation? Also I'm more concerned about the intuitiveness of interpolation. (0 * 1% + 1 * 99%) = 0 doesn't seem to be something we desire. Not sure whether the spec says anything about it, though.
Hm, right.

But Blink shares Servo's behavior for the RGB components (but not the A component).

There's a way of avoiding the reverse mapping from being an issue -- divide by 256 and then add a half step (or, add 0.5 and divide by 256.). It will still roundtrip, and you won't have the interpolation issue anymore.
Attachment #8861225 - Flags: review?(cam) → review?(dbaron)
Giving this to David, who likely has a better idea of any web compat issues relating to this.
After disabling scrollbars (which cause a lot of stylo failures and tend to mask the true reason behind failures), this seems to account for a ton of failures too.
Priority: P2 → P1
(In reply to Simon Sapin (:SimonSapin) from comment #5)
> We want to convert each color channel between f32 values nominally in the
> 0.0 to 1.0 inclusive range, and u8 bytes from 0 to 255. Some properties nice
> to have are:
> 
> * 0 maps to 0.0
> * 255 maps to 1.0
> * Values in between map linearly
> 
> * 0.0 (and anything under) maps to 0
> * 1.0 (and anything over) maps to 255
> * Equal parts of the 0.0 to 1.0 range map to each integer
> 
> * A u8 -> f32 -> u8 round-trip gives the initial value

Of these statements, there's one that I don't quite agree with, which is the "Equal parts of the 0.0 to 1.0 range map to each integer".  The alternative that's always made more sense to me is that the highest and lowest integers have half the space in the 0.0 to 1.0 range, since half of the range that "would" map to them is outside of 0.0-1.0.

(This is thinking about it only in terms of "nice to have" rather than in terms of compatibility or testing.)

> To preserve the "equal parts" property, we want the mapping to be:
> 
> * 0.0 to 0.25 (also -inf to 0.0) => 0
> * 0.25 to 0.5 => 1
> * 0.5 to 0.75 => 2
> * 0.75 to 1.0 (also 1.0 to +inf) => 3

But with the alternative, with the modified "Equal parts" statement that only uses half a part for 0 and 255, suggests instead that the mappings be 0.0-0.167 => 0, 0.167-0.5 => 1, 0.5-0.833 => 2, and 0.833-1.0 => 3.

I believe (from reading nsStyleUtil::FloatToColorComponent) that this is what Gecko does, since it multiplies by 255 and then *rounds*.  This also appears to match the Chromium code cited in comment 3.

This also has the advantage that the integer->float mapping is the center of the range for the float->integer mapping, which I think is another "nice" property that you omitted from your original list.
Comment on attachment 8861225 [details]
Bug 1340484 - Round RGB values when obtaining from HSL instead of flooring;

https://reviewboard.mozilla.org/r/133196/#review139004

This patch introduces an inconsistency that I'm not comfortable with.

Prior to this patch, this code probably should have been using nsStyleUtil::FloatToColorComponent().  However, it was doing the same *thing* as nsStyleUtil::FloatToColorComponent.  (See also nsStyleUtil::ColorComponentToFloat, which is an interesting function that exists for serialization, and which I'd be a little worried about breaking!)

However, with this patch, we'd be doing rounding on the alpha component differently in rgba(100%, 100%, 100%, 0.7) than we would in rgba(255, 255, 255, 0.7).  (And see also the hsla() handling just a few lines below, which would continue matching the latter!)


I also tend to prefer the existing Gecko behavior; I think having the integer->float mapping hit the midpoints of the float->integer mapping is valuable, and worth having the half-size ranges at the endpoints.  But if you want to change it, you'd need to actually change it fully, and not go halfway.  (And I'd still be worried about compatibility.)
Attachment #8861225 - Flags: review?(dbaron) → review-
> This also appears to match the Chromium code cited in comment 3.

As I mentioned before, this behavior only matches the chromium code for alphas, but not for the rgb components (which match servo).


https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp?rcl=7a4e3a4e6fa17894c3a4cdc0f03ed3380fe468f9&l=426
In this case, I'm going to fix this up in cssparser.
So I wasn't seeing as many try passes as I expected, and found more failures I hadn't noticed before when going through the reftests.

There are a bunch of reftests that just compare images. Which have this same problem. Which aren't fixed by the above patch.

Which confused me, until I realized that the errors were only in the transparent region. For image-only documents we have a background color of hsl(0, 0%, 90%). This is broken in a different way; Gecko does this wrong: https://dxr.mozilla.org/mozilla-central/rev/81977c96c6ff49e4b70f88a55f38d47f5e54a08b/gfx/src/nsColor.cpp#345

There we are multiplying by 255 and flooring, but Gecko multiplies by 255 and rounds everywhere. My patch made Servo also multiply by 255 and round everywhere, but this continued to not work because Gecko does something different.

I'm going to fix that.
Added patch to fix HSL stuff. I don't know if I need to clamp here, `uint8_t(255.5f)` seems to round to `255` fine.
Attachment #8861225 - Flags: review?(xidorn+moz)
Comment on attachment 8861225 [details]
Bug 1340484 - Round RGB values when obtaining from HSL instead of flooring;

Looks fine to me, but I'm not very familiar with the formula so still defer to dbaron.
Attachment #8861225 - Flags: review?(dbaron)
Comment on attachment 8861225 [details]
Bug 1340484 - Round RGB values when obtaining from HSL instead of flooring;

https://reviewboard.mozilla.org/r/133196/#review140422

r=dbaron, but it would be good to also add a test (mochitest or web-platform-test?) where you check the computed value
Attachment #8861225 - Flags: review?(dbaron) → review+
(And probably good to explicitly add a test for the cases that will do 255.5 -> 255 rounding.)
Attachment #8861225 - Flags: review?(xidorn+moz)
I think the test looks fine. MozReview still thinks dbaron has r+ed the patch, so I guess I don't need to r+ again.
> Of these statements, there's one that I don't quite agree with, which is the
> "Equal parts of the 0.0 to 1.0 range map to each integer".  The alternative
> that's always made more sense to me is that the highest and lowest integers
> have half the space in the 0.0 to 1.0 range, since half of the range that
> "would" map to them is outside of 0.0-1.0.
>
> (This is thinking about it only in terms of "nice to have" rather than in
> terms of compatibility or testing.)

Alright. It seems like my list of "nice to have" properties was subjective, so I’ll take https://github.com/servo/rust-cssparser/pull/142 to align with Gecko.


> I don't know if I need to clamp here, `uint8_t(255.5f)` seems to round to `255` fine.

Isn’t that undefined behavior? I don’t know if C++ differs from Rust here: https://github.com/rust-lang/rust/issues/10184
> Isn’t that undefined behavior?

My handy copy of K&R says:

  When a value of floating type is converted to integral type, the fractional part is discarded;
  if the resulting value cannot be represented in the integral type, the behavior is undefined.

and I'm pretty sure C and C++ since then have had the same behavior.  So uint8_t(255.5f) is defined behavior and gives 255.
There already is a ClampColor there which explicitly clamps and also rounds so I just used it instead.
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cc84c3af5a2a
Round RGB values when obtaining from HSL instead of flooring; r=dbaron
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bcede94db99c
Update reftest expectations from stylo merge; r=manishearth
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f1f1f8041fd5
More expectation updates; r=manishearth
So `uint8_t(255.5f)` is defined, but `uint8_t(256.0f)` is not?
https://hg.mozilla.org/mozilla-central/rev/cc84c3af5a2a
https://hg.mozilla.org/mozilla-central/rev/bcede94db99c
https://hg.mozilla.org/mozilla-central/rev/f1f1f8041fd5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
> So `uint8_t(255.5f)` is defined, but `uint8_t(256.0f)` is not?

If I understand correctly, yes.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: