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

Bad subpixel AA inside stacking context for mask #2657

Closed
mstange opened this issue Apr 13, 2018 · 6 comments
Closed

Bad subpixel AA inside stacking context for mask #2657

mstange opened this issue Apr 13, 2018 · 6 comments

Comments

@mstange
Copy link
Contributor

@mstange mstange commented Apr 13, 2018

https://bugzilla.mozilla.org/show_bug.cgi?id=1454097

Now that we're using stacking contexts for masks (bug 1412375), if you go to a site with a long URL, like this one, the text in the URL bar will look funny. Firefox applies a fade-out mask effect to long URLs.

I think this is happening because we're incorrectly allowing the use of subpixel AA inside the intermediate surface for the stacking context, even though we're drawing onto transparency.

We should do the following:

  1. Force grayscale AA inside stacking contexts unless we know that the text is being drawn onto opaque pixels within that stacking context.
  2. As a future optimization and quality improvement, detect the case where the grouping provided by the stacking context is unnecessary, and discard the intermediate surface and apply the mask to the contents directly. This would give us the previous appearance for the URL bar.
@mrobinson
Copy link
Member

@mrobinson mrobinson commented Apr 13, 2018

@mstange Do you mind expanding on why the grouping is unnecessary here?

@glennw
Copy link
Member

@glennw glennw commented Apr 13, 2018

I think the problem is:

let allow_subpixel_aa = composite_ops.count() == 0 &&

If this becomes:

let allow_subpixel_aa = composite_mode.is_none();

Then I think this will fix the problem.

The test itself is still super-conservative - in addition to the optimization mentioned in (2) above, we'll also want to detect cases where we have an intermediate surface but we know the background is opaque.

We don't need to do this now, but we should do it in the future to allow subpixel AA in more cases than we currently do. Detecting (what I expect to be) the common case for this should be really simple - we just determine when checking the visibility of a picture if the local rect of any solid rects in this picture cover the entire local rect of the picture. This would mean that it's safe to use subpixel AA (and could also be used as a cheap occlusion cull for any primitives that come before such a rect in this picture).

Anyway, hopefully the one line fix mentioned above should be enough to fix the reported bug here. I can try it next week if no-one else gets to it first.

@Gankra
Copy link
Contributor

@Gankra Gankra commented Apr 13, 2018

@mrobinson I believe grouping is unnecessary in this case because there's only one primitive that's getting masked (a textrun). Potentially an easy case to notice in gecko.

@mstange
Copy link
Contributor Author

@mstange mstange commented Apr 13, 2018

That's right. Another case where grouping is unnecessary is when you have multiple primitives but they don't overlap; but that case is probably a lot harder to detect.

@Gankra
Copy link
Contributor

@Gankra Gankra commented Apr 13, 2018

Confirmed glenn's fix seems to work (although I'm on a high-dpi display so it's really hard to tell if it's causing too much greyscale AA). Just need to figure out a good regression test...

Gankra added a commit to Gankra/webrender that referenced this issue Apr 15, 2018
Gankra added a commit to Gankra/webrender that referenced this issue Apr 16, 2018
bors-servo added a commit that referenced this issue Apr 16, 2018
hotfix for #2657, prevent subpixel in masks

people are noticing this a lot, so let's just do this now and figure out the more robust solutions later.

#2657 still needs a proper regression test, which I'll try to knock out tomorrow

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

@glennw glennw commented Apr 17, 2018

Fixed by #2662.

@glennw glennw closed this Apr 17, 2018
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.