Bug 232832 – [GPU Process] [Filters 8/23] Move the software FilterEffect functions to separate applier classes

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Said Abou-Hallawa]

This will simplify the interface of the FilterEffect superclasses and separate the implementation of applying the FilterEffect from its data.

[Radar WebKit Bug Importer]

<rdar://problem/85424121>

[Said Abou-Hallawa]

Created attachment 444754 [details]
Patch

[Said Abou-Hallawa]

Created attachment 444771 [details]
Patch

[Said Abou-Hallawa]

Created attachment 444774 [details]
Patch

[Said Abou-Hallawa]

Created attachment 444778 [details]
Patch

[Cameron McCormack (:heycam)]

Comment on attachment 444778 [details]
Patch

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

> Source/WebCore/platform/graphics/filters/FEColorMatrix.cpp:138
> +static void effectApplyAccelerated(Uint8ClampedArray& pixelArray, const Vector<float>& values, float components[9], IntSize bufferSize, ColorMatrixType matrixType)

Pre-existing, but the function name would read better as "applyEffectAccelerated".

> Source/WebCore/platform/graphics/filters/FEColorMatrix.cpp:242
> +static void effectType(Uint8ClampedArray& pixelArray, const Vector<float>& values, float components[9], ColorMatrixType matrixType)

And maybe this can be "applyAffectUnaccelerated"?

> Source/WebCore/platform/graphics/filters/FEColorMatrix.cpp:354
> +    FEColorMatrixSoftware::applyPlatform(pixelArray, values, components, pixelArrayDimensions, m_type);

I know this is pre-existing too, but I find it weird how we always pass in both values and components, but only use one or the other depending on the feColorMatrix type, and when values is used, components remains uninitialized. Perhaps calculateSaturateComponents/calculateHueRotateComponents should write their output into values, and applyPlatform and its callees can assert the length of values it's expecting depending on the type?

> Source/WebCore/platform/graphics/filters/FEComponentTransfer.cpp:125
> +    ASSERT(static_cast<size_t>(function.type) < WTF_ARRAY_LENGTH(callEffect));

Why drop this down from ASSERT_WITH_SECURITY_IMPLICATION? Seems important to avoid calling through random pointers after the end of the array.

> Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp:214
> +static ALWAYS_INLINE void setInteriorPixels(PaintingData& paintingData, int clipRight, int clipBottom, int yStart, int yEnd)

(It seems like most of the functions that take a PaintingData& could take a const reference.)

> Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp:223
>      // m_divisor cannot be 0, SVGFEConvolveMatrixElement ensures this

s/m_divisor/divisor/

> Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp:-284
> -template<bool preserveAlphaValues>

I assume untemplatizing these functions doesn't impact performance?

> Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp:395
> +    static constexpr int s_minimalRectDimension = (100 * 100); // Empirical data limit for parallel jobs

I think per https://webkit.org/code-style-guidelines/ the "s_" prefix is only for static member variables.

> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:471
> +    static constexpr int s_minimalRectDimension = 100 * 100; // Empirical data limit for parallel jobs

Same here, no "s_".

> Source/WebCore/platform/graphics/filters/FELighting.cpp:420
> +    static constexpr int s_minimalRectDimension = 100 * 100; // Empirical data limit for parallel jobs

And here.

> Source/WebCore/platform/graphics/filters/FELighting.cpp:576
> +    data.widthDecreasedByOne = size.width() - 1;
> +    data.heightDecreasedByOne = size.height() - 1;

I'd probably call these "widthMinusOne" and "heightMinusOne".

But I wonder if it really helps to store these values with one already subtracted. I don't think it makes the FELighting::drawLighting code easier to read.

> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:149
> +static void platofrmApplyGeneric(const PaintingData& paintingData, int startY, int endY)

"platformApplyGeneric"

> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:191
> +static void platofrmApplyWorker(PlatformApplyParameters* param)

"platformApplyWorker"

> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:196
> +static void platofrmApply(const PaintingData& paintingData)

"platformApply"

> Source/WebCore/platform/graphics/filters/FETurbulence.cpp:118
> +static const int s_perlinNoise = 4096;
> +static const long s_randMaximum = 2147483647; // 2**31 - 1
> +static const int s_randAmplitude = 16807; // 7**5; primitive root of m
> +static const int s_randQ = 127773; // m / a
> +static const int s_randR = 2836; // m % a

Pre-existing but I don't think these should have the "s_" prefix either.

> Source/WebCore/platform/graphics/filters/FETurbulence.cpp:121
> +static const int s_blockSize = 256;
> +static const int s_blockMask = s_blockSize - 1;

And these ones that moved shouldn't either.

> Source/WebCore/platform/graphics/filters/FETurbulence.cpp:136
> +    inline long random()

inline is redundant here

> Source/WebCore/platform/graphics/filters/FETurbulence.cpp:417
> +static void platofrmApplyGeneric(const IntRect& filterRegion, const FloatSize& filterScale, Uint8ClampedArray& pixelArray, const PaintingData& paintingData, StitchData stitchData, int startY, int endY)

"platformApplyGeneric"

> Source/WebCore/platform/graphics/filters/FETurbulence.cpp:438
> +static void platofrmApplyWorker(PlatformApplyParameters* parameters)

"platformApplyWorker"

> Source/WebCore/platform/graphics/filters/FETurbulence.cpp:443
> +static void platofrmApply(const IntRect& filterRegion, const FloatSize& filterScale, Uint8ClampedArray& pixelArray, StitchData& stitchData, PaintingData& paintingData)

"platformApply"

And can stitchData and paintingData be const references?

> Source/WebCore/platform/graphics/filters/FETurbulence.cpp:448
> +    static const int s_minimalRectDimension = (100 * 100); // Empirical data limit for parallel jobs.

And here.

> Source/WebCore/platform/graphics/filters/PointLightSource.h:55
> +    mutable FloatPoint3D m_bufferPosition;

Should m_bufferPosition (and the one in SpotSlightSource) be moved to LightSource::PaintingData?

[Said Abou-Hallawa]

Comment on attachment 444778 [details]
Patch

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

>> Source/WebCore/platform/graphics/filters/FEColorMatrix.cpp:138
>> +static void effectApplyAccelerated(Uint8ClampedArray& pixelArray, const Vector<float>& values, float components[9], IntSize bufferSize, ColorMatrixType matrixType)
> 
> Pre-existing, but the function name would read better as "applyEffectAccelerated".

It was renamed applyPlatformAccelerated().

>> Source/WebCore/platform/graphics/filters/FEColorMatrix.cpp:242
>> +static void effectType(Uint8ClampedArray& pixelArray, const Vector<float>& values, float components[9], ColorMatrixType matrixType)
> 
> And maybe this can be "applyAffectUnaccelerated"?

It was renamed applyPlatformUnaccelerated().

>> Source/WebCore/platform/graphics/filters/FEColorMatrix.cpp:354
>> +    FEColorMatrixSoftware::applyPlatform(pixelArray, values, components, pixelArrayDimensions, m_type);
> 
> I know this is pre-existing too, but I find it weird how we always pass in both values and components, but only use one or the other depending on the feColorMatrix type, and when values is used, components remains uninitialized. Perhaps calculateSaturateComponents/calculateHueRotateComponents should write their output into values, and applyPlatform and its callees can assert the length of values it's expecting depending on the type?

I changed the way we apply FilterEffects. I added the class FEColorMatrixSoftwareApplier which manages the calculation of the components.

>> Source/WebCore/platform/graphics/filters/FEComponentTransfer.cpp:125
>> +    ASSERT(static_cast<size_t>(function.type) < WTF_ARRAY_LENGTH(callEffect));
> 
> Why drop this down from ASSERT_WITH_SECURITY_IMPLICATION? Seems important to avoid calling through random pointers after the end of the array.

This was changed back to ASSERT_WITH_SECURITY_IMPLICATION().

>> Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp:214
>> +static ALWAYS_INLINE void setInteriorPixels(PaintingData& paintingData, int clipRight, int clipBottom, int yStart, int yEnd)
> 
> (It seems like most of the functions that take a PaintingData& could take a const reference.)

Yes some of the function can take const reference. But the rest can't take a const reference because PaintingData holds a reference to dstPixelArray which is changed in these functions.

>> Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp:223
>>      // m_divisor cannot be 0, SVGFEConvolveMatrixElement ensures this
> 
> s/m_divisor/divisor/

Fixed.

>> Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp:-284
>> -template<bool preserveAlphaValues>
> 
> I assume untemplatizing these functions doesn't impact performance?

I do not think this will impact the performance. In fact this template function is very strange. We either allocate float totals[4] or float totals[3] depending on the value of the template parameter 'preserveAlphaValues'. The MSVC compiler was giving the warning  '4789' and  this is why we were disabling it above.

>> Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp:395
>> +    static constexpr int s_minimalRectDimension = (100 * 100); // Empirical data limit for parallel jobs
> 
> I think per https://webkit.org/code-style-guidelines/ the "s_" prefix is only for static member variables.

Fixed.

>> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:471
>> +    static constexpr int s_minimalRectDimension = 100 * 100; // Empirical data limit for parallel jobs
> 
> Same here, no "s_".

Fixed.

>> Source/WebCore/platform/graphics/filters/FELighting.cpp:420
>> +    static constexpr int s_minimalRectDimension = 100 * 100; // Empirical data limit for parallel jobs
> 
> And here.

Fixed.

>> Source/WebCore/platform/graphics/filters/FELighting.cpp:576
>> +    data.heightDecreasedByOne = size.height() - 1;
> 
> I'd probably call these "widthMinusOne" and "heightMinusOne".
> 
> But I wonder if it really helps to store these values with one already subtracted. I don't think it makes the FELighting::drawLighting code easier to read.

Yes you are right. Adding DecreasedByOne to the 'width' and 'height' makes the code harder to read. I removed DecreasedByOne from the members' names and I changed the code accordingly.

>> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:149
>> +static void platofrmApplyGeneric(const PaintingData& paintingData, int startY, int endY)
> 
> "platformApplyGeneric"

Fixed.

>> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:191
>> +static void platofrmApplyWorker(PlatformApplyParameters* param)
> 
> "platformApplyWorker"

Fixed.

>> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:196
>> +static void platofrmApply(const PaintingData& paintingData)
> 
> "platformApply"

Fixed.

>> Source/WebCore/platform/graphics/filters/FETurbulence.cpp:118
>> +static const int s_randR = 2836; // m % a
> 
> Pre-existing but I don't think these should have the "s_" prefix either.

Removed.

>> Source/WebCore/platform/graphics/filters/FETurbulence.cpp:121
>> +static const int s_blockMask = s_blockSize - 1;
> 
> And these ones that moved shouldn't either.

Removed.

>> Source/WebCore/platform/graphics/filters/FETurbulence.cpp:136
>> +    inline long random()
> 
> inline is redundant here

Removed.

>> Source/WebCore/platform/graphics/filters/FETurbulence.cpp:417
>> +static void platofrmApplyGeneric(const IntRect& filterRegion, const FloatSize& filterScale, Uint8ClampedArray& pixelArray, const PaintingData& paintingData, StitchData stitchData, int startY, int endY)
> 
> "platformApplyGeneric"

Fixed.

>> Source/WebCore/platform/graphics/filters/FETurbulence.cpp:438
>> +static void platofrmApplyWorker(PlatformApplyParameters* parameters)
> 
> "platformApplyWorker"

Fixed.

>> Source/WebCore/platform/graphics/filters/FETurbulence.cpp:448
>> +    static const int s_minimalRectDimension = (100 * 100); // Empirical data limit for parallel jobs.
> 
> And here.

Fixed.

>> Source/WebCore/platform/graphics/filters/PointLightSource.h:55
>> +    mutable FloatPoint3D m_bufferPosition;
> 
> Should m_bufferPosition (and the one in SpotSlightSource) be moved to LightSource::PaintingData?

Yes they can. But let's do this in a separate patch.

[Said Abou-Hallawa]

Created attachment 445001 [details]
Patch

[Said Abou-Hallawa]

Created attachment 445004 [details]
Patch

[Said Abou-Hallawa]

Created attachment 445045 [details]
Patch

[Said Abou-Hallawa]

Created attachment 445047 [details]
Patch

[Said Abou-Hallawa]

Created attachment 445048 [details]
Patch

[Said Abou-Hallawa]

Created attachment 445049 [details]
Patch

[Cameron McCormack (:heycam)]

Comment on attachment 445048 [details]
Patch

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

> Source/WebCore/ChangeLog:234
> +        Move filing the structure LightingData from FELighting::drawLighting()

"filling"

> Source/WebCore/platform/graphics/filters/FETurbulence.cpp:128
> +    static const int perlinNoise = 4096;
> +    static const long randMaximum = 2147483647; // 2**31 - 1
> +    static const int randAmplitude = 16807; // 7**5; primitive root of m
> +    static const int randQ = 127773; // m / a
> +    static const int randR = 2836; // m % a
> +
> +    static const int blockSize = 256;
> +    static const int blockMask = blockSize - 1;

Now that these are member variables again I guess they should get their "s_" prefixes back! :/

[Said Abou-Hallawa]

Created attachment 445054 [details]
Patch

[EWS]

Committed r286140 (244527@main): <https://commits.webkit.org/244527@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445054 [details].