Closed Bug 687188 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

Details

Attachments

(2 files)

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.
Attachment #560648 - Flags: review?(jmuizelaar)
I'm some what worried about the performance impact that this will have.
Shouldn't the first and last index in the gradient bitmap cache contain the upper and lower bound colors?
(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.
(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?
(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.
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.
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.
Attachment #561615 - Flags: feedback?(jmuizelaar)
(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 on attachment 560648 [details] [diff] [review]
Clamp to the stop values

This seems ok to me.
Attachment #560648 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b40846bc3c8
Assignee: nobody → matt.woodrow
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/2b40846bc3c8
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla10
Attachment #561615 - Flags: feedback?(jmuizelaar)
You need to log in before you can comment on or make changes to this bug.