Bug 202815 – [FTW] Correct radial gradient handling of various radius orderings

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brent Fulgham]

The initial Direct2D Radial Gradient implementation did not properly handle cases where the order of the two radius arguments were not small-to-large. This led to incorrect drawing in some cases tested in the 'canvas/Philip' test set.

This patch handles these cases.

[Brent Fulgham]

Created attachment 380664 [details]
Patch

[Brent Fulgham]

This corrects three radius gradient test cases.

[Per Arne Vollan]

Comment on attachment 380664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380664&action=review

> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:137
> -        [&] (const RadialData& data) {
> +        [&] (RadialData& data) {

It does not seem like the data parameter is modified, can we keep the const modifier?

[Brent Fulgham]

Comment on attachment 380664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380664&action=review

>> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:137
>> +        [&] (RadialData& data) {
> 
> It does not seem like the data parameter is modified, can we keep the const modifier?

Yes. I modified while experimenting with another change, and forgot to put this back. I'll fix it.

[Per Arne Vollan]

Comment on attachment 380664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380664&action=review

> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:99
> +                    std::stable_sort(gradientStops.begin(), gradientStops.end(), [](const D2D1_GRADIENT_STOP& a, const D2D1_GRADIENT_STOP& b) {
> +                        return a.position < b.position;
> +                    });

Is sorting needed here, or is it sufficient to reverse the list of gradient stops?

[Fujii Hironori]

Comment on attachment 380664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380664&action=review

> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:79
>              if (data.startRadius && data.endRadius) {

Which test cases are using this code path?
I can't find code setting startRadius and endRadius.
Can they be negative values?

[Fujii Hironori]

Comment on attachment 380664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380664&action=review

> Source/WebCore/PlatformFTW.cmake:3
> +else ()

Use NOT. And, indent the the inside.

if (NOT APPLE)
    include(platform/ImageDecoders.cmake)
endif ()

[Fujii Hironori]

Comment on attachment 380664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380664&action=review

>> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:79
>>              if (data.startRadius && data.endRadius) {
> 
> Which test cases are using this code path?
> I can't find code setting startRadius and endRadius.
> Can they be negative values?

I found it. It is canvas.createRadialGradient.
https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/createRadialGradient

negative values are checked in CanvasRenderingContext2DBase::createRadialGradient.

[Fujii Hironori]

Comment on attachment 380664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380664&action=review

> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:83
>                      startPosition = 1.0 / startPosition;

Can startPosition be zero? Then, zero-devision can happen.

[Fujii Hironori]

Comment on attachment 380664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380664&action=review

>> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:83
>>                      startPosition = 1.0 / startPosition;
> 
> Can startPosition be zero? Then, zero-devision can happen.

Oops. Never mind. It is always >1.0.

[Fujii Hironori]

Comment on attachment 380664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380664&action=review

>>> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:79
>>>              if (data.startRadius && data.endRadius) {
>> 
>> Which test cases are using this code path?
>> I can't find code setting startRadius and endRadius.
>> Can they be negative values?
> 
> I found it. It is canvas.createRadialGradient.
> https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/createRadialGradient
> 
> negative values are checked in CanvasRenderingContext2DBase::createRadialGradient.

I think this is not a right condition for restoring start position. It should be

  if (data.startRadius || data.endRadius) {

I think swapping startRadius and endRadius should be done before. Then, this condition can be:

  if (data.startRadius) {

I think this is the condition which is easy to understand when restoring start position is needed.

[Fujii Hironori]

Comment on attachment 380664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380664&action=review

>>>> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:79
>>>>              if (data.startRadius && data.endRadius) {
>>> 
>>> Which test cases are using this code path?
>>> I can't find code setting startRadius and endRadius.
>>> Can they be negative values?
>> 
>> I found it. It is canvas.createRadialGradient.
>> https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/createRadialGradient
>> 
>> negative values are checked in CanvasRenderingContext2DBase::createRadialGradient.
> 
> I think this is not a right condition for restoring start position. It should be
> 
>   if (data.startRadius || data.endRadius) {
> 
> I think swapping startRadius and endRadius should be done before. Then, this condition can be:
> 
>   if (data.startRadius) {
> 
> I think this is the condition which is easy to understand when restoring start position is needed.

You should split reversing stops and restoring the first position

 if (data.startRadius > data.endRadius) {
   // Reverse stops...
   std::swap(data.startRadius, data.endRadius);
 }
 if (data.startRadius) {
   // Restore first position...
 }

[Fujii Hironori]

Comment on attachment 380664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380664&action=review

> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:143
> +                offset.setHeight(offset.height() - radiusDiff);

In my understanding this is the real offset. Am I worng? https://ibb.co/hFfD7D0

[Brent Fulgham]

(In reply to Fujii Hironori from comment #6)
> Comment on attachment 380664 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380664&action=review
> 
> > Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:79
> >              if (data.startRadius && data.endRadius) {
> 
> Which test cases are using this code path?

We hit this code path in the following cases:

canvas/philip/tests/2d.gradient.radial.inside2.html
canvas/philip/tests/2d.gradient.radial.inside3.html
canvas/philip/tests/2d.gradient.radial.outside2.html
canvas/philip/tests/2d.gradient.radial.outside3.html
canvas/philip/tests/2d.gradient.radial.touch2.html

[Brent Fulgham]

Comment on attachment 380664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380664&action=review

>> Source/WebCore/PlatformFTW.cmake:3
>> +else ()
> 
> Use NOT. And, indent the the inside.
> 
> if (NOT APPLE)
>     include(platform/ImageDecoders.cmake)
> endif ()

Okay

>> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:143
>> +                offset.setHeight(offset.height() - radiusDiff);
> 
> In my understanding this is the real offset. Am I worng? https://ibb.co/hFfD7D0

Yes -- I think that is correct.

[Fujii Hironori]

Comment on attachment 380664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380664&action=review

>>> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:143
>>> +                offset.setHeight(offset.height() - radiusDiff);
>> 
>> In my understanding this is the real offset. Am I worng? https://ibb.co/hFfD7D0
> 
> Yes -- I think that is correct.

Then, offset should look like:

offset = (point1 * startRadius - point0 * endRadius) / (startRadius - endRadius)

[Brent Fulgham]

Created attachment 381074 [details]
Patch

[Brent Fulgham]

(In reply to Fujii Hironori from comment #16)
> Comment on attachment 380664 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380664&action=review
> 
> >>> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:143
> >>> +                offset.setHeight(offset.height() - radiusDiff);
> >> 
> >> In my understanding this is the real offset. Am I worng? https://ibb.co/hFfD7D0
> > 
> > Yes -- I think that is correct.
> 
> Then, offset should look like:
> 
> offset = (point1 * startRadius - point0 * endRadius) / (startRadius -
> endRadius)

Thank you -- that actually corrected one additional test! :-)

[Per Arne Vollan]

Comment on attachment 381074 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381074&action=review

> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:92
> +            if (data.startRadius) {
>                  float startPosition = std::abs(data.startRadius / data.endRadius);

Do we need to check that data.endRadius > 0 here, to avoid division by zero?

[Brent Fulgham]

Comment on attachment 381074 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381074&action=review

>> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:92
>>                  float startPosition = std::abs(data.startRadius / data.endRadius);
> 
> Do we need to check that data.endRadius > 0 here, to avoid division by zero?

We require that data.startRadius and data.endRadius are > 0 on lines 79/80, so I don't think so?

[Per Arne Vollan]

Comment on attachment 381074 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381074&action=review

>>> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:92
>>>                  float startPosition = std::abs(data.startRadius / data.endRadius);
>> 
>> Do we need to check that data.endRadius > 0 here, to avoid division by zero?
> 
> We require that data.startRadius and data.endRadius are > 0 on lines 79/80, so I don't think so?

We seem to require that data.endRadius >= 0, perhaps the release assert should be changed?

[Brent Fulgham]

Comment on attachment 381074 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381074&action=review

>>>> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:92
>>>>                  float startPosition = std::abs(data.startRadius / data.endRadius);
>>> 
>>> Do we need to check that data.endRadius > 0 here, to avoid division by zero?
>> 
>> We require that data.startRadius and data.endRadius are > 0 on lines 79/80, so I don't think so?
> 
> We seem to require that data.endRadius >= 0, perhaps the release assert should be changed?

Oh, I see. Good point. I'll change that. However, we require that data.startRadius <= data.endRadius, and then check that data.startRadius is non-zero, so I don't think it's possible to get here with a zero data.endRadius. But I'll add a RELEASE_ASSERT just to be sure.

[Brent Fulgham]

Created attachment 381138 [details]
Patch

[Per Arne Vollan]

Comment on attachment 381138 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381138&action=review

R=me.

> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:147
> +            if (!circleIsEntirelyContained(data.point0, data.startRadius, data.point1, data.endRadius))
> +                offset = (data.point1.scaled(data.startRadius) - data.point0.scaled(data.endRadius)) / (data.endRadius - data.startRadius);

Why was this check needed? Did adding this check fix more tests?

[WebKit Commit Bot]

Comment on attachment 381138 [details]
Patch

Clearing flags on attachment: 381138

Committed r251221: <https://trac.webkit.org/changeset/251221>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56355288>

[Brent Fulgham]

Comment on attachment 381138 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381138&action=review

>> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:147
>> +                offset = (data.point1.scaled(data.startRadius) - data.point0.scaled(data.endRadius)) / (data.endRadius - data.startRadius);
> 
> Why was this check needed? Did adding this check fix more tests?

Yes. The behavior of the gradient is different if one circle is completely contained within the other. Then the focus calculation that Fujii proposed is not valid and produces incorrect rendering.