Bug 100533 – [CSS Shaders] CustomFilterOperation should be converted to ValidatedCustomFilterOperation before using it

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alexandru Chiculita]

CustomFilterOperation is too hard to use in platform code and we need a simplified version of it. This bug will create a new class called ValidatedCustomFilterOperation and use that instead of the CustomFilterOperation in the platform code.

The difference between CustomFilterOperation and ValidatedCustomFilterOperation is that the later already has the shaders loaded and verified to comply with the specification. At that point the shaders should be ready to be used in a WebGL compliant context.

[Alexandru Chiculita]

Created attachment 171743 [details]
Patch V1

[Dean Jackson]

Comment on attachment 171743 [details]
Patch V1

View in context: https://bugs.webkit.org/attachment.cgi?id=171743&action=review

> Source/WebCore/platform/graphics/filters/ValidatedCustomFilterOperation.h:66
> +    CustomFilterMeshType meshType() const { return m_meshType; }
>  private:

Nit: space between these lines

> Source/WebCore/rendering/FilterEffectRenderer.cpp:342
> +            // CUSTOM operations are alwasy converted to VALIDATED_CUSTOM before getting here.

typo "always"

[Alexandru Chiculita]

Created attachment 171950 [details]
Patch V2

Sorry, I noticed that there was an issue with the old patch. I've split updateOrRemoveFilterEffect and removed the assert_not_reached in computeFilterOperations (I've added a comment to explain why).

[Dean Jackson]

Comment on attachment 171950 [details]
Patch V2

View in context: https://bugs.webkit.org/attachment.cgi?id=171950&action=review

> Source/WebCore/ChangeLog:44
> +        layer is updated. Otherwise the composited layer will never see a loaded filter in the first call.
> +
> +        (WebCore):
> +        (WebCore::RenderLayer::isCSSCustomFilterEnabled):
> +        (WebCore::RenderLayer::computeFilterOperations):
> +        (WebCore::RenderLayer::updateOrRemoveFilterClients): Split updateOrRemoveFilterEffect into 2 functions.
> +        This one is supposed to add the clients needed to load network resources.
> +
> +        (WebCore::RenderLayer::updateOrRemoveFilterEffectRenderer): Figures out if a software fallback is needed 
> +        and creates a FilterEffectRenderer.
> +
> +        * rendering/RenderLayer.h:

You have a few blank lines in there.

> Source/WebCore/platform/graphics/filters/ValidatedCustomFilterOperation.h:68
> +    CustomFilterMeshType meshType() const { return m_meshType; }
>  private:
> +    
>      virtual bool operator==(const FilterOperation& o) const

I actually was asking for the empty line between meshType() and private :)

