Bug 88704 – [Qt][Texmap] Falling leaves demo missing clipping.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Dongseong Hwang]

The bug originated from begining native painting mode of QPainter after enabling
scissor test, because begining native painting mode disables scissor test.

[Dongseong Hwang]

Created attachment 146691 [details]
patch v.1

[Noam Rosenthal]

Comment on attachment 146691 [details]
patch v.1

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

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:-270
>      glDisable(GL_DEPTH_TEST);
>      glEnable(GL_SCISSOR_TEST);
> -#if PLATFORM(QT)
> -    if (m_context) {
> -        QPainter* painter = m_context->platformContext();
> -        painter->save();
> -        painter->beginNativePainting();
> -    }
> -#endif

Shouldn't the new code be called before the other GL calls?

[Dongseong Hwang]

Comment on attachment 146691 [details]
patch v.1

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

>> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:-270
>> -#endif
> 
> Shouldn't the new code be called before the other GL calls?

I think there is no problem to put initializeOpenGLShims() before PLATFORM(QT) code, because initializeOpenGLShims() does not change any GL states and QPainter does not need something like OpenGLShims.
Moreover, I think it is more obvious to save gl states for QPainter before starting Texmap routines like TextureMapperGL::endPainting restores gl states for QPainter after all Texmap tasks are finished.

[Dongseong Hwang]

Oops, I made typo
(In reply to comment #3)
> (From update of attachment 146691 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146691&action=review
> 
> >> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:-270
> >> -#endif
> > 
> > Shouldn't the new code be called before the other GL calls?
> 
> I think there is no problem to put initializeOpenGLShims() before PLATFORM(QT) code, because initializeOpenGLShims() does not change any GL states and QPainter does not need something like OpenGLShims.
                                                                                 before -> after
> Moreover, I think it is more obvious to save gl states for QPainter before starting Texmap routines like TextureMapperGL::endPainting restores gl states for QPainter after all Texmap tasks are finished.

[Dongseong Hwang]

(In reply to comment #2)
> (From update of attachment 146691 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146691&action=review
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:-270
> >      glDisable(GL_DEPTH_TEST);
> >      glEnable(GL_SCISSOR_TEST);
> > -#if PLATFORM(QT)
> > -    if (m_context) {
> > -        QPainter* painter = m_context->platformContext();
> > -        painter->save();
> > -        painter->beginNativePainting();
> > -    }
> > -#endif
> 
> Shouldn't the new code be called before the other GL calls?

Would you like to explain more detail. I think I don't understand what you mean.

[Noam Rosenthal]

(In reply to comment #5)
> (In reply to comment #2)
> > (From update of attachment 146691 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=146691&action=review
> > 
> > > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:-270
> > >      glDisable(GL_DEPTH_TEST);
> > >      glEnable(GL_SCISSOR_TEST);
> > > -#if PLATFORM(QT)
> > > -    if (m_context) {
> > > -        QPainter* painter = m_context->platformContext();
> > > -        painter->save();
> > > -        painter->beginNativePainting();
> > > -    }
> > > -#endif
> > 
> > Shouldn't the new code be called before the other GL calls?
> 
> Would you like to explain more detail. I think I don't understand what you mean.

i'm saying it should look more like this:

> > > -#if PLATFORM(QT)
> > > -    if (m_context) {
> > > -        QPainter* painter = m_context->platformContext();
> > > -        painter->save();
> > > -        painter->beginNativePainting();
> > > -    }
> > > -#endif
> > >      glDisable(GL_DEPTH_TEST);
> > >      glEnable(GL_SCISSOR_TEST);
> >

[Dongseong Hwang]

Created attachment 147022 [details]
patch v.2

[Dongseong Hwang]

Comment on attachment 147022 [details]
patch v.2

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

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:270
>      data().previousDepthState = glIsEnabled(GL_DEPTH_TEST);

I'm afraid that it is right to save previous states before begin NativePainting, because restoring previous states is before ending NativePainting in TextureMapperGL::endPainting().
It is asymmetric between beginPainting and endPainting.

[Noam Rosenthal]

Comment on attachment 147022 [details]
patch v.2

OK, I misread the patch. LGtm.

[WebKit Review Bot]

Comment on attachment 147022 [details]
patch v.2

Clearing flags on attachment: 147022

Committed r120066: <http://trac.webkit.org/changeset/120066>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.