906462 - Background images on about:newtab slow down painting on hi-dpi screens

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

about:newtab with its noisy background image combined with a gradient, and its thumbnails implemented as background images is quite slow on my Retina MBP.

Opening a new tab has a noticeable delay. Closing new tabs and switching to them shows the same issues. On a retina screen, the tabopen animation is completely skipped when the browser is maximized. The animation improves a little for smaller windows.

All these symptoms are not present on my other, less powerful machines with lower resolutions. CC'ing roc to see if there's anything we can do about this.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Use <img> elements for thumbnails on about:newtab

Removing the noisy background image and using <img> elements for the thumbnails yields a huge improvement, especially for maximized windows. We unfortunately lose the background-size:cover functionality as that's not available for images.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: (WIP) provide cover/contain sizing for images

I wonder if this is something we could do in case fixing background image painting is too hard. We could hook this up to something like -moz-image-sizing:cover/contain. This seems to work fine but it should probably automatically set a clipping rect to not draw outside of the image frame.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Here's a profile of opening a single about:newtab page, that has been preloaded in the background:

http://people.mozilla.com/~bgirard/cleopatra/#report=fee988ad2fea957e7f55947d4797cbe773e09317

Comment 4

[Matt Woodrow (:mattwoodrow)]

We've actually got the optimization mentioned in bug 761009 implemented now, but it only takes effect under some conditions.

In this case we're skipping it because the backgrounds are being clipped to the padding box rather than the border box. I can't think of any reason why we couldn't fix this case though.

We also require that the scaled image exactly covers the border area. This is because we haven't implemented support for tiling, or preventing sampling outside the intended area when drawing part of an image.

The latter is the case we're hitting here, background-size:cover is causing the bottom 15 pixels of the image to be cut off.

I'm not sure how easy it is to handle this correctly when drawing with the GPU.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess we're scaling the background image and/or the thumbnails.

Can we avoid that by providing the background image at higher resolution (via a media query say) so it doesn't need to be scaled? That would look better too.

Same goes for the thumbnails. We should be taking the snapshots at 2x resolution so they don't need to be scaled and they look better.

Comment 6

[Matt Woodrow (:mattwoodrow)]

I'm not sure what you mean sorry.

I think the problem is that the thumbnail isn't the same aspect ratio as the div we're trying to use it as a background for. We scale the thumbnail to the same width as the div, but preserving aspect ratio. This leaves us with the thumbnail being 15pixels taller than the div (at least with my setup), and we fallback to the CPU to do the clipping.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think with "contain" and "cover" we can assume no atlasing/spriting and allow ourselves to sample from anywhere in the image. That should fix this.

Comment 8

