Bug 88704 - [Qt][Texmap] Falling leaves demo missing clipping.
Summary: [Qt][Texmap] Falling leaves demo missing clipping.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-09 00:34 PDT by Dongseong Hwang
Modified: 2012-06-12 06:33 PDT (History)
2 users (show)

See Also:


Attachments
patch v.1 (2.76 KB, patch)
2012-06-09 00:36 PDT, Dongseong Hwang
no flags Details | Formatted Diff | Diff
patch v.2 (2.54 KB, patch)
2012-06-11 23:47 PDT, Dongseong Hwang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dongseong Hwang 2012-06-09 00:34:40 PDT
The bug originated from begining native painting mode of QPainter after enabling
scissor test, because begining native painting mode disables scissor test.
Comment 1 Dongseong Hwang 2012-06-09 00:36:22 PDT
Created attachment 146691 [details]
patch v.1
Comment 2 Noam Rosenthal 2012-06-09 09:22:19 PDT
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?
Comment 3 Dongseong Hwang 2012-06-10 16:31:23 PDT
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.
Comment 4 Dongseong Hwang 2012-06-10 16:33:40 PDT
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.
Comment 5 Dongseong Hwang 2012-06-11 04:34:29 PDT
(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.
Comment 6 Noam Rosenthal 2012-06-11 06:08:57 PDT
(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);
> >
Comment 7 Dongseong Hwang 2012-06-11 23:47:22 PDT
Created attachment 147022 [details]
patch v.2
Comment 8 Dongseong Hwang 2012-06-11 23:53:22 PDT
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.
Comment 9 Noam Rosenthal 2012-06-12 06:26:56 PDT
Comment on attachment 147022 [details]
patch v.2

OK, I misread the patch. LGtm.
Comment 10 WebKit Review Bot 2012-06-12 06:33:53 PDT
Comment on attachment 147022 [details]
patch v.2

Clearing flags on attachment: 147022

Committed r120066: <http://trac.webkit.org/changeset/120066>
Comment 11 WebKit Review Bot 2012-06-12 06:33:57 PDT
All reviewed patches have been landed.  Closing bug.