[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 #12 from Huang Dongsung <luxtella at company100.net>  2012-10-23 04:22:25 PST ---
(From update of attachment 169381)
View in context: https://bugs.webkit.org/attachment.cgi?id=169381&action=review

>> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:76
>> +{
> 
> 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.

Thanks for your careful review!

Done.

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

Done.
I filed Bug 100107.

>> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:145
>> +    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.

Done.

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

Done. You are right. The bunch of the checks are only in createCustomFilterEffect now.

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

Done.

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

I put ASSERT(m_context) because we have guarantee that the m_context is not 0. We set m_context in the constructor of FECustomFilter. createCustomFilterEffect in FilterEffectRenderer.cpp dose not create FECustomFilter if the m_context is 0.
Moreover, many methods of FECustomFilter already use the m_context without the checks. IMHO if we put this back in, other folks will be confused when reading here.

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