[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 #5 from Joshua Netterfield <jnetterfield at rim.com>  2012-08-31 12:35:28 PST ---
(In reply to comment #4)
> (From update of attachment 161729 [details])
> 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.

I wrote this first, but then didn't like it, so wrote what you see now. The difference was a bit more extreme two weeks ago, but there's still more code in common (40 lines) than not in common (36 lines). If any of the parameters change, you'd have to change it in two places, so it's a bit awkward. It's not a huge deal obviously, so if you prefer that, I can revert it to what I made before?

> 
> > 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.

I think all platforms typedef Platform3DObject as GC3Duint, as defined in GraphicsTypes3D.h. Is this not the case? Is code in -OpenGLCommon not cross-platform?

> 
> > 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.

Will do.

> 
> You can have a typedef in the ValidatedProgram for the CustomFilterPlatformCompiledProgram to  CustomFilterCompiledProgramOpenGLCommon on Blackberry.
> 
> Note that compiledProgram will co-exist with platformCompiledProgram.

There isn't currently non-accelerated filter effects, but for other platforms this would be true.

> 
> > 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. 

Ok, that's just a matter of renaming things I think.

> 
> 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.

Sure.

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