Last Comment Bug 823147 - Avoid hitting D2D slow path when drawing radial gradients from css
: Avoid hitting D2D slow path when drawing radial gradients from css
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal with 2 votes (vote)
: mozilla21
Assigned To: Jeff Muizelaar [:jrmuizel]
:
Mentors:
Depends on: 833586
Blocks: 708069 823141 825244 826965 832374 834019
  Show dependency treegraph
 
Reported: 2012-12-19 11:15 PST by Jeff Muizelaar [:jrmuizel]
Modified: 2013-05-02 16:10 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
-
wontfix
+
fixed
fixed
-
wontfix
unaffected


Attachments
This random collection of removals of firstStop might do the trick. (4.19 KB, patch)
2012-12-19 12:03 PST, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Review
A new approach (1.03 KB, patch)
2013-01-18 14:41 PST, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Review
Avoid moving the start location of non-repeating radial gradients (3.75 KB, patch)
2013-01-19 07:42 PST, Jeff Muizelaar [:jrmuizel]
roc: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Review

Description Jeff Muizelaar [:jrmuizel] 2012-12-19 11:15:17 PST
This seems to happen quite a bit because we don't have an inner radius of 0
Comment 1 Jeff Muizelaar [:jrmuizel] 2012-12-19 12:03:53 PST
Created attachment 693989 [details] [diff] [review]
This random collection of removals of firstStop might do the trick.
Comment 2 Jeff Muizelaar [:jrmuizel] 2012-12-19 13:52:09 PST
Roc,

D2D doesn't support inner radii != 0. What are the reasons we need to adjust the gradient to start at the first stop? Should we only avoid that in the radial case, or can we get away with not adjusting it the rest of the time too.
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-12-19 15:29:38 PST
    // Cairo gradients must have stop positions in the range [0, 1]. So,
    // stop positions will be normalized below by subtracting firstStop and then
    // multiplying by stopScale.

So, for radial gradients it's OK to map lastStop to 1 and map 0 to 0, since firstStop must be >= 0. However, for linear gradients we could have a negative firstStop and cairo doesn't support that so we have to remap it at least in the case where it's negative.
Comment 4 Jeff Muizelaar [:jrmuizel] 2013-01-18 10:31:56 PST
https://tbpl.mozilla.org/?tree=Try&rev=0b379e8acd18
Comment 5 Jeff Muizelaar [:jrmuizel] 2013-01-18 12:44:33 PST
https://tbpl.mozilla.org/?tree=Try&rev=aff3fbe7f96a
Comment 6 Jeff Muizelaar [:jrmuizel] 2013-01-18 14:41:31 PST
Created attachment 704092 [details] [diff] [review]
A new approach
Comment 7 Jeff Muizelaar [:jrmuizel] 2013-01-19 07:42:42 PST
Created attachment 704226 [details] [diff] [review]
Avoid moving the start location of non-repeating radial gradients

This avoids moving the start position of non-repeating radial gradients so that they will go down the D2D fast path.
Comment 8 Jeff Muizelaar [:jrmuizel] 2013-01-19 07:49:18 PST
https://tbpl.mozilla.org/?tree=Try&rev=9a6701a80638
Comment 10 Ryan VanderMeulen [:RyanVM] 2013-01-23 08:41:38 PST
https://hg.mozilla.org/mozilla-central/rev/76611c3de0d7
Comment 11 (dormant account) 2013-01-28 11:42:28 PST
Can we uplift this? This makes pages like http://www.psfk.com/2013/01/honda-gear-concept-is-brands-return-to-cool.html not DoS my browser.
Comment 12 godotcg-rg 2013-01-30 02:59:24 PST
I think https://bugzilla.mozilla.org/show_bug.cgi?id=813957 may report the same problem.
I'm not able to tell. But I point at it so you can close it if it's indeed related and fixed by this bug.
Comment 13 Lukas Blakk [:lsblakk] use ?needinfo 2013-01-30 12:17:57 PST
We'll track and uplift for Aurora, but it's too late to take this on beta and it doesn't meet ESR criteria so minusing for those.
Comment 14 Lukas Blakk [:lsblakk] use ?needinfo 2013-02-11 10:18:02 PST
Can we get an uplift nomination here?  Merge Day is next Tuesday and it would be great to have this land while 20 is still on Aurora and before FF 20 beta 1.
Comment 15 Jeff Muizelaar [:jrmuizel] 2013-02-11 11:13:45 PST
Comment on attachment 704226 [details] [diff] [review]
Avoid moving the start location of non-repeating radial gradients

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Not a regression
User impact if declined: Really bad gradient performance on some pages. Without this, we basically denial of service some machines.
Testing completed (on m-c, etc.): It has been on m-c since 1-23 (19 days ago)
Risk to taking this patch (and alternatives if risky): Some gradients will draw wrong. However, this is very easy to back out, and we're more likely to find any problems on release.
String or UUID changes made by this patch: None
Comment 16 Lukas Blakk [:lsblakk] use ?needinfo 2013-02-11 11:14:35 PST
Comment on attachment 704226 [details] [diff] [review]
Avoid moving the start location of non-repeating radial gradients

Glad to hear this is easy to back out, so let's go ahead with uplift and watch for regressions while 20 is on Beta.
Comment 17 Ryan VanderMeulen [:RyanVM] 2013-02-11 13:35:12 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/28cfe506a7d9

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