1506996 - We are painting the whole tracking protection animation sprite sheet every frame

Status: RESOLVED FIXED

(Firefox :: Protections UI, defect, P1)

Description

[Jeff Muizelaar [:jrmuizel]]

This is really bad for performance:
https://perfht.ml/2zOSyj5

Comment 1

[Jeff Muizelaar [:jrmuizel]]

A good first step would be to see if it gets better with fewer frames in the animation. We might be beyond the limit of frames that can be drawn ahead of time.

Comment 2

[(no longer active)]

Markus, any chance you could have a look please to see if comment 1 is what's happening?  You can see this icon in action on any site with a lot of trackers, like nytimes.com.  Thanks!

Comment 3

[Bobby Holley (:bholley)]

Yeah, I've seen this sprite clogging up the texture cache. Because it's long, it gets a standalone texture, and we end up with many copies of it.

Comment 4

[Markus Stange [:mstange]]

The rasterized size on a 2x scaling display is 2496x40, and 2496 * 40 * 4 bytes, which is just under 400KB. This should be cacheable and should only be rasterized once. We need to find out why we're not caching it, or what's invalidating the cache.

Comment 5

[Markus Stange [:mstange]]

The cacheability is decided at https://searchfox.org/mozilla-central/rev/d850d799a0009f851b5535580e0a8b4bb2c591d7/image/VectorImage.cpp#1176-1184 .

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

The fill and fill-opacity are constantly changing. Since the drawing parameters are different, we can't reuse what we have cached so we redraw the SVG each time.

Comment 7

[Andrew Osmond [:aosmond] (he/him)]

This is apparent if you look at the memory report, this is a partial snapshot with the relevant bits:

├───20,166,160 B (10.64%) -- images
│   ├──19,342,032 B (10.21%) -- chrome
│   │  ├──19,324,656 B (10.20%) -- vector/used
│   │  │  ├──16,581,088 B (08.75%) -- image(1248x20, chrome://browser/skin/tracking-protection-animation.svg)
│   │  │  │  ├──13,657,664 B (07.21%) -- unlocked/factor2
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ff3c3c3c fillOpa=0.3 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ff403a3e fillOpa=0.805195 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ff44393f fillOpa=0.81039 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ff4c3643 fillOpa=0.820779 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ff503445 fillOpa=0.825974 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ff563247 fillOpa=0.833766 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ff5a3049 fillOpa=0.838961 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ff602e4c fillOpa=0.846753 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ff642c4d fillOpa=0.851948 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ff6a2a50 fillOpa=0.85974 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ff6e2952 fillOpa=0.864935 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ff742654 fillOpa=0.872727 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ff782556 fillOpa=0.877922 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ff7c2358 fillOpa=0.590909 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ff80225a fillOpa=0.888312 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ff84205b fillOpa=0.893507 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ff8c1d5f fillOpa=0.903896 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ff8e1c60 fillOpa=0.906493 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ff961963 fillOpa=0.916883 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ff981864 fillOpa=0.919481 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ff9c1766 fillOpa=0.924675 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ffa21468 fillOpa=0.932468 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ffaa116c fillOpa=0.942857 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ffae106e fillOpa=0.948052 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ffb20e6f fillOpa=0.953247 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ffb60c71 fillOpa=0.958442 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ffbc0a74 fillOpa=0.966234 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ffc00975 fillOpa=0.971429 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ffc60678 fillOpa=0.979221 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ffc80579 fillOpa=0.981818 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ffd0027c fillOpa=0.992208 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ffd2027d fillOpa=0.981818 strokeOpa=1 ) ])
│   │  │  │  │  ├─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ffd2027d fillOpa=0.994805 strokeOpa=1 ) ])
│   │  │  │  │  └─────401,696 B (00.21%) ++ surface(2496x40, svgContext:[ viewport=(1248x20) contextPaint=( fill=ffd6007f fillOpa=1 strokeOpa=1 ) ])

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Johann, can we bake the fill/fill-opacity changes into the actual frames of the animation. The current situation is causing a pretty noticeable performance problem.

