1054016 - Test performance impact of using SVG for Main Toolbar icons

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

One of the great things about Linux is how customizable it is. You can tweak practically everything.

One of the worst things about Linux is how customizable it is. You can tweak practically everything. That means that there is a humongous amount of variation that applications need to account for to run on Linux if they want to look "native".

Firefox currently ships with a set of static icons in a PNG. We use these instead of the stock GTK icons for most toolbar and menu icons.

This works for the majority of the Linux distros in their default states, but for heavily customized distros (or distros which we may be unfamiliar with), the colours that we've selected for the icons are bad. They might be hard to see, or clash wildly with the toolbar background.

One solution to this problem is to render the icons using SVG. That means that at run time, we can sample certain system colours, and rasterize a set of icons that are guaranteed to look how the system theme intends them to. That's great.

It also means that we don't need a separate set of images for hidpi displays.

We lightly investigated this back when we flipped the icons from stock GTK to our static PNGs back in bug 874674. What we found was that ts_paint would likely be affected by the cost of computing the SVG icons when the window first opens. Because of this, along with the time-crunch that Australis was under, we bailed on that plan.

I think we should re-investigate this, and work with platform to try to see if there's anything they can do to speed our SVG icons up if there is indeed a detectable regression.

I suggest we write a patch to replace the current icons on Linux with SVG icons, and get a sense of how much it hurts on our Talos tests.

If it doesn't hurt at all, and we don't feel like there's a detectable performance regression while using the browser, we should just land it and investigate doing the same for OS X and Windows.

If it doesn't hurt our Talos numbers but we do feel a detectable performance regression, we should try to write new tests to capture that regression.

If it hurts our Talos numbers, we should get profiles from the test runs, and send them off to platform (and jwatt) so they can see if there's anything that can be done to speed them up.

This bug was spun out of bug 1027080.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Out of curiosity, do we happen to have the Toolbar icons for Linux as SVG somewhere that we can test with?

Comment 2

[Yann Brelière]

Does this block Bug 1038334 - HiDPI support for Linux (or bug 967100)?

Comment 3

[Stephen Horlander [:shorlander]]

Attachment: Toolbar.svg

SVG version of the Toolbar spritesheet

Comment 4

[Stephen Horlander [:shorlander]]

Attachment: Toolbar-inverted

SVG version of the Toolbar-inverted spritesheet.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Thanks Stephen,

