[Webkit-unassigned] [Bug 54456] Optimizing lightning filter to ARM-neon SIMD instruction set

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=54456


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #86762|review?                     |review-
               Flag|                            |




--- Comment #38 from Nikolas Zimmermann <zimmermann at kde.org>  2011-04-09 00:00:32 PST ---
(From update of attachment 86762)
View in context: https://bugs.webkit.org/attachment.cgi?id=86762&action=review

I think the patch is great, if Gavin has no more concerns or any of the other folks familiar with the actual code I'd like to see it go in :-) Still some thoughs:

> Source/WebCore/platform/graphics/filters/FELighting.cpp:461
> +    // Set lightnig arguments.

typo: lighting.

>> Source/WebCore/platform/graphics/filters/arm/FELightingNEON.cpp:205
>> +
>> +asm (
> 
> Extra space before ( in function call  [whitespace/parens] [4]

You could add "// NOLINT" here, so it stops reporting this error.

> Source/WebCore/platform/graphics/filters/arm/FELightingNEON.cpp:290
> +    // Multiply the alpha values by the X and Y matricies.

typo: matrices.

>> Source/WebCore/platform/graphics/filters/arm/FELightingNEON.cpp:455
>> +);
> 
> The parameter name "NL" adds no information, so it should be removed.  [readability/parameter_name] [5]

Same here.

>> Source/WebCore/platform/graphics/filters/arm/FELightingNEON.h:27
>> +#ifndef FELightingNeonSIMD_h
> 
> #ifndef header guard has wrong style, please use: WTF_FELightingNEON_h  [build/header_guard] [5]

Seems like a style check bug? Did someone already report it?

> Source/WebCore/platform/graphics/filters/arm/FELightingNEON.h:44
> +#define FLAG_POINT_LIGHT                 0x01
> +#define FLAG_SPOT_LIGHT                  0x02
> +#define FLAG_CONE_EXPONENT_IS_1          0x04
> +
> +// Otherwise: Diffuse light.
> +#define FLAG_SPECULAR_LIGHT              0x10
> +#define FLAG_DIFFUSE_CONST_IS_1          0x20
> +#define FLAG_SPECULAR_EXPONENT_IS_1      0x40

Can't we use enums for this?
enum LightingFlags (
    FlagPointLight = 1 << 0,
..
};

> Source/WebCore/platform/graphics/filters/arm/FELightingNEON.h:86
> +extern short s_FELightingConstantsForNeon[];

No way we can use a "short feLightingConstantsForNeon()" free function here?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.