David Anderson [:dvander] - inactive, e-mail if emergency Reporter
	Description • 7 years ago

Currently there are some obscure cases in layers where, for a component-alpha layer with a temporary surface, we copy the backdrop into the intermediate surface before rendering the container [1].

It's difficult to construct a case where this shows up, and the complexity is kind of high to make it work. Matt and I were thinking of adding a pref to disable it, and use greyscale AA for these cases instead, to see whether anyone complains on Nightly.

Any thoughts, Jeff?

[1] http://searchfox.org/mozilla-central/rev/79f625ac56d936ef5d17ebe13a123b25efee4241/gfx/layers/Layers.cpp#1432

	David Anderson [:dvander] - inactive, e-mail if emergency Reporter
	Comment 1 • 7 years ago

Matt and I are having trouble even making a test case that would trigger this path. I've attached one attempt. With Skia, I get greyscale AA on the layer that requires a surface copy. That layer uses component alpha, and the "texture on white" buffer contains nothing but white pixels. 

With direct2d on, we do full subpixel AA, and the surface copy is required. When I disable surface copying, the text is clearly broken.

To make matters more confusing, Advanced Layers + d2d renders the subpixel AA on this test correctly, even though it does not support surface copying.

Chrome, Edge, and Safari all do greyscale AA here.

	Matt Woodrow (:mattwoodrow)
	Comment 2 • 7 years ago

Here's a testcase for the most basic use case where we currently use component alpha.

From some quick testing, it looks like safari, chrome (on OSX and Windows) and Edge all use greyscale for the text, whereas we get subpixel-AA (via the use of component alpha layers).

Component alpha layers really hurt performance (they double memory usage, and raster time for the layer, as well as being slower to composite), and they also account for a fair bit of code complexity within Layers, so I think we should make sure we're ok with paying this cost.

Note that we also have greyscale AA for software compositing (when e10s is enabled), we only use component alpha layers for the accelerated compositors.

I think we should have a chat in SF next month to discuss possible paths forward.

Some open questions:

* Do other browsers ever get subpixel-AA for these testcases? They didn't for my testing, but it's possible that there are other variables that I didn't test (screen resolution?).

* Do our users appreciate the higher text quality for these pages? Do they appreciate it enough to justify the performance hit?

* Edge seems to do really nice rendering even though it's greyscale, can we do this?

* Do the answers to the above question vary between operating systems? We've tried to disable subpixel-AA entirey before on OSX and had a lot of pushback, but just disabling it for the hard cases, and only on Windows might give different results.

CC'ing a few more people who might be interested.

	Matt Woodrow (:mattwoodrow)
	Comment 3 • 7 years ago

Apologies, meant to cc everyone, not ni.

	Mason Chang [Inactive] [:mchang]
	Comment 4 • 7 years ago

