pixels: Store decoded images as RGBA premultiplied data #40158
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🔨 Triggering try run (#18800532404) for Linux (WPT) |
|
Test results for linux-wpt from try job (#18800532404): Flaky unexpected result (20)
Stable unexpected results that are known to be intermittent (23)
Stable unexpected results (74)
|
|
|
ff1b015 to
70389a1
Compare
|
🔨 Triggering try run (#18801523051) for Linux (WPT) |
|
Test results for linux-wpt from try job (#18801523051): Flaky unexpected result (22)
Stable unexpected results that are known to be intermittent (18)
Stable unexpected results (10)
|
|
|
Member
Author
|
I added a note in the PR description about the two new WebGL failures. Summary: I think this change is important even though it causes a pretty minor regression in the WebGL comformance suite. |
70389a1 to
0e78411
Compare
Member
I think that's reasonable, but we should document this (losing correctness for performance) in places were we are currently premultiplying and open a new issue about it (also link it in the comments). Do we have any numbers of performance improvement? |
sagudev
reviewed
Oct 26, 2025
Member
Author
I can try to get some numbers tomorrow. Performance number might be hard to get (and I suspect that other things are dominating as performance is still very bad for canvas), but I can do some simple counting of the conversions that we avoid in Servo. |
tharkum
reviewed
Oct 27, 2025
| // to the source rectangle with formatting. | ||
| let snapshot = raster_image.as_snapshot(); | ||
| let Some(bitmap_data) = | ||
| ImageBitmap::crop_and_transform_bitmap_data(snapshot, sx, sy, sw, sh, options) |
Contributor
There was a problem hiding this comment.
ImageBitmap::crop_and_transform_bitmap_data(raster_image.as_snapshot(), ....) ?
Member
Author
There was a problem hiding this comment.
I could do this though it adds a line of code for every argument of crop_and_transform_bitmap_data due to rustfmt. Normally I'm happy to do this, but this function takes 6 arguments.
Member
Author
I've gone ahead and added a comment to the place where we convert image data to premultiplied data with a link to #40257.
I ran https://cookieclicker.eu/cookieclicker/ before and after this change, placing a print statement right before Before: Unfortunately, Cookie Clicker is still slow with my change, but I think this is a big step on the way toward fixing it. |
0e78411 to
6e54ff4
Compare
Member
Author
|
@sagudev I think I've responded to all of your comments. Thanks for the review. PTAL. |
|
🔨 Triggering try run (#18901069038) for Linux (WPT) |
|
Test results for linux-wpt from try job (#18901069038): Flaky unexpected result (20)
Stable unexpected results that are known to be intermittent (14)
|
|
✨ Try run (#18901069038) succeeded. |
sagudev
approved these changes
Oct 30, 2025
Comment on lines
+402
to
+405
| bytes.extend_from_slice(&[rgb[2], rgb[1], rgb[0], 0xff]); | ||
| } | ||
| ( | ||
| WebRenderImageFormat::BGRA8, |
Member
There was a problem hiding this comment.
non-blocking: Given that we started preferring RBGA everywhere else, should we use it here too?
Member
Author
There was a problem hiding this comment.
Yeah, I was thinking that this might be a good idea, but was planning on doing it in a followup in order to minimize the changes in one PR.
Comment on lines
+293
to
+294
| /// Whether or not all of the frames of this image are opaque. | ||
| pub is_opaque: bool, |
Member
There was a problem hiding this comment.
though(non-blocking): Maybe we should make this per frame? Although I am not sure how much that would matter in practice.
Member
Author
There was a problem hiding this comment.
This is actually a bit tricky, because the value is per ImageKey in WebRender and we share an ImageKey among all frames of an animated image. As many gifs are looping, perhaps the potential performance isn't so important ATM.
Member
There was a problem hiding this comment.
This is actually a bit tricky, because the value is per
ImageKeyin WebRender and we share anImageKeyamong all frames of an animated image. As many gifs are looping, perhaps the potential performance isn't so important ATM.
IIRC Every time you update image you also provide the flags (if opaque) so it's per image rather then per ImageKey. But I agree that this might not be really important or worth the complexity.
mrobinson
commented
Oct 31, 2025
Comment on lines
+402
to
+405
| bytes.extend_from_slice(&[rgb[2], rgb[1], rgb[0], 0xff]); | ||
| } | ||
| ( | ||
| WebRenderImageFormat::BGRA8, |
Member
Author
There was a problem hiding this comment.
Yeah, I was thinking that this might be a good idea, but was planning on doing it in a followup in order to minimize the changes in one PR.
6e54ff4 to
0ae2fa4
Compare
Canvas expects input data to be in RGBA premultiplied format and WebRender already supports RGBA and BGRA data as long as they are premultiplied. Pre-multiplying up front allows: - Avoiding many conversion when painting images to canvas. - Passing the `RasterImage` IpcSharedMemory of the image instead of creating a new one with the premultiplied data every time we upload to WebRender. This is a big deal for animated gifs, because before every frame was creating a new shared memory segment. It seems that for rasterized SVGs were were already putting premultplied data into the `RasterImage` so it's quite likely SVGs were being composited incorrectly. Signed-off-by: Martin Robinson <mrobinson@igalia.com>
0ae2fa4 to
b95822e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Canvas expects input data to be in RGBA premultiplied format and
WebRender already supports RGBA and BGRA data as long as they are
premultiplied. Pre-multiplying up front allows:
RasterImageIpcSharedMemory of the image instead of creatinga new one with the premultiplied data every time we upload to
WebRender. This is a big deal for animated gifs, because before every
frame was creating a new shared memory segment.
It seems that for rasterized SVGs were were already putting
premultplied data into the
RasterImageso it's quite likely SVGs werebeing composited incorrectly.
Testing: This causes 8 tests to start passing and 2 tests to fail in the WebGL conformance suite.
The failures are due to the fact that premultiplying alpha is lossy when alpha is 0. In that case,
the resulting color of a blend operation might be wrong. This is typically only a problem if you
use RGBA data as RGB data, which is pretty unusual. In the case that you are blending with
RGBA the final color values will be 0 or close to 0 anyway. Gecko solves this issue by having a
cacheable surface generation API that can fetch both premulitplied and unpremulitplied data
from things like image elements. We do not have that yet, but I argue that this change
is important anyway due to the amount that it reduces memory and file descriptor usage
as well as the cost of copying image data so much in memory.