Bug 110790 – [texmap] Do not apply clip state if redundant

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Bruno Abinader (history only)]

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.

[Bruno Abinader (history only)]

Created attachment 190118 [details]
Patch

Proposed patch

[Antonio Gomes]

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?

[Bruno Abinader (history only)]

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

[Noam Rosenthal]

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

[Bruno Abinader (history only)]

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

[Noam Rosenthal]

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

[Bruno Abinader (history only)]

Created attachment 190274 [details]
Patch

Updated patch, now using only the clip state bool check.

[Bruno Abinader (history only)]

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?

[Kalyan]

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

[Kalyan]

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

[Bruno Abinader (history only)]

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

[Kalyan]

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

[Bruno Abinader (history only)]

(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

[Bruno Abinader (history only)]

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.

[Bruno Abinader (history only)]

Created attachment 190487 [details]
Patch

Proposed patch

[WebKit Review Bot]

Comment on attachment 190487 [details]
Patch

Clearing flags on attachment: 190487

Committed r144169: <http://trac.webkit.org/changeset/144169>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Bruno Abinader (history only)]

ps: Qt context ownership will be handled on bug 110966.

[WebKit Review Bot]

Re-opened since this is blocked by bug 111065

[Bruno Abinader (history only)]

All dependencies are now fixed, closing bug.