Bug 179933 - FELighting cleanup and optimization
Summary: FELighting cleanup and optimization
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-21 17:12 PST by Simon Fraser (smfr)
Modified: 2017-11-23 14:23 PST (History)
7 users (show)

See Also:


Attachments
Patch (32.83 KB, patch)
2017-11-21 17:19 PST, Simon Fraser (smfr)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2017-11-21 17:12:06 PST
FELighting cleanup and optimization
Comment 1 Simon Fraser (smfr) 2017-11-21 17:19:54 PST
Created attachment 327438 [details]
Patch
Comment 2 EWS Watchlist 2017-11-21 17:21:00 PST
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.
Comment 3 Darin Adler 2017-11-22 10:03:41 PST
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.
Comment 4 Simon Fraser (smfr) 2017-11-23 14:22:59 PST
https://trac.webkit.org/changeset/225122/webkit
Comment 5 Radar WebKit Bug Importer 2017-11-23 14:23:27 PST
<rdar://problem/35678184>