I've had this disabled locally for a week now system wide. I notice it right away when running on my external non hidpi monitor but never notice it on my retina display. If the scale factor/hdpi is sufficient I think we can disable it. I vote that we try this on nightly for a few days.

That seems pretty reasonable to me. Can we only disable it for screens that are actually retina, to avoid the issue with your external display? We should also probably include some of the UX guys on this bug to help evaluate this.

Yes I should of made this more clear. Detecting what display we're on is a blocker for this. This should also help us for retina perf where we are behind :(. Currently a window can't span both a 2.0 scaled screen vs. a non scaled screen. So perhaps we should check the content scale to avoid the problem where one external monitor is considered hidpi but not the other.

With component alpha on non retina this is noticable. Default Safari: Transition is really bad even on retina Default Canary: Transition is good even without hidpi. Canary switch to grayscale but keeps a good representation of greyscale coverage while we do not, particular coverage pixel often get a dark pixel. If we can fix this and match chrome behavior we can probably disable on non hidpi.

Comment on attachment 8346203 [details] [diff] [review] patch Got a new patch coming but it only disables some subpixelAA. Still hinting down where the rest is coming from.

Comment on attachment 8346344 [details] [diff] [review] Part 2: Disable subpixelaa + component alpha Review of attachment 8346344 [details] [diff] [review]: ----------------------------------------------------------------- I think this is ok implementation wise. Do we have any data on how much of a performance difference it makes? It should be fine to land this for nightlies to see if the text quality reduction is noticeable. Let's get some of the UX guys cc'd on this bug to help evaluate the visual change, and we can see if it's worth the performance win before this goes onto aurora.

On my machine running scaled 1080p this can save 4-8ms of upload time as well as a large chunk of memory. I ran some non scientific test where the upload times shrunk quite a bit. I didn't even look at the rasterize times.

I'm just working from the assumption that at least some people will complain about this change, and we want to be able to justify it.

I don't think so. Have you tried the test case? Its not noticeable. No browsers on Mac do it. But perhaps metro? I don't think android uses subpizel aa, I know component alpha is disabled its there.

No browsers on Mac do what? Safari definitely gives me subpixel AA for text when I'm at retina resolution.

You're right. Maybe it was just active layers. https://tbpl.mozilla.org/?tree=Try&rev=279d07129ccf

I had to adjust the fuzzy factor for scaled3d test. Because we disable subpixel AA on active layers and this implements it properly now.

This was unexpected: Regression: Mozilla-Inbound - SVG, Opacity Row Major - MacOSX 10.6 (rev4) - 2.32% increase ------------------------------------------------------------------------------------------ Previous: avg 414.625 stddev 2.781 of 12 runs up to revision 2e5ff5614254 New : avg 424.250 stddev 2.251 of 12 runs since revision 47aac229cc2d Change : +9.625 (2.32% / z=3.461) Graph : http://mzl.la/1dc8gl5

https://hg.mozilla.org/integration/mozilla-inbound/rev/47aac229cc2d https://hg.mozilla.org/integration/mozilla-inbound/rev/7b9ff0798201

https://hg.mozilla.org/mozilla-central/rev/7b9ff0798201 https://hg.mozilla.org/mozilla-central/rev/47aac229cc2d

Benoit: The change in text rendering is very noticeable on my Retina MacBook Pro. Text is much lighter and thinner. Is this the expected change? This screenshot compare yesterday's Nightly build (on the left) with your patch (on the right).

When we did some testing offline the change wasn't really noticeable without zooming really. Are you running a 13/15 inch model? What is your screen resolution settings (unscaled?)?

