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

Incorrect text shadows with 0 blur on color emoji #1917

Closed
lsalzman opened this issue Oct 23, 2017 · 4 comments
Closed

Incorrect text shadows with 0 blur on color emoji #1917

lsalzman opened this issue Oct 23, 2017 · 4 comments

Comments

@lsalzman
Copy link
Contributor

@lsalzman lsalzman commented Oct 23, 2017

In the text shadow with 0 blur case, we use the normal text shader. For color emoji, this shader samples the bitmap color data, and then multiplies by the shadow color, when it should just be simply the shadow color. We probably need to punt back to the text shadow shader somehow or modify the text shader somehow to handle this case.

@kvark
Copy link
Member

@kvark kvark commented Oct 24, 2017

I'm not sure I understand why the shadow application for blur=0 should be different (replace vs multiply) from the more general shadow case.
Also, this needs a corresponding Bugzilla issue.

@mstange
Copy link
Contributor

@mstange mstange commented Oct 27, 2017

It shouldn't be different. The bug is that it is different at the moment. The blur shader ignores R/G/B, the text shader respects it (which is incorrect for shadows).

@mstange
Copy link
Contributor

@mstange mstange commented Oct 28, 2017

I suggest closing this issue in favor of #1959 which has a screenshot and is easier to understand.

bors-servo added a commit that referenced this issue Dec 18, 2017
ignore glyph color data when doing fast shadows

This addresses issues #1959 and #1917. We need to swizzle away the glyph's color data for emoji fast shadows. Rather than introducing more shaders/ifdefs to deal with this, I added a reasonably cheap programmable swizzle in the shader to control this. A side benefit of this is that we also get rid of the need for the separate shader with the subpixel-with-bg-color blending passes as they now just use the swizzle mechanism.

In the process of this I noticed that the idea of directly stuffing the shadow color into the font on the cloned text run primitive for the fast shadow has a downside: it will usually cause the font to hash differently and so make duplicate entries in the glyph cache. So I added a shadow_color field back onto TextRunPrimitiveCpu, serving dual purposes that 1) lets the font itself hash the same while also 2) indicating down to the batching code that we have to use shadows in the first place (and thus need to enable the swizzle).

<!-- 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/2223)
<!-- Reviewable:end -->
@glennw
Copy link
Member

@glennw glennw commented Dec 19, 2017

Fixed in #2223.

@glennw glennw closed this Dec 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.