Closed Bug 823147 Opened 8 years ago Closed 8 years ago

Avoid hitting D2D slow path when drawing radial gradients from css

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox18 --- wontfix
firefox19 - wontfix
firefox20 + fixed
firefox21 --- fixed
firefox-esr17 - wontfix
b2g18 --- unaffected

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

This seems to happen quite a bit because we don't have an inner radius of 0
Assignee: nobody → jmuizelaar
Blocks: 823141
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.
    // 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.
Blocks: 832374
Attached patch A new approach (obsolete) — Splinter Review
Attachment #693989 - Attachment is obsolete: true
This avoids moving the start position of non-repeating radial gradients so that they will go down the D2D fast path.
Attachment #704092 - Attachment is obsolete: true
Attachment #704226 - Flags: review?(roc)
Blocks: 833586
https://hg.mozilla.org/mozilla-central/rev/76611c3de0d7
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Blocks: 834019
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.
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.
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.
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.
Flags: needinfo?(jmuizelaar)
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
Attachment #704226 - Flags: approval-mozilla-aurora?
Flags: needinfo?(jmuizelaar)
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.
Attachment #704226 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.