pixels: Actually write pixels in `MULTIPLY` `generic_transform_inplace` operations by tharkum · Pull Request #36895 · servo/servo

tharkum · 2025-05-07T12:45:18Z

Multiply operations applied in generic_transform_inplace were
calculating the new pixel values, but not actually writing them.
This changes fixes that issue.

Testing: /webgl/tests/conformance/context/premultiplyalpha-test.html
Fixes: #35759

tharkum · 2025-05-07T12:45:47Z

@sagudev < Please take a look.

cc @xiaochengh

mrobinson

Does this fix a particular issue? It's not discussed in the PR description (which is also using the old message template) nor is there any test included. Please either include a test or describe why one isn't required.

sagudev · 2025-05-07T13:05:10Z

components/pixels/lib.rs

@@ -64,10 +64,16 @@ pub fn rgba8_byte_swap_colors_inplace(pixels: &mut [u8]) {
 pub fn rgba8_byte_swap_and_premultiply_inplace(pixels: &mut [u8]) {


We should replace this function with generic_transform_inplace::<1, true, false>(pixels) anyway.

Replace it inside this PR?

Hm, lets make it in this PR, this way we ensure that other changes are better tested.

tharkum · 2025-05-07T13:11:28Z

Does this fix a particular issue? It's not discussed in the PR description (which is also using the old message template) nor is there any test included. Please either include a test or describe why one isn't required.

There are no actual working WPT/manual test to verify with CI.

Currently I am working on createImageBitmap() API and on the following WPT test this change are fixing incorrect behaviour (html/canvas/element/manual/imagebitmap/createImageBitmap-premultiplyAlpha.html).

I split this change from main draft PR to merge it before (may be someone will meet with it also).

So please recommend that is the proper way:

To merge createImageBitmap PR and later this PR (with working related WPT test)
Do manual test (need to think how to do it without ImageBitmap+ImageData) for this PR

components/pixels/lib.rs

sagudev · 2025-05-07T13:21:18Z

Add extra checks to prevent premultiply inplace if alpha component is fully opaque (255).

Why? I think this does not change results so it's only premature optimization (or do we have benches that shows otherwise).

sagudev · 2025-05-07T13:24:08Z

There are no actual working WPT/manual test to verify with CI.

Really? I think some webgl/2d or maybe even webgpu should catch this. It is also possible that they are masked by other failures.

tharkum · 2025-05-07T13:24:45Z

Add extra checks to prevent premultiply inplace if alpha component is fully opaque (255).

Why? I think this does not change results so it's only premature optimization (or do we have benches that shows otherwise).

Yes, it is optimization (results before and after will be the same, so no point to do multiply at all).

sagudev · 2025-05-07T13:37:40Z

Add extra checks to prevent premultiply inplace if alpha component is fully opaque (255).

Why? I think this does not change results so it's only premature optimization (or do we have benches that shows otherwise).

Yes, it is optimization (results before and after will be the same, so no point to do multiply at all).

I think this should be part of separate PR, let's keep this about fixing current impl.

We also have some existing benches for pixels crate, but its hard to make representative benches. Also note that branchless is sometimes more performant even if doing more work.

tharkum · 2025-05-07T16:49:06Z

@sagudev @mrobinson < I rewrited PR message and remove all changes related to code cleanup and alpha multiply optimization (they will be done in another PRs)

github-actions · 2025-05-07T16:50:47Z

🔨 Triggering try run (#14888835620) for Linux (WPT)

github-actions · 2025-05-07T17:21:28Z

Test results for linux-wpt from try job (#14888835620):

Flaky unexpected result (13)

TIMEOUT /FileAPI/url/url-in-tags-revoke.window.html (#19978)
- TIMEOUT [expected PASS] subtest: Fetching a blob URL immediately before revoking it works in <script> tags.
```
Test timed out
```
OK /FileAPI/url/url-with-fetch.any.html (#21517)
- FAIL [expected PASS] subtest: Revoke blob URL after calling fetch, fetch should succeed
```
promise_test: Unhandled rejection with value: object "TypeError: Network error occurred"
```
OK /FileAPI/url/url-with-fetch.any.worker.html (#21517)
- FAIL [expected PASS] subtest: Revoke blob URL after calling fetch, fetch should succeed
```
promise_test: Unhandled rejection with value: object "TypeError: Network error occurred"
```
OK /fetch/metadata/generated/css-font-face.https.sub.tentative.html (#32732)
- PASS [expected FAIL] subtest: sec-fetch-dest
- FAIL [expected PASS] subtest: sec-fetch-user
```
promise_test: Unhandled rejection with value: object "Error: Failed to query for recorded headers."
```
- PASS [expected FAIL] subtest: sec-fetch-storage-access - Cross-site
ERROR /fetch/metadata/generated/serviceworker.https.sub.html (#36247)
- PASS [expected FAIL] subtest: sec-fetch-site - Same origin, no options - registration
OK /html/browsers/browsing-the-web/navigating-across-documents/009.html (#24456)
- FAIL [expected PASS] subtest: Link with onclick form submit to javascript url with document.write and href navigation
```
assert_array_equals: expected property 1 to be "href" but got "click" (expected array ["write", "href"] got ["write", "click"])
```
OK /html/browsers/browsing-the-web/navigating-across-documents/navigation-unload-cross-origin.sub.window.html (#29056)
- PASS [expected FAIL] subtest: Cross-origin navigation started from unload handler must be ignored
OK /html/browsers/windows/browsing-context-names/duplicate-name-order.html (#34623)
- PASS [expected FAIL] subtest: Duplicate name lookup order
CRASH [expected OK] /html/semantics/embedded-content/media-elements/media_fragment_seek.html (#24114)
OK [expected TIMEOUT] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-1.html (#22647)
- FAIL [expected TIMEOUT] subtest: Check that popups from a sandboxed iframe escape the sandbox if allow-popups-to-escape-sandbox is used
```
assert_equals: It came from a sandboxed iframe expected "null" but got "http://web-platform.test:8000"
```
OK [expected TIMEOUT] /performance-timeline/navigation-id-detached-frame.tentative.html (#34773)
- PASS [expected TIMEOUT] subtest: The navigation_id getter does not crash a window of detached frame
OK /resize-observer/change-layout-in-error.html (#32629)
- PASS [expected FAIL] subtest: Changing layout in window error handler should not result in lifecyle loop when resize observer loop limit is reached.
OK /xhr/send-redirect.htm (#32026)
- FAIL [expected PASS] subtest: XMLHttpRequest: send() - Redirects (basics) (307, GET, content.py)
```
assert_equals: expected (string) "GET" but got (object) null
```

Stable unexpected results that are known to be intermittent (18)

TIMEOUT [expected OK] /_mozilla/mozilla/window_resizeTo.html (#36741)
- TIMEOUT [expected PASS] subtest: Popup onresize event fires after resizeTo
```
Test timed out
```
OK /_webgl/conformance/textures/misc/texture-upload-size.html (#21770)
- FAIL [expected PASS] subtest: WebGL test #44
```
assert_true: could not create image (SVG) expected true got false
```
FAIL [expected PASS] /css/css-sizing/dynamic-available-size-iframe.html (#31559)
FAIL [expected PASS] /css/css-tables/table-cell-overflow-auto-scrolled.html (#35011)
OK /fetch/metadata/generated/css-font-face.sub.tentative.html (#34624)
- PASS [expected FAIL] subtest: sec-fetch-storage-access - Not sent to non-trustworthy same-origin destination
OK /html/browsers/browsing-the-web/navigating-across-documents/empty-iframe-load-event.html (#29066)
- FAIL [expected PASS] subtest: Check execution order from nested timeout
```
assert_equals: Expected nested setTimeout to run second expected true but got false
```
- FAIL [expected PASS] subtest: Check execution order on load handler
```
assert_equals: Expected onload to run first expected false but got true
```
OK /html/browsers/browsing-the-web/navigating-across-documents/initial-empty-document/iframe-src-aboutblank-navigate-immediately.html (#29048)
- PASS [expected FAIL] subtest: Navigating to a different document with form submission
OK /html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-top.html (#21351)
- PASS [expected FAIL] subtest: Fragment Navigation: When fragid is TOP scroll to the top of the document
ERROR [expected TIMEOUT] /html/canvas/element/manual/imagebitmap/createImageBitmap-invalid-args.html (#32745)
FAIL [expected PASS] /html/canvas/element/manual/text/canvas.2d.disconnected.html (#30063)
OK [expected TIMEOUT] /html/interaction/focus/the-autofocus-attribute/supported-elements.html (#24145)
- FAIL [expected NOTRUN] subtest: Host element with delegatesFocus should support autofocus
```
assert_equals: expected Element node &lt;div autofocus=""&gt;&lt;/div&gt; but got Element node &lt;body&gt;&lt;/body&gt;
```
- FAIL [expected NOTRUN] subtest: Host element with delegatesFocus including no focusable descendants should be skipped
```
assert_equals: expected Element node &lt;input autofocus=""&gt;&lt;/input&gt; but got Element node &lt;body&gt;&lt;/body&gt;
```
- FAIL [expected NOTRUN] subtest: Area element should support autofocus
```
promise_test: Unhandled rejection with value: object "TypeError: w.document.querySelector(...) is null"
```
TIMEOUT [expected OK] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-1.html (#24066)
- NOTRUN [expected FAIL] subtest: Check that popups from a sandboxed iframe do not escape the sandbox
OK /html/semantics/forms/form-submission-0/text-plain.window.html (#28687)
- FAIL [expected PASS] subtest: text/plain: 0x00 in name (normal form)
```
assert_equals: expected "a\0b=c\r\n" but got ""
```
- PASS [expected FAIL] subtest: text/plain: double quote in name (formdata event)
OK /html/semantics/scripting-1/the-script-element/module/dynamic-import/blob-url.any.worker.html (#33909)
- PASS [expected FAIL] subtest: Revoking a blob URL immediately after calling import will not fail
OK /navigation-timing/test-navigation-type-reload.html (#33334)
- PASS [expected FAIL] subtest: Reload domComplete > Original domComplete
- PASS [expected FAIL] subtest: Reload domContentLoadedEventEnd > Original domContentLoadedEventEnd
- PASS [expected FAIL] subtest: Reload domContentLoadedEventStart > Original domContentLoadedEventStart
- PASS [expected FAIL] subtest: Reload domInteractive > Original domInteractive
- PASS [expected FAIL] subtest: Reload fetchStart > Original fetchStart
OK /webaudio/the-audio-api/the-audiobuffersourcenode-interface/sub-sample-buffer-stitching.html (#22849)
- FAIL [expected PASS] subtest: X Stitched sine-wave buffers at sample rate 43800 does not equal [0,0.06264832615852356,0.12505052983760834,0.18696144223213196,0.24813786149024963,0.308339387178421,0.36732959747314453,0.4248766601085663,0.480754554271698,0.5347436666488647,0.5866320133209229,0.6362156271934509,0.6832997798919678,0.7276994585990906,0.7692402601242065,0.8077589869499207...] with an element-wise tolerance of {"absoluteThreshold":0.0038986,"relativeThreshold":0}. Index Actual Expected AbsError RelError Test threshold [14650] -1.3876453847634351e+29 8.6956524848937988e-1 1.3876453847634351e+29 1.5957921354079763e+29 3.8985999999999999e-3 [14651] 3.0547976493835449e-1 8.9879405498504639e-1 5.9331429004669189e-1 6.6012262403823208e-1 3.8985999999999999e-3 Max AbsError of 1.3876453847634351e+29 at index of 14650. Max RelError of 1.5957921354079763e+29 at index of 14650.
```
assert_true: expected true got false
```
- FAIL [expected PASS] subtest: X SNR (-539.4114839674221 dB) is not greater than or equal to 65.737. Got -539.4114839674221.
```
assert_true: expected true got false
```
ERROR [expected OK] /webxr/render_state_update.https.html (#27535)
OK /workers/WorkerGlobalScope-close.html (#23064)
- FAIL [expected PASS] subtest: Test sending a message after closing.
```
assert_not_equals: got disallowed value "pong"
```

Stable unexpected results (1)

OK /_webgl/conformance/context/premultiplyalpha-test.html
- PASS [expected FAIL] subtest: WebGL test #62
- PASS [expected FAIL] subtest: WebGL test #69

github-actions · 2025-05-07T17:21:37Z

⚠️ Try run (#14888835620) failed.

sagudev · 2025-05-07T17:23:35Z

OK /_webgl/conformance/context/premultiplyalpha-test.html
* PASS [expected FAIL] subtest: `WebGL test #62`

* PASS [expected FAIL] subtest: `WebGL test #69`

Just as I though, this is actually tested 🎉so it needs updated expectations. But otherwise LGTM.

The RGB components of pixel wasn't overwritten after alpha multiply operation. Testing: /webgl/tests/conformance/context/premultiplyalpha-test.html Fixes: servo#35759 Signed-off-by: Andrei Volykhin <[email protected]>

mrobinson requested changes May 7, 2025

View reviewed changes

sagudev reviewed May 7, 2025

View reviewed changes

components/pixels/lib.rs Show resolved Hide resolved

tharkum force-pushed the pixels-rgba-premultiply branch from 406d696 to 039333c Compare May 7, 2025 13:19

tharkum closed this May 7, 2025

tharkum reopened this May 7, 2025

tharkum force-pushed the pixels-rgba-premultiply branch from 039333c to 457a88d Compare May 7, 2025 16:40

sagudev added the T-linux-wpt Do a try run of the WPT label May 7, 2025

github-actions bot removed the T-linux-wpt Do a try run of the WPT label May 7, 2025

sagudev closed this May 7, 2025

sagudev reopened this May 7, 2025

pixels: Overwrite RGB components on alpha multiply

0deb401

The RGB components of pixel wasn't overwritten after alpha multiply operation. Testing: /webgl/tests/conformance/context/premultiplyalpha-test.html Fixes: servo#35759 Signed-off-by: Andrei Volykhin <[email protected]>

tharkum force-pushed the pixels-rgba-premultiply branch from 457a88d to 0deb401 Compare May 7, 2025 21:57

sagudev approved these changes May 8, 2025

View reviewed changes

sagudev enabled auto-merge May 8, 2025 03:39

sagudev requested a review from mrobinson May 8, 2025 03:39

mrobinson changed the title ~~pixels: Overwrite RGB components on alpha multiply~~ pixels: Actually write pixels in MULTIPLY generic_transform_inplace operations May 8, 2025

mrobinson approved these changes May 8, 2025

View reviewed changes

sagudev added this pull request to the merge queue May 8, 2025

Merged via the queue into servo:main with commit bc9f55b May 8, 2025
24 checks passed

tharkum deleted the pixels-rgba-premultiply branch May 8, 2025 08:33

		@@ -64,10 +64,16 @@ pub fn rgba8_byte_swap_colors_inplace(pixels: &mut [u8]) {
		pub fn rgba8_byte_swap_and_premultiply_inplace(pixels: &mut [u8]) {

Uh oh!

Conversation

tharkum commented May 7, 2025 • edited by mrobinson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tharkum commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrobinson left a comment

Choose a reason for hiding this comment

Uh oh!

sagudev May 7, 2025

Choose a reason for hiding this comment

Uh oh!

tharkum May 7, 2025

Choose a reason for hiding this comment

Uh oh!

sagudev May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tharkum May 7, 2025

Choose a reason for hiding this comment

Uh oh!

tharkum commented May 7, 2025

Uh oh!

Uh oh!

sagudev commented May 7, 2025

Uh oh!

sagudev commented May 7, 2025

Uh oh!

tharkum commented May 7, 2025

Uh oh!

sagudev commented May 7, 2025

Uh oh!

tharkum commented May 7, 2025

Uh oh!

github-actions bot commented May 7, 2025

Uh oh!

github-actions bot commented May 7, 2025

Uh oh!

github-actions bot commented May 7, 2025

Uh oh!

sagudev commented May 7, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tharkum commented May 7, 2025 •

edited by mrobinson

Loading

tharkum commented May 7, 2025 •

edited

Loading

sagudev May 7, 2025 •

edited

Loading