Bug 142885 – FEMorphology::platformApplyGeneric() should bail out if the radius is less than or equal to zero.

• Component: SVG
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

The caller of this function FEMorphology::platformApplySoftware() bails out early if the radius is less than or equal to zero. However when applying the scaling, the resulted radius might be less than zero due to overflow. This radius is passed to FEMorphology::platformApplyGeneric() via FEMorphology::platformApply() and the calculations are carried out needlessly.

We need to clean up this function a little to handle this corner case and to remove repeated code to inline functions.

[Said Abou-Hallawa]

Created attachment 249073 [details]
Patch

[Said Abou-Hallawa]

Related to <rdar://problem/20127673>

[Brent Fulgham]

This seems to cause several errors on Windows. If my bots start doing their job, you should see error information soon!

[Said Abou-Hallawa]

Created attachment 249117 [details]
Patch

[Said Abou-Hallawa]

Created attachment 249122 [details]
Patch

[Darin Adler]

Comment on attachment 249122 [details]
Patch

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

> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:86
> +inline bool shouldSupersedeExtremum(unsigned char newValue, unsigned char currentValue, MorphologyOperatorType type)

Since these functions are in the .cpp file, they should say "static" so they get internal linkage.

> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:92
> +inline int pixelArrayIndex(int x, int y, int width, int colorChannel)

Since these functions are in the .cpp file, they should say "static" so they get internal linkage.

> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:97
> +inline unsigned char columnExtremum(const Uint8ClampedArray* srcPixelArray, int x, int yStart, int yEnd, int width, unsigned colorChannel, MorphologyOperatorType type)

Since these functions are in the .cpp file, they should say "static" so they get internal linkage.

> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:108
> +inline unsigned char kernelExtremum(const Vector<unsigned char>& kernel, MorphologyOperatorType type)

Since these functions are in the .cpp file, they should say "static" so they get internal linkage.

> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:110
> +    unsigned char extremum = kernel[0];

Can this function be called on an empty vector?

> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:113
> +        if (shouldSupersedeExtremum(value, extremum, type))
> +            extremum = value;

This does extra work for the first character of the kernel. Can we change the code so it doesn’t do this? Maybe we have to forgo the new-style for loop to do that.

> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:143
> +                unsigned char extremum = columnExtremum(srcPixelArray, x, yStartExtrema, yEndExtrema, width, colorChannel, m_type);
> +                extrema.append(extremum);

I don’t think we should bother with the local variable here.

> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:154
> +                    unsigned char extremum = columnExtremum(srcPixelArray, xEnd, yStartExtrema, yEndExtrema + 1, width, colorChannel, m_type);
> +                    extrema.append(extremum);

I don’t think we should bother with the local variable here.

> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:158
> +                unsigned char extremum = kernelExtremum(extrema, m_type);
> +                dstPixelArray->set(pixelArrayIndex(x, y, width, colorChannel), extremum);

I don’t think we should bother with the local variable here.

[David Kilzer (:ddkilzer)]

<rdar://problem/20127673>

[Said Abou-Hallawa]

Created attachment 249254 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 249122 [details]
Patch

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

>> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:86
>> +inline bool shouldSupersedeExtremum(unsigned char newValue, unsigned char currentValue, MorphologyOperatorType type)
> 
> Since these functions are in the .cpp file, they should say "static" so they get internal linkage.

Done.

>> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:92
>> +inline int pixelArrayIndex(int x, int y, int width, int colorChannel)
> 
> Since these functions are in the .cpp file, they should say "static" so they get internal linkage.

Done.

>> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:97
>> +inline unsigned char columnExtremum(const Uint8ClampedArray* srcPixelArray, int x, int yStart, int yEnd, int width, unsigned colorChannel, MorphologyOperatorType type)
> 
> Since these functions are in the .cpp file, they should say "static" so they get internal linkage.

Done.

>> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:108
>> +inline unsigned char kernelExtremum(const Vector<unsigned char>& kernel, MorphologyOperatorType type)
> 
> Since these functions are in the .cpp file, they should say "static" so they get internal linkage.

Done.

>> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:110
>> +    unsigned char extremum = kernel[0];
> 
> Can this function be called on an empty vector?

No.

