Closed Bug 895135 Opened 6 years ago Closed 6 years ago

Avoid normalizing gradients with all stops starting at the same location to the same position on the 0-1 line.

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Currently if we have all the stops at the same location. We end up putting them all at 0. This makes things harder for backends that implement gradients with a lookup table because they don't have room for multiples entries at the zero location. If instead we leave the stops at the same location on the 0-1 line we'll have a better chance of drawing them correctly.
Attached patch A bad implementation of this. (obsolete) — Splinter Review
This works, but was made by fiddling with things until it worked. I plan on writing something that makes more sense. One of things that I'm undecided about is whether to avoid normalizing all non-repeating gradients that have stops inbetween 0 and 1 or to just handle the case mentioned above.
Roc, does this idea seem reasonable to you?
Flags: needinfo?(roc)
I don't understand. Your patch maps stops in the same place to 0.0, which is what you said in comment #0 you don't want to do.
Flags: needinfo?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> I don't understand. Your patch maps stops in the same place to 0.0, which is
> what you said in comment #0 you don't want to do.

For repeating gradients it continues to do so. For non-repeating ones it shouldn't. Either way, I'm more interested in feedback on the idea and not the patch.
Attached patch patch (obsolete) — Splinter Review
Here's a better implementation of this.
Attachment #777397 - Attachment is obsolete: true
Attachment #777789 - Flags: feedback?(roc)
Here are the reftest results for this change:
https://tbpl.mozilla.org/?tree=Try&rev=4de1667a3f18
This just basically does what the bug title asks for and is much better than the previous versions

A stylistic question:
if (stopOrigin > 0)
   stopOrigin = 0;

or

stopOrigin = min(stopOrigin, 0);
Attachment #777789 - Attachment is obsolete: true
Attachment #777789 - Flags: feedback?(roc)
Attachment #778231 - Flags: review?(roc)
Comment on attachment 778231 [details] [diff] [review]
A much nicer version that finally makes things clear

Review of attachment 778231 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSRendering.cpp
@@ +2285,5 @@
> +  if (!aGradient->mRepeating || stopDelta == 0) {
> +    if (stopOrigin > 0)
> +      stopOrigin = 0;
> +    if (stopEnd < 1)
> +      stopEnd = 1;

use min/max
Attachment #778231 - Flags: review?(roc) → review+
Blocks: 638664
Blocks: 524173
https://hg.mozilla.org/mozilla-central/rev/9497f8c1a115
Assignee: nobody → jmuizelaar
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
This patch seems to have caused a substantial performance gain for gradients.  At least it's fixed (or mostly fixed) bug 896703 on the trunk -- an old bug that goes back at least to FF 16.

Could this patch be backported to aurora (the 24 branch), or even beta (the 23 branch), at least in principle?
Depends on: 916535
Depends on: 976409
Depends on: 1242145
You need to log in before you can comment on or make changes to this bug.