[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 #10 from Chiculita Alexandru <achicu at adobe.com>  2012-10-22 10:57:33 PST ---
(From update of attachment 169381)
View in context: https://bugs.webkit.org/attachment.cgi?id=169381&action=review

Thanks! Looks good, I like the new approach where the CustomFilterRenderer is not messing around with textures anymore.

I have a few minor comments. Going this way, it looks like CustomFilterRenderer could be really decoupled from the Global Context and that would simplify a bunch of the other checks.

> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:76
> +PassRefPtr<CustomFilterRenderer> CustomFilterRenderer::create(CustomFilterGlobalContext* customFilterGlobalContext, const CustomFilterProgramInfo& programInfo, const CustomFilterParameterList& parameters,
> +    unsigned meshRows, unsigned meshColumns, CustomFilterOperation::MeshBoxType meshBoxType, CustomFilterOperation::MeshType meshType)
> +{

I would remove the need to pass the customFIlterGlobalContext at this point. It looks like you only need the context and the validated program. Let's make this class as simple as possible, so that it can be embedded anywhere.

> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:116
> +    m_contextSize = size;

Can you add a fixme and a new bug to remove m_contextSize from the CustomFilterRenderer? I think we would need something like CustomFilterRendererState that will contain the size and maybe other parameters in the future. We should pass that to bindProgramBuffers instead of storing it.

> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:145
> +    // FIXME: Sharing the mesh would just save the time needed to upload it to the GPU, so I assume we could
> +    // benchmark that for performance.
> +    // https://bugs.webkit.org/show_bug.cgi?id=88429
> +    m_mesh = CustomFilterMesh::create(m_context.get(), m_meshColumns, m_meshRows, FloatRect(0, 0, 1, 1), m_meshType);

Move this to a separate method called initializeMeshIfNeeded(), so that it is not guarded by the first return. It should not be an issue, but looks weird to have it in a method called initializeCompiledProgram.

Also, make the call after checking that the compiled program is initialized in prepareForDrawing.

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:68
> +    RefPtr<FECustomFilter> feCustomFilter = adoptRef(new FECustomFilter(filter, customFilterGlobalContext, programInfo, parameters, meshRows, meshColumns, meshBoxType, meshType));

It looks like you could remove the dependency on customFilterGlobalContext and just pass in the validated program + the GraphicsContext3D directly. That will simplify a bunch of the checks we do here.

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:72
> +    ASSERT(feCustomFilter->m_context.get());

Right now the context is never null, but it might be in the future if shaders are disabled after a GPU reset.

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:-122
> -    if (!m_context)

At this point there is no guarantee that the m_context is not 0. I think you should put this back in.

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