[Webkit-unassigned] [Bug 94725] [CSS Shaders] [BlackBerry] Refactor CustomFilterMesh and CustomFilter*Program

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from Chiculita Alexandru <achicu at adobe.com>  2012-08-31 12:04:10 PST ---
(From update of attachment 161729)
View in context: https://bugs.webkit.org/attachment.cgi?id=161729&action=review

Looks good! Thanks for working on this. I just had different view on the overall architecture for this.

> Source/WebCore/platform/graphics/filters/CustomFilterCompiledProgram.cpp:145
> +    m_positionAttribLocation = getAttribLocation("a_position");

This is the only common part of this two implementations. Initially I thought the CustomFilterValidatedProgram would have references to two different Compiled Programs:
1. CustomFilterCompiledProgram as it is today - This will be used whenever it decides to run the shader in software, ie SVG when the feCustom tag will be implemented.
2. CustomFilterPlatformCompiledProgram which would be used and owned by the platform.

This will eliminate the need to have all this virtual functions and if you look at it the reused part is really small.

> Source/WebCore/platform/graphics/filters/CustomFilterCompiledProgram.h:86
> +    virtual Platform3DObject compileShader(GC3Denum shaderType, const String& shaderString) = 0;

Some platforms might not have access to Platform3DObject. That's also an argument for having two different classes for the CompiledPrograms.

> Source/WebCore/platform/graphics/filters/CustomFilterCompiledProgram.h:122
> +class CustomFilterCompiledProgramGC3D : public CustomFilterCompiledProgram {

If you follow my pattern CustomFilterCompiledProgramGC3D will not exist anymore.

> Source/WebCore/platform/graphics/filters/CustomFilterGlobalContext.cpp:77
>  void CustomFilterGlobalContext::prepareContextIfNeeded(HostWindow* hostWindow)

I think hostWindow will become unused parameter.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:108
> +        m_compiledProgram = CustomFilterCompiledProgramOpenGLCommon::create(m_validatedVertexShader, m_validatedFragmentShader);

I think this should go into something named CustomFilterValidatedProgram::platformCompiledProgram() which will do nothing on other platforms for now and on Blackberry will create the CustomFilterCompiledProgramOpenGLCommon.

You can have a typedef in the ValidatedProgram for the CustomFilterPlatformCompiledProgram to  CustomFilterCompiledProgramOpenGLCommon on Blackberry.

Note that compiledProgram will co-exist with platformCompiledProgram.

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:206
> +    m_mesh = CustomFilterMeshGC3D::create(m_context.get(), m_meshColumns, m_meshRows, 

I think we could use a different pattern on CustomFilterMesh. Let's extract that into two different classes:
CustomFilterMesh will just be the generator of the mesh coordinates. While the actual loading into GPU and reference keeping will be moved to an object called CustomFilterMeshOwnerGC3D or CustomFilterMeshOwnerOpenGLCommon. The last two classes do not need to be related as they are used in different contexts.  

The construction pattern of CustomFilterMeshOwnerGC3D would look like: CustomFilterMeshOwnerGC3D::create(CustomFilterMesh::create(...)); The same can be applied for CustomFilterMeshOwnerOpenGLCommon.

This way CustomFilterMesh will just work in the CPU memory. The Owner will upload it on the GPU and keep a reference to the native vertex buffers. The Owner will not really need to keep the Mesh object alive after it uploads to GPU.

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