Bug 94728 – [BlackBerry] CSS Shader implementation for BlackBerry port

• Component: WebKit BlackBerry
• Status: RESOLVED INVALID

[Joshua Netterfield]

https://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/index.html#feCustomElement

[Arvid Nilsson]

I'm currently looking at this.

[Arvid Nilsson]

Created attachment 162132 [details]
Patch

[Alexandru Chiculita]

Comment on attachment 162132 [details]
Patch

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

Looks good to me.

> Source/WebCore/platform/graphics/blackberry/LayerFilterRenderer.h:286
> +    CustomFilterGlobalContext m_programValidator;

It looks like we need to extract the validator out of CustomFilterGlobalContext. Do you mind adding a bug for that and assign it to me?

> Source/WebCore/platform/graphics/blackberry/LayerFilterRenderer.h:287
> +    HashSet<LayerFilterMesh*> m_meshSet;

I think it would be awesome if m_meshSet could also go into the CustomFilterGlobalContext, but that's ok for now. There's a different bug for that https://bugs.webkit.org/show_bug.cgi?id=88429 . It may be worth adding a fixme linking to the bug.

> Source/WebCore/rendering/FilterEffectRenderer.cpp:207
> +#if !PLATFORM(BLACKBERRY)

This will also disable FilterOperation::REFERENCE. Is that what you want?

[Arvid Nilsson]

Comment on attachment 162132 [details]
Patch

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

Thanks for looking into this, even though it's port specific =)

>> Source/WebCore/platform/graphics/blackberry/LayerFilterRenderer.h:286
>> +    CustomFilterGlobalContext m_programValidator;
> 
> It looks like we need to extract the validator out of CustomFilterGlobalContext. Do you mind adding a bug for that and assign it to me?

Will do, the goal here is to not have any object which could possibly create a GraphicsContext3D on our compositing thread.

>> Source/WebCore/platform/graphics/blackberry/LayerFilterRenderer.h:287
>> +    HashSet<LayerFilterMesh*> m_meshSet;
> 
> I think it would be awesome if m_meshSet could also go into the CustomFilterGlobalContext, but that's ok for now. There's a different bug for that https://bugs.webkit.org/show_bug.cgi?id=88429 . It may be worth adding a fixme linking to the bug.

In this case, that's in opposition to the previous comment - we don't want a CustomFilterGlobalContext at all on our compositing thread, since GraphicsContext3D is WebKit-thread specific.

CustomFilterGlobalContext should have a CustomFilterProgramValidator and a list of meshes, but we won't be reusing that infrastructure in the BlackBerry LayerFilterRenderer - instead, it will just have it's own separate CustomFilterProgramValidator and list of meshes.

>> Source/WebCore/rendering/FilterEffectRenderer.cpp:207
>> +#if !PLATFORM(BLACKBERRY)
> 
> This will also disable FilterOperation::REFERENCE. Is that what you want?

I've been trying to figure out what Joshua was trying to accomplish here, but I'm not certain why he disabled this code...

[Arvid Nilsson]

Comment on attachment 162132 [details]
Patch

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

>>> Source/WebCore/platform/graphics/blackberry/LayerFilterRenderer.h:286
>>> +    CustomFilterGlobalContext m_programValidator;
>> 
>> It looks like we need to extract the validator out of CustomFilterGlobalContext. Do you mind adding a bug for that and assign it to me?
> 
> Will do, the goal here is to not have any object which could possibly create a GraphicsContext3D on our compositing thread.

I created https://bugs.webkit.org/show_bug.cgi?id=95804

[Arvid Nilsson]

Comment on attachment 162132 [details]
Patch

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

>>> Source/WebCore/rendering/FilterEffectRenderer.cpp:207
>>> +#if !PLATFORM(BLACKBERRY)
>> 
>> This will also disable FilterOperation::REFERENCE. Is that what you want?
> 
> I've been trying to figure out what Joshua was trying to accomplish here, but I'm not certain why he disabled this code...

Mystery solved, the only thing standing between us and FECustomFilter is the Texture naming conflict described in https://bugs.webkit.org/show_bug.cgi?id=95781. I'm gonna re-enable this code in anticipation of a patch for https://bugs.webkit.org/show_bug.cgi?id=95781.

[Arvid Nilsson]

Created attachment 162142 [details]
Patch

[Arvid Nilsson]

Comment on attachment 162142 [details]
Patch

We plan to enable CSS shaders a little bit later when the implementation is updated to match the latest spec. I'll rename this bug slightly.

[Alexandru Chiculita]

(In reply to comment #8)
> (From update of attachment 162142 [details])
> We plan to enable CSS shaders a little bit later when the implementation is updated to match the latest spec. I'll rename this bug slightly.

I thought that we are following the spec closely. The remaining issues should be around the filter margins, the filter area used by the feCustomFilter and implementing the remaining parameter types. We are also working to remove the texture access.

Are you thinking about some specific issue?

[Arvid Nilsson]

Created attachment 162880 [details]
Patch

[Arvid Nilsson]

(In reply to comment #10)
> Created an attachment (id=162880) [details]
> Patch

As a bonus, this one actually applies to upstream! There was a conflict in platformBlackBerry.cmake in the other patches.

[George Staikos]

Comment on attachment 162880 [details]
Patch

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

> Source/WebKit/blackberry/Api/WebPage.cpp:573
> +#if ENABLE(CSS_SHADERS)

This looks wrong.  I think it's a bad merge...

[Arvid Nilsson]

Comment on attachment 162880 [details]
Patch

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

>> Source/WebKit/blackberry/Api/WebPage.cpp:573
>> +#if ENABLE(CSS_SHADERS)
> 
> This looks wrong.  I think it's a bad merge...

This was actually how Josh put it in his patch. I think the reasoning was, if we don't have webgl, we won't have graphicscontext3d, and css shaders can't work. However, the build system should make sure to enable css_shaders only if webgl is on, so moving it out of te webgl #ifdef would make sense.

[Arvid Nilsson]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 162142 [details] [details])
> > We plan to enable CSS shaders a little bit later when the implementation is updated to match the latest spec. I'll rename this bug slightly.
> 
> I thought that we are following the spec closely. The remaining issues should be around the filter margins, the filter area used by the feCustomFilter and implementing the remaining parameter types. We are also working to remove the texture access.
> 
> Are you thinking about some specific issue?

I was thinking of "Renoving the texture access", specifically.