[Avi Halachmi (:avih)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> ... This leaves us with
> the thumbnail being 15pixels taller than the div (at least with my setup),

The size and aspect ratio of the div depend on the window size and aspect ratio.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Allow ImageLayerization for images using 'contain' and 'cover'

I'm not sure if this will help performance much without more work.

I've verified that we create ImageLayers but for the static new-tab page I guess they'll be in inactive opacity containers so they'll still not be stretched and composited on the GPU. For that we need something more like GPUImageScalingEnabled().

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

It seems like the patch does help a little bit, only. As I just found out, the biggest win comes from actually removing the background image and the linear gradient entirely. I wonder if there's something we can do about that? Is a 48x48 sprite too expensive? Should we have a bigger tile?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Aha! So it *is* the page background after all, eh?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What does the linear gradient actually do? It doesn't seem to do anything useful as far as I can tell.

Have you tried providing a 2x resolution background image so on Retina displays we don't have to scale the tile?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

My patch here causes a test failure in B2G R10:
https://tbpl.mozilla.org/php/getParsedLog.php?id=27288561&tree=Try&full=1

Comment 14

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 797078 [details] [diff] [review]
Allow ImageLayerization for images using 'contain' and 'cover'

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

::: layout/base/nsDisplayList.cpp
@@ +1910,5 @@
>  nsDisplayBackgroundImage::BuildLayer(nsDisplayListBuilder* aBuilder,
>                                       LayerManager* aManager,
>                                       const ContainerParameters& aParameters)
>  {
> +  nsRefPtr<ImageLayer> layer = aManager->CreateImageLayer();

Why this change? We don't want to create new layers every paint.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 797078 [details] [diff] [review]
Allow ImageLayerization for images using 'contain' and 'cover'

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

::: layout/base/nsDisplayList.cpp
@@ +1910,5 @@
>  nsDisplayBackgroundImage::BuildLayer(nsDisplayListBuilder* aBuilder,
>                                       LayerManager* aManager,
>                                       const ContainerParameters& aParameters)
>  {
> +  nsRefPtr<ImageLayer> layer = aManager->CreateImageLayer();

This change is wrong. I'll revert it.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

See comment #12.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Sorry for the delay. Yes, a 2x resolution bg image helps a lot.

It seems like we should still look into replacing the bg images as the newtab animation is a lot more smooth when that is done, too. Maybe attachment 797078 [details] [diff] [review] would also help as well with that? I can't tell because it unfortunately doesn't apply anymore.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Remove bg gradients and provide a 2x resolution bg image for hi-dpi screens

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> What does the linear gradient actually do? It doesn't seem to do anything
> useful as far as I can tell.

I have no idea, I removed them and we'll see if shorlander agrees.

This patch also adds a 2x resolution bg image.

Stephen, can you please tell if there's a reason to keep those gradients? Also, you may want to provide a better @2x version of the bg image? I'll attach the one from the patch, too.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Attachment: [e-mail]

The bg image with 2x the resolution.

Comment 20

[Stephen Horlander [:shorlander]]

Comment on attachment 810444 [details] [diff] [review]
Remove bg gradients and provide a 2x resolution bg image for hi-dpi screens

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

(In reply to Tim Taubert [:ttaubert] from comment #18)
> Created attachment 810444 [details] [diff] [review]
> Remove bg gradients and provide a 2x resolution bg image for hi-dpi screens
> 
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> > What does the linear gradient actually do? It doesn't seem to do anything
> > useful as far as I can tell.
> 
> I have no idea, I removed them and we'll see if shorlander agrees.
> 
> This patch also adds a 2x resolution bg image.
> 
> Stephen, can you please tell if there's a reason to keep those gradients?
> Also, you may want to provide a better @2x version of the bg image? I'll
> attach the one from the patch, too.

If we remove the gradient I would like to lighten the whole page. I think we can just remove the noise texture entirely. Although if we do it on the about:newtab page we should also do it to about:home.

background-color: hsl(0,0%,95%) should be light enough.

Thanks!

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Stephen Horlander [:shorlander] from comment #20)
> If we remove the gradient I would like to lighten the whole page. I think we
> can just remove the noise texture entirely. Although if we do it on the
> about:newtab page we should also do it to about:home.
> 
> background-color: hsl(0,0%,95%) should be light enough.

Will do, thanks for the quick response!

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Remove noise backgrounds for about:newtab and about:home

This patch removes the noise.png background images (and gradients) from about:newtab and about:home. I move the newtab/noise.png images to the devtools folder as it turned out that devtools was referencing it.

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 811526 [details] [diff] [review]
Remove noise backgrounds for about:newtab and about:home

> #newtab-scrollbox:not([page-disabled]) {
>-  background-color: rgb(229,229,229);
>-  background-image: url(chrome://browser/skin/newtab/noise.png),
>-                    linear-gradient(rgba(255,255,255,.5), rgba(255,255,255,.2));
>-  background-attachment: fixed;
>+  background-color: hsl(0,0%,95%);
> }

Please add color:black here, even though a custom color is used for various (hopefully all) text-related elements. Better safe than sorry.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #23)
> Please add color:black here, even though a custom color is used for various
> (hopefully all) text-related elements. Better safe than sorry.

Will do, thanks!

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/7b0138abb5a6

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/7b0138abb5a6

Comment 27

[Avi Halachmi (:avih)]

(In reply to Tim Taubert [:ttaubert] from comment #22)
> ... I move the newtab/noise.png images to the
> devtools folder as it turned out that devtools was referencing it.

Maybe we could also do the same trick on devtools, if it improves rendering performance?

Comment 28

[Paul Rouget [:paul]]

(In reply to Avi Halachmi (:avih) from comment #27)
> (In reply to Tim Taubert [:ttaubert] from comment #22)
> > ... I move the newtab/noise.png images to the
> > devtools folder as it turned out that devtools was referencing it.
> 
> Maybe we could also do the same trick on devtools, if it improves rendering
> performance?

The connect page (the page the uses this image) is disabled by default.
Bug filed: bug 921835

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 811526 [details] [diff] [review]
Remove noise backgrounds for about:newtab and about:home

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None.
User impact if declined: Shitty tabopen animation on hi-res displays.
Testing completed (on m-c, etc.): Landed on m-c.
Risk to taking this patch (and alternatives if risky): Low risk patch that only changes styles.
String or IDL/UUID changes made by this patch: None.

In terms of performance, it would be great if we could uplift this to Aurora and Beta. On Retina displays, the tabopen animation is almost back to previous performance.

I don't know though if we usually uplift visual changes like this to Beta? It seems like a minor change (we'll have a lighter background on about:newtab and about:home) and the perf win is very noticeable.

Comment 30

[Alex Keybl [:akeybl]]

Comment on attachment 811526 [details] [diff] [review]
Remove noise backgrounds for about:newtab and about:home

This appears to be a very minor visual change, so we'll take the perf win on branches (although this doesn't meet typical uplift criteria).

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

Thanks!

https://hg.mozilla.org/releases/mozilla-aurora/rev/e9b9a61a3236
https://hg.mozilla.org/releases/mozilla-beta/rev/efb226e93d5e

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Not sure why this bug is still open.

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

If attachment 797078 [details] [diff] [review] would improve things even more it would be great to have that rebased and maybe landed as well?

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It caused a test failure, see comment #13. If it's not important I don't want to spend time on it right now.

Comment 35

[Tim Taubert [:ttaubert] (inactive)]

Ok, it's not super important but it would be great to address this rather sooner than later. We've made a big improvement by removing the background image/gradient but the animation is still not as smooth as it can be.

Filed bug 927228 so that we can fix it later.

Comment 36

[Bogdan Maris, Desktop QA]

Tim can you or anyone else with a hi-dpi screen please check that this is verified on Firefox 26 beta 1 and latest Aurora 27.0a2?

Comment 37

[Tim Taubert [:ttaubert] (inactive)]

This is definitely fixed, we're doing much better now on Retina. We're still not on par with images (instead of bg images) but I'm hoping for bug 927228.

Comment 38

[Tracy Walker [:tracy]]

I don't have a retina display, but do have a hi-dpi Surface Pro.  In full screen, New Tab Open/Close responsiveness is good with smooth automation.