Last Comment Bug 752918 - Convert expensive SVG masks to clip-paths
: Convert expensive SVG masks to clip-paths
Status: RESOLVED FIXED
[Snappy]
: perf
Product: Firefox
Classification: Client Software
Component: Theme (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 15
Assigned To: Jet Villegas (:jet)
:
Mentors:
: 589983 (view as bug list)
Depends on: 828073
Blocks: tabswitch
  Show dependency treegraph
 
Reported: 2012-05-08 08:19 PDT by Jet Villegas (:jet)
Modified: 2013-01-11 02:22 PST (History)
26 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch converts SVG masks to clipPath elements. Need profiling. (8.50 KB, patch)
2012-05-08 08:19 PDT, Jet Villegas (:jet)
roc: feedback+
Details | Diff | Splinter Review
Final patch per feedback comments. r=dao (12.33 KB, patch)
2012-05-10 05:40 PDT, Jet Villegas (:jet)
no flags Details | Diff | Splinter Review
screenshot of winstripe-urlbar-back-button-mask (above) and ...-clip-path (below) (4.59 KB, image/png)
2012-05-10 14:00 PDT, Dão Gottwald [:dao]
no flags Details
Patch converts SVG masks to clipPath elements. (12.88 KB, patch)
2012-05-11 02:59 PDT, Jet Villegas (:jet)
no flags Details | Diff | Splinter Review
This patch reduces precision for less verbose SVG code. (12.50 KB, patch)
2012-05-11 04:04 PDT, Jet Villegas (:jet)
dao+bmo: review+
Details | Diff | Splinter Review

Description Jet Villegas (:jet) 2012-05-08 08:19:37 PDT
Created attachment 621984 [details] [diff] [review]
Patch converts SVG masks to clipPath elements. Need profiling.

per Roc: I think we might also be able to replace the SVG mask with an SVG clip-path. clip-path should be significantly more efficient since it doesn't require per-pixel luminance calculations.
Comment 1 :Ehsan Akhgari 2012-05-08 08:22:48 PDT
Comment on attachment 621984 [details] [diff] [review]
Patch converts SVG masks to clipPath elements. Need profiling.

I don't know anything about our theme stuff, Dao should be the person looking at this.
Comment 2 Dão Gottwald [:dao] 2012-05-09 05:26:21 PDT
Comment on attachment 621984 [details] [diff] [review]
Patch converts SVG masks to clipPath elements. Need profiling.

You need to update browser/themes/winstripe/browser.css too.

nit: please replace "-mask" with "-clipPath" in the SVG node ids.
Comment 3 Jet Villegas (:jet) 2012-05-10 05:40:28 PDT
Created attachment 622694 [details] [diff] [review]
Final patch per feedback comments. r=dao

Addresses feedback comments. Need a final r+ prior to landing.
Comment 4 Dão Gottwald [:dao] 2012-05-10 14:00:06 PDT
Created attachment 622902 [details]
screenshot of winstripe-urlbar-back-button-mask (above) and ...-clip-path (below)

Note the pixels bleeding through behind the back button especially at the bottom. It looks like the clip path is misaligned or doesn't quite have the right shape.
Comment 5 Jet Villegas (:jet) 2012-05-11 02:59:38 PDT
Created attachment 623087 [details] [diff] [review]
Patch converts SVG masks to clipPath elements.

This patch includes clip-paths that very precisely follow the outlines of the original mask shapes they replace. This should resolve any compositing bleed issues.
Comment 6 Jet Villegas (:jet) 2012-05-11 04:04:56 PDT
Created attachment 623098 [details] [diff] [review]
This patch reduces precision for less verbose SVG code.

This patch simplifies the paths, reduces precision for less verbose SVG markup. Note: this SVG is generated by the Inkscape SVG editor.
Comment 7 Dão Gottwald [:dao] 2012-05-11 07:27:59 PDT
Comment on attachment 623098 [details] [diff] [review]
This patch reduces precision for less verbose SVG code.

Thanks!
Comment 9 Matt Brubeck (:mbrubeck) 2012-05-14 14:14:06 PDT
https://hg.mozilla.org/mozilla-central/rev/ec19a7304528
Comment 10 Matt Brubeck (:mbrubeck) 2012-05-16 19:19:35 PDT
It looks like this caused a 2% regression in the Talos SVG benchmark, at least on Mac OS X 10.6:
http://graphs.mozilla.org/graph.html#tests=[[22,63,21]]&sel=1336616135166,1337220935166

Should it be backed out?
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-16 20:06:43 PDT
I don't believe that's possible :-)
Comment 12 Dão Gottwald [:dao] 2012-07-18 02:42:46 PDT
*** Bug 589983 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.