Baseline: https://tbpl.mozilla.org/?tree=Try&rev=88e66d799202
Patch: https://tbpl.mozilla.org/?tree=Try&rev=523beda538ac
Compare talos: http://compare-talos.mattn.ca/?oldRevs=88e66d799202&newRev=523beda538ac&server=graphs.mozilla.org&submit=true

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Grrr... this is annoying, Talos other won't run, and that's the one with ts_paint and tpaint. :(

I've filed bug 1064095 for that.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

I was able to get talos other to run by just running all talos tests.

Baseline: https://tbpl.mozilla.org/?tree=Try&rev=dc7161bbfb6c
Patch: https://tbpl.mozilla.org/?tree=Try&rev=93bb31ac0e52
Compare talos: http://compare-talos.mattn.ca/?oldRevs=dc7161bbfb6c&newRev=93bb31ac0e52&server=graphs.mozilla.org&submit=true

So ts_paint (opening the first browser window in a session) is majorly regressed here - by about 16-17% for both Linux 32 and 64.

I'll see if I can get some profiles from these to pass over to jwatt.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Sorry it took so long - I had to write some scripts to symbolize these profiles, and that was kind of a chore.

Some system symbols are still missing, unfortunately. Tell me if these are useful or not:

Baseline: http://people.mozilla.org/~bgirard/cleopatra/#report=613b067388ae638bdcb3aefd6a942fb34e35815e
SVG icons: http://people.mozilla.org/~bgirard/cleopatra/#report=41f0b40e994d5efa71ec4e0ec853a2e30bb89245

These profiles represent a single run of ts_paint - so samples from the start of the browser process, all the way to MozAfterPaint being fired.

The point where the SVG icons come into play should be pretty obvious in that second profile - 191 samples are spent on "Refresh 3" and "DisplayList #3".

Notably, in baseline we spend 21 samples inside DisplayList::Draw, whereas in the SVG patch we spend 187 samples inside DisplayList::Draw.

Anything useful in here, jwatt?

Comment 9

[Jonathan Watt [:jwatt]]

Great stuff! Thanks for your work on this, Mike and Stephen!

Mike: those profiles are useful, although unfortunately the "SVG icons" profile doesn't extend down into the DisplayList::Draw calls in the "DisplayList #3" part of the results. Nevertheless, we can now see that it is the painting (as opposed to reflow, or display list building, for example) that is the issue here. We can also see that a lot of DisplayList::Draw calls were made, which seems inconsistent with only painting one path element per icon, and the fact that the very limited conversion to SVG in the Try push seems to mean that only one SVG icon is painted when the browser loads. With that information I took a closer look at the painting under a debugger.

First an explanation of why we're seeing multiple DisplayList::Draw calls. This is due to the way that sets of icons are stored in a single SVG file (probably the right thing to do), and due to the way that icons are referenced out of the SVG file. In https://hg.mozilla.org/try/rev/93bb31ac0e52 the indicators.css file uses -moz-image-rect to reference and paint an icon out of the SVG file, clipped to a specific rectangle to only paint one icon. The first time that we paint an icon out of the SVG file is expensive because ClippedImage::DrawSingleTile currently invokes its wrapped VectorImage in a way that draws the entire SVG file (so all of the icons in the SVG) when painting a single icon, and then takes only the required area from the result in order to show the required icon. More specifically, here:

http://mxr.mozilla.org/mozilla-central/source/image/src/ClippedImage.cpp?rev=3d51132a0099&mark=386-386#373

we set the size that the code goes on to paint the SVG VectorImage with to the entire size of the SVG file.

However, if we were to go on to convert all of the codebase to paint icons using Toolbar.svg instead of Toolbar.png we would not need to paint the SVG again since VectorImage caches the rendering of the SVG. So a fuller conversion should not result in much more (if any) of a performance hit than the limited conversion in the Try push (and there will also be a perf win due to no longer loading Toolbar.png).

All that said, it seems clear that the one off paint of Toolbar.svg is still more expensive than Toolbar.png.

I made a content variant of what the browser is doing in a reduced testcase to to take a closer look at why the SVG is expensive to paint. Unsurprisingly virtually all the time is being spent applying the SVG filter to the icons.

As far as I can tell the filter is used to give the icons a sort of milled out appearance, so I assume that it is necessary (although perhaps it could be made less expensive?). CC'ing mstange to see if he has any comments or if he expects the ongoing filter code work to help here.

I'm also CC'ing Seth to see if he has any thoughts on cutting down on the amount of the SVG file that we paint up-front. Currently we only seem to paint six icons (back, reload, bookmark, show bookmarks and open menu) by default when the browser loads, whereas there are 37 icons in Toolbar.svg (so perhaps we could do a sixth of the work at startup time).

From the Desktop team it would be informative to get perf numbers on a complete conversion from Toolbar.png to Toolbar.svg. It would also be useful to get numbers of that same conversion but with the filter effect removed from Toolbar.svg (changing its 'id' attribute will stop it being applied to the icons) to confirm whether or not the performance issues are purely down to having to do live filtering in the browser. (Changing to use Toolbar-inverted.svg could probably be left out of any testing for now, since converting to just Toolbar.svg is enough to get perf numbers.)

Comment 10

[Markus Stange [:mstange]]

(In reply to Jonathan Watt [:jwatt] from comment #9)
> I made a content variant of what the browser is doing in a reduced testcase
> to to take a closer look at why the SVG is expensive to paint.
> Unsurprisingly virtually all the time is being spent applying the SVG filter
> to the icons.

Can you check how much of a difference setting color-interpolation-filters="sRGB" on the <filter> element makes? And whether the patch in bug 1064875 makes the composite operations better or worse?

(In reply to Jonathan Watt [:jwatt] from comment #9)
> As far as I can tell the filter is used to give the icons a sort of milled
> out appearance, so I assume that it is necessary

It looks like the filter is a drop shadow effect, equivalent to what you'd get if you used "filter: drop-shadow(0 1px rgba(0,0,0,.25));" with layout.css.filters.enabled set to true.

> CC'ing mstange to see if he has any comments or if
> he expects the ongoing filter code work to help here.

The ongoing filter work will accelerate filters on the compositor that have active layers, so filters inside SVG images won't benefit from it.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Thanks jwatt - glad the profiles were useful. :)

I can certainly provide try pushes / profiles with the filter removed. You asked as well for a "complete conversion from Toolbar.png to Toolbar.svg". Not sure what you mean by that - my patch should have swapped all toolbar icons to use SVG instead of the PNG. Were there icons that you saw that were still using the PNG file?

Comment 12

[Jonathan Watt [:jwatt]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #11)
> I can certainly provide try pushes / profiles with the filter removed.

Great, thanks!

> You
> asked as well for a "complete conversion from Toolbar.png to Toolbar.svg".
> Not sure what you mean by that - my patch should have swapped all toolbar
> icons to use SVG instead of the PNG. Were there icons that you saw that were
> still using the PNG file?

Yes, I did, but now that I checked again I see that those are in non-Linux code so not relevant to Linux-only profiling.

Comment 13

[Jiehong]

Hello,

Any news on this matter?

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Finally got around to this. I've applied the patch to default with the filter removed.

Baseline: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e5d3e8599178
Patch (#nofilter): https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4acd3bf4c442
Compare talos: http://compare-talos.mattn.ca/?oldRevs=e5d3e8599178&newRev=4acd3bf4c442&server=graphs.mozilla.org&submit=true

I've also pre-emptively pushed for profiles for both baseline and patch:

Baseline profile: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=70e81a806148
Patch profile: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=71e8ca064fff

I'll get that posted once these are done.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Garr, forgot to use Linux-specific try syntax on those first two links, since the "other" talos test doesn't run on Fx > 32 without it.

So ignore the first three links in my last comment (the other two are fine), and replace them with this instead:

Baseline: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=083f3fdb2e47
Patch (#nofilter): https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ffa8530e80c8
Compare talos: http://compare-talos.mattn.ca/?oldRevs=083f3fdb2e47&newRev=ffa8530e80c8&server=graphs.mozilla.org&submit=true

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Here are the new profiles:

Baseline: http://people.mozilla.org/~bgirard/cleopatra/#report=4fbc4b04f1575811207c8340625cd31af484dc4c
SVG patch (#nofilter): http://people.mozilla.org/~bgirard/cleopatra/#report=6ebbd06f8eb100f5bb2689b02af0f36bea6d0875

So, looking at the compare talos, taking out the filter took out a massive chunk of the regression for ts_paint - if you look at the compare talos in comment 7, we were regressing in the neighbourhood of 16.5% on ts_paint.

In the more recent compare talos in comment 15, with the filter removed, we regress more like 3% instead. That's quite an improvement! It's possible that other improvements in SVG rendering since that time have also contributed to this improvement, as I based the recent pushes off of a more recent tip of mozilla-central.

Anyhow, we're in a much better position it seems with the filter removed. jwatt, looking at the profiles above, is there any more room for improvement?

Comment 17

[Jonathan Watt [:jwatt]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #16)
> Here are the new profiles:
> 
> SVG patch (#nofilter):
> http://people.mozilla.org/~bgirard/cleopatra/
> #report=6ebbd06f8eb100f5bb2689b02af0f36bea6d0875

It looks like there are only four samples in SVG code, which doesn't look like enough. Did this profile run for long enough?

> In the more recent compare talos in comment 15, with the filter removed, we
> regress more like 3% instead. That's quite an improvement!

But you will want to keep the filters to get the icons to render correctly, right? Or are you thinking of having different looking icons?

> It's possible
> that other improvements in SVG rendering since that time have also
> contributed to this improvement, as I based the recent pushes off of a more
> recent tip of mozilla-central.

It would be great to have baseline, with filters and without filters comparisons. :)

> Anyhow, we're in a much better position it seems with the filter removed.
> jwatt, looking at the profiles above, is there any more room for improvement?

As I mentioned above, the profile looks a bit wrong, so I'm not able to answer that just yet.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Here's the comparison between the same baseline in comment 14, and the SVG patch with the filter rules put back in. ts_paint regresses by ~16% there on Linux 32 (which is the one I did 5 retriggers on for each push), so I think it's fair to say that the filter introduces a pretty big bottleneck.

Getting you those profiles now...

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Here's another profile with the SVG icons but no filtering:

http://people.mozilla.org/~bgirard/cleopatra/#report=361c491d63bf5b1cb3c1c3c403dfc0232642fd56

I, again, don't see much in the way of SVG method calls in this profile. I should point out that for ts_paint, we get a single profile per run of the test, since the test involves starting the browser and shutting it down once the first window has painted.

Am I correct when I say that this profile is about as useful as the one I linked to in comment 16?

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Also, shorlander, how critical is the filter to the appearance of the icons? I think we've pretty definitively identified it as a major bottleneck here... are there alternatives to achieve the same effect?

Comment 21

[Stephen Horlander [:shorlander]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #20)
> Also, shorlander, how critical is the filter to the appearance of the icons?
> I think we've pretty definitively identified it as a major bottleneck
> here... are there alternatives to achieve the same effect?

Maybe. I can play around with some other ideas. I am doubtful though that we will be able to achieve the right per-platform styles without using some filters.

Comment 22

[Jonathan Watt [:jwatt]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #19)
> Am I correct when I say that this profile is about as useful as the one I
> linked to in comment 16?

Unfortunately so. :)

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #20)
> Also, shorlander, how critical is the filter to the appearance of the icons?
> I think we've pretty definitively identified it as a major bottleneck
> here... are there alternatives to achieve the same effect?

It's just useful to have confirmation that the perf hit is down to the filters. The hit isn't so huge that we couldn't potentially make it up in other areas though. If this is something that the Firefox team would particularly like to be able to use then I think it would be worth trying to get this into my Q2 or future goals. Let me know how much of a benefit you feel you'd get from it and I'll then consider having a closer look at the cost if it looks to be worth digging into further.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Note that e10s is likely to give us better ts_paint and tpaint numbers[1]. We might be able to justify eating into some of those gains with SVG icons if it results in better appearance.

[1]: http://avih.github.io/blog/2015/03/19/firefox-e10s-performance-on-talos/

Comment 24

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

Can you confirm that your SVG icons render with acceptable quality at a variety of different sizes? Because up until now I'd assumed they wouldn't.

Comment 25

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

I'm not surprised that filter is expensive. Here it is:
    <filter id="filter-effects-toolbar">
      <feOffset in="SourceGraphic" dx="0" dy="1" result="fe-toolbar-offset" />
      <feFlood flood-color="black" flood-opacity=".7" result="fe-toolbar-flood" />
      <feComposite operator="out" in="fe-toolbar-flood" in2="fe-toolbar-offset" />
      <feGaussianBlur stdDeviation=".25" result="fe-toolbar-blur" />
      <feComposite operator="atop" in="fe-toolbar-blur" in2="SourceGraphic" result="fe-toolbar-composite" />
      
      <feColorMatrix in="fe-toolbar-offset" type="matrix" values="1  1  1  0 1
                                                                  1 -1  1  0 1
                                                                  1  1 -1  0 1
                                                                  0  0  0 .5 0" result="fe-toolbar-colormatrix" />

      <feMerge>
        <feMergeNode in="fe-toolbar-colormatrix" />
        <feMergeNode in="fe-toolbar-composite" />
      </feMerge>
    </filter>

So that's 7 temporary images and composite operations. I don't really understand why it's all there, but any simplifications we can do would improve performance.

Comment 26

[Stephen Horlander [:shorlander]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #25)
> I'm not surprised that filter is expensive. Here it is:
>     <filter id="filter-effects-toolbar">
>       <feOffset in="SourceGraphic" dx="0" dy="1" result="fe-toolbar-offset"
> />
>       <feFlood flood-color="black" flood-opacity=".7"
> result="fe-toolbar-flood" />
>       <feComposite operator="out" in="fe-toolbar-flood"
> in2="fe-toolbar-offset" />
>       <feGaussianBlur stdDeviation=".25" result="fe-toolbar-blur" />
>       <feComposite operator="atop" in="fe-toolbar-blur" in2="SourceGraphic"
> result="fe-toolbar-composite" />
>       
>       <feColorMatrix in="fe-toolbar-offset" type="matrix" values="1  1  1  0
> 1
>                                                                   1 -1  1  0
> 1
>                                                                   1  1 -1  0
> 1
>                                                                   0  0  0 .5
> 0" result="fe-toolbar-colormatrix" />
> 
>       <feMerge>
>         <feMergeNode in="fe-toolbar-colormatrix" />
>         <feMergeNode in="fe-toolbar-composite" />
>       </feMerge>
>     </filter>
> 
> So that's 7 temporary images and composite operations. I don't really
> understand why it's all there, but any simplifications we can do would
> improve performance.

It's been a while since I wrote that and I probably don't have the best grasp on SVG filters to begin with :) All of that is to achieve a few effects:

1) Inner Shadow — Dark with a slight blur
2) Drop Shadow — Light without a blur

There are more ways than one to achieve the same combination of effects, but so far I haven't found a way that seems significantly less complex.

If there is someone with a better grasp of SVG filters, or just SVG in general, I would appreciate any help with a simpler approach.

Comment 27

[Jet Villegas (inactive)]

(In reply to Jonathan Watt [:jwatt]
> Depends on: 686875, 741760, 1015996, 1016445, 1016673, 1016676, 1016680, 1016703, 1017058

I'm still not clear that:
(the sum of these bugs getting fixed) == (awesome SVG toolbar icons)

The 7 image copies that roc pointed out aren't going away with the bugs listed. Could we get the desired effect with a SVG file that has two images: 
1. the pre-rendered shadow (ie. without using feOffset, feFlood, & feComposite) with just the feGaussianBlur applied.
2. the actual icons placed on top of #1.

Comment 28

[Jonathan Watt [:jwatt]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> Can you confirm that your SVG icons render with acceptable quality at a
> variety of different sizes? Because up until now I'd assumed they wouldn't.

mconley, specifically I think roc has in mind the issues described here:

http://www.pushing-pixels.org/2011/11/04/about-those-vector-icons.html

and whether anyone has confirmed that us using SVG icons isn't going to cause us to run into the same issues and result in us reverting back to raster images. If we're going to spend time working on the performance of SVG icons we want to know that they will actually be used as a result.

Comment 29

[Jonathan Watt [:jwatt]]

(In reply to Jet Villegas (:jet) from comment #27)
> I'm still not clear that:
> (the sum of these bugs getting fixed) == (awesome SVG toolbar icons)

Yeah, fixing those bugs isn't sufficient, I was just adding things that would/could help.

Comment 30

[Blake Winton (:bwinton) (:☕️)]

(In reply to Jonathan Watt [:jwatt] from comment #28)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> > Can you confirm that your SVG icons render with acceptable quality at a
> > variety of different sizes? Because up until now I'd assumed they wouldn't.
> 
> mconley, specifically I think roc has in mind the issues described here:
> 
> http://www.pushing-pixels.org/2011/11/04/about-those-vector-icons.html
> 
> and whether anyone has confirmed that us using SVG icons isn't going to
> cause us to run into the same issues and result in us reverting back to
> raster images. If we're going to spend time working on the performance of
> SVG icons we want to know that they will actually be used as a result.

So, it seems to me that a lot of the problems they're talking about are assuming you start with a 128x128 (or larger!) icon, and then scale it down, whereas I can only assume Stephen (and Michael) would start with an 18x18px icon, and scale it up, thus avoiding that set of problems.  To further mitigate the problems they mention, we also only need to scale from 100% to 200% (Uh, on Mac and Windows.  I couldn't find an icon zoom feature in Ubuntu, nor could I find any information on what resolutions they support.)

Comment 31

[Mike Conley (:mconley) (:⚙️)]

Sorry this fell off my radar. :(

(In reply to Blake Winton (:bwinton) from comment #30)
> (In reply to Jonathan Watt [:jwatt] from comment #28)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> > > Can you confirm that your SVG icons render with acceptable quality at a
> > > variety of different sizes? Because up until now I'd assumed they wouldn't.
> > 
> > mconley, specifically I think roc has in mind the issues described here:
> > 
> > http://www.pushing-pixels.org/2011/11/04/about-those-vector-icons.html
> > 
> > and whether anyone has confirmed that us using SVG icons isn't going to
> > cause us to run into the same issues and result in us reverting back to
> > raster images. If we're going to spend time working on the performance of
> > SVG icons we want to know that they will actually be used as a result.
> 
> So, it seems to me that a lot of the problems they're talking about are
> assuming you start with a 128x128 (or larger!) icon, and then scale it down,
> whereas I can only assume Stephen (and Michael) would start with an 18x18px
> icon, and scale it up, thus avoiding that set of problems.  To further
> mitigate the problems they mention, we also only need to scale from 100% to
> 200% 

Yes, I believe this is also the case - the icons would start small, and scale up nicely.

> (Uh, on Mac and Windows.  I couldn't find an icon zoom feature in
> Ubuntu, nor could I find any information on what resolutions they support.)

The benefit for Linux GTK is that we could sample the system theme colours for the icons.

So I'd really like to move this forward somehow.

shorlander - what do you think of jet's suggestion in comment 27?

Comment 32

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

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #31)
> (In reply to Blake Winton (:bwinton) from comment #30)
> > (In reply to Jonathan Watt [:jwatt] from comment #28)
> > > mconley, specifically I think roc has in mind the issues described here:
> > > 
> > > http://www.pushing-pixels.org/2011/11/04/about-those-vector-icons.html
> > > 
> > > and whether anyone has confirmed that us using SVG icons isn't going to
> > > cause us to run into the same issues and result in us reverting back to
> > > raster images. If we're going to spend time working on the performance of
> > > SVG icons we want to know that they will actually be used as a result.
> > 
> > So, it seems to me that a lot of the problems they're talking about are
> > assuming you start with a 128x128 (or larger!) icon, and then scale it down,
> > whereas I can only assume Stephen (and Michael) would start with an 18x18px
> > icon, and scale it up, thus avoiding that set of problems.  To further
> > mitigate the problems they mention, we also only need to scale from 100% to
> > 200% 
> 
> Yes, I believe this is also the case - the icons would start small, and
> scale up nicely.

That article claims that that approach is also unsatisfactory ("icons at higher resolutions should not feel too sparse").

Can whoever's responsible for signing off on the visual quality of the Firefox UI please check a mockup with these 18x18px icons scaled up, and sign off on the quality? Is that you?

Comment 33

[Stephen Horlander [:shorlander]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #31)
> > (In reply to Blake Winton (:bwinton) from comment #30)
> > > (In reply to Jonathan Watt [:jwatt] from comment #28)
> > > > mconley, specifically I think roc has in mind the issues described here:
> > > > 
> > > > http://www.pushing-pixels.org/2011/11/04/about-those-vector-icons.html
> > > > 
> > > > and whether anyone has confirmed that us using SVG icons isn't going to
> > > > cause us to run into the same issues and result in us reverting back to
> > > > raster images. If we're going to spend time working on the performance of
> > > > SVG icons we want to know that they will actually be used as a result.
> > > 
> > > So, it seems to me that a lot of the problems they're talking about are
> > > assuming you start with a 128x128 (or larger!) icon, and then scale it down,
> > > whereas I can only assume Stephen (and Michael) would start with an 18x18px
> > > icon, and scale it up, thus avoiding that set of problems.  To further
> > > mitigate the problems they mention, we also only need to scale from 100% to
> > > 200% 
> > 
> > Yes, I believe this is also the case - the icons would start small, and
> > scale up nicely.
> 
> That article claims that that approach is also unsatisfactory ("icons at
> higher resolutions should not feel too sparse").
> 
> Can whoever's responsible for signing off on the visual quality of the
> Firefox UI please check a mockup with these 18x18px icons scaled up, and
> sign off on the quality? Is that you?

It's definitely true that SVG does not scale cleanly for highly detailed icons on standard (low) res displays. The scaling problem is made worse when you are scaling at arbitrary sizes e.g. trying to go from say 100% to say 137%. There doesn’t seem to be any concept of automatic pixel snapping in SVG to mitigate this.

SVG does scale cleanly if the artwork is flatter/simpler (like our toolbar icons), the scaling factor is 2x (or 3x, 4x, etc.), and is further mitigated by high resolution (i.e. Retina) displays. Since hi-res displays have more pixels to play with the blurring is typically not as pronounced or even noticeable at all without a magnifying glass.

I made this quick page to show how SVG scales at a few scaling factors: http://people.mozilla.org/~shorlander/bugs/svg-scaling-test/

If you look on a standard resolution display you can see that at 125% and 150% there is some blurriness but at 200%, 300% and 400% it looks clean. If you view it on a high-res display there is still a little blurriness at 125% and 150% but it’s not very noticeable.

So considering those variables it will mostly work fine for what we want to use it for. Which is scaling simple icons for use on standard and high-resolution displays. There will probably still be some cases where we will want to use specific assets or bitmaps where SVG doesn’t scale as cleanly as we would like.

As Mike mentions in comment 31 though, the reason for this bug was only partially about scaling. The primary reason for wanting to use SVG here was so that we could create icons that adapted to the system theme.

Comment 34

[Stephen Horlander [:shorlander]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #31)
> shorlander - what do you think of jet's suggestion in comment 27?

That might work. I have a few different variations I would like to try to find some kind of compromise between complexity and desired appearance.

Comment 35

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

One thing that I only just realized is that the attached Toolbar.svg test is a bit misleading because it renders on a white background, but the outer shadow is basically invisible on white backgrounds.

My eyesight's pretty bad, but FWIW the blur on the inner shadow is basically invisible to me without massive zooming.

Comment 36

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

Here's my analysis of how much we could reasonably improve the software filter code to speed up this filter:

First we can replace
      <feFlood flood-color="black" flood-opacity=".7" result="fe-toolbar-flood" />
      <feComposite operator="out" in="fe-toolbar-flood" in2="fe-toolbar-offset" />
with the equivalent
      <feColorMatrix in="fe-toolbar-offset" type="matrix" values="0 0 0 0    0
                                                                  0 0 0 0    0
                                                                  0 0 0 0    0
                                                                  0 0 0 -0.7 0.7"/>
which should be faster since it avoids a copy.

feColorMatrix commutes with feGaussianBlur. Swapping the above colorMatrix to happen after the Gaussian blur lets us do more simplifications, giving:

<feOffset in="SourceGraphic" dx="0" dy="1" result="fe-toolbar-offset" />
<feGaussianBlur stdDeviation="0.25" result="fe-toolbar-blur" />
<feColorMatrix in="fe-toolbar-offset" type="matrix" values="0 0 0 0    0
                                                            0 0 0 0    0
                                                            0 0 0 0    0
                                                            0 0 0 -0.7 0.7" result='"fe-toolbar-blur"/>
<feComposite operator="atop" in="fe-toolbar-blur" in2="SourceGraphic" result="fe-toolbar-composite"/>
<feColorMatrix in="fe-toolbar-offset" type="matrix" values="1  1  1  0 1
                                                            1 -1  1  0 1
                                                            1  1 -1  0 1
                                                            0  0  0 .5 0" result="fe-toolbar-colormatrix"/>
<feMerge>
  <feMergeNode in="fe-toolbar-colormatrix" />
  <feMergeNode in="fe-toolbar-composite" />
</feMerge>

There are some optimizations we could do inside Gecko to speed this up, without an enormous amount of work:
1) Allow lifting of feOffset primitives into their consumers. In this filter, we'd pad the temporary SourceGraphic buffer with an extra row of pixels above, and then pass into feGaussianBlur and the second feColorMatrix a different offset into that buffer, getting the feOffset "for free".
2) Optimize feGaussianBlur for the case where only its output's alpha channel is used (as in this case, and generally common for drop shadows), generating an A8 surface and doing only a single AlphaBoxBlur.
3) Extend FilterNodeCompositeSoftware::Render as used by feComposite here to support an input A8 surface A, an input surface B, and an input colormatrix M, and compute M(A) OP B in a single pass.
4) Extend FilterNodeCompositeSoftware::Render as used by feMerge here to support input surfaces A and B, and an input colormatrix M, and computes M(A) OP B. When this is the last use of B, steal B's buffer and compute the result in-place.

If we did all those, I think the filter implementation would boil down to:
a) render SourceGraphic to a padded temporary buffer
b) allocate temp A8 surface and perform feGaussianBlur, reading directly from SourceGraphic (optimizations #1 and #2) and doing a single alpha blur.
c) allocate temp surface for 'fe-toolbar-composite' and render it (optimization #3).
d) render feMerge result, reusing feOffseted-SourceGraphic buffer (optimizations #1 and #4).

Of course, some or all of these optimizations might not be worth doing.

It might also be worth investigating if, for this very small Gaussian standard deviation, we could do less work than the full 3 box blurs in each direction without loss of quality. E.g. we could use an explicit convolution kernel in each direction, or maybe get good enough results by using just 1 or 2 box blurs.

One more thing that I think could speed things up significantly: reorganize the Toolbar.svg file so that all icons subject to the same filter are in a single <g> with the filter applied to the whole lot. That should significantly reduce the filter processing overhead, and these icons are small enough that we shouldn't blow out the cache processing them all at once. That should definitely help a lot when we're using D2D GPU filtering.

Comment 37

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

Actually someone should try the last item first, since it's just possible that will give a significant improvement with very little work.

Comment 38

[Mike Conley (:mconley) (:⚙️)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #37)
> Actually someone should try the last item first, since it's just possible
> that will give a significant improvement with very little work.

I'm guessing that's this part:

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36)
> 
> One more thing that I think could speed things up significantly: reorganize
> the Toolbar.svg file so that all icons subject to the same filter are in a
> single <g> with the filter applied to the whole lot. That should
> significantly reduce the filter processing overhead, and these icons are
> small enough that we shouldn't blow out the cache processing them all at
> once. That should definitely help a lot when we're using D2D GPU filtering.

Is that something you could put together, shorlander?

Comment 39

[Stephen Horlander [:shorlander]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #38)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #37)
> > Actually someone should try the last item first, since it's just possible
> > that will give a significant improvement with very little work.
> 
> I'm guessing that's this part:
> 
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36)
> > 
> > One more thing that I think could speed things up significantly: reorganize
> > the Toolbar.svg file so that all icons subject to the same filter are in a
> > single <g> with the filter applied to the whole lot. That should
> > significantly reduce the filter processing overhead, and these icons are
> > small enough that we shouldn't blow out the cache processing them all at
> > once. That should definitely help a lot when we're using D2D GPU filtering.
> 
> Is that something you could put together, shorlander?

This is still on my radar, have just been busy with a few other projects.

Comment 40

[Stephen Horlander [:shorlander]]

(In reply to Stephen Horlander [:shorlander] from comment #39)
> This is still on my radar, have just been busy with a few other projects.

I have been working on this off and on. After a lot of thought and some discussion with various people I would like to change this bug into something a little different.

There are a lot of problems with the way we currently create and use assets. I think we should fix that in a more permanent and thought out process. Being able to use SVG, and how we are able to use it, is a key component of that.

I created a Google doc that covers an initial meeting we had to discuss it. See the "The Problem:" section for the Why, the "Goals:" section for the What and the "The Plan™:" section for the How.

https://docs.google.com/document/d/1lwKOHavo-ngB473LyW1WH9abUt-Jv_hBEwntEeZwn-o/edit?usp=sharing

This is my proposal, I would love to hear ideas on the approach. Also ideas on any ways to solve the items list in the Problem section of that doc.

I have two patches I will attach to this bug. One that just replaces Toolbar.png with Toolbar.svg with some modifications to the structure and the filter for perf testing. And another that is a proof-of-concept for what I laid out in the above document.

Comment 41

[Stephen Horlander [:shorlander]]

Attachment: SVG Toolbar Assets - Experiment #1 - Sprite document; SVG drop shadow filter on <g> wraping all icons; gradient fill for one icon

This patch replaces Toolbar.png with a similar SVG based sprite.

I cleaned up the filter a bit and nested the icons in a group as roc suggested.

Comment 42

[Stephen Horlander [:shorlander]]

Attachment: SVG Toolbar Assets - Experiment #2 - Individual SVG images; coloring SVG filter, inner shadow SVG filter and CSS drop-shadow filter applied to each embedding element; no gradient fills; extraneous -moz-image-region props

This patch adds individual flat black SVG icons for all of the Toolbar icons and styles them externally with a referenced CSS filter.

Comment 43

[:Gijs (he/him)]

The plan seems sensible to me. Obviously, whether it works as-is would depend on whether the SVG rendering perf hit is not too big. I don't have an informed opinion about that.

If the SVG rendering perf hit is non-trivial, it should still be possible to reuse this process and automatically generate the multitude of spritesheets as PNGs, automatically generate the relevant CSS, and ship the rasterized images instead.

Comment 44

[Blake Winton (:bwinton) (:☕️)]

(In reply to :Gijs Kruitbosch from comment #43)
> If the SVG rendering perf hit is non-trivial, it should still be possible to
> reuse this process and automatically generate the multitude of spritesheets
> as PNGs, automatically generate the relevant CSS, and ship the rasterized
> images instead.

Indeed, I have some code that does something quite similar to that at https://github.com/bwinton/svg-magic  :)

Comment 45

[Stephen Horlander [:shorlander]]

(In reply to :Gijs Kruitbosch from comment #43)
> If the SVG rendering perf hit is non-trivial, it should still be possible to
> reuse this process and automatically generate the multitude of spritesheets
> as PNGs, automatically generate the relevant CSS, and ship the rasterized
> images instead.

We have discussed this, and as Blake mentions, done some work around it. This would certainly be a huge step up from where we are currently.

There are a few reasons I am interested in using SVG and some sort of dynamic process:

1) It allows us to scale them in different contexts without generating different bitmaps, so we could for example not need a different set of PNGs for different screen resolutions
  - This also future proofs our assets some as screens change

2) It opens up opportunities for more comprehensive theming (e.g. authors provide a color and we can style all of the icons for them)

3) Perhaps most importantly is that it provides a way for add-on authors to supply a very simple set of scalable graphics that would "just work" and fit in with our styling on different platforms

Assuming the situation where SVG won't work, would it be possible to render the SVG out to PNG on the front-end also? For example there could be automated pre-rendering of our known assets at build time, but add-on authors could still provide a simple SVG that could be rendered to an appropriately styled PNG and cached on the front-end?

Similarly could we do the same thing for theming where providing a color would re-render the icons and output a PNG?

Comment 46

[:Gijs (he/him)]

(In reply to Stephen Horlander [:shorlander] from comment #45)
> Assuming the situation where SVG won't work, would it be possible to render
> the SVG out to PNG on the front-end also? For example there could be
> automated pre-rendering of our known assets at build time, but add-on
> authors could still provide a simple SVG that could be rendered to an
> appropriately styled PNG and cached on the front-end?
> 
> Similarly could we do the same thing for theming where providing a color
> would re-render the icons and output a PNG?

In theory yes, but I don't know that/how we could do that without a flash of unstyled content type interruption on the first start where whatever our expectations are, and whatever we have cached, no longer match.

Comment 47

[Stephen Horlander [:shorlander]]

(In reply to :Gijs Kruitbosch from comment #46)
> (In reply to Stephen Horlander [:shorlander] from comment #45)
> > Assuming the situation where SVG won't work, would it be possible to render
> > the SVG out to PNG on the front-end also? For example there could be
> > automated pre-rendering of our known assets at build time, but add-on
> > authors could still provide a simple SVG that could be rendered to an
> > appropriately styled PNG and cached on the front-end?
> > 
> > Similarly could we do the same thing for theming where providing a color
> > would re-render the icons and output a PNG?
> 
> In theory yes, but I don't know that/how we could do that without a flash of
> unstyled content type interruption on the first start where whatever our
> expectations are, and whatever we have cached, no longer match.

The case I am thinking of would be the install time case. Installing an add-on, changing a theme, etc; where you would expect the things to be happening and and it wouldn't be separability distracting. We would have to keep the generated images between restarts.

Comment 48

[:Gijs (he/him)]

(In reply to Stephen Horlander [:shorlander] from comment #47)
> (In reply to :Gijs Kruitbosch from comment #46)
> > (In reply to Stephen Horlander [:shorlander] from comment #45)
> > > Assuming the situation where SVG won't work, would it be possible to render
> > > the SVG out to PNG on the front-end also? For example there could be
> > > automated pre-rendering of our known assets at build time, but add-on
> > > authors could still provide a simple SVG that could be rendered to an
> > > appropriately styled PNG and cached on the front-end?
> > > 
> > > Similarly could we do the same thing for theming where providing a color
> > > would re-render the icons and output a PNG?
> > 
> > In theory yes, but I don't know that/how we could do that without a flash of
> > unstyled content type interruption on the first start where whatever our
> > expectations are, and whatever we have cached, no longer match.
> 
> The case I am thinking of would be the install time case. Installing an
> add-on, changing a theme, etc; where you would expect the things to be
> happening and and it wouldn't be separability distracting. We would have to
> keep the generated images between restarts.

These things don't necessarily happen during a browser run, and aren't always user initiated.

Another issue would be that we couldn't really store these in the app dir (specifically inside omni.ja) because of incremental updates and such, and so it might quite seriously impact startup IO + time.

I guess the main point would be that doing this at build-time would provide a lot fewer headaches than doing this at run-time and redoing it whenever some factor changed.

Comment 49

[Jonathan Kingston [:jkt] he/him]

Was playing about with this on the plane as had time to kill, the toolbar.png doesn't have a corresponding 2x for Linux so for quite some time I have thought it looked tired. Annoyingly some icons like Lightbeam and Hello look much sharper which makes the issue worse.

I would suggest using SVG as source images at build time would be a great first step, not only does it make the icons more maintainable it solves the linux icon issue. I can pick up the first step if you are busy Stephen? (I'm considering just pushing a 2x patch for now as I didn't have the correct source images)

Comment 50

[Justin Dolske [:Dolske]]

Comment on attachment 8728034 [details] [diff] [review]
SVG Toolbar Assets - Experiment #1 - Sprite document; SVG drop shadow filter on <g> wraping all icons; gradient fill for one icon

Without thinking about it too deeply, this seems like a sensible step to me.

I do wonder a little bit about what we want to do (ideally or in the long-term) w.r.t. this being a spritesheet. I wonder if it still has any impact on perf (now that we're in an omnijar world). But we can consider this separately -- I don't think there's any harm in keeping it, other than it probably being an annoyance to regenerate the whole thing when just 1 icon changes.

Comment 51

[Justin Dolske [:Dolske]]

I did some try pushes to take a peek a perf, this is on current m-c tip, changeset 3e04659fdf6a. Null is without any patches, Exp1 is with attachment 8728034 [details] [diff] [review], Exp2 is with attachment 8728039 [details] [diff] [review].

Null: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cb8a5c02f71
Exp1: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2354abf1ad2a
Exp2: https://treeherder.mozilla.org/#/jobs?repo=try&revision=38a0e2fde1bb

Comment 52

[:Gijs (he/him)]

(In reply to Justin Dolske [:Dolske] from comment #51)
> I did some try pushes to take a peek a perf, this is on current m-c tip,
> changeset 3e04659fdf6a. Null is without any patches, Exp1 is with attachment
> 8728034 [details] [diff] [review], Exp2 is with attachment 8728039 [details] [diff] [review]
> [diff] [review].
> 
> Null: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cb8a5c02f71
> Exp1: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2354abf1ad2a
> Exp2: https://treeherder.mozilla.org/#/jobs?repo=try&revision=38a0e2fde1bb

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=6cb8a5c02f71&newProject=try&newRevision=2354abf1ad2a&framework=1&showOnlyImportant=0

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=6cb8a5c02f71&newProject=try&newRevision=38a0e2fde1bb&framework=1&showOnlyImportant=0

Both of these regress ts_paint and sessionrestore, and the second also regresses tpaint. All of this is on linux64, still waiting on retriggers for osx and windows to come through, but it seems likely they'll show similar patterns.

Comment 53

[Stephen Horlander [:shorlander]]

(In reply to :Gijs Kruitbosch from comment #52)
> Both of these regress ts_paint and sessionrestore, and the second also
> regresses tpaint. All of this is on linux64, still waiting on retriggers for
> osx and windows to come through, but it seems likely they'll show similar
> patterns.

So, it looks kind of bad, but I don't have a good sense of what the test is measuring in terms of perceived perf.

What would be a good next step? Can we improve perf? Think of a different way to do it?

Comment 54

[:Gijs (he/him)]

(In reply to Stephen Horlander [:shorlander] from comment #53)
> (In reply to :Gijs Kruitbosch from comment #52)
> > Both of these regress ts_paint and sessionrestore, and the second also
> > regresses tpaint. All of this is on linux64, still waiting on retriggers for
> > osx and windows to come through, but it seems likely they'll show similar
> > patterns.
> 
> So, it looks kind of bad, but I don't have a good sense of what the test is
> measuring in terms of perceived perf.
> 
> What would be a good next step? Can we improve perf? Think of a different
> way to do it?

I don't know enough about SVG and how it's currently optimized to be able to suggest much beyond the previously discussed "switch to using pre-built PNGs for all the things".

The only thing I noticed is that the SVG in the patch uses a filter - is that necessary? Does removing the filter fix any of this? It also looks like it uses a compositing matrix to achieve a .8 alpha for the shadow, which feels like it could be done by just picking a semi-transparent color instead?

Maybe dholbert has better suggestions?

Comment 55

[Daniel Holbert [:dholbert]]

(Punting my needinfo to jwatt, as he's already involved here and has more context & a better handle on the perf impacts of various svg features than I do.)

(FWIW, I agree with Gijs that it'd be worth testing without any SVG Filters (and perhaps without SVG gradients as well), to see if that helps from a perf perspective. Not that we'd land in that configuration, just from a diagnosing-the-perf-hit perspective.)

Comment 56

[Stephen Horlander [:shorlander]]

(In reply to :Gijs Kruitbosch from comment #54)
> The only thing I noticed is that the SVG in the patch uses a filter - is
> that necessary? Does removing the filter fix any of this? It also looks like
> it uses a compositing matrix to achieve a .8 alpha for the shadow, which
> feels like it could be done by just picking a semi-transparent color instead?

As I mentioned in the linked document (https://docs.google.com/document/d/1lwKOHavo-ngB473LyW1WH9abUt-Jv_hBEwntEeZwn-o/edit?usp=sharing) one of the ideas behind using SVG was to make it adaptable. E.g. we provide a shape and Firefox provides the color. This would make it easier to style things per platform and to maintain the assets. I don't see a way to do that without a filter, but maybe there is a way?

There are also considerations around using filters for effects such as a drop-shadow. While it's possible that we might not even want to do that right now, I don't think that will always be the case.

Another consideration for SVG perf beyond to the main Toolbar: We are currently using more SVG is other parts of the browser chrome, parts that might not be triggering test regressions.

Comment 57

[:Gijs (he/him)]

(In reply to Stephen Horlander [:shorlander] from comment #56)
> (In reply to :Gijs Kruitbosch from comment #54)
> > The only thing I noticed is that the SVG in the patch uses a filter - is
> > that necessary? Does removing the filter fix any of this? It also looks like
> > it uses a compositing matrix to achieve a .8 alpha for the shadow, which
> > feels like it could be done by just picking a semi-transparent color instead?
> 
> As I mentioned in the linked document
> (https://docs.google.com/document/d/1lwKOHavo-ngB473LyW1WH9abUt-
> Jv_hBEwntEeZwn-o/edit?usp=sharing) one of the ideas behind using SVG was to
> make it adaptable. E.g. we provide a shape and Firefox provides the color.
> This would make it easier to style things per platform and to maintain the
> assets. I don't see a way to do that without a filter, but maybe there is a
> way?

Well, it depends what you're coloring (I'm not sure, from that doc?), and if a flat color is enough or if you need a gradient. The fill color of the glyph is in CSS in the images you've attached, not in the filter. Is that not what you meant, and if so, what did you mean?

> There are also considerations around using filters for effects such as a
> drop-shadow. While it's possible that we might not even want to do that
> right now, I don't think that will always be the case.

Sure, I'm just saying that it looks to me like the drop-shadow filter could be simplified, if nothing else, and that it's possible that there would be less of a perf issue without it.

> Another consideration for SVG perf beyond to the main Toolbar: We are
> currently using more SVG is other parts of the browser chrome, parts that
> might not be triggering test regressions.

Sure. I'm not saying we shouldn't improve perf, but it'd be useful to understand what is causing the bad perf to begin with, and then we can look at what solutions are open to us. One thing to check in that department would be if the filters are involved or not. See also comment #55 .

Comment 58

[Stephen Horlander [:shorlander]]

(In reply to :Gijs Kruitbosch from comment #57)
> (In reply to Stephen Horlander [:shorlander] from comment #56)
> > (In reply to :Gijs Kruitbosch from comment #54)
> > > The only thing I noticed is that the SVG in the patch uses a filter - is
> > > that necessary? Does removing the filter fix any of this? It also looks like
> > > it uses a compositing matrix to achieve a .8 alpha for the shadow, which
> > > feels like it could be done by just picking a semi-transparent color instead?
> > 
> > As I mentioned in the linked document
> > (https://docs.google.com/document/d/1lwKOHavo-ngB473LyW1WH9abUt-
> > Jv_hBEwntEeZwn-o/edit?usp=sharing) one of the ideas behind using SVG was to
> > make it adaptable. E.g. we provide a shape and Firefox provides the color.
> > This would make it easier to style things per platform and to maintain the
> > assets. I don't see a way to do that without a filter, but maybe there is a
> > way?
> 
> Well, it depends what you're coloring (I'm not sure, from that doc?), and if
> a flat color is enough or if you need a gradient. The fill color of the
> glyph is in CSS in the images you've attached, not in the filter. Is that
> not what you meant, and if so, what did you mean?

In Experiment #1 the color and opacity is set in the CSS:

>      fill: ButtonText;
>      fill-opacity: .85;
>      filter: url(#filter-effects-toolbar);

The filter effect is for the drop-shadow. The gradient is only applied to one shape, not all of the toolbar icons.

In Experiment #2 the filter is specifically for applying color only to the black areas of the provided shape:

>  <filter id="colorOverlay-toolbar">
>    <feColorMatrix in="SourceGraphic"
>      type="matrix"
>      values=".92 0 0 .08 0
>              .92 0 0 .08 0
>              .92 0 0 .08 0
>                0 0 0   1 0" />
>  </filter>

and

>[cui-areatype="toolbar"] .toolbarbutton-icon,
>[cui-areatype="toolbar"] .toolbarbutton-menubutton-dropmarker > .dropmarker-icon,
>#nav-bar-overflow-button .toolbarbutton-icon,
>#PanelUI-menu-button .toolbarbutton-icon {
>  filter: url("chrome://browser/skin/toolbar-effects.svg#colorOverlay-toolbar") url("chrome://browser/skin/toolbar-effects.svg#innerShadow") drop-shadow(0 1px 0 hsla(0,0%,100%,.5));
>}

Anyway, I will make another patch without gradients or filters.

Comment 59

[Stephen Horlander [:shorlander]]

Attachment: SVG Toolbar Assets - Experiment #3 - Same as #1, but without the filters or the gradient

Same as #1 (replaces Toolbar.png with Toolbar.svg) but removes SVG gradients and filters.

Comment 60

[Stephen Horlander [:shorlander]]

Attachment: SVG Toolbar Assets - Experiment #4 - Same as #2, but without the filters (or gradients)

Same as #2 (Adds individual SVG files for each toolbar icon) but removes SVG gradients and filters.

Comment 61

[:Gijs (he/him)]

Off the same parent as the earlier trypushes so we can compare correctly:

#3: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8f478cf724c

#4:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa423a0ff755

Comment 62

[Stephen Horlander [:shorlander]]

#3 — https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=6cb8a5c02f71&newProject=try&newRevision=c8f478cf724c&framework=1&showOnlyImportant=0

#4 — https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=6cb8a5c02f71&newProject=try&newRevision=aa423a0ff755&framework=1&showOnlyImportant=0

Comment 63

[Justin Dolske [:Dolske]]

Looks better at a glance -- sessionrestore opt is 3.5% slower on Linux, but looking at the mozilla-inbound graph, it's noisy enough that it might not be real.

This does illustrate a downside of using SVG directly -- the actual content/style of our image assets can now affect things like performance/memory/CPU. My comment 50 ponder about getting rid of the spritesheet might be worth thinking about more, since I assume we're paying the cost of rendering everything in Toolbar.svg instead of just what's needed.

What's the next step here? If it's the gradients/filters that are slow, is there a better way to optimize the SVG? Is this something that might be fixable in platform, or it just inherently expensive and we'll need to work around it somehow?

Comment 64

[Mike Conley (:mconley) (:⚙️)]

It's possible we've talked about this before, but I wonder if a one-time rasterization could be done from the SVG during the first start, which is then cached for subsequent browser starts. We'd need to invalidate that cache if the user changes display settings, themeing, etc, which might be tricky. Just something I thought I'd throw down.

Comment 65

[:Gijs (he/him)]

FWIW, it doesn't look like any of the #3 and #4 talos runs were retriggered. Doing that now.

(In reply to Justin Dolske [:Dolske] from comment #63)
> This does illustrate a downside of using SVG directly -- the actual
> content/style of our image assets can now affect things like
> performance/memory/CPU. My comment 50 ponder about getting rid of the
> spritesheet might be worth thinking about more, since I assume we're paying
> the cost of rendering everything in Toolbar.svg instead of just what's
> needed.

I think #4 and #2 use individual images? We can cross-compare those runs and see if it makes a difference. For #1 and #2, #2 was worse...

Comment 66

[Stephen Horlander [:shorlander]]

(In reply to :Gijs Kruitbosch from comment #65)
> I think #4 and #2 use individual images? We can cross-compare those runs and
> see if it makes a difference. For #1 and #2, #2 was worse...

My guess would be that #2 was worse than #1 because of the way it was using filters. But it would be interesting to compare a sprite vs. individual files.

Comment 67

[:Gijs (he/him)]

After some retriggers #3 looks bad but #4 looks fine, so I think comment #66 is probably correct. Which is annoying because between #1 and #2, the first was better, but without any filters, it seems that that ends up being worse again. Which makes me suspect that dolske's hunch in comment #63 is somewhat correct in that the larger files influence this too. I wonder if the translate() also impacts perf here and if pre-multiplying the path coordinates would be faster...

Of course, without the filters I presume that experiment 4 doesn't actually look correct? Stephen?

Comment 68

[Stephen Horlander [:shorlander]]

(In reply to :Gijs Kruitbosch from comment #67)
> Of course, without the filters I presume that experiment 4 doesn't actually
> look correct? Stephen?

That's correct. #3 and #4 were just simple shapes with a black fill. Which isn't going to be what we want usually.

Well, I say that, but in the majority of places we are using icons currently they are pretty simple. So we could define simple styles on an SVG if we know the precise context we are going to be using it in. 

Although that would mean we would need multiple similar SVGs for different parts of the UI and we wouldn't really end up in a better place WRT to maintenance or flexibility. At which point it would probably make more sense to figure out a way to use a common set of vectors to output raster images and sidestep the whole SVG perf part of the problem.

Comment 69

[:Gijs (he/him)]

(In reply to Stephen Horlander [:shorlander] from comment #68)
> (In reply to :Gijs Kruitbosch from comment #67)
> > Of course, without the filters I presume that experiment 4 doesn't actually
> > look correct? Stephen?
> 
> That's correct. #3 and #4 were just simple shapes with a black fill. Which
> isn't going to be what we want usually.
> 
> Well, I say that, but in the majority of places we are using icons currently
> they are pretty simple. So we could define simple styles on an SVG if we
> know the precise context we are going to be using it in. 
> 
> Although that would mean we would need multiple similar SVGs for different
> parts of the UI and we wouldn't really end up in a better place WRT to
> maintenance or flexibility. At which point it would probably make more sense
> to figure out a way to use a common set of vectors to output raster images
> and sidestep the whole SVG perf part of the problem.

We could potentially do a build-time generation of some of the styles and generate the relevant files that way (including colors + shadows as just bits of SVG, reducing the perf impact the filters seem to be having), but to a certain degree I guess that's not much better than pre-rendering the raster images.

What that *does* gain us is having fluent scaling across multiple resolutions, which is especially nice on Windows, where as of win10 we can go up to and over 300% dpi or whatever. Shipping lots of PNGs to deal with that isn't attractive, and you'd still see scaling artifacts on things like 125 or 110%.
It'll also gain us some download size reduction (PNGs compress like ... and SVGs compress a lot better, just being text...) and disk size reduction for omni.ja. Depending on how that's mmapped or whatever to cache those files, it might even lightly reduce runtime memory usage.

Comment 70

[Stephen Horlander [:shorlander]]

Attachment: SVG Toolbar Assets - Experiment #5 - Same as #2, but with only the drop-shadow filter applied to each embedding element

This patch uses individual SVG icons with a basic fill and the filter: drop-shadow(); no other filters.

Comment 71

[:Gijs (he/him)]

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=95e07170cf6e

Leaving needinfo to do retriggers and check perfherder.

Comment 72

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #71)
> remote:  
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=95e07170cf6e
> 
> Leaving needinfo to do retriggers and check perfherder.

These are also sadfaces, unfortunately - regressions on sessionrestore and so on.

Comment 73

[Matthew N. [:MattN]]

I've been loosely following along… thanks Gijs for the talos comparisons. I'm still traumatized from dealing with them from Australis.

A few quick comments which are probably obvious but I'm point them out there as my perspective:
a) If perf allows, the ideal situation IMO is not using a sprite sheet
** I would prefer shared CSS and .inc.svg files over a sprite sheet so our CSS is much simpler and it's easier to tweak a single image (either in m-c or as an extension).
b) Doing work on first run to rasterize images then cache them will slow startup and seems complex enough that I'm skeptical that it's a good idea. We would possibly want to rasterize multiple versions of the images e.g:
* multi dppx for multi-display situations: think dragging between display or plugging in a display later
* Regular and inverted colours - If the point of rasterizing ahead of time is for speed, should we care about speed of hovering to preview a dark LWT (it's already horrible due to network load time of the header image) or if the user switches between dark and light LWT?
* System colours: Hopefully none of our SVG icons will want to use system colours as that will add many new triggers and cache keys for invalidation.

Comment 74

[Jonathan Watt [:jwatt]]

(In reply to Justin Dolske [:Dolske] from comment #63)
> My comment 50 ponder about getting rid of the
> spritesheet might be worth thinking about more, since I assume we're paying
> the cost of rendering everything in Toolbar.svg instead of just what's
> needed.

Splitting the icons into individual SVG files will almost certainly hurt performance. bug 999931 showed us that one of the major sources of perf issues for SVG-as-an-image is that SVG is a document format and requires us to create an nsDocument etc. and that's expensive. It helps to only pay the cost once by putting all the icons in a single SVG file. If it isn't already, we should make the image code's caching smart enough that the document is only rendered once, then when icons are referenced they are simply blit'ed from the cached rendering.

> What's the next step here? If it's the gradients/filters that are slow, is
> there a better way to optimize the SVG? Is this something that might be
> fixable in platform, or it just inherently expensive and we'll need to work
> around it somehow?

mstange is the best person to ask about speeding up filters. Drop shadows should be faster using |filter: drop-shadow()| than using SVG filter. Was #5 noticeably different to the SVG filters case?

It does sound like filter performance is the big stumbling block that we may not be able to get around though.

Comment 75

[Jonathan Watt [:jwatt]]

The tests #1 - #5 are a good start to give us a general idea of what issues we have, but really we need tests that we can closely profile as we were doing in bug 999931. What would be very helpful is if we had equivalents to #1-#5 as single HTML files with an <img> element referencing one of the icons (or possibly an <img> tag for each icon). Having tests that are as close as possible to how the icons do/might load but where the test can be loaded as a page allows local profiling that can give us a much better view of where load time is being spent. We can then start answering the question of whether the time sinks are fixable or not. Can someone create those tests?

Comment 76

[Jonathan Watt [:jwatt]]

Actually to keep things as close to browser startup as possible, it would be ideal if the test files displayed the icons that are typically visible just after startup, but before the user interacts with the browser.

Comment 77

[u428464]

FWIW Chrome is now rendered with SVG instead of PNG at least on Chrome OS, but soon on Windows and OSX too : http://sebastien-gabriel.com/work/chrome-cros

Comment 78

[Stephen Horlander [:shorlander]]

(In reply to Jonathan Watt [:jwatt] from comment #76)
> Actually to keep things as close to browser startup as possible, it would be
> ideal if the test files displayed the icons that are typically visible just
> after startup, but before the user interacts with the browser.

Gijs suggested I use XUL for these tests since that is closer to the actual conditions. So you will need allow people.mozilla.org to run XUL or run it locally. Or if you think HTML is still better I can do that too (with some adjustments) :)

http://people.mozilla.org/~shorlander/bugs/Bug-1054016/svg-test-01/index.xul
http://people.mozilla.org/~shorlander/bugs/Bug-1054016/svg-test-02/index.xul
http://people.mozilla.org/~shorlander/bugs/Bug-1054016/svg-test-03/index.xul
http://people.mozilla.org/~shorlander/bugs/Bug-1054016/svg-test-04/index.xul
http://people.mozilla.org/~shorlander/bugs/Bug-1054016/svg-test-05/index.xul
http://people.mozilla.org/~shorlander/bugs/Bug-1054016/svg-test-06/index.xul

Tests description:
svg-test-01: Toolbar.svg sprite made with by translating icons + custom SVG filter
svg-test-02: Individual SVG files + some custom filters + drop-shadow filter
svg-test-03: Toolbar.svg sprite made with by translating icons NO filters
svg-test-04: Individual SVG files NO filters
svg-test-05: Individual SVG files + drop-shadow filter
svg-test-06: Single SVG file NOT a sprite; referencing paths with #target NO filters

Comment 79

[:Paolo Amadini]

Note that I'm working in bug 1274480 on landing something we can use for permission icons, which are not displayed by default. I'll add more details there, the short version is that it sounds like svg-test-06 combined with SVG filters.

Comment 80

[Stephen Horlander [:shorlander]]

(In reply to :Paolo Amadini from comment #79)
> Note that I'm working in bug 1274480 on landing something we can use for
> permission icons, which are not displayed by default. I'll add more details
> there, the short version is that it sounds like svg-test-06 combined with
> SVG filters.

I can't remember if this has come up, or if it's even possible, but one reason for using filters is to dynamically set the color of the icon (the other being any kind of shadow / special-effects). Would it be possible to programmatically change the fill color based on platform and context? Would that even be faster?

Comment 81

[Jonathan Watt [:jwatt]]

(In reply to Stephen Horlander [:shorlander] from comment #80)
> Would it be possible to programmatically change the fill color
> based on platform and context?

That's bug 1058040.

> Would that even be faster?

We're seeing that filters are impacting performance here, so avoiding them should certainly help. The cost to passing context information through to the SVG image when painting it should be negligible so it should be a straight win.

Comment 82

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #81)
> (In reply to Stephen Horlander [:shorlander] from comment #80)
> > Would it be possible to programmatically change the fill color
> > based on platform and context?
> 
> That's bug 1058040.

shorlander, if we fixed that bug, would that do away with the need for filters if we were to do a straight PNG -> SVG swap given the current icon designs. If any icons would still need to use filters for some other reason, do any of those icons end up being displayed during startup (thinking specifically of ts_paint).

Comment 83

[Jonathan Watt [:jwatt]]

(In reply to :Gijs Kruitbosch from comment #65)
> For #1 and #2, #2 was worse...

Comparing #1 and #2 isn't really useful FWIW, since #2 has two additional filter effects being applied (to color the icons, and to provide an inner shadow), and #2 could have benefited from the removal of the -moz-image-region properties but didn't.

Comment 84

[Jonathan Watt [:jwatt]]

This bug has become quite a chore to process given the number of comments, the subtleties of the details of the various "experiments", and the sometimes vague and ambiguous summaries of the profiling of the experiments. I've tried to pull out some of the more interesting information that is probably useful to keep around (even once this bug is resolved) and put it into a wiki page:

https://wiki.mozilla.org/SVG:Icons

Comment 85

[Jonathan Watt [:jwatt]]

As for the experiments so far, they've provided enough information to point us in a promising direction.

Comment 67 claimed "#4 looks fine", and actually, in combination with a fix for bug 1058040 (obviating the need to use filters for coloring the icons), should actually be fairly close to an acceptable patch. I've unbitrotted experiment #4 and removed the extraneous -moz-image-region properties and to validate the claim in comment 67 on Try.

In addition to that shorlander is working on another experiment that I've proposed. Basically the idea would be to do something similar to experiment #3, but using two separate sprite sheets. Specifically (1) separating the icons shown during startup out into a separate sprite sheet to avoid the overhead of rendering *all* the icons during startup and (2) getting rid of any coloring filters with the assumption that we could do coloring more performantly by fixing bug 1058040.

If either of these experiments looks good then we can take a serious look at fixing bug 1058040. Hopefully my claim that the cost to passing context information through to the SVG image when painting it should be negligible is close to the mark.

Comment 86

[Stephen Horlander [:shorlander]]

(In reply to Jonathan Watt [:jwatt] from comment #82)
> (In reply to Jonathan Watt [:jwatt] from comment #81)
> > (In reply to Stephen Horlander [:shorlander] from comment #80)
> > > Would it be possible to programmatically change the fill color
> > > based on platform and context?
> > 
> > That's bug 1058040.
> 
> shorlander, if we fixed that bug, would that do away with the need for
> filters if we were to do a straight PNG -> SVG swap given the current icon
> designs. If any icons would still need to use filters for some other reason,
> do any of those icons end up being displayed during startup (thinking
> specifically of ts_paint).

For all of the main toolbar icons and panel icons we can remove the special effects and not need the filters.

We do still use full color icons for the places iconography. These would likely require some gradients and maybe a blur filter or two. These show up in the sidebars which could be shown on start-up but are not by default: http://cl.ly/1806430c151f

Comment 87

[:Paolo Amadini]

(In reply to Jonathan Watt [:jwatt] from comment #84)
> https://wiki.mozilla.org/SVG:Icons

From the wiki page and the descriptions of the experiments, it seems to me the approach of a single square SVG file containing all the glyphs hasn't been tested for performance yet.

This is the approach I'm using in bug 1274480 for non-performance-sensitive icons, because it's very compact and it makes it really easy to add new glyphs. Apparently this doesn't suffer from the "Don't bundle too many icons together" issue because the canvas size doesn't increase when a new glyph is added.

It might still suffer from the inability to apply the same filter once on a larger canvas - but if most of the time required by the filter is in the image processing part, applying the filter many times on smaller canvases on an as-needed basis might end up taking the same time.

I'm also using the following filter to get the same practical effect as bug 1058040 for the "fill" property on monochrome icons, though probably in a much less performant way:

<feComposite in="FillPaint" in2="SourceGraphic" operator="in"/>

This filter may however be faster than the others, being a single composite operation.

Comment 88

[Jonathan Watt [:jwatt]]

(In reply to :Paolo Amadini from comment #87)
> From the wiki page and the descriptions of the experiments, it seems to me
> the approach of a single square SVG file containing all the glyphs hasn't
> been tested for performance yet.

I wasn't quite clear what you meant from that description, but from looking at your patch it seems like you're using the default approach of the script I wrote at:

https://github.com/fxos-components/fxos-icons/blob/master/bin/create-svg-icon-set-file.py

I.e. to stack all the icons over one another and use CSS to hide all of them except the icon targeted by the the fragment identifier in the URI the SVG is accessed by. I've referred to this as an "icon stack" elsewhere FWIW. The problem with this approach is that it currently results in the worst of both worlds in terms of performance. ImageLib currently creates separate SVG document instances for different fragment identifiers, so you end up with one document per icon _and_ those documents contain all the additional icons making them more expensive (even though you don't paint them, there's still parsing and other costs). Bug 1027106 covers fixing this. Anyway, this is why we currently lay the icons out beside each other in the sprite document and use -moz-image-region/-moz-image-rect to show only the icon we want. ImageLib has optimizations for this approach which result in only one SVG document instance being created.

> This is the approach I'm using in bug 1274480 for non-performance-sensitive
> icons, because it's very compact and it makes it really easy to add new
> glyphs.

It also has the advantage that icons can be referenced by name rather than by a clip area.

> Apparently this doesn't suffer from the "Don't bundle too many icons
> together" issue because the canvas size doesn't increase when a new glyph is
> added.

It's not so much about canvas size as it is about the number of icons that need to be rendered. Rendering time is reduced because all but one icon is hidden.

> It might still suffer from the inability to apply the same filter once on a
> larger canvas - but if most of the time required by the filter is in the
> image processing part, applying the filter many times on smaller canvases on
> an as-needed basis might end up taking the same time.

Right now the expense of filters seems to pretty much out-weight everything else in the cases we've tested here.

> I'm also using the following filter to get the same practical effect as bug
> 1058040 for the "fill" property on monochrome icons, though probably in a
> much less performant way:
> 
> <feComposite in="FillPaint" in2="SourceGraphic" operator="in"/>
> 
> This filter may however be faster than the others, being a single composite
> operation.

This method of using a filter to color an icon is certainly more readable than the use of feColorMatrix, and should be faster too.

Comment 89

[Jonathan Watt [:jwatt]]

Attachment: SVG Toolbar Assets - Experiment #4v2 - Same as #2, but without the filters (or gradients) or extraneous -moz-image-region props

Comment 90

[Stephen Horlander [:shorlander]]

Attachment: SVG Toolbar Assets - Experiment #6 - Toolbar.svg, Menu.svg and Menu-small.svg

This patch adds three SVG based sprites: Toolbar.svg, Menu.svg and Menu-small.svg.

As suggested in Comment 85 this should split the files based whether or not they are likely to load at start-up:

- Toolbar.svg covers everything in the main window by default and anything the user may have customized into it.
- Menu.svg and Menu-small.svg contains common icons that can be found in the menu panel that aren't visible at start-up but are common secondary UI.

Also working on a patch with a slightly different segmentation of icons.

Comment 91

[Stephen Horlander [:shorlander]]

Attachment: SVG Toolbar Assets - Experiment #7 - Icons-Primary-16.svg, Icons-Secondary-16.svg and Icons-Secondary-32.svg

Mostly like Experiment #6. The difference is that the icons bundled in Icon-Primary-16.svg are only the default icons. It excludes icons that might be customized.

I don't think this is the right approach since we can't know the start-up state beyond the default, but it might be worth testing.

Comment 92

[Jonathan Watt [:jwatt]]

Thanks, Stephen. The extension in experiments #6 and #7 to convert even more icons to SVG makes it difficult to make direct performance comparisons to experiment #4v2 unfortunately, but this is still useful.

Comment 93

[Jonathan Watt [:jwatt]]

Experiment #4v2:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=d151ca70facd&newProject=try&newRevision=bc623f290919&framework=1&showOnlyImportant=0

Looks good, which seems to confirms Gijs claim in comment 67.


Experiment #6:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=d151ca70facd&newProject=try&newRevision=18efeb4987c9&framework=1&showOnlyImportant=0

Some minor (2-3%) sessionrestore opt regressions, but other than that it looks good.


Experiment #7:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=d151ca70facd&newProject=try&newRevision=c760e4a63a57&framework=1&showOnlyImportant=0

Looks good.


So I'll get started on bug 1058040.

Comment 94

[Dão Gottwald [:dao]]

Bug 1297806 replaced menuPanel.png and menuPanel-small.png with SVGs. It looks like this improved talos cart numbers even beyond the original regression that the bug was filed for.

Comment 95

[Dão Gottwald [:dao]]

I'm told this is currently blocked by bug 1058040. Why is that? It seems to me that we should be able to move forward here without that. Context paint would be handy and allow us to simplify things further, but we can do that later as a followup once bug 1058040 is ready.

Comment 96

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #95)
> I'm told this is currently blocked by bug 1058040. Why is that? It seems to
> me that we should be able to move forward here without that. Context paint
> would be handy and allow us to simplify things further, but we can do that
> later as a followup once bug 1058040 is ready.

I think the main reason right now is that if we use SVG today and want to avoid a perf hit, we have to duplicate the SVGs in order to have them in different colours/shades for different OSes, meaning it would not really simplify/help our CSS/imagery system as-is, as we'd need separate rules for every button on every OS variation (esp. on Windows). Can you clarify if you think that that would still be better than the current .png system, or if you think there is an alternative way to use SVG here, or something else?

Comment 97

[Dão Gottwald [:dao]]

We need three variants, one using -moz-fieldtext, a white one and a black one. We can have the shapes in a single file and re-use them in three SVGs via #include. We can also keep using sprites for the time being (like I did in bug 1297806), such that we can switch from one SVG to the other without having extra rules for all buttons.

Comment 98

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #97)
> We need three variants, one using -moz-fieldtext, a white one and a black
> one.

I'm confused. The current icons don't use -moz-fieldtext or black on OS X or Windows. Are you suggesting to only use this on Linux? Or change the icon colors on Windows/OS X? Or something else?

Comment 99

[Dão Gottwald [:dao]]

Should have been -moz-dialogtext, not -moz-fieldtext, but yes, I am assuming that we are going to pick up the text color from the toolbar (with an opacity <1 in order let the toolbar's -moz-dialog hue shine through). This is what I did in bug 1297806.

Comment 100

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #99)
> Should have been -moz-dialogtext, not -moz-fieldtext, but yes, I am assuming
> that we are going to pick up the text color from the toolbar (with an
> opacity <1 in order let the toolbar's -moz-dialog hue shine through). This
> is what I did in bug 1297806.

Changing the colours of the icons to be flat (some currently use gradients) and the same colour as text in the toolbar seems like a significant change that needs UX approval, and off-hand I don't understand why we would want to do that, besides "it's the only way we can currently use SVG without causing perf regressions", which doesn't seem like a very strong reason to me.

Comment 101

[Dão Gottwald [:dao]]

As I understand it we could have a shadow or gradient on top of the fill color. In any case, I'm not going to create the SVGs myself, we'd get them from Stephen, which already implies UX approval.

Comment 102

[Stephen Horlander [:shorlander]]

(In reply to Dão Gottwald [:dao] from comment #95)
> I'm told this is currently blocked by bug 1058040. Why is that? It seems to
> me that we should be able to move forward here without that. Context paint
> would be handy and allow us to simplify things further, but we can do that
> later as a followup once bug 1058040 is ready.

Two reasons:

1) Performance

Jonathan tested all of the various toolbar SVG approaches and the one that showed the most promise was individual SVGs with a simple color fill. I agree we could do that now with a bunch of separate SVG files. Which leads to the second reason:


2) Maintenance and Implementation

As I outlined in Comment 40 and the linked Google Doc I would like to arrive at a process that passes the perf barrier but that also gives us a better system than the cumbersome one we currently have. 

Converting everything to an SVG process is potentially going to be a lot work. If we are going to do it I would like to do it right the first time. Or as right as possible anyway :)

My big concern with doing an initial step is that it would require producing a bunch of slightly different SVGs. Which IMO doesn't move us in the right direction. Once bug 1058040 is fixed we can have one set of SVG files and we can handle all of the styling in browser.css by context; whether that means we need color keywords (-moz-dialogtext, highlight, etc.) or if we want to select a different hardcoded color.

Comment 103

[Jonathan Watt [:jwatt]]

I noticed in bug 1347543 comment 13 that the patch in that bug leaves Toolbar.png in place along with many references to it in the CSS. I *think* in that case the new CSS referencing the new SVG icons may be overriding all the relevant Toolbar.png references, preventing Toolbar.png from being loaded during startup.

However, there may be a problem in this bug. On further examination it seems that there are problems with some/most of this bug's "Experiment" patches.

#1 - only loads SVG for the bookmarks "dropmarker-icon"
#2 - the bookmarks "dropmarker-icon" still uses Toolbar.png?
#3 - only loads SVG for the bookmarks "dropmarker-icon"
#4 - the bookmarks "dropmarker-icon" still uses Toolbar.png?
#5 - the bookmarks "dropmarker-icon" still uses Toolbar.png?
#6 - maybe okay
#7 - maybe okay

If Toolbar.png was being loaded at startup for any of these experiments, the Talos results for those experiments were worse than they should have been. We would then have been loading Toolbar.png (which, given the way that it's loaded involves rendering *all* the icons inside it) *PLUS* the SVG icons. Obviously that's pretty much guaranteed to render more slowly.

Comment 104

[Stephen Horlander [:shorlander]]

So we won't know the real impact of this change until we have a final patch replacing all of the icons with SVG versions and removing all PNG versions?

Comment 105

[Jonathan Watt [:jwatt]]

See my latest comment in bug 1347543 comment 16 - I think the Talos numbers we got over in that bug are probably okay.

As for the experiments in this bug, I'm doubtful the data we got for the experiments is reliable, able to inform us about the performance change of switching to SVG, or able to inform us about which method of using SVG icons performs best.

Comment 106

[Jonathan Watt [:jwatt]]

I don't think we'd want to do any more investigation in this bug. Let's close this one and if any further investigating needs to be done we can do that in bug 1347543 or a new bug.