[kov's GTK+ EWS bot]

Comment on attachment 171950 [details]
Patch V2

Attachment 171950 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14696469

[Early Warning System Bot]

Comment on attachment 171950 [details]
Patch V2

Attachment 171950 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14697446

[Dongseong Hwang]

Comment on attachment 171950 [details]
Patch V2

View in context: https://bugs.webkit.org/attachment.cgi?id=171950&action=review

> Source/WebCore/rendering/RenderLayer.cpp:5126
> +            RefPtr<CustomFilterValidatedProgram> validatedProgram = globalContext->getValidatedProgram(program->programInfo());

As you filed Bug 100906, all Accelerated Compositings(a.k.a AC) do not want to compile here, because AC can not use the same GraphicsContext3D belonging to CustomFilterGlobalContext. AC would compile shaders again.
I think "PassRefPtr<FECustomFilter> createCustomFilterEffect() in FilterEffectRenderer" should compile in software path.
And each AC will compile in their own code.

Will you fix this in Bug 100906 later?

[Build Bot]

Comment on attachment 171950 [details]
Patch V2

Attachment 171950 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14697449

[Alexandru Chiculita]

The other patch got landed first, so I've updated the FilterEffectRenderer to use the new constant style. 100890 will actually add the new mesh box type.

Landed in http://trac.webkit.org/changeset/133242.

[Dongseong Hwang]

Comment on attachment 171950 [details]
Patch V2

View in context: https://bugs.webkit.org/attachment.cgi?id=171950&action=review

>> Source/WebCore/rendering/RenderLayer.cpp:5126
>> +            RefPtr<CustomFilterValidatedProgram> validatedProgram = globalContext->getValidatedProgram(program->programInfo());
> 
> As you filed Bug 100906, all Accelerated Compositings(a.k.a AC) do not want to compile here, because AC can not use the same GraphicsContext3D belonging to CustomFilterGlobalContext. AC would compile shaders again.
> I think "PassRefPtr<FECustomFilter> createCustomFilterEffect() in FilterEffectRenderer" should compile in software path.
> And each AC will compile in their own code.
> 
> Will you fix this in Bug 100906 later?

I said the wrong fact. We do not compile here, but we still have the similar problem.

CustomFilterGlobalContext has two roles: supplying GraphicsContext3D and caching CustomFilterValidatedProgram (especially caching gl compiled program to not recompile).

I think we do not use CustomFilterGlobalContext here because AC can not use CustomFilterGlobalContext owned by RenderView.
ValidatedCustomFilterOperation should not know CustomFilterValidatedProgram. ValidatedCustomFilterOperation should know just validated program strings as you filed Bug 100906.
We need to perform "RefPtr<CustomFilterValidatedProgram> validatedProgram = globalContext->getValidatedProgram(program->programInfo());" in "PassRefPtr<FECustomFilter> createCustomFilterEffect() in FilterEffectRenderer"
Each AC can use CustomFilterGlobalContext and perform "globalContext->getValidatedProgram()" in its own code too.

Finally, Thanks for this patch!

[Alexandru Chiculita]

(In reply to comment #10)
> (From update of attachment 171950 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171950&action=review
> 
> >> Source/WebCore/rendering/RenderLayer.cpp:5126
> >> +            RefPtr<CustomFilterValidatedProgram> validatedProgram = globalContext->getValidatedProgram(program->programInfo());
> > 
> > As you filed Bug 100906, all Accelerated Compositings(a.k.a AC) do not want to compile here, because AC can not use the same GraphicsContext3D belonging to CustomFilterGlobalContext. AC would compile shaders again.
> > I think "PassRefPtr<FECustomFilter> createCustomFilterEffect() in FilterEffectRenderer" should compile in software path.
> > And each AC will compile in their own code.
> > 
> > Will you fix this in Bug 100906 later?
> 
> I said the wrong fact. We do not compile here, but we still have the similar problem.
> 
> CustomFilterGlobalContext has two roles: supplying GraphicsContext3D and caching CustomFilterValidatedProgram (especially caching gl compiled program to not recompile).
> 
> I think we do not use CustomFilterGlobalContext here because AC can not use CustomFilterGlobalContext owned by RenderView.
> ValidatedCustomFilterOperation should not know CustomFilterValidatedProgram. ValidatedCustomFilterOperation should know just validated program strings as you filed Bug 100906.
> We need to perform "RefPtr<CustomFilterValidatedProgram> validatedProgram = globalContext->getValidatedProgram(program->programInfo());" in "PassRefPtr<FECustomFilter> createCustomFilterEffect() in FilterEffectRenderer"
> Each AC can use CustomFilterGlobalContext and perform "globalContext->getValidatedProgram()" in its own code too.
> 
> Finally, Thanks for this patch!

In https://bugs.webkit.org/show_bug.cgi?id=100906 I will add a base class to be used by the ValidatedCustomFilterProgram. In the webkit process it will keep a reference to the Validated program. That way we have a reference to the caching system. The UI process will keep a reference to the other class that will just store empty strings.

The advantage is that the validated program can actually keep a reference to the class that stores the strings, using the platform program. That way we could keep and id instead of serializing the shader every time. I will do that one too and then you will need to update your rendering patches.

[Dongseong Hwang]

(In reply to comment #11)
> That way we could keep and id instead of serializing the shader every time.

Good idea!

> I will do that one too and then you will need to update your rendering patches.

Yes, I will.