Bug 108686 – [Qt][EFL][WebGL] Minor refactoring of GraphicsSurface/GraphicsSurfaceGLX

• Component: WebGL
• Status: RESOLVED FIXED

[Viatcheslav Ostapenko]

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.

[Viatcheslav Ostapenko]

Created attachment 186128 [details]
Patch

[Kenneth Rohde Christiansen]

Please cc kalyan

[Noam Rosenthal]

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?

[Viatcheslav Ostapenko]

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

[Kalyan]

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

[Kalyan]

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

[Viatcheslav Ostapenko]

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

[Kalyan]

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.

[Zeno Albisser]

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?

[Viatcheslav Ostapenko]

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!

[Viatcheslav Ostapenko]

Created attachment 186390 [details]
Patch

[Noam Rosenthal]

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" :)

[Viatcheslav Ostapenko]

Created attachment 187751 [details]
Patch

[Viatcheslav Ostapenko]

Created attachment 187755 [details]
Patch

[WebKit Review Bot]

Comment on attachment 187755 [details]
Patch

Clearing flags on attachment: 187755

Committed r142587: <http://trac.webkit.org/changeset/142587>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.