[Webkit-unassigned] [Bug 98989] [CSS Shaders] Add CustomFilterRenderer to reuse this class by Accelerated Compositing.

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from Chiculita Alexandru <achicu at adobe.com>  2012-10-16 15:01:46 PST ---
(From update of attachment 168138)
View in context: https://bugs.webkit.org/attachment.cgi?id=168138&action=review

Looks good! Thanks for the patch.

> Source/WebCore/ChangeLog:16
> +        CustomFilterGlobalContext and CustomFilterOperation, and it is possible if we

I would remove the dependency on the CustomFilterOperation. Most probably the platform code has no access to it. Anyway, FECustomFilter had no dependency on that, so please remove it.

> Source/WebCore/ChangeLog:21
> +        No new tests. Covered by css1/filters/custom

nit: typo css1 -> css3

> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:55
> +PassRefPtr<CustomFilterRenderer> CustomFilterRenderer::create(CustomFilterGlobalContext* customFilterGlobalContext, CustomFilterOperation* operation)

This layer should not use the CustomFilterOperation directly. I would rather keep the same constructor as the FECustomFilter had before.

> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:148
> +bool CustomFilterRenderer::render(Uint8ClampedArray* source, Uint8ClampedArray* destination, const IntSize& size)

I think this one could have a better name to explain that it would read back the pixels. I suppose at some point you will add one to execute only on textures, so that the Accelerated Pipeline doesn't read back.

> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:474
> +static void orthogonalProjectionMatrix(TransformationMatrix& matrix, float left, float right, float bottom, float top)
> +{
> +    ASSERT(matrix.isIdentity());
> +

I think static methods would better be placed at the beginning of the file.

> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.h:67
> +    enum CustomFilterDrawType {
> +        NEEDS_INPUT_TEXTURE,
> +        NO_INPUT_TEXTURE

I know you just copy-pasted this one, but it seems like NO_INPUT_TEXTURE is never used. Can you please remove CustomFilterDrawType type? I think it was intended to help implementing a renderer that has the input already as a texture instead of just some bytes in the memory.

For the accelerated compositor you will need a different render method that will take a source texture ID and a destination texture ID, so maybe we could add it back in a different form at that time.

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:44
> +FECustomFilter::FECustomFilter(Filter* filter, PassRefPtr<CustomFilterRenderer> renderer)

I think FECustomFilter should create its own CustomFilterRenderer and not be passed one. CustomFilterRenderer would then be just an implementation detail.

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