Bug 179933 – FELighting cleanup and optimization

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Fraser (smfr)]

FELighting cleanup and optimization

[Simon Fraser (smfr)]

Created attachment 327438 [details]
Patch

[EWS Watchlist]

Attachment 327438 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/filters/FELighting.cpp:269:  Declaration has space between type name and * in factorX * static_cast  [whitespace/declaration] [3]
ERROR: Source/WebCore/platform/graphics/filters/FELighting.cpp:269:  static_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/graphics/filters/FELighting.cpp:270:  Declaration has space between type name and * in factorY * static_cast  [whitespace/declaration] [3]
ERROR: Source/WebCore/platform/graphics/filters/FELighting.cpp:270:  static_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 327438 [details]
Patch

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

Looks great. Hope the test coverage is already good.

> Source/WebCore/platform/graphics/filters/DistantLightSource.cpp:45
> +        cosf(azimuth) * cosf(elevation),
> +        sinf(azimuth) * cosf(elevation),
> +        sinf(elevation)

Seems nicer to use the overloaded std::cos and std::sin rather than the float-only sinf/cosf.

> Source/WebCore/platform/graphics/filters/FELighting.cpp:270
> +            factorX * static_cast<float>(normal2DVector.width()) * data.surfaceScale,
> +            factorY * static_cast<float>(normal2DVector.height()) * data.surfaceScale,

While they are not new, I don’t think these static_cast<float> are helpful or needed. I think the compiler knows how to do "float * int * float -> float" without casting help.

> Source/WebCore/platform/graphics/filters/FELighting.cpp:296
> +    unsigned char pixelValue[4] = {

Why "4" here, but "3" below in the call to setRange? Should be 3.

In code below you use uint8_t for this kind of thing, as does much new code. I think you should use it here too. Benefits: uint8_t is shorter and helps clarify that these bytes are not "characters". Disadvantages: type definition rather than built-in name for the type.

> Source/WebCore/platform/graphics/filters/FELighting.cpp:299
> +        static_cast<unsigned char>(lightStrength * lightingData.colorVector.x()),
> +        static_cast<unsigned char>(lightStrength * lightingData.colorVector.y()),
> +        static_cast<unsigned char>(lightStrength * lightingData.colorVector.z()),

Truncation is what we want here, not rounding?

> Source/WebCore/platform/graphics/filters/FELighting.cpp:307
> +void FELighting::platformApplyGenericPaint(const LightingData& data, const LightSource::PaintingData& paintingData, int startY, int endY)

The names start/end don’t make it clear to me that this stops *before* the end. Wish there was consistent naming for this.

> Source/WebCore/platform/graphics/filters/FELighting.cpp:309
> +    ASSERT(startY);

Why is this an important assertion?

> Source/WebCore/platform/graphics/filters/FELighting.cpp:396
> +    TimingScope scope("FELighting::drawLighting", 100);

I think you want to remove this before landing?

> Source/WebCore/platform/graphics/filters/FELighting.cpp:403
> +    paintingData.intialLightingData.colorVector = FloatPoint3D(m_lightingColor.red(), m_lightingColor.green(), m_lightingColor.blue());

I think it would be nice to use a helper function rather than writing out the integer-to-float conversion here, so this can work properly with ExtendedColor. Although I suppose that would only be sufficient to initialize the color vector, not necessarily enough to do the rest. One confusing aspect is that there are at least two kinds of floating point color channels, 0.0->1.0 and 0.0->255.0. Or is the second one 0.0->255.99999999? So using Color::getRGBA would not help.

> Source/WebCore/platform/graphics/filters/FELighting.h:68
> +        uint8_t alpha[3][3] { };

Might consider using std::array instead.

> Source/WebCore/platform/graphics/filters/FELighting.h:70
> +        // The implemtations are lined up to make comparing indices easier.

Misspelling of implementations here.

> Source/WebCore/platform/graphics/filters/FELighting.h:85
> +        uint8_t topLeft() const             { return alpha[0][0]; }
> +        uint8_t left() const                { return alpha[1][0]; }
> +        uint8_t bottomLeft() const          { return alpha[2][0]; }
> +
> +        uint8_t top() const                 { return alpha[0][1]; }
> +        uint8_t center() const              { return alpha[1][1]; }
> +        uint8_t bottom() const              { return alpha[2][1]; }
> +
> +        void setTop(uint8_t value)          { alpha[0][1] = value; }
> +        void setCenter(uint8_t value)       { alpha[1][1] = value; }
> +        void setBottom(uint8_t value)       { alpha[2][1] = value; }
> +
> +        void setTopRight(uint8_t value)     { alpha[0][2] = value; }
> +        void setRight(uint8_t value)        { alpha[1][2] = value; }
> +        void setBottomRight(uint8_t value)  { alpha[2][2] = value; }

Our coding style guidelines say we don’t line things up vertically like this.

> Source/WebCore/platform/graphics/filters/FELighting.h:87
> +        static void shiftRow(uint8_t alpha[3])

Using std::array& here would give us better type checking, but this function is only used in shift() below, so I guess that’s no big deal.

> Source/WebCore/platform/graphics/filters/PointLightSource.cpp:54
> +    return {
> +        lightVector,
> +        { },
> +        lightVector.length()
> +    };

I would write this on a single line. Not sure these always need to be vertical. Maybe even the same thing for the lightVector above.

> Source/WebCore/platform/graphics/filters/SpotLightSource.cpp:86
> +        return {
> +            lightVector,
> +            { },
> +            lightVectorLength
> +        };

Ditto.

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/225122/webkit

[Radar WebKit Bug Importer]

<rdar://problem/35678184>