Bug 98990

Summary: [Texmap][CSS Shaders] Enable CSS Shaders in TextureMapperGL
Product: WebKit Reporter: Dongseong Hwang <dongseong.hwang>
Component: Layout and RenderingAssignee: Alexandru Chiculita <achicu>
Status: RESOLVED FIXED    
Severity: Normal CC: achicu, dino, gustavo, noam, philn, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 98733, 98989, 99908, 100905, 101071    
Bug Blocks: 74651, 101801    
Attachments:
Description Flags
Patch
noam: review-, webkit-ews: commit-queue-
Patch V2 noam: review+

Description Dongseong Hwang 2012-10-10 21:24:55 PDT
Hardware-accelerate CSS Shaders using CustomFilterRenderer in TextureMapperGL.
Comment 1 Dongseong Hwang 2012-10-20 01:22:41 PDT
Created attachment 169766 [details]
Patch
Comment 2 Early Warning System Bot 2012-10-20 01:27:08 PDT
Comment on attachment 169766 [details]
Patch

Attachment 169766 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14459819
Comment 3 Early Warning System Bot 2012-10-20 01:27:59 PDT
Comment on attachment 169766 [details]
Patch

Attachment 169766 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14473297
Comment 4 Dongseong Hwang 2012-10-20 01:30:19 PDT
(In reply to comment #2)
> (From update of attachment 169766 [details])
> Attachment 169766 [details] did not pass qt-ews (qt):
> Output: http://queues.webkit.org/results/14459819

Build fails because this bug depends on Bug 98989. I don't know why ews run.

Anyway, this is the first enabling CSS Shaders on Accelerated Compositing! :)
Comment 5 Noam Rosenthal 2012-10-20 08:26:29 PDT
Comment on attachment 169766 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:72
> +class TextureMapperCustomFilterRendererMapHolder {
> +#if ENABLE(CSS_SHADERS)
> +public:
> +    typedef HashMap<const CustomFilterOperation*, RefPtr<CustomFilterRenderer> > CustomFilterRendererMap;
> +    virtual CustomFilterRendererMap* customFilterRendererMap() = 0;
> +    virtual void setCustomFilterGlobalContext(PassOwnPtr<CustomFilterGlobalContext>) = 0;
> +    virtual CustomFilterGlobalContext* customFilterGlobalContext() = 0;
> +#endif

Why not have the renderer map as a member of TextureMapper, instead of virtualizing the holder?
The patch would be a lot shorter and succinct if you did that...
Comment 6 Dongseong Hwang 2012-10-20 20:18:15 PDT
Comment on attachment 169766 [details]
Patch

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

>> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:72
>> +#endif
> 
> Why not have the renderer map as a member of TextureMapper, instead of virtualizing the holder?
> The patch would be a lot shorter and succinct if you did that...

Thanks for review. It's very reasonable concern.

This complexity is because of one requirement: destruction.

We must destruct CustomFilterGlobalContext when we navigate another page, because CustomFilterGlobalContext caches css shaders compiled programs. It is why RenderView holds CustomFilterGlobalContext in software path.
We must destruct CustomFilterRenderer when given RenderLayer dose not have the style including given custom filter. For example, javascript removes css shaders in the given element. It is why RenderLayer holds FilterEffectRenderer and FilterEffectRenderer holds CustomFilterRenderer in software path.
So I decides the root TextureMapperLayer holds CustomFilterGlobalContext like RenderView, and each TxtureMapperLayer holds CustomFilterRenderer like RenderLayer.

I had implemented both as a member of TextureMapper. I had troubles to destruct both in right time.

I wish TextureMapper holds both classes because it would be more succinct as you mentioned. If you give me ideas, I'll do that :)
Comment 7 Noam Rosenthal 2012-10-21 09:39:59 PDT
> I had implemented both as a member of TextureMapper. I had troubles to destruct both in right time.
> 
> I wish TextureMapper holds both classes because it would be more succinct as you mentioned. If you give me ideas, I'll do that :)

How about a pattern similar to adoptImageBackingStore/releaseImageBackingStore, where LayerTreeCoordinator tracks which custom filter programs are actually in use, and sends separate messages to the proxy to create/destroy them?
Comment 8 Dongseong Hwang 2012-10-21 21:49:31 PDT
(In reply to comment #7)
> How about a pattern similar to adoptImageBackingStore/releaseImageBackingStore, where LayerTreeCoordinator tracks which custom filter programs are actually in use, and sends separate messages to the proxy to create/destroy them?

Good suggestion for the purpose of TextureMapper holding CustomFilterGlobalContext and CustomFilterRenderer.

However, if we make LayerTreeCoordinator track which custom filter programs are actually in use, we must implement similar code in PageClientQt and AcceleratedCompositingContextGL. IMHO this way which we introduce to reduce complexity may be more complex than previous way (TextureMaperLayer holds CustomFilterRenderer).
On the other hands, I think it is natural for TextureMaperLayer to hold CustomFilterRenderer like RenderLayer. When readers compares the difference of css shaders implementation between software path and TexMap, the approach of this patch helps readers understand easily.

I'm looking forward your feedback again :)
Comment 9 Simon Hausmann 2012-10-21 22:59:10 PDT
Comment on attachment 169766 [details]
Patch

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

> Source/WebCore/platform/graphics/filters/CustomFilterGlobalContext.cpp:95
> -    m_context = GraphicsContext3D::create(attributes, hostWindow, GraphicsContext3D::RenderOffscreen);
> -    if (!m_context)
> +    RefPtr<GraphicsContext3D> context = GraphicsContext3D::create(attributes, hostWindow, GraphicsContext3D::RenderOffscreen);
> +    if (!context)
>          return;
> +    prepareContextIfNeeded(context.release());
> +}
> +
> +void CustomFilterGlobalContext::prepareContextIfNeeded(PassRefPtr<GraphicsContext3D> context)
> +{
> +    ASSERT(context.get());
> +    if (m_context.get()) {
> +        ASSERT(m_context.get() == context.get());
> +        return;
> +    }

Why not avoid the expensive call to GraphicsContext3D::create if m_context is already set?
Comment 10 Dongseong Hwang 2012-10-22 01:41:07 PDT
Comment on attachment 169766 [details]
Patch

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

>> Source/WebCore/platform/graphics/filters/CustomFilterGlobalContext.cpp:95
>> +    }
> 
> Why not avoid the expensive call to GraphicsContext3D::create if m_context is already set?

Thanks for review.

CustomFilterGlobalContext::prepareContextIfNeeded(HostWindow* hostWindow) already returns without calling GraphicsContext3D::create if m_context is already set.

void CustomFilterGlobalContext::prepareContextIfNeeded(HostWindow* hostWindow)
{
    if (m_context.get())
        return;
    ....
}

In addition, prepareContextIfNeeded(HostWindow* hostWindow) has existed already and only software path uses this method.
TexMap uses prepareContextIfNeeded(PassRefPtr<GraphicsContext3D> context).

Please let me know if you have any concerns more :)
Comment 11 Noam Rosenthal 2012-10-22 07:36:51 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > How about a pattern similar to adoptImageBackingStore/releaseImageBackingStore, where LayerTreeCoordinator tracks which custom filter programs are actually in use, and sends separate messages to the proxy to create/destroy them?
> 
> Good suggestion for the purpose of TextureMapper holding CustomFilterGlobalContext and CustomFilterRenderer.
> 
> However, if we make LayerTreeCoordinator track which custom filter programs are actually in use, we must implement similar code in PageClientQt and AcceleratedCompositingContextGL. IMHO this way which we introduce to reduce complexity may be more complex than previous way (TextureMaperLayer holds CustomFilterRenderer).
> On the other hands, I think it is natural for TextureMaperLayer to hold CustomFilterRenderer like RenderLayer. When readers compares the difference of css shaders implementation between software path and TexMap, the approach of this patch helps readers understand easily.
> 
> I'm looking forward your feedback again :)

OK, another option.
TextureMapperLayer will ref/deref the use of a CustomFilterProgram in a function called from flushCompositingState, when it knows which new filters its receiving. This would be equivalent to how stuff is done in WebCore. The shared context itself will be held inside TextureMapper, but the filters will arrive to the paint functions already prepared during the flush phase.
Comment 12 Dongseong Hwang 2012-10-22 15:58:45 PDT
(In reply to comment #11)
> (In reply to comment #8)
> > (In reply to comment #7)
> OK, another option.
> TextureMapperLayer will ref/deref the use of a CustomFilterProgram in a function called from flushCompositingState, when it knows which new filters its receiving. This would be equivalent to how stuff is done in WebCore. The shared context itself will be held inside TextureMapper, but the filters will arrive to the paint functions already prepared during the flush phase.

That's good idea! I'll do that :)
Comment 13 Noam Rosenthal 2012-10-25 07:36:09 PDT
Comment on attachment 169766 [details]
Patch

Clearing review flag for now, see previous comments.
Comment 14 Alexandru Chiculita 2012-11-09 14:50:41 PST
Created attachment 173377 [details]
Patch V2

I've used the new CustomFilterRenderer to draw the filter.
Comment 15 WebKit Review Bot 2012-11-09 14:55:12 PST
Attachment 173377 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:896:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Noam Rosenthal 2012-11-09 15:01:49 PST
Comment on attachment 173377 [details]
Patch V2

Very nice! Thank you.
Comment 17 Noam Rosenthal 2012-11-09 15:02:28 PST
Comment on attachment 173377 [details]
Patch V2

Clearing cq flags because of the style issue
Comment 18 Alexandru Chiculita 2012-11-09 15:50:04 PST
Landed manually in http://trac.webkit.org/changeset/134125 .
Comment 19 Dongseong Hwang 2012-11-09 16:15:02 PST
(In reply to comment #18)
> Landed manually in http://trac.webkit.org/changeset/134125 .

Great!

(In reply to comment #11)
> (In reply to comment #8)
> OK, another option.
> TextureMapperLayer will ref/deref the use of a CustomFilterProgram in a function called from flushCompositingState, when it knows which new filters its receiving. This would be equivalent to how stuff is done in WebCore. The shared context itself will be held inside TextureMapper, but the filters will arrive to the paint functions already prepared during the flush phase.

I'll add the feature to cache programs as you mentioned.
Comment 20 Alexandru Chiculita 2012-11-12 08:17:44 PST
(In reply to comment #19)
> (In reply to comment #18)
> > Landed manually in http://trac.webkit.org/changeset/134125 .
> 
> Great!
> 
> (In reply to comment #11)
> > (In reply to comment #8)
> > OK, another option.
> > TextureMapperLayer will ref/deref the use of a CustomFilterProgram in a function called from flushCompositingState, when it knows which new filters its receiving. This would be equivalent to how stuff is done in WebCore. The shared context itself will be held inside TextureMapper, but the filters will arrive to the paint functions already prepared during the flush phase.
> 
> I'll add the feature to cache programs as you mentioned.

I want to do it in two steps, but I didn't had the time to analyze it completely yet:
1. CustomFilterValidatedProgram is already unique for that particular combination of filters, so I would like to make sure that that particular feature is transfered to the UI process. We can get rid of the shader serialization. We can do it like this:

Add a new type of object that is going to be referenced by the CustomFilterValidatedProgram as the platform shader.
When initially created, this platform shader will not have an assigned ID yet.
When the shader passes through the ValidatedCustomFilterOperation serialization for the first time, it will retrieve an ID (just increment a static variable) and write down the shader strings and other info like the program type. Next time the serialization code sees the same program, it will just write the ID.

Deallocation is special and can be achieved by injecting a callback into this new platform custom filter program object. It will need to announce the coordinated graphics about the delation of the program. Also, make sure that the callback is removed when the coordinated graphics manager is deleted.

2. After we got the shader on the UI web process, we could reuse the old pre-compiled shader using the ID deserialized from the WebProcess. Note that this ID is not the ID allocated by OpenGL. It's just allocated on the WebProcess side as needed. We need to keep a hash map in the UI process from these ID to the WebCustomFilterPrograms. We will need to store a reference to the compiled program in WebCustomFilterProgram and use that from the WebCore side. The problem is that WebCustomFilterProgram is a WebKit2 class, so we need a Client like interface in WebCore that will be implemented by the WebKit2's WebCustomFilterProgram. Something like virtual int getProgramIdUsingContext (GraphicsContext3D*);

What do you think?
Comment 21 Noam Rosenthal 2012-11-12 10:15:34 PST
(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > Landed manually in http://trac.webkit.org/changeset/134125 .
> > 
> > Great!
> > 
> > (In reply to comment #11)
> > > (In reply to comment #8)
> > > OK, another option.
> > > TextureMapperLayer will ref/deref the use of a CustomFilterProgram in a function called from flushCompositingState, when it knows which new filters its receiving. This would be equivalent to how stuff is done in WebCore. The shared context itself will be held inside TextureMapper, but the filters will arrive to the paint functions already prepared during the flush phase.
> > 
> > I'll add the feature to cache programs as you mentioned.
> 
> I want to do it in two steps, but I didn't had the time to analyze it completely yet:
> 1. CustomFilterValidatedProgram is already unique for that particular combination of filters, so I would like to make sure that that particular feature is transfered to the UI process. We can get rid of the shader serialization. We can do it like this:
> 
> Add a new type of object that is going to be referenced by the CustomFilterValidatedProgram as the platform shader.
> When initially created, this platform shader will not have an assigned ID yet.
> When the shader passes through the ValidatedCustomFilterOperation serialization for the first time, it will retrieve an ID (just increment a static variable) and write down the shader strings and other info like the program type. Next time the serialization code sees the same program, it will just write the ID.
> 
> Deallocation is special and can be achieved by injecting a callback into this new platform custom filter program object. It will need to announce the coordinated graphics about the delation of the program. Also, make sure that the callback is removed when the coordinated graphics manager is deleted.
> 
> 2. After we got the shader on the UI web process, we could reuse the old pre-compiled shader using the ID deserialized from the WebProcess. Note that this ID is not the ID allocated by OpenGL. It's just allocated on the WebProcess side as needed. We need to keep a hash map in the UI process from these ID to the WebCustomFilterPrograms. We will need to store a reference to the compiled program in WebCustomFilterProgram and use that from the WebCore side. The problem is that WebCustomFilterProgram is a WebKit2 class, so we need a Client like interface in WebCore that will be implemented by the WebKit2's WebCustomFilterProgram. Something like virtual int getProgramIdUsingContext (GraphicsContext3D*);
> 
> What do you think?
Sounds like a plan!
Comment 22 Dongseong Hwang 2012-11-12 14:34:16 PST
(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > I want to do it in two steps, but I didn't had the time to analyze it completely yet:
> > 1. CustomFilterValidatedProgram is already unique for that particular combination of filters, so I would like to make sure that that particular feature is transfered to the UI process. We can get rid of the shader serialization. We can do it like this:
> > 
> > Add a new type of object that is going to be referenced by the CustomFilterValidatedProgram as the platform shader.
> > When initially created, this platform shader will not have an assigned ID yet.
> > When the shader passes through the ValidatedCustomFilterOperation serialization for the first time, it will retrieve an ID (just increment a static variable) and write down the shader strings and other info like the program type. Next time the serialization code sees the same program, it will just write the ID.
> > 
> > Deallocation is special and can be achieved by injecting a callback into this new platform custom filter program object. It will need to announce the coordinated graphics about the delation of the program. Also, make sure that the callback is removed when the coordinated graphics manager is deleted.
> > 
> > 2. After we got the shader on the UI web process, we could reuse the old pre-compiled shader using the ID deserialized from the WebProcess. Note that this ID is not the ID allocated by OpenGL. It's just allocated on the WebProcess side as needed. We need to keep a hash map in the UI process from these ID to the WebCustomFilterPrograms. We will need to store a reference to the compiled program in WebCustomFilterProgram and use that from the WebCore side. The problem is that WebCustomFilterProgram is a WebKit2 class, so we need a Client like interface in WebCore that will be implemented by the WebKit2's WebCustomFilterProgram. Something like virtual int getProgramIdUsingContext (GraphicsContext3D*);
> > 
> > What do you think?
> Sounds like a plan!

Absolutely great plan! I'm looking forward :)
Comment 23 Alexandru Chiculita 2012-11-13 15:48:30 PST
(In reply to comment #22)
> Absolutely great plan! I'm looking forward :)

@Huang: Let me know if you want to work on any of that.
Comment 24 Dongseong Hwang 2012-11-13 15:54:10 PST
(In reply to comment #23)
> (In reply to comment #22)
> > Absolutely great plan! I'm looking forward :)
> 
> @Huang: Let me know if you want to work on any of that.

I'm fine :) Your plan is great. I want you to improve CSS Shaders on TexMap!