Bug 98990

Created attachment 169766 [details] Patch

Comment on attachment 169766 [details] Patch Attachment 169766 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14459819

Comment on attachment 169766 [details] Patch Attachment 169766 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14473297

(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 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 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 :)

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

(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 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 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 :)

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

(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 on attachment 169766 [details] Patch Clearing review flag for now, see previous comments.

Created attachment 173377 [details] Patch V2 I've used the new CustomFilterRenderer to draw the filter.

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 on attachment 173377 [details] Patch V2 Very nice! Thank you.

Comment on attachment 173377 [details] Patch V2 Clearing cq flags because of the style issue

Landed manually in http://trac.webkit.org/changeset/134125 .

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

(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?

(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!

(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 :)

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

(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!