Although I changed the code a little since the filter was not giving symmetric results, both the old and new code should never hit this case. In the new patch:
1. The initial kernel size = radiusX
2. In the same loop we call kernelExtremum() we add new pixel values = width - radiusX
3. And we only remove number of pixels = width - radiusX - 1
4. And since the additions starts before the removal we are sure that the size of the kernel should be >= radiusX + 1

>> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:113
>> +            extremum = value;
> 
> This does extra work for the first character of the kernel. Can we change the code so it doesn’t do this? Maybe we have to forgo the new-style for loop to do that.

Done.

>> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:143
>> +                extrema.append(extremum);
> 
> I don’t think we should bother with the local variable here.

Done.

>> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:154
>> +                    extrema.append(extremum);
> 
> I don’t think we should bother with the local variable here.

Done.

>> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:158
>> +                dstPixelArray->set(pixelArrayIndex(x, y, width, colorChannel), extremum);
> 
> I don’t think we should bother with the local variable here.

Done.

[Said Abou-Hallawa]

1. I found an issue with the original code. If the radius is zero, the original code was treating this case as an error and hence the source of the filter was not even displayed. In this case the filter should just be ignored as if it did not exist.

2. I found another issue with building the kernel of the filter. Its size was equal to radiusX + 1. This resulted in asymmetric filter effect in some simple cases (see the attached test case and compare the left border of the rectangle with Chrome). The initial size should be equal to radiusX. While adding to it and removing from it, its size should be not less than radiusX + 1.

[Said Abou-Hallawa]

Created attachment 249259 [details]
asymmetric test case

[Said Abou-Hallawa]

I found another issue with the previous patch. When scaling the radius, the result might be equal to zero and this case I was treating this as an error. I was calling dstPixelArray->zeroFill() which was preventing the source of the filter to even be displayed. We should be consistent when dealing with the radius and the scaled radius. 

1. If the radius (or the scaled radius) is less than zero => treat this is an error.
2. If the radius (or the scaled radius) is equal to zero => ignore the filter.

Please notice that:

1. The radius can > 0 and scaled_radius = 0 because it is scaled down and the result is less than 1. So the integer casting returns 0.

2. The radius can > 0 and scaled_radius < 0 because an overflow happens when scaling up the radius.

[Said Abou-Hallawa]

I did not find a test case to discover the bug I introduced in my previous patch. So I added a new test case to handle the three cases of the radius and make sure our handling is compliant with the specs.

Since I did a lot of changes beyond the scope of fixing Darin's comments and since I added a new test case, I am submitting the new patch for review.

[Said Abou-Hallawa]

Created attachment 249277 [details]
zero radius test case

[Dean Jackson]

Comment on attachment 249254 [details]
Patch

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

> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:112
> +    Vector<unsigned char>::const_iterator iter = kernel.begin();
> +    unsigned char extremum = *iter;
> +    for (Vector<unsigned char>::const_iterator end = kernel.end(); ++iter != end; ) {

We can't use a new-style for loop here?

> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:198
> +    // input radius is less than zero or an overflow happens when scaling it

Nit/Typo/Whatever :)  - start with a capital letter and end with a full stop.

> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:204
> +    // input radius is equal to zero or the scaled radius is less than one.

Same.

[Said Abou-Hallawa]

Created attachment 249539 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #15)
> Comment on attachment 249254 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249254&action=review
> 
> > Source/WebCore/platform/graphics/filters/FEMorphology.cpp:112
> > +    Vector<unsigned char>::const_iterator iter = kernel.begin();
> > +    unsigned char extremum = *iter;
> > +    for (Vector<unsigned char>::const_iterator end = kernel.end(); ++iter != end; ) {
> 
> We can't use a new-style for loop here?
> 

I did use the new-style for loop here in the previous patch but Darin's did not like we had to do extra work for the first element. So I had to forgo the new-style for loop to do that.

> > Source/WebCore/platform/graphics/filters/FEMorphology.cpp:198
> > +    // input radius is less than zero or an overflow happens when scaling it
> 
> Nit/Typo/Whatever :)  - start with a capital letter and end with a full stop.
> 

Done.

> > Source/WebCore/platform/graphics/filters/FEMorphology.cpp:204
> > +    // input radius is equal to zero or the scaled radius is less than one.
> 
> Same.

Done.

[WebKit Commit Bot]

Comment on attachment 249539 [details]
Patch

Clearing flags on attachment: 249539

Committed r182067: <http://trac.webkit.org/changeset/182067>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.