Avoid hitting D2D slow path when drawing radial gradients from css

RESOLVED FIXED in Firefox 20

Status

()

Core
Graphics
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

(Depends on: 1 bug)

unspecified
mozilla21
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox18 wontfix, firefox19- wontfix, firefox20+ fixed, firefox21 fixed, firefox-esr17- wontfix, b2g18 unaffected)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
This seems to happen quite a bit because we don't have an inner radius of 0
(Assignee)

Comment 1

4 years ago
Created attachment 693989 [details] [diff] [review]
This random collection of removals of firstStop might do the trick.
Assignee: nobody → jmuizelaar
(Assignee)

Updated

4 years ago
Blocks: 823141
(Assignee)

Comment 2

4 years ago
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: 826965

Updated

4 years ago
Blocks: 832374
(Assignee)

Comment 4

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=0b379e8acd18
(Assignee)

Comment 5

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=aff3fbe7f96a
(Assignee)

Comment 6

4 years ago
Created attachment 704092 [details] [diff] [review]
A new approach
Attachment #693989 - Attachment is obsolete: true
(Assignee)

Comment 7

4 years ago
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.
Attachment #704092 - Attachment is obsolete: true
Attachment #704226 - Flags: review?(roc)
(Assignee)

Comment 8

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=9a6701a80638
Attachment #704226 - Flags: review?(roc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/76611c3de0d7

Updated

4 years ago
Blocks: 833586
Blocks: 825244
No longer blocks: 833586
Depends on: 833586
https://hg.mozilla.org/mozilla-central/rev/76611c3de0d7
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21

Updated

4 years ago
Blocks: 834019
Blocks: 708069

Comment 11

4 years ago
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.

Updated

4 years ago
status-b2g18: --- → unaffected
status-firefox18: --- → affected
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox21: --- → fixed
status-firefox-esr17: --- → affected
tracking-firefox19: --- → ?
tracking-firefox20: --- → ?

Comment 12

4 years ago
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.
tracking-firefox19: ? → -
tracking-firefox20: ? → +
tracking-firefox-esr17: --- → -
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)
(Assignee)

Comment 15

4 years ago
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+
status-firefox19: affected → wontfix
https://hg.mozilla.org/releases/mozilla-aurora/rev/28cfe506a7d9
status-firefox20: affected → fixed
status-firefox18: affected → wontfix
status-firefox-esr17: affected → wontfix
You need to log in before you can comment on or make changes to this bug.