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

support color masking of images #2969

Merged
merged 1 commit into from Aug 14, 2018

Conversation

Projects
None yet
3 participants
@lsalzman
Contributor

lsalzman commented Aug 13, 2018

This is to help support the necessary fixes for Gecko bug https://bugzilla.mozilla.org/show_bug.cgi?id=1479196

Tofu glyphs will be drawn out of an atlas, and to avoid the need to have to generate a new atlas for every single color or opacity combination, this just allows push_image to specify a color which can then be used for masking or opacity.

Most of the plumbing is already there, just some parameters need to be added to support this.


This change is Reviewable

@gw3583

Reviewed 17 of 17 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lsalzman)


webrender/src/batch.rs, line 726 at r1 (raw file):

                                                let prim_header_index = prim_headers.push(&prim_header, [
                                                    uv_rect_address.as_int(),
                                                    (ShaderColorMode::Image as i32) << 16 |

How did this used to work previously and why do we need to change it now on types other than images?

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Aug 13, 2018

Contributor

This basically looks good, but I'm somewhat confused about the shader color mode change on some of the batching code. If I'm reading it correctly it looks like it was wrong previously, and is now right? But if so, how come we don't have test failures from those? I feel like I'm mis-reading part of the patch :)

We'll also need a try run before merging.

Contributor

gw3583 commented Aug 13, 2018

This basically looks good, but I'm somewhat confused about the shader color mode change on some of the batching code. If I'm reading it correctly it looks like it was wrong previously, and is now right? But if so, how come we don't have test failures from those? I feel like I'm mis-reading part of the patch :)

We'll also need a try run before merging.

@lsalzman

It worked previously because we always passed in the premultiplied color white to the shader. The old shader color mode only accessed color.a, which if you squint at it, behaves exactly the same as the premultiplied color white, but would break if any other color was passed. So I had to change the color mode to access the RGB components of the color as well, not just the alpha component.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lsalzman)

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Aug 13, 2018

Contributor

@lsalzman Ah that makes sense - so it was broken, but not in any user-visible way previously 💩

Contributor

gw3583 commented Aug 13, 2018

@lsalzman Ah that makes sense - so it was broken, but not in any user-visible way previously 💩

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Aug 13, 2018

Contributor

r=me once we have a try result posted here.

Contributor

gw3583 commented Aug 13, 2018

r=me once we have a try result posted here.

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Aug 14, 2018

Contributor

That looks like a lot of orange on the try - but it's also not the try configuration I normally use, so maybe those are expected.

I leave it to your wise judgment :)

@bors-servo delegate+

Contributor

gw3583 commented Aug 14, 2018

That looks like a lot of orange on the try - but it's also not the try configuration I normally use, so maybe those are expected.

I leave it to your wise judgment :)

@bors-servo delegate+

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 14, 2018

Contributor

✌️ @lsalzman can now approve this pull request

Contributor

bors-servo commented Aug 14, 2018

✌️ @lsalzman can now approve this pull request

@lsalzman

This comment has been minimized.

Show comment
Hide comment
@lsalzman

This comment has been minimized.

Show comment
Hide comment
@lsalzman
Contributor

lsalzman commented Aug 14, 2018

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 14, 2018

Contributor

📌 Commit c75c32d has been approved by lsalzman

Contributor

bors-servo commented Aug 14, 2018

📌 Commit c75c32d has been approved by lsalzman

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 14, 2018

Contributor

⌛️ Testing commit c75c32d with merge f087a9f...

Contributor

bors-servo commented Aug 14, 2018

⌛️ Testing commit c75c32d with merge f087a9f...

bors-servo added a commit that referenced this pull request Aug 14, 2018

Auto merge of #2969 - lsalzman:color-image, r=lsalzman
support color masking of images

This is to help support the necessary fixes for Gecko bug https://bugzilla.mozilla.org/show_bug.cgi?id=1479196

Tofu glyphs will be drawn out of an atlas, and to avoid the need to have to generate a new atlas for every single color or opacity combination, this just allows push_image to specify a color which can then be used for masking or opacity.

Most of the plumbing is already there, just some parameters need to be added to support this.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2969)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 14, 2018

Contributor

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: lsalzman
Pushing f087a9f to master...

Contributor

bors-servo commented Aug 14, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: lsalzman
Pushing f087a9f to master...

@bors-servo bors-servo merged commit c75c32d into servo:master Aug 14, 2018

3 of 4 checks passed

code-review/reviewable 1 discussion left (lsalzman)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment