Bug 94728 - [BlackBerry] CSS Shader implementation for BlackBerry port
Summary: [BlackBerry] CSS Shader implementation for BlackBerry port
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Arvid Nilsson
URL:
Keywords:
Depends on: 71401 93869 94724 94725 94726 94736
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-22 10:48 PDT by Joshua Netterfield
Modified: 2014-01-28 08:26 PST (History)
7 users (show)

See Also:


Attachments
Patch (47.93 KB, patch)
2012-09-04 16:46 PDT, Arvid Nilsson
no flags Details | Formatted Diff | Diff
Patch (46.76 KB, patch)
2012-09-04 18:01 PDT, Arvid Nilsson
no flags Details | Formatted Diff | Diff
Patch (44.51 KB, patch)
2012-09-07 15:04 PDT, Arvid Nilsson
staikos: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Arvid Nilsson 2012-09-03 07:16:48 PDT
I'm currently looking at this.
Comment 2 Arvid Nilsson 2012-09-04 16:46:32 PDT
Created attachment 162132 [details]
Patch
Comment 3 Alexandru Chiculita 2012-09-04 17:28:03 PDT
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?
Comment 4 Arvid Nilsson 2012-09-04 17:38:39 PDT
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...
Comment 5 Arvid Nilsson 2012-09-04 17:51:26 PDT
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
Comment 6 Arvid Nilsson 2012-09-04 17:56:25 PDT
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.
Comment 7 Arvid Nilsson 2012-09-04 18:01:00 PDT
Created attachment 162142 [details]
Patch
Comment 8 Arvid Nilsson 2012-09-07 10:06:00 PDT
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.
Comment 9 Alexandru Chiculita 2012-09-07 13:45:20 PDT
(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?
Comment 10 Arvid Nilsson 2012-09-07 15:04:03 PDT
Created attachment 162880 [details]
Patch
Comment 11 Arvid Nilsson 2012-09-07 15:06:49 PDT
(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.
Comment 12 George Staikos 2012-09-07 15:15:14 PDT
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...
Comment 13 Arvid Nilsson 2012-09-07 15:29:53 PDT
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.
Comment 14 Arvid Nilsson 2012-09-07 15:31:07 PDT
(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.