(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> Created attachment 8869915 [details] 
> Some open questions:
> 
> * Do other browsers ever get subpixel-AA for these testcases? They didn't
> for my testing, but it's possible that there are other variables that I
> didn't test (screen resolution?).

I did lots of testing. Here are some notes and configurations. Simple here means dvander's test case. Advanced here means Matt's Firefox logo test case. Yes means there was subpixel AA. No means no subpixel AA.

Windows 7 - No HiDPI screen
Chrome - Simple - No
Chrome - Advanced - Yes
IE 11 - Simple - Yes
IE 11 - Advanced - No

OS X Sierra
Safari - Retina - Simple - Yes
Safari - Retina - Advanced - No
Safari - Non-Hidpi - Simple - No
Safari - Non-Hidpi - Advanced - No

Chrome - Retina - Simple - No
Chrome - Retina - Advanced - No
Chrome - Non-Hidpi - Simple - No
Chrome - Non-Hidpi - Advanced - Yes

Windows 10 - Hi DPI
Edge - Simple - Yes
Edge - Advanced - No
Chrome - Simple - No
Chrome - Advanced - No

Windows 10 - Non-Hidpi
Edge - simple - yes
Edge - Advanced - No
Chrome - Simple - No
Chrome - Advanced - Yes

Generally looks like on Matt's case, Chrome will enable subpixel AA on non-hidpi screens. I wonder why I'm getting different results from you on the hidpi cases for Edge / IE.

	Mason Chang [Inactive] [:mchang]
	Comment 5 • 7 years ago

(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> Component alpha layers really hurt performance (they double memory usage,
> and raster time for the layer, as well as being slower to composite), and
> they also account for a fair bit of code complexity within Layers, so I
> think we should make sure we're ok with paying this cost.
> 
> Some open questions:

> * Do our users appreciate the higher text quality for these pages? Do they
> appreciate it enough to justify the performance hit?

I do not think this is worth the performance hit. We've had a regression in the tabs on Windows where we revert to greyscale AA for long tab titles for multiple releases. Either (a) None of the developers are on Windows enough to care or (b) Nobody noticed. Fixing subpixel AA seemed like a high priority on mac but not on windows. See bug 307833. IMHO, if the other browsers don't try too hard to always enable subpixel AA, I don't think we should either.
 
> * Edge seems to do really nice rendering even though it's greyscale, can we
> do this?

I'm not quite sure what you mean here, for your test case? I'm sure we should be able to do this.

> * Do the answers to the above question vary between operating systems? We've
> tried to disable subpixel-AA entirey before on OSX and had a lot of
> pushback, but just disabling it for the hard cases, and only on Windows
> might give different results.

The browsers seem to do this depending on DPI more than OS.

	Markus Stange [:mstange]
	Comment 6 • 7 years ago

(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> * Do other browsers ever get subpixel-AA for these testcases? They didn't
> for my testing, but it's possible that there are other variables that I
> didn't test (screen resolution?).

I know that Chrome tries really hard to get subpixel AA on non-HiDPI, unless there's a transform animation (or will-change) - it will usually turn off layerization if automatic layerization would cause a loss of subpixel AA. But on HiDPI devices, Chrome doesn't put any effort in preserving subpixel AA.

> We've
> tried to disable subpixel-AA entirey before on OSX and had a lot of
> pushback,

The situation was a bit more subtle than that. The goal was to disable subpixel AA on OS X, but only on HiDPI, and only in those cases where we'd need component alpha layers. The reason those patches were backed out was that they caused two mostly unrelated regressions: We lost subpixel AA on non-HiDPI context menus and panels, and the non-subpixel text looked too thin. I think we should try this again, but without causing those regressions.
(The former happened because context menus and panels are slightly transparent, so we had always been calling drawTarget->SetPermitSubpixelAA(false) for them, but before those patches we had ignored that call on DrawtTargetCG. And the latter happened because if you ask CoreGraphics to disable subpixel AA it uses a different way to composite text which looks thinner; Skia gives us a bit more control there.)

	David Anderson [:dvander] - inactive, e-mail if emergency Reporter
	Comment 7 • 7 years ago

On my Acer quantum reference machine, the initial test case (that needs a surface copy) is greyscale on Chrome. Matt's testcase uses subpixel AA on the same machine. On my hi-dpi desktop, both tests are greyscale in Chrome (matching what Markus said).

Regardless of the outcome of this bug, we get greyscale AA on the first test case when using Skia, but still proceed with component-alpha layers. That seems like a waste.

But since no one noticed this, I'd argue that we should do greyscale AA when we would need a surface copy, and then drop this logic from the compositor. And D2D would match the Skia behavior, and we'd also have fewer component-alpha layers.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 8 • 7 years ago

Can somebody post a patch for comment 7 (or something else perhaps), then we can decide if it's good enough?  Seems gmail (bug 1365823) is hitting this.

	David Anderson [:dvander] - inactive, e-mail if emergency Reporter
	Comment 9 • 7 years ago

AL made disabling this easy since it doesn't support it.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 10 • 7 years ago

Comment on attachment 8883373 [details] [diff] [review]
patch

Review of attachment 8883373 [details] [diff] [review]:
-----------------------------------------------------------------

What are the implications of this change?  Nothing changes on Windows with advanced layers, things get faster on OS X, since we're using Skia the font doesn't change (at all?) much?

	David Anderson [:dvander] - inactive, e-mail if emergency Reporter
	Comment 11 • 7 years ago

(In reply to Milan Sreckovic [:milan] from comment #10)
> Comment on attachment 8883373 [details] [diff] [review]
> patch
> 
> Review of attachment 8883373 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What are the implications of this change?  Nothing changes on Windows with
> advanced layers, things get faster on OS X, since we're using Skia the font
> doesn't change (at all?) much?

This will switch from subpixel AA to greyscale AA, when: a transparent container layer with an intermediate surface is on top of an opaque layer, and the intermediate surface has a non-integer 2D transform. A scrollable div with opacity and text often triggers it. In this case the compositor has to do a readback from the opaque layer, and in many cases we just end up using greyscale (with component alpha) anyway on the content side, so the readback and component alpha are useless.

As far as we know every other browser does greyscale AA in this scenario.

	Matt Woodrow (:mattwoodrow)
	Comment 12 • 7 years ago

The primary concern that Jeff raised (with both this, and going further and disabling all component alpha layers) is the case where we toggle in and out of this path at a high frequency (either due to scrolling or an animation).

That would have the effect of switching between subpixel-AA and greyscale text, which will almost certainly be noticeable, and would make the text appear to flicker.

I think we should go ahead for now, but it's something to look out for, and we might need to consider making the greyscale-AA fallback persist (even after the complex case conditions are no longer met) just to avoid the transition.

	David Anderson [:dvander] - inactive, e-mail if emergency Reporter
	Comment 13 • 7 years ago

Okay, let's give this a shot and see if we get complaints.

	Pulsebot
	Comment 14 • 7 years ago

Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8eb05f9fc699
Use greyscale instead of subpixel AA for complex intermediate surfaces. (bug 1366618, r=milan)

	Carsten Book [:Tomcat]
	Comment 15 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/8eb05f9fc699

	David Anderson [:dvander] - inactive, e-mail if emergency Reporter
	Comment 16 • 7 years ago

I'm going to back this out for now since it broke Mac, and I don't have time to investigate. We could consider making this change Windows-only, which is effectively what AL does (by virtue of not supporting Mac).

	Pulsebot
	Comment 17 • 7 years ago

Backout by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2afa381beba
Backed out changeset 8eb05f9fc699 for breaking Mac URL bars.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 18 • 7 years ago

Merged backout https://hg.mozilla.org/mozilla-central/rev/b2afa381beba

	Firefox Bug Husbandry Bot
	Comment 19 • 7 years ago

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals