687188 - [Skia] Skia radial gradients should use the 0/1 color stop values as it's clamp value

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: 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]]

I'm some what worried about the performance impact that this will have.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

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)]

(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]]

(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)]

(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)]

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)]

Attachment: 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)]

(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]]

Comment on attachment 560648 [details] [diff] [review]
Clamp to the stop values

This seems ok to me.

Comment 10

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b40846bc3c8

Comment 11

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/2b40846bc3c8