Closed Bug 977796 Opened 6 years ago Closed 6 years ago

Disable subpixel anti-aliasing during customize mode transition.

Categories

(Firefox :: Theme, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

Details

(Whiteboard: [Australis:P2])

Attachments

(1 file, 2 obsolete files)

This is being split off from bug 974607.

Subpixel anti-aliasing is expensive on Windows when we're using Direct2D layers acceleration. This is contributing to the slow painting during the customize mode transition.

We can work around this by temporarily defeating subpixel anti-aliasing during the transition.

This compare talos shows the result of doing so:

http://compare-talos.mattn.ca/?oldRevs=21e77b801df3&newRev=b5d485143452&server=graphs.mozilla.org&submit=true

This should be a nice perf win for the transition on Windows.
Note that I also tested this on OS X and Linux, and didn't see the same improvements (in fact, it made things worse[1]), which is why I'm only going to try this for Windows.

[1]: http://compare-talos.mattn.ca/?oldRevs=71efe1f6d4a7&newRev=fdc2ed44e85d&server=graphs.mozilla.org&submit=true
Attached patch Patch v1 (obsolete) — Splinter Review
How do you feel about this, Jared?
Attachment #8383260 - Flags: review?(jaws)
Comment on attachment 8383260 [details] [diff] [review]
Patch v1

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

Thank you for investigating this, it looks to be a pretty good boost based on your compare-talos link in comment #0.

::: browser/themes/windows/browser.css
@@ +2500,5 @@
>  
>  %include ../shared/customizableui/customizeMode.inc.css
>  
> +#main-window:-moz-any([customize-entering],[customize-exiting]) label {
> +  transform: perspective(1px);

Please please please include an explanation of why this is necessary. `blame` exists, but this will look really weird to the casual code peruser.

Secondly, instead of using a tag name here, can we reference one of the CSS classes that is causing the issue? I would suspect it is `.toolbarbutton-multiline-text`.
Attachment #8383260 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> Please please please include an explanation of why this is necessary. `blame`
> exists, but this will look really weird to the casual code peruser.

Very good call. I'll add a note.

> Secondly, instead of using a tag name here, can we reference one of the CSS
> classes that is causing the issue? I would suspect it is
> `.toolbarbutton-multiline-text`.

Almost all of the text is causing the issue, including:

1) Selected tab label
2) Menu panel item labels
3) Menu panel footer item labels
4) Palette item labels
5) The header text for customize mode
6) The items in the footer for the palette

Did you want me to add more selectors so that I target all of these individually?
Flags: needinfo?(jaws)
Comment on attachment 8383260 [details] [diff] [review]
Patch v1

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

Also, can we use something like perspective(0.01px) ?

Hm, I'm just worried about the cost of this rule. Let's keep this as-is for now, since as you've mentioned it is quite hard to catch all of the cases (.toolbarbutton-text, .toolbarbutton-multiline-text would cover the majority of the text though).
Flags: needinfo?(jaws)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Comment on attachment 8383260 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8383260 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Also, can we use something like perspective(0.01px) ?
> 
> Hm, I'm just worried about the cost of this rule. Let's keep this as-is for
> now, since as you've mentioned it is quite hard to catch all of the cases
> (.toolbarbutton-text, .toolbarbutton-multiline-text would cover the majority
> of the text though).

Sounds good!
Attached patch Patch v1.1 (r+'d by jaws) (obsolete) — Splinter Review
Added some documentation, and switched to perspective(0.01px).
Attachment #8383260 - Attachment is obsolete: true
Attachment #8383491 - Flags: review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/a267ac44d506

Feels good to land a patch!
Whiteboard: [Australis:P2] → [Australis:P2][fixed-in-fx-team]
The patch you landed didn't change the perspective to 0.01px. Maybe you forgot to qref? Also, we should keep an eye on the CART tests to make sure that this does as comment #0 said it would do.
Also, no documentation.
Ugh, whoops. I forgot how to qref.
Attachment #8383491 - Attachment is obsolete: true
Attachment #8383495 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/26020d6a32f2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P2][fixed-in-fx-team] → [Australis:P2]
Target Milestone: --- → Firefox 30
This descendent selector is kind of crazy. Can you not apply the transform to some large container (e.g. #main-window) rather than every single label?
Flags: needinfo?(mconley)
(In reply to Dão Gottwald [:dao] from comment #13)
> This descendent selector is kind of crazy. Can you not apply the transform
> to some large container (e.g. #main-window) rather than every single label?

It doesn't seem like it - from the looks of it, I have to apply the transform directly to the element with the text in it in order to trick Layout into thinking that the colour behind the text might be transparent.

Applying the transform to the main-window, or any of the other parent elements of those labels doesn't seem to be enough to fool Layout and defeat subpixel AA.

Open to suggestions though.
Flags: needinfo?(mconley)
Ugh, I didn't notice but my comment for the corrected landing got midaired.

Here it is:

Backed out as https://hg.mozilla.org/integration/fx-team/rev/3ea8562b6bac

Landed the right stuff as https://hg.mozilla.org/integration/fx-team/rev/26020d6a32f2
Comment on attachment 8383495 [details] [diff] [review]
Patch v1.1 (for reals) (r+'d by jaws)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

Australis


User impact if declined: 

A slower customization mode transition.


Testing completed (on m-c, etc.): 

Baking on mozilla-central for a day, and extensive local testing.


Risk to taking this patch (and alternatives if risky): 

Very, very little. It is just CSS, and the rule only applies for a 150ms window.


String or IDL/UUID changes made by this patch:

None.
Attachment #8383495 - Flags: approval-mozilla-aurora?
Attachment #8383495 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Uh, whoops - I thought Sylvestre approved these, so it's a=sledru on the commit message. My bad.

https://hg.mozilla.org/releases/mozilla-aurora/rev/306e5fffe72b
I'm a monster.
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.