Bug 110790

Summary: [texmap] Do not apply clip state if redundant
Product: WebKit Reporter: Bruno Abinader (history only) <bruno.abinader>
Component: Layout and RenderingAssignee: Bruno Abinader (history only) <bruno.abinader>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, igor.oliveira, kalyan.kondapally, kenneth, kondapallykalyan, luiz, noam, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 111065, 111566    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Bruno Abinader (history only) 2013-02-25 13:43:47 PST
Adds a pointer from the last context and a bool to check if clip state has been changed since last apply. If the context is the same and the clip state hasn't changed, then do nothing.
Comment 1 Bruno Abinader (history only) 2013-02-25 13:46:51 PST
Created attachment 190118 [details]
Patch

Proposed patch
Comment 2 Antonio Gomes 2013-02-26 06:21:34 PST
Comment on attachment 190118 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Adds a pointer from the last context and a bool to check if clip state
> +        has been changed since last apply. If the context is the same and the
> +        clip state hasn't changed, then do nothing.
> +
> +        No behavior changes, thus covered by existing tests.

if it has measurable performance gains, it should be mentioned.

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:182
> +    clipStateChangedSinceLastApply = true;

should last context be reset as well?
Comment 3 Bruno Abinader (history only) 2013-02-26 06:30:46 PST
(In reply to comment #2)
> (From update of attachment 190118 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190118&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        Adds a pointer from the last context and a bool to check if clip state
> > +        has been changed since last apply. If the context is the same and the
> > +        clip state hasn't changed, then do nothing.
> > +
> > +        No behavior changes, thus covered by existing tests.
> 
> if it has measurable performance gains, it should be mentioned.

Indeed. I've been using gDEBugger to profile OpenGL calls while running CSS 3D Transforms-related examples, and noticed a ~30% reduction in glGetIntegeri calls, same for glStencil{Op,Func}.

> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:182
> > +    clipStateChangedSinceLastApply = true;
> 
> should last context be reset as well?

Yes, this is done in TextureMapperGL::ClipStack::apply(), which is the only entry point for the context object pointer.
Comment 4 Noam Rosenthal 2013-02-26 06:33:48 PST
Comment on attachment 190118 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.h:93
> +            : lastContext(0)

Do we ever change the context for a TextureMapperGL??
Comment 5 Bruno Abinader (history only) 2013-02-26 06:38:26 PST
(In reply to comment #4)
> (From update of attachment 190118 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190118&action=review
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperGL.h:93
> > +            : lastContext(0)
> 
> Do we ever change the context for a TextureMapperGL??

From my tests, lastContext remains with the same points in both Qt/EFL ports, however I'm not sure about the other ports. Said this, this actually makes me think whether it would be good to have the context object pointer passed on a separate function (or even at ClipStack ctor) instead of apply(), what do you think?
Comment 6 Noam Rosenthal 2013-02-26 06:40:50 PST
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 190118 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=190118&action=review
> > 
> > > Source/WebCore/platform/graphics/texmap/TextureMapperGL.h:93
> > > +            : lastContext(0)
> > 
> > Do we ever change the context for a TextureMapperGL??
> 
> From my tests, lastContext remains with the same points in both Qt/EFL ports, however I'm not sure about the other ports. Said this, this actually makes me think whether it would be good to have the context object pointer passed on a separate function (or even at ClipStack ctor) instead of apply(), what do you think?

TMGL should always have the same context.
Your idea seems reasonsable... However let's have only the clipStateIsDirty flag here and not deal with a context change.
Comment 7 Bruno Abinader (history only) 2013-02-26 06:48:48 PST
Created attachment 190274 [details]
Patch

Updated patch, now using only the clip state bool check.
Comment 8 Bruno Abinader (history only) 2013-02-26 07:15:56 PST
While discussing the implementation with Igor, he noticed that for Qt port, the context ownership is from the Qt layer, thus vulnerable to clip state changes not covered by TextureMapperGL. Shall we use #ifdef's to EFL and GTK ports (which has the ownership of the context object) then?
Comment 9 Kalyan 2013-02-26 10:53:47 PST
(In reply to comment #8)
> While discussing the implementation with Igor, he noticed that for Qt port, the context ownership is from the Qt layer, thus vulnerable to clip state changes not covered by TextureMapperGL. Shall we use #ifdef's to EFL and GTK ports (which has the ownership of the context object) then?


This is a general issue and Depends on what the context is doing. What textureMapperGL does is it creates a wrapper for the current context and uses it. It manipulates certain states and reset's them to there original values.  Now assumption here seems to be that the Context (which texturemapper wraps around) doesn't manipulate the  clip state during it's life time. So I think we need some kind of general decision whether TextureMapperGL tracks GL states or is it the caller responsibility. For Qt and EFL ports the main concern should be with the Context created on UI process side. Efl port doesn't manipulate the clip state and apps dont have any control on it (directly atleast).
Comment 10 Kalyan 2013-02-26 11:33:27 PST
(In reply to comment #9)
> (In reply to comment #8)
> > While discussing the implementation with Igor, he noticed that for Qt port, the context ownership is from the Qt layer, thus vulnerable to clip state changes not covered by TextureMapperGL. Shall we use #ifdef's to EFL and GTK ports (which has the ownership of the context object) then?
> 
> 
> doesn't manipulate the  clip state during it's life time. So I think we need >some kind of general decision whether TextureMapperGL tracks GL states or is it >the caller responsibility. For Qt and EFL ports the main concern should be with >the Context created on UI process side. Efl port doesn't manipulate the clip >state and apps dont have any control on it (directly atleast).

What we could have is a way (perhaps some optimization flag??) to inform TextureMapperGL that it is the only one manipulating the context state and thus doesn't have to worry about resetting and setting the states every frame(i.e in begin painting, begin clip etc). I see a hackish way to do it already by calling beingPainting/clip once and not calling the end counterparts :) Perhaps we need a bit more elegant solution to handle this case where TextureMapperGL is the only one manipulating the Context state.
Comment 11 Bruno Abinader (history only) 2013-02-26 12:12:49 PST
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > While discussing the implementation with Igor, he noticed that for Qt port, the context ownership is from the Qt layer, thus vulnerable to clip state changes not covered by TextureMapperGL. Shall we use #ifdef's to EFL and GTK ports (which has the ownership of the context object) then?
> > 
> > 
> > doesn't manipulate the  clip state during it's life time. So I think we need >some kind of general decision whether TextureMapperGL tracks GL states or is it >the caller responsibility. For Qt and EFL ports the main concern should be with >the Context created on UI process side. Efl port doesn't manipulate the clip >state and apps dont have any control on it (directly atleast).
> 
> What we could have is a way (perhaps some optimization flag??) to inform TextureMapperGL that it is the only one manipulating the context state and thus doesn't have to worry about resetting and setting the states every frame(i.e in begin painting, begin clip etc). I see a hackish way to do it already by calling beingPainting/clip once and not calling the end counterparts :) Perhaps we need a bit more elegant solution to handle this case where TextureMapperGL is the only one manipulating the Context state.

I agree with that, however this approach sounds feasible only on EFL port AFAIK. Shall it be a platform-specific implementation or so?
Comment 12 Kalyan 2013-02-26 15:02:17 PST
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (In reply to comment #8)

> I agree with that, however this approach sounds feasible only on EFL port AFAIK. Shall it be a platform-specific implementation or so?

In short it would be quite useful in cases where context doesn't manipulate states having an impact on TMGL. Ignore my earlier comments as I was of the impression that we are optimizing the save/restore state of TMGL. I would follow it up with another issue. Sorry for the unnecessary noise.

Bit confused as to why patch doesn't apply for Qt. If I understood the patch correctly, we optimize apply() calls between a given combination of reset() and pop() (i.e begin/end calls). Isn't that the case here??
Comment 13 Bruno Abinader (history only) 2013-02-27 03:26:07 PST
(In reply to comment #12)
> Bit confused as to why patch doesn't apply for Qt. If I understood the patch correctly, we optimize apply() calls between a given combination of reset() and pop() (i.e begin/end calls). Isn't that the case here??

Yes, that is correct. However, on Qt, the context ownership belongs does not belong to WebKit[1], it may be used/clipped elsewhere from TextureMapperGL[2] and might result in unexpected inconsistencies.

Links:
[1] http://qt.gitorious.org/qt/qtdeclarative/blobs/stable/src/quick/items/qquickthreadedwindowmanager.cpp#line142
[2] http://qt.gitorious.org/qt/qtdeclarative/blobs/stable/src/quick/items/qquickthreadedwindowmanager_p.h#line86
Comment 14 Bruno Abinader (history only) 2013-02-27 03:46:19 PST
As discussed with No'am, the issue with Qt indeed exists but is out of the scope of this patch (we can create another bug to handle that). I'm going to provide an updated patch with a cleaner variable name in a few mins.
Comment 15 Bruno Abinader (history only) 2013-02-27 03:50:00 PST
Created attachment 190487 [details]
Patch

Proposed patch
Comment 16 WebKit Review Bot 2013-02-27 04:07:54 PST
Comment on attachment 190487 [details]
Patch

Clearing flags on attachment: 190487

Committed r144169: <http://trac.webkit.org/changeset/144169>
Comment 17 WebKit Review Bot 2013-02-27 04:07:59 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 Bruno Abinader (history only) 2013-02-27 04:19:12 PST
ps: Qt context ownership will be handled on bug 110966.
Comment 19 WebKit Review Bot 2013-02-28 03:46:45 PST
Re-opened since this is blocked by bug 111065
Comment 20 Bruno Abinader (history only) 2013-03-12 10:31:55 PDT
All dependencies are now fixed, closing bug.