Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-color-4] Update lab and lch tests to use rectangles #27202

Merged

Conversation

@weinig
Copy link
Contributor

@weinig weinig commented Jan 15, 2021

When running the lab and lch test in WebKit, the use of text to try and match colors caused small differences in antialiasing causing the tests to fail. By changing the tests to use rectangles for comparisons, it is much easier to see that they are passing or failing and don't have these antialiasing issues.

Copy link
Contributor

@svgeesus svgeesus left a comment

I agree that solid rectangular patches of color are more accurate (the angle subtended is greater) and more testable (measuring the patch is possible) than text.

@svgeesus svgeesus self-requested a review Jan 16, 2021
Copy link
Contributor

@svgeesus svgeesus left a comment

I see some lint errors, which although trivial will block mergin. Please fix them.

css/css-color/lab-006.html:11: Whitespace at EOL (TRAILING WHITESPACE)
css/css-color/lab-005.html:11: Whitespace at EOL (TRAILING WHITESPACE)
css/css-color/lch-006.html:11: Whitespace at EOL (TRAILING WHITESPACE)
css/css-color/lch-004.html:11: Whitespace at EOL (TRAILING WHITESPACE)
css/css-color/lab-004.html:11: Whitespace at EOL (TRAILING WHITESPACE)
css/css-color/lab-007.html:11: Whitespace at EOL (TRAILING WHITESPACE)

@weinig weinig force-pushed the weinig:weinig/lab-lch-color-text-to-rect branch from b0d8096 to 26cd3f6 Jan 16, 2021
@weinig weinig requested a review from svgeesus Jan 16, 2021
Copy link
Contributor

@svgeesus svgeesus left a comment

Overall this looks good. Since you were seeing small roundoff errors I recalculated all of the conversions and, while they were mostly correct to three decimal places, some showed truncation rather than rounding and a few showed cumulative roundoff error that affected the second or, in one case, the first decimal place.
So, sorry to be a pain but please add in these more accurate conversions, just so we can be sure roundoff is not causing an issue.

<meta name="assert" content="lab() with no alpha">
<style>
.test {color: lab(46.277% -47.562 48.583)} /* green (sRGB #008000) converted to Lab */
.test { background-color: red; width: 12em; height: 12em; }
.test { background-color: lab(46.277% -47.562 48.583); } /* green (sRGB #008000) converted to Lab */

This comment has been minimized.

@svgeesus

svgeesus Jan 17, 2021
Contributor

Checking that conversion, let's use a more precise one. The existing 3dp value has rounding error (two values truncated rather than rounded)

lab(46.2775% -47.5621 48.5837)

@@ -2,10 +2,9 @@
<meta charset="utf-8">
<title>CSS Color 4: Specifying Lab and LCH</title>
<style>
.match { color: rgb(75.62% 30.45% 47.56%)} /* lab(50 50 0) converted to sRGB */
.test { background-color: rgb(75.62% 30.45% 47.56%); width: 12em; height: 12em; } /* lab(50% 50 0) converted to sRGB */

This comment has been minimized.

@svgeesus

svgeesus Jan 17, 2021
Contributor

Checking again, let's use

rgb(75.6208% 30.4487% 47.5634%)

.test { color: lab(50% 50 0)}
.match { color: rgb(75.62%, 30.45%, 47.56%)} /* lab(50 50 0) converted to sRGB */
.test { background-color: red; width: 12em; height: 6em; margin-top: 0; }
.ref { background-color: rgb(75.62%, 30.45%, 47.56%); width: 12em; height: 6em; margin-bottom: 0; }/* lab(50% 50 0) converted to sRGB */

This comment has been minimized.

@svgeesus

svgeesus Jan 17, 2021
Contributor

and the same here,

rgb(75.6208% 30.4487% 47.5634%)

