Bug 89277 – [Texmap] Share gaussian formula between shaders in TextureMapperShaderManager.

• Component: Platform
• Status: RESOLVED FIXED

[Dongseong Hwang]

Remove "FIXME : share gaussian formula between shaders" in TextureMapperShaderManager.

Moreover, this patch can increase performance, because it calculates gaussian kernel only one time.

[Dongseong Hwang]

Created attachment 147958 [details]
patch v.1

[Dongseong Hwang]

Comment on attachment 147958 [details]
patch v.1

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

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:43
> +                              STRINGIFY_VAL(src)

It is needless, but I amended it for symmetric with FRAGMENT_SHADER.

[Dongseong Hwang]

Comment on attachment 147958 [details]
patch v.1

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

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:46
> +                                STRINGIFY_VAL(src)

I add STRINGIFY_VAL in order to use macro in shader code, especially  GAUSSIAN_KERNEL_HALF_WIDTH and GAUSSIAN_KERNEL_STEP.

[Noam Rosenthal]

Comment on attachment 147958 [details]
patch v.1

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

> Source/WebCore/ChangeLog:8
> +        [Texmap] Share gaussian formula between shaders in TextureMapperShaderManager.
> +        https://bugs.webkit.org/show_bug.cgi?id=89277
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        No new tests. This patch doesn't change behavior.

You're doing a lot more than that, actually moving to a more accurate gaussian blur...
I like that, but please describe in the Changelog.

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:672
> +        }
> +        // Normalize the kernel

Extra line

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:678
> +    }
> +    return kernel;

Extra line

[Dongseong Hwang]

Created attachment 148000 [details]
patch v.2

[Dongseong Hwang]

I'm glad because you like this. :)

[Noam Rosenthal]

Comment on attachment 148000 [details]
patch v.2

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

Awesome! Please fix nit-picks before committing.

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:658
> +    return exp(- (x * x) / 2.f);

Remove space between - and (
Use 2. instead of 2.f

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:678
> +    if (!prepared) {
> +        kernel[0] = gauss(0);
> +        float sum = kernel[0];
> +        for (unsigned i = 1; i < GAUSSIAN_KERNEL_HALF_WIDTH; ++i) {
> +            kernel[i] = gauss(i * GAUSSIAN_KERNEL_STEP);
> +            sum += 2 * kernel[i];
> +        }
> +
> +        // Normalize the kernel
> +        float scale = 1 / sum;
> +        for (unsigned i = 0; i < GAUSSIAN_KERNEL_HALF_WIDTH; ++i)
> +            kernel[i] *= scale;
> +        prepared = true;
> +    }

Better to do 
if (prepared)
    return kernel;

[Dongseong Hwang]

Created attachment 148004 [details]
patch v.3

Good advice.

[WebKit Review Bot]

Comment on attachment 148004 [details]
patch v.3

Clearing flags on attachment: 148004

Committed r120545: <http://trac.webkit.org/changeset/120545>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.