Comment 9

[Johann Hofmann [:johannh]]

Ouch.

[Tracking Requested - why for this release]:
Perf impact on everyone when we enable cookie restrictions by default in 65.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> Johann, can we bake the fill/fill-opacity changes into the actual frames of
> the animation. The current situation is causing a pretty noticeable
> performance problem.

We could do that if we make two different versions, for light and dark themes...

But, in general, how can we make it easier for front-end developers to avoid these cases? We're using SVGs with context-fill a lot and also in our animations. It would be great if there were guidelines and/or tooling to avoid SVG performance disasters.

Another animation that might be doing this wrong is the new-bookmark-falls-into-library animation, though it probably doesn't matter as much.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

(In reply to Johann Hofmann [:johannh] from comment #9)
> Ouch.
> 
> [Tracking Requested - why for this release]:
> Perf impact on everyone when we enable cookie restrictions by default in 65.
> 
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> > Johann, can we bake the fill/fill-opacity changes into the actual frames of
> > the animation. The current situation is causing a pretty noticeable
> > performance problem.
> 
> We could do that if we make two different versions, for light and dark
> themes...
> 
> But, in general, how can we make it easier for front-end developers to avoid
> these cases? We're using SVGs with context-fill a lot and also in our
> animations. It would be great if there were guidelines and/or tooling to
> avoid SVG performance disasters.

A general guideline is to think of context-fill as changing the SVG. So when you animate context-fill we need to repaint the whole thing. More generally, the front-end should only animate properties that can be animated on the compositor. I believe the dev tools expose this information.

> Another animation that might be doing this wrong is the
> new-bookmark-falls-into-library animation, though it probably doesn't matter
> as much.

We should still fix it. The impact on memory usage would probably be noticeable as we end up storing a copy of the image for all the different context-fills.

Comment 11

[Johann Hofmann [:johannh]]

Ok, just so that we don't forget about it, if I'm not misreading the CSS this should affect the bookmark and pocket library animations as well:

https://searchfox.org/mozilla-central/rev/c0b26c40769a1e5607a1ae8be37fe64df64fc55e/browser/themes/shared/toolbarbutton-icons.inc.css#407
https://searchfox.org/mozilla-central/rev/c0b26c40769a1e5607a1ae8be37fe64df64fc55e/browser/components/pocket/skin/pocket.css#119

And the overflow animation?

https://searchfox.org/mozilla-central/rev/c0b26c40769a1e5607a1ae8be37fe64df64fc55e/browser/themes/shared/toolbarbutton-icons.inc.css#296

Comment 12

[Sam Foster [:sfoster] (he/him)]

(In reply to Johann Hofmann [:johannh] from comment #11)
> Ok, just so that we don't forget about it, if I'm not misreading the CSS
> this should affect the bookmark and pocket library animations as well:

If you can file these :johannh, we could divide them up? We'll have to evaluate each individually anyway. 

Animating the fill and fill-opacity (and potentially stroke and stroke-opacity) was a handy way to have the animations go from "whatever-color-it-was" to some predefined color like --toolbarbutton-icon-fill-attention. In /think/ that in practice that is probably a choice of one of 2 colors for light/normal and dark themes, but it also allowed for alternate palettes and other scenarios where the initial or destination color could vary. I'll happily trade real performance today for a hypothetical theme option but I want to make sure we don't regress anything. There is next to no test coverage for this stuff and it relies heavily on manual testing - visually checking all the possible states and state transitions.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

The start color of these animations can be changed by themes,
https://searchfox.org/mozilla-central/rev/647b9eb1099e373e079e16f147f74f3d1d65eed3/toolkit/components/extensions/parent/ext-theme.js#161-166

Is there a CSS filter() we can apply that would get us similar color changing without having to change the SVG?

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Alternatively, if we could build the SVG during run-time based on the colors being used, then we could hardcode the various colors in to the runtime-created image.

Comment 15

[Sam Foster [:sfoster] (he/him)]

I think the issue is the changing of the color *in every frame* by animating fill and fill-opacity, which are -moz-context-properties. I think we might be able to rework the (static) SVGs to still parameterize the colors via CSS but hard-code the opacity into each frame of the SVG animation, so the image as whole doesnt change each frame.

Comment 16

[Markus Stange [:mstange]]

Creating an image at runtime seems vastly preferable from a performance perspective.

The point of the spritesheet + transform animation approach was to run the animation on the compositor, so that it's smooth even when the main thread is busy. Animations of fill/fill-opacity and filter run on the main thread, so they unfortunately defeat the purpose of this animation approach. Filters are also expensive at the moment; they add more work per pixel, on the main thread.

Comment 17

[Markus Stange [:mstange]]

Oh, comment 15 also sounds good. As long as these properties don't change during the animation, you're fine.

Comment 18

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1506996 - Bake animation of color and opacity into the .svg

Comment 19

[Sam Foster [:sfoster] (he/him)]

(In reply to Sam Foster [:sfoster] from comment #18)
> Created attachment 9026218 [details]
> Bug 1506996 - Bake animation of color and opacity into the .svg

I wanted to see how this plays out so I tried some stuff out on the bookmark animation. I was able to eliminate the `fill` CSS animation by cross-fading overlapping elements in the SVG frames from the start color (context-fill) to the end color (context-stroke) - these correspond to the default toolbar fill color and the attention color. The opacity animation is harder as this ramps in from the default toolbar icon opacity - which varies by platform. In this patch I shot the middle by animating the first 10 frames from 0.8 opacity to 1.0. but that's probably not quite right.

Comment 20

[Amy Lee [:amylee] UX]

Attachment: shield_icon_default.svg

Hi, 

I've attached the shield animation svg with colour transitions baked in. Dark theme version to come. Let me know if anything else is needed.

Comment 21

[Amy Lee [:amylee] UX]

Attachment: shield_icon_default.svg

Comment 22

[Amy Lee [:amylee] UX]

Attachment: shield_icon_dark.svg

Dark theme version

Comment 23

[Johann Hofmann [:johannh]]

Attachment: Bug 1506996 - Hardcode colors in the content blocking shield animation to avoid animating context properties. r=jaws,sfoster

This is a (hopefully not permanent) workaround to prevent expensive SVG animation stemming
from the fact that we used to animate the context-fill and context-fill-opacity properties
on the content blocking shield SVG.

With the upcoming cookie restrictions enabled by default this animation will be shown
on many webpages, potentially turning into a perf problem.

The short term solution is to simply hard code colors and make two versions of the animation:
One for dark theme and one for regular theme icon colors. Note that certain themes are able to
change the icon colors, thus this isn't a perfect solution and would look slightly ugly with those
themes.

We will, later, with less pressure from a perf side, look at how we can make this icon
(and other icons that have the same problem) be compatible with custom icon colors in themes.

Comment 24

[Johann Hofmann [:johannh]]

Jared, Sam, this is what I talked about earlier. It's a short-term fix for this problem so that it doesn't block content blocking in 65. We lose the ability to apply custom icon colors to this animation for now, but otherwise this should be fine I hope. I'm happy to file a follow-up bug for fixing it properly if you think this approach is okay.

Sam, I know you probably have ideas to optimize this SVG. Happy to incorporate them :)

Note that I kept context-fill in the last element of both SVGs to support things like hover color etc.

Thanks!

Comment 25

[Johann Hofmann [:johannh]]

Also thank you Amy for providing these SVGs so quickly!

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a72dafbb2e80
Hardcode colors in the content blocking shield animation to avoid animating context properties. r=jaws,sfoster

Comment 27

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/a72dafbb2e80