@@ -2,10 +2,9 @@
<meta charset="utf-8">
<title>CSS Color 4: Specifying Lab and LCH</title>
<style>
.match { color: rgb(10.79%, 75.55%, 66.40%)} /* lab(70 -45 0) converted to sRGB */
.test { background-color: rgb(10.79%, 75.55%, 66.40%); width: 12em; height: 12em; } /* lab(70% -45 0) converted to sRGB */

This comment has been minimized.

@svgeesus

svgeesus Jan 17, 2021
Contributor

recalculating,

rgb(10.7906% 75.5567% 66.3982%)

.test { color: lab(70% -45 0)}
.match { color: rgb(10.79%, 75.55%, 66.40%)} /* lab(70 -45 0) converted to sRGB */
.test { background-color: red; width: 12em; height: 6em; margin-top: 0; }
.ref { background-color: rgb(10.79%, 75.55%, 66.40%); width: 12em; height: 6em; margin-bottom: 0; } /* lab(70% -45 0) converted to sRGB */

This comment has been minimized.

@svgeesus

svgeesus Jan 17, 2021
Contributor

and here

rgb(10.7906% 75.5567% 66.3982%)

.test { color: lch(70% 45 -180)}
.match { color: rgb(10.79%, 75.55%, 66.40%)} /* lch(70 45 180) converted to sRGB */
.test { background-color: red; width: 12em; height: 6em; margin-top: 0; }
.ref { background-color: rgb(10.79%, 75.55%, 66.40%); width: 12em; height: 6em; margin-bottom: 0; } /* lch(70% 45 180) converted to sRGB */

This comment has been minimized.

@svgeesus

svgeesus Jan 17, 2021
Contributor

rgb(10.7906% 75.5567% 66.3982%)

<meta charset="utf-8">
<title>CSS Color 4: Specifying Lab and LCH</title>
<style>
.test { background-color: rgb(76.62%, 66.36%, 5.58%); width: 12em; height: 12em; } /* lch(70% 70 90) converted to sRGB */

This comment has been minimized.

@svgeesus

svgeesus Jan 17, 2021
Contributor

rgb(76.6254% 66.3607% 5.5775%)

.test { color: lch(70% 70 90)}
.match { color: rgb(76.62%, 66.36%, 5.58%)} /* lch(70 70 90) converted to sRGB */
.test { background-color: red; width: 12em; height: 6em; margin-top: 0; }
.ref { background-color: rgb(76.62%, 66.36%, 5.58%); width: 12em; height: 6em; margin-bottom: 0; } /* lch(70% 70 90) converted to sRGB */

This comment has been minimized.

@svgeesus

svgeesus Jan 17, 2021
Contributor

rgb(76.6254% 66.3607% 5.5775%)

<meta charset="utf-8">
<title>CSS Color 4: Specifying Lab and LCH</title>
<style>
.test { background-color: rgb(12.8%, 53.1%, 92.5%); width: 12em; height: 12em; } /* lch(55% 60 270) converted to sRGB */

This comment has been minimized.

@svgeesus

svgeesus Jan 17, 2021
Contributor

rgb(12.8128% 53.105% 92.7645%)

.test { color: lch(56% 58 275)}
.match { color: rgb(12.81%, 53.10%, 92.76%)} /* lch(56 58 275) converted to sRGB */
.test { background-color: red; width: 12em; height: 6em; margin-top: 0; }
.ref { background-color: rgb(12.8%, 53.1%, 92.5%); width: 12em; height: 6em; margin-bottom: 0; } /* lch(55% 60 270) converted to sRGB */

This comment has been minimized.

@svgeesus

svgeesus Jan 17, 2021
Contributor

rgb(12.8128% 53.105% 92.7645%)

@weinig
Copy link
Contributor Author

@weinig weinig commented Jan 17, 2021

Will do. How do you feel about updating these to use color(srgb ...) rather than rgb(...)?

At least for WebKit, that puts us down a more consistent path of using floats for everything, and will also work for tests of display-p3 and so on.

@dbaron
Copy link
Member

@dbaron dbaron commented Jan 17, 2021

Wouldn't that make the tests no longer work in a bunch of implementations that do support lab() and lch()? It seems like it might make sense to make a duplicate set of tests with that comparison, but replacing the existing ones seems suboptimal.

@weinig
Copy link
Contributor Author

@weinig weinig commented Jan 17, 2021

Wouldn't that make the tests no longer work in a bunch of implementations that do support lab() and lch()? It seems like it might make sense to make a duplicate set of tests with that comparison, but replacing the existing ones seems suboptimal.

Are there implementations that support lab() and lch() but not color(srgb ...)?

@dbaron
Copy link
Member

@dbaron dbaron commented Jan 17, 2021

Er, sorry, maybe not... I think I was getting things mixed up. (Though I'm not saying there aren't, either...)

But it wouldn't surprise me if there ended up being some in the future.

@weinig
Copy link
Contributor Author

@weinig weinig commented Jan 17, 2021

Fair enough, let's stick with rgb() since it doesn't seem to be a problem currently (though I haven't tried to see if things still pass with the extended precision values yet, will report back once I do).

That said, I want to try to articulate the benefit (for the WebKit implementation at least) of using color(srgb ...) as the reference rather than rgb(...) again, since I don't think I was very clear and perhaps this will lead to more specificity. I don't want anything I write below to be seen as things I think should be specified, just trying to document the current state of things.

Right now, WebKit has two ways of storing colors, either as a set of 4 8-bit integers (rgba-8888 style) or as a set of 4 32-bit floats (rgba-FFFF style). The decision on which format to use is currently entirely based on how the color was specified, with rgba(), rgb(), hsl(), hsla() and the name and hex notations all using the rgba-8888 format, where as color(srgb ...) uses the rgba-FFFF format.

This is observable in two ways.

The first way it is observable is via serialization. Colors stored as rgba-8888 will be serialized using the rgba() notation (or rgb() if alpha is 255), colors store as rgba-FFFF will be serialized as color(srgb ...).

The second way it is observable (though in this case, only by a user, not via script) is via the precision loss and how that might effect conversion when drawing the color into a backing store.

I am not sure there is all that much important rationale behind this current state. When the rgb() and rgba() notations were introduced, we only had rgba-8888, so that is how they have been for a long time. When we added support for color(srgb ...) it was in concert with adding support for color(display-p3 ...) and deeper buffers, so using rgba-FFFF made sense.

I think this raises some interesting questions about what aspects of these decisions should be specified (if any).

@svgeesus
Copy link
Contributor

@svgeesus svgeesus commented Jan 18, 2021

I think this raises some interesting questions about what aspects of these decisions should be specified (if any).

On that note, following CSSWG discussion, CSS Color 4 has this to say about precision when serializing:

4.7.2. Serializing sRGB values

hex colors
rgb() and rgba() values
hsl() and hsla() values
hwb() values
named colors

The precision with which sRGB component values are retained, and thus the number of significant figures in the serialized value, is not defined in this specification, but must at least be sufficient to round-trip eight bit values. Values must be rounded towards +∞, not truncated.

but

4.7.4 Serializing values of the color() function

The precision with which color() component values are retained, and thus the number of significant figures in the serialized value, is not defined in this specification, but for sRGB and CMYK colorspaces must at least be sufficient to round-trip values with eight bit precision; this will result in at least two decimal places unless trailing zeroes have been omitted.

For the predefined colorspaces other than sRGB, which have wider gamuts and thus require more bits to avoid banding, the minimum precision for round-tripping is as follows:

(which then has a minimum of 10 bits for P3, etc)

So spec-wise, because of legacy implementations, it is conformant for either rgb() or color(srgb) to return serialization to 8 bit precision. But this will result in roundoff error for the % form or for non-integer values in the <number> form.

@svgeesus
Copy link
Contributor

@svgeesus svgeesus commented Jan 18, 2021

I can certainly see the value in having a duplicate set of tests where the reference is color(srgb). Maybe named like lab-nnn-b.html so that pairs of tests are easier to see?

…risons to avoid antialiasing artifacts from text rendering causing failures.
@weinig weinig force-pushed the weinig:weinig/lab-lch-color-text-to-rect branch from 26cd3f6 to b4e5caa Jan 21, 2021
@weinig
Copy link
Contributor Author

@weinig weinig commented Jan 21, 2021

Ok, these have all been updated with the recalculated values. Sorry for the delay. I kept things as rgb() for now.

@weinig weinig requested a review from svgeesus Jan 21, 2021
webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this pull request Jan 21, 2021
…port wide color

https://bugs.webkit.org/show_bug.cgi?id=220684

Reviewed by Simon Fraser.

LayoutTests/imported/w3c:

Update existing lab/lch tests to match new more precise values from web-platform-tests/wpt#27202
and add two new tests for colors outside the sRGB gamut that will be upstreamed to WPT after landing.

* web-platform-tests/css/css-color/lab-001.html:
* web-platform-tests/css/css-color/lab-002.html:
* web-platform-tests/css/css-color/lab-003.html:
* web-platform-tests/css/css-color/lab-004-expected.html:
* web-platform-tests/css/css-color/lab-004.html:
* web-platform-tests/css/css-color/lab-005-expected.html:
* web-platform-tests/css/css-color/lab-005.html:
* web-platform-tests/css/css-color/lab-006-expected.html:
* web-platform-tests/css/css-color/lab-006.html:
* web-platform-tests/css/css-color/lab-007-expected.html:
* web-platform-tests/css/css-color/lab-007.html:
* web-platform-tests/css/css-color/lab-008-expected.html: Added.
* web-platform-tests/css/css-color/lab-008.html: Added.
* web-platform-tests/css/css-color/lch-001.html:
* web-platform-tests/css/css-color/lch-002.html:
* web-platform-tests/css/css-color/lch-003.html:
* web-platform-tests/css/css-color/lch-004-expected.html:
* web-platform-tests/css/css-color/lch-004.html:
* web-platform-tests/css/css-color/lch-005-expected.html:
* web-platform-tests/css/css-color/lch-005.html:
* web-platform-tests/css/css-color/lch-006-expected.html:
* web-platform-tests/css/css-color/lch-006.html:
* web-platform-tests/css/css-color/lch-007-expected.html:
* web-platform-tests/css/css-color/lch-007.html:
* web-platform-tests/css/css-color/lch-008-expected.html: Added.
* web-platform-tests/css/css-color/lch-008.html: Added.

Source/WebCore:

To avoid clamping device independent colors to sRGB we convert colors without a direct
CoreGraphics representation to ExtendedSRGB to ensure the full gamut is preserved.

While CoreGraphics does support the Lab colorspace on some systems (Big Sur and later),
to ensure all versions of WebKit can support wide color Lab this approach provides the
most coverage.

* platform/graphics/ColorConversion.cpp:
* platform/graphics/ColorConversion.h:
* platform/graphics/ColorTypes.h:
Add support for ExtendedSRGBA and LinearExtendedSRGBA color types.

* platform/graphics/cg/ColorCG.cpp:
(WebCore::leakCGColor):
Update fallback conversion to convert to an extended color space to avoid premature
clamping.


Canonical link: https://commits.webkit.org/233219@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@271712 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Copy link
Contributor

@svgeesus svgeesus left a comment

Thanks, this all looks great now!

@svgeesus svgeesus merged commit 755a4a0 into web-platform-tests:master Jan 22, 2021
32 checks passed
32 checks passed
update-pr-preview
Details
Azure Pipelines Build #20210121.46 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
download-firefox-nightly Community-TC (pull_request)
Details
infrastructure/ tests Community-TC (pull_request)
Details
infrastructure/ tests (Python 3) Community-TC (pull_request)
Details
lint Community-TC (pull_request)
Details
resources/ tests (Python 2) Community-TC (pull_request)
Details
resources/ tests (Python 3.6) Community-TC (pull_request)
Details
resources/ tests (Python 3.8) Community-TC (pull_request)
Details
sink-task Community-TC (pull_request)
Details
tools/ integration tests (Python 2) Community-TC (pull_request)
Details
tools/ integration tests (Python 3.6) Community-TC (pull_request)
Details
tools/ integration tests (Python 3.8) Community-TC (pull_request)
Details
tools/ unittests (Python 2) Community-TC (pull_request)
Details
tools/ unittests (Python 3.6) Community-TC (pull_request)
Details
tools/ unittests (Python 3.8) Community-TC (pull_request)
Details
update-built Community-TC (pull_request)
Details
wpt-chrome-dev-results Community-TC (pull_request)
Details
wpt-chrome-dev-results-without-changes Community-TC (pull_request)
Details
wpt-chrome-dev-stability Community-TC (pull_request)
Details
wpt-decision-task Community-TC (pull_request)
Details
wpt-firefox-nightly-results Community-TC (pull_request)
Details
wpt-firefox-nightly-results-without-changes Community-TC (pull_request)
Details
wpt-firefox-nightly-stability Community-TC (pull_request)
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
yury-s pushed a commit to yury-s/webkit that referenced this pull request Jan 25, 2021
…port wide color

https://bugs.webkit.org/show_bug.cgi?id=220684

Reviewed by Simon Fraser.

LayoutTests/imported/w3c:

Update existing lab/lch tests to match new more precise values from web-platform-tests/wpt#27202
and add two new tests for colors outside the sRGB gamut that will be upstreamed to WPT after landing.

* web-platform-tests/css/css-color/lab-001.html:
* web-platform-tests/css/css-color/lab-002.html:
* web-platform-tests/css/css-color/lab-003.html:
* web-platform-tests/css/css-color/lab-004-expected.html:
* web-platform-tests/css/css-color/lab-004.html:
* web-platform-tests/css/css-color/lab-005-expected.html:
* web-platform-tests/css/css-color/lab-005.html:
* web-platform-tests/css/css-color/lab-006-expected.html:
* web-platform-tests/css/css-color/lab-006.html:
* web-platform-tests/css/css-color/lab-007-expected.html:
* web-platform-tests/css/css-color/lab-007.html:
* web-platform-tests/css/css-color/lab-008-expected.html: Added.
* web-platform-tests/css/css-color/lab-008.html: Added.
* web-platform-tests/css/css-color/lch-001.html:
* web-platform-tests/css/css-color/lch-002.html:
* web-platform-tests/css/css-color/lch-003.html:
* web-platform-tests/css/css-color/lch-004-expected.html:
* web-platform-tests/css/css-color/lch-004.html:
* web-platform-tests/css/css-color/lch-005-expected.html:
* web-platform-tests/css/css-color/lch-005.html:
* web-platform-tests/css/css-color/lch-006-expected.html:
* web-platform-tests/css/css-color/lch-006.html:
* web-platform-tests/css/css-color/lch-007-expected.html:
* web-platform-tests/css/css-color/lch-007.html:
* web-platform-tests/css/css-color/lch-008-expected.html: Added.
* web-platform-tests/css/css-color/lch-008.html: Added.

Source/WebCore:

To avoid clamping device independent colors to sRGB we convert colors without a direct
CoreGraphics representation to ExtendedSRGB to ensure the full gamut is preserved.

While CoreGraphics does support the Lab colorspace on some systems (Big Sur and later),
to ensure all versions of WebKit can support wide color Lab this approach provides the
most coverage.

* platform/graphics/ColorConversion.cpp:
* platform/graphics/ColorConversion.h:
* platform/graphics/ColorTypes.h:
Add support for ExtendedSRGBA and LinearExtendedSRGBA color types.

* platform/graphics/cg/ColorCG.cpp:
(WebCore::leakCGColor):
Update fallback conversion to convert to an extended color space to avoid premature
clamping.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@271712 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants