Last Comment Bug 687188 - [Skia] Skia radial gradients should use the 0/1 color stop values as it's clamp value
: [Skia] Skia radial gradients should use the 0/1 color stop values as it's cla...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla10
Assigned To: Matt Woodrow (:mattwoodrow) (PTO until 27 June)
:
Mentors:
Depends on:
Blocks: 687187
  Show dependency treegraph
 
Reported: 2011-09-16 16:29 PDT by Matt Woodrow (:mattwoodrow) (PTO until 27 June)
Modified: 2011-11-17 20:45 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Clamp to the stop values (3.14 KB, patch)
2011-09-16 16:29 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
jmuizelaar: review+
Details | Diff | Review
Clamp to the stop values approach 2 (9.13 KB, patch)
2011-09-21 16:52 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review

Description Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2011-09-16 16:29:43 PDT
Created attachment 560648 [details] [diff] [review]
Clamp to the stop values

Skia radial gradients are using the first/last index in the gradient bitmap cache to perform clamping. When you have really small stop offsets near the edges, this can produce incorrect results.
Comment 1 Jeff Muizelaar [:jrmuizel] 2011-09-20 14:39:59 PDT
I'm some what worried about the performance impact that this will have.
Comment 2 Jeff Muizelaar [:jrmuizel] 2011-09-20 14:48:13 PDT
Shouldn't the first and last index in the gradient bitmap cache contain the upper and lower bound colors?
Comment 3 Bas Schouten (:bas.schouten) 2011-09-20 14:49:42 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> Shouldn't the first and last index in the gradient bitmap cache contain the
> upper and lower bound colors?

Not if the resolution is not high enough I guess, i.e. if you had a stop at 0.9999 and 1.0, with certain resolutions the last index would return the color at 0.9999.
Comment 4 Jeff Muizelaar [:jrmuizel] 2011-09-20 14:53:34 PDT
(In reply to Bas Schouten (:bas) from comment #3)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> > Shouldn't the first and last index in the gradient bitmap cache contain the
> > upper and lower bound colors?
> 
> Not if the resolution is not high enough I guess, i.e. if you had a stop at
> 0.9999 and 1.0, with certain resolutions the last index would return the
> color at 0.9999.

But wouldn't it be desirable to explicitly have the first and last colors match the upper and lower bound colors?
Comment 5 Bas Schouten (:bas.schouten) 2011-09-20 14:59:07 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> (In reply to Bas Schouten (:bas) from comment #3)
> > (In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> > > Shouldn't the first and last index in the gradient bitmap cache contain the
> > > upper and lower bound colors?
> > 
> > Not if the resolution is not high enough I guess, i.e. if you had a stop at
> > 0.9999 and 1.0, with certain resolutions the last index would return the
> > color at 0.9999.
> 
> But wouldn't it be desirable to explicitly have the first and last colors
> match the upper and lower bound colors?

Yes, I believe that's what I do in radial gradients for Azure-D2D. I think Matt told me that's not what Skia does though, that might be the actual patch we want.
Comment 6 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2011-09-21 00:58:22 PDT
I think that would cause us to fail other tests though.

For the case where we have:

red - 0
green - 0.00001
green - 1

We want to render green within the bounds of the gradient for an reasonable scale, and red everywhere outside of the gradient bounds.

We could extend the cache array by two pixels and access them using cache[-1] and cache[0xFF + 1]. That's fairly ugly though :)

I've also realized that the same problem exists for linear gradients (and is causing us to fail a reftest), so we need a solution that will work for this case too.
Comment 7 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2011-09-21 16:52:38 PDT
Created attachment 561615 [details] [diff] [review]
Clamp to the stop values approach 2

Another attempt at this. This fixes all of our failing tests at least.

Doesn't fix the shadeSpan16 code, and I'm really unsure of how to handle dithering here.
Comment 8 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2011-09-21 17:04:51 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
> I'm some what worried about the performance impact that this will have.

What exactly do you think would cause a performance regression? My changes to the radial gradient code were essentially just an expansion of the SkClampMax inline function, I can't see this having any effect on the number of branches per pixel.

If it was just recomputing the pre-multiplied color, then this new patch should fix that, as well as the possible overhead of a function call.
Comment 9 Jeff Muizelaar [:jrmuizel] 2011-09-23 14:26:11 PDT
Comment on attachment 560648 [details] [diff] [review]
Clamp to the stop values

This seems ok to me.
Comment 10 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2011-10-28 00:17:58 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b40846bc3c8
Comment 11 Matt Brubeck (:mbrubeck) 2011-10-28 12:20:22 PDT
https://hg.mozilla.org/mozilla-central/rev/2b40846bc3c8

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