I just got this change in today's Nightly update, and my immediate impression (running on a 15" Retina MacBook Pro at the standard "best for Retina" resolution, 2880x1800) is that it makes a lot of pages look quite washed-out. The text looks substantially thinner/lighter/weaker than the default text rendering seen in other applications. My guess is that if we ship this in a Firefox update, a lot of Retina-display users will find it a pretty disconcerting and unwelcome change. They may not be able to articulate exactly what is wrong, but it'll feel as though suddenly many of their pages have had the contrast turned down. :( I don't think we should do this. The user owns their text rendering setting (System Preferences / General / Use LCD Smoothing...), and we have no business overriding that choice.

Screenshot comparing text rendering in Nightly before (left) and after (right) this update.

I upgraded nightly and I notice the change. It wasn't my intention for the difference to be this noticeable. I'll double check my tests again. I had spent more time checking the different between component alpha and not.

I don't think we actually respect the system preferences option fwiw.

Can someone explain to my why this looks the way it looks with CA disabled? In particular why does the contrast seem lower. I am not sure I understand what causes that. More edgy font outlines I would understand.

I think this should be reverted. Subpixel AA is a user pref and for most users it's on by default. If you pref something else, fine, we can add an override option but in general we should roughly match Safari on the same machine. If we need a Firefox-specific way of overriding the setting, fine, but it should *not* be subpixel AA should not be disabled by default like this. Was this a perf optimization? It's not really clear from the description/comments. The "default to grayscale on HiDPI" produces many instances of washed out text, as in the attached example from asahi.com. Firefox trunk is on the left, Safari on the right. Note how the weather info (天気) is faded, along with the subject headings (i.e. the line starting with 2013).

(In reply to Andreas Gal :gal from comment #28) > Can someone explain to my why this looks the way it looks with CA disabled? > In particular why does the contrast seem lower. I am not sure I understand > what causes that. More edgy font outlines I would understand. That's the nature of subpixel AA vs. grayscale text rendering. Under OSX, open Preferences > General and toggle "Use LCD font smoothing when available" and note how the text looks distinctly sharper with it enabled.

It is indeed a performance optimization to switch to greyscale AA. subpixel AA is more expensive to render even for the simple cases. When we have text over transparency within a layer (like when we have text scrolling over a fixed background) we draw the text twice (onto buffers with black/white backgrounds) and then recover the alpha during composition. This makes painting the layer take more than twice as long, doubles the memory usage, and increase the composition cost. We were hoping we could avoid all of that given the high resolution of retina screens. It appears that maybe we can't :)

Also interesting is that on my retina MBP (running at full native resolution) has also had subpixel AA turned off for popups. My main window still gets it though.

We should try to do like Safari and Chrome do: subpixel AA when we can and none otherwise.

Discussion on subpixel positioning (mainly not AA) and high dpi devices (from Behdad), interesting and relevant read: https://docs.google.com/document/d/1wpzgGMqXgit6FBVaO76epnnFC_rQPdVKswrDQWyqO1M/edit

Since I get the feeling the ticket I created is not getting any attention (Bug 950511): this patch introduced a regression for non-HiDPI users as well. Subpixel antialiasing is disabled for context menus on OS X now resulting in a very non-native look and feel.

So this got reopened, but the patches are still in the tree, right? I just got updated to a build with this patch in it, and trying to use bugzilla is very difficult: the test is quite a bit harder to read... If we're not planning to ship this, can we at least back it out in the meanwhile?

I need to check the backout since I'm leaving part 1 in: https://tbpl.mozilla.org/?tree=Try&rev=57634ef71b3f

The version of DrawTargetCairo.h currently in the tree also results in a compile error for me: When I compile Moz2D with MOZ2D_CG=true, I get the following error message with both Clang++ 5.0 and GCC 4.8: In file included from Factory.cpp:32: ./DrawTargetCG.h:156:16: error: 'SetPermitSubpixelAA' marked 'override' but does not override any member functions virtual void SetPermitSubpixelAA(bool aPermitSubpixelAA) MOZ_OVERRIDE; ^ 1 error generated.

That seems odd; isn't it overriding the function from DrawTarget in 2D.h? http://mxr.mozilla.org/mozilla-central/source/gfx/2d/2D.h#929

Oddly enough, the method is not virtual in the Moz2D repository (linked to in the Wiki https://wiki.mozilla.org/Platform/GFX/Moz2D ), hence probably the error. If that is not where I'm supposed to get Moz2D, it would be good if the Wiki pointed to the right place...

Looks like the patch from bug 926023, which made that method virtual, didn't get applied to the standalone Moz2D repository. Matt? Bas?

(In reply to Jonathan Kew (:jfkthame) from comment #41) > Looks like the patch from bug 926023, which made that method virtual, didn't > get applied to the standalone Moz2D repository. Matt? Bas? Looks like we forgot that :(. I'll fix this.

Please also make sure to have a look at the case reported through bug 950511. It's still broken in today's Nightly version.

(In reply to Benoit Girard (:BenWa) from comment #37) > I need to check the backout since I'm leaving part 1 in: > https://tbpl.mozilla.org/?tree=Try&rev=57634ef71b3f Does "leaving part 1 in" here explain why bug 950511 remains broken? Should that be reconsidered?

(In reply to Jonathan Kew (:jfkthame) from comment #44) > Does "leaving part 1 in" here explain why bug 950511 remains broken? Should > that be reconsidered? Remains broken in those builds or in on trunk cause it hasn't landed because of the Reftest failures.

Can we please do something here? Using Bugzilla is incredibly eyestrain-inducing due to this bug...

I think we need to just back both patches out (because of the problems reported here, and bug 950511). I'll probably do that tomorrow unless BenWa objects or beats me to it.

Was waiting for a good time on inbound. https://hg.mozilla.org/integration/mozilla-inbound/rev/b10b52d2f9bf I left Part 1 since it's the missing support for moz2d and it would of needed a backout from the moz2d repo. bug 950511 still needs to be investigated because this backout doesn't resolve it.

(In reply to Benoit Girard (:BenWa) from comment #48) > Was waiting for a good time on inbound. > > https://hg.mozilla.org/integration/mozilla-inbound/rev/b10b52d2f9bf > > I left Part 1 since it's the missing support for moz2d and it would of > needed a backout from the moz2d repo. > > bug 950511 still needs to be investigated because this backout doesn't > resolve it. I'm 99% sure that Part 1 is causing 950511. OSX context menus have a partially transparent background, so we shouldn't get subpixel AA for them ever. But previously we just ignored this, and drew subpixel AA anyway, and it looks fine. With Part 1, we now (correctly?) disable subpixel AA for these transparent surfaces, which unfortunately looks considerably worse. Note that the cairo-cg backend also ignores the allow-subpixel-AA setting, which is why this worked pre-azure. I think we need to back this out to preserve our existing behaviour for now. We probably want to have a discussion (work week maybe?) about when it's ok to use subpixel AA on a partially transparent background.

Based on the comments here and the noticeability apparent in the attached screenshots I think we need to track this.

The current state is that part 1 is still landed, on both mozilla-central and aurora (29), and part 2 was briefly landed and then backed out and is currently not on any branch. In the mozilla.dev.platform thread titled "Subpixel AA text rendering on OSX", the consensus was that we should back out part 1 as well. This will fix bug 950511. I will do the backout as soon as mozilla-inbound reopens.

Backed out part 1 on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/5c04b296c127

and on Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/ae49057fdd6e

I'm removing the tracking flag because this bug will either be left open until we reconsider it, or it will be wontfixed. We should move the tracking flag to bug 950511 instead, which is the regression that the patch here caused.

After backing this out, I had to disable a reftest that landed while part 1 was in the tree, see bug 967834.

For the time being this is WONTFIX because of how different the font rendering is with and without subpixel AA. It seems to not only drop subpixel AA but also significantly change the weight of the font. We can't do this without a significant change in the font rendering. We may revisit this decision later.