[webkit-reviews] review granted: [Bug 232832] [GPU Process] [Filters 9/17] Make the software filters functions static and remove them from the FilterEffect classes : [Attachment 444778] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Nov 21 23:18:32 PST 2021


Cameron McCormack (:heycam) <heycam at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 232832: [GPU Process] [Filters 9/17] Make the software filters functions
static and remove them from the FilterEffect classes
https://bugs.webkit.org/show_bug.cgi?id=232832

Attachment 444778: Patch

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




--- Comment #6 from Cameron McCormack (:heycam) <heycam at apple.com> ---
Comment on attachment 444778
  --> https://bugs.webkit.org/attachment.cgi?id=444778
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?


More information about the webkit-reviews mailing list