Bug 108686

Summary: [Qt][EFL][WebGL] Minor refactoring of GraphicsSurface/GraphicsSurfaceGLX
Product: WebKit Reporter: Viatcheslav Ostapenko <ostap73>
Component: WebGLAssignee: Viatcheslav Ostapenko <ostap73>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, kalyan.kondapally, kenneth, noam, rafael.lobo, webkit.review.bot, zeno
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Viatcheslav Ostapenko 2013-02-01 13:44:58 PST
Remove unused platformSurface()/m_platformSurface from GraphicsSurface.
Move m_texture from GraphicsSurface to GLX GraphicsSurfacePrivate. It is used only by GraphicsSurfaceGLX. Win and Mac implementation store texture inside private.
Comment 1 Viatcheslav Ostapenko 2013-02-01 13:50:05 PST
Created attachment 186128 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2013-02-01 14:00:48 PST
Please cc kalyan
Comment 3 Noam Rosenthal 2013-02-03 01:06:39 PST
Comment on attachment 186128 [details]
Patch

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

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:440
> +
> +    GLuint texture() const
> +    {
> +        if (!m_texture) {
> +            GLXPixmap pixmap = glxPixmap();
> +            if (!pixmap)
> +                return 0;
> +
> +            GraphicsSurfacePrivate* that = const_cast<GraphicsSurfacePrivate*>(this);

Why make this const?
Comment 4 Viatcheslav Ostapenko 2013-02-03 01:47:08 PST
(In reply to comment #3)
> (From update of attachment 186128 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186128&action=review
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:440
> > +
> > +    GLuint texture() const
> > +    {
> > +        if (!m_texture) {
> > +            GLXPixmap pixmap = glxPixmap();
> > +            if (!pixmap)
> > +                return 0;
> > +
> > +            GraphicsSurfacePrivate* that = const_cast<GraphicsSurfacePrivate*>(this);
> 
> Why make this const?

Texture is allocated once, but read often. Just to give compiler chance to optimize it. Have seen this pattern in many places in webkit&Qt.
Comment 5 Kalyan 2013-02-03 02:49:35 PST
Comment on attachment 186128 [details]
Patch

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

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:363
> +            pGlXBindTexImageEXT(display(), glxPixmap(), GLX_FRONT_EXT, 0);

Should we bind and release the texture immediately?? Is there any reason why we dont release it till next swapbuffer call??

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:447
> +            pGlXBindTexImageEXT(display(), pixmap, GLX_FRONT_EXT, 0);

If we agree that it is k to bind and release the texture in swapbuffer call, we could call swapbuffer here to have the updated content.

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:474
> +            pGlXReleaseTexImageEXT(display(), glxPixmap(), GLX_FRONT_EXT);

we dont have to do this, incase we already release the texture in swapbuffer call
Comment 6 Kalyan 2013-02-03 03:05:55 PST
Comment on attachment 186128 [details]
Patch

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

>> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:363
>> +            pGlXBindTexImageEXT(display(), glxPixmap(), GLX_FRONT_EXT, 0);
> 
> Should we bind and release the texture immediately?? Is there any reason why we dont release it till next swapbuffer call??

To clarify, what I meant was to reverse the order. Instead of release -> bind we should have bind -> release
Comment 7 Viatcheslav Ostapenko 2013-02-03 06:57:41 PST
(In reply to comment #6)
> (From update of attachment 186128 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186128&action=review
> 
> >> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:363
> >> +            pGlXBindTexImageEXT(display(), glxPixmap(), GLX_FRONT_EXT, 0);
> > 
> > Should we bind and release the texture immediately?? Is there any reason why we dont release it till next swapbuffer call??
> 
> To clarify, what I meant was to reverse the order. Instead of release -> bind we should have bind -> release

Originally it was like this in my patch, but somebody had concerns about it.
It works either way on ATI fglrx and mesa, but rebind makes problems on nvidia. It doesn't work either way on xorg-radeon ;)
Comment 8 Kalyan 2013-02-03 10:08:18 PST
Comment on attachment 186128 [details]
Patch

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

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:364
> +

k, LGTM.
Comment 9 Zeno Albisser 2013-02-04 08:07:27 PST
Comment on attachment 186128 [details]
Patch

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

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:358
> +        // re-bind texture the XWindow.

I'm not sure what you meant to say here. " ... has to re-bind the texture to the XWindow." may be?
Comment 10 Viatcheslav Ostapenko 2013-02-04 08:23:44 PST
Comment on attachment 186128 [details]
Patch

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

>> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:358
>> +        // re-bind texture the XWindow.
> 
> I'm not sure what you meant to say here. " ... has to re-bind the texture to the XWindow." may be?

Oh! Yes! Thanks!
Comment 11 Viatcheslav Ostapenko 2013-02-04 08:43:10 PST
Created attachment 186390 [details]
Patch
Comment 12 Noam Rosenthal 2013-02-11 13:43:35 PST
Comment on attachment 186390 [details]
Patch

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

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:357
> +        // The buffers are being switched on the writing side, the reading side has to

switched, you mean swapped?

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:361
> +            // Release previous lock and rebind texture to surface to get frame update.

"to get frame update", you mean "to ensure the frame updates"? or "facilitate a frame update"?

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:450
> +        if (!m_texture) {
> +            GLXPixmap pixmap = glxPixmap();
> +            if (!pixmap)
> +                return 0;
> +
> +            GraphicsSurfacePrivate* that = const_cast<GraphicsSurfacePrivate*>(this);
> +            glGenTextures(1, &(that->m_texture));
> +            glBindTexture(GL_TEXTURE_2D, that->m_texture);
> +            glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
> +            glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
> +            glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
> +            glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
> +            pGlXBindTexImageEXT(display(), pixmap, GLX_FRONT_EXT, 0);
> +        }
> +
> +        return m_texture;

I'd prefer to construct this method differently:
1. return early if there is an m_texture.
2. use a local texture variable, and then at the end do the const_cast thing to save it to a member.
This way the area where you const_cast is easy to read and understand, without "that" :)
Comment 13 Viatcheslav Ostapenko 2013-02-11 19:06:37 PST
Created attachment 187751 [details]
Patch
Comment 14 Viatcheslav Ostapenko 2013-02-11 19:16:21 PST
Created attachment 187755 [details]
Patch
Comment 15 WebKit Review Bot 2013-02-11 23:28:23 PST
Comment on attachment 187755 [details]
Patch

Clearing flags on attachment: 187755

Committed r142587: <http://trac.webkit.org/changeset/142587>
Comment 16 WebKit Review Bot 2013-02-11 23:28:27 PST
All reviewed patches have been landed.  Closing bug.