Bug 90881 – [Qt][WK2] Implement GraphicsSurface for Linux/GLX.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Zeno Albisser]

We already have the GraphicsSurface implementation for Mac.
We need a similar implementation for Linux based on GLX as well.

[Zeno Albisser]

Created attachment 151700 [details]
patch for review.

[WebKit Review Bot]

Attachment 151700 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:90:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:91:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:92:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:93:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 151700 [details]
patch for review.

Attachment 151700 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13207286

[Early Warning System Bot]

Comment on attachment 151700 [details]
patch for review.

Attachment 151700 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13212019

[Noam Rosenthal]

(In reply to comment #4)
> (From update of attachment 151700 [details])
> Attachment 151700 [details] did not pass qt-wk2-ews (qt):
> Output: http://queues.webkit.org/results/13212019

I think we need a config test for XComposite.

[Noam Rosenthal]

Comment on attachment 151700 [details]
patch for review.

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

> Source/WebCore/ChangeLog:16
> +        * platform/graphics/surfaces/GraphicsSurface.cpp:
> +        (WebCore::GraphicsSurface::create):
> +        Move creating GraphicsSurface instance into

Please add some indentations in the Changelog.

> Source/WebCore/Target.pri:4178
> +        xlibAvailable() {
> +            SOURCES += platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp
> +        }

We need a config test for XComposite, and probably add XComposite in the bots.

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:84
> +QVector<int> qglxBuildSpec()

static

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:226
> +    if (pGlBlitFramebuffer && pGlBindFramebuffer && pGlXBindTexImageEXT && pGlXReleaseTexImageEXT)

This is a bit verbose. better use one of those, or have an "initialized" static variable.

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:228
> +        return true;
> +    QOpenGLContext* glContext = p->glContext();

Add empty line

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:246
> +    if (m_texture)
> +        glDeleteTextures(1, &m_texture);
> +
> +    glGenTextures(1, &m_texture);

Do you really need to delete and recreate the texture? If so, please add a comment.

> Source/WebKit2/ChangeLog:13
> +        * Shared/ShareableSurface.cpp:
> +        (WebKit::ShareableSurface::create):
> +        Only create a GraphicsSurface from a ShareableSurface::Handle
> +        in case the Handle contains a valid GraphicsSurface token.

Indent

> Source/WebKit2/UIProcess/LayerTreeCoordinatorProxy.cpp:71
> +    // Create a unique key out of layerID and tileID.
> +    uint64_t key = layerID;
> +    key <<= 32;
> +    key |= tileID;

put this in a different function

> Tools/qmake/mkspecs/features/features.prf:174
> +    haveQt(5):!win32-*: DEFINES += WTF_USE_GRAPHICS_SURFACE=1

i wouldn't say this is right; It wouldn't work in embedded Linux or even N9.

[Kenneth Rohde Christiansen]

Comment on attachment 151700 [details]
patch for review.

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

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:68
> +        refCount--;

--refCount

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:71
> +            if (window)
> +                delete window;

delete window is enough, you do not need to null check

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:78
> +    static int refCount;
> +    static QWindow* window;

Any reason why not to add s_* ?

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:120
> +        if (m_glContext)
> +            delete m_glContext;

no need for null check

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:130
> +        if (m_surface) {
> +            delete m_surface;
> +            m_surface = 0;
> +        }

same here

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:195
> +        if (!m_surface->isVisible())
> +            return;

newline after return

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:197
> +        while (!m_surface->isExposed())
> +            QCoreApplication::processEvents();

I would also newline after this

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:233
> +    pGlBlitFramebuffer = reinterpret_cast<PFNGLBLITFRAMEBUFFERPROC>(glContext->getProcAddress("glBlitFramebuffer"));
> +    return pGlBlitFramebuffer && pGlBindFramebuffer && pGlXBindTexImageEXT && pGlXReleaseTexImageEXT;

I would also prefer a newline before the return here

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:253
> +    pGlXBindTexImageEXT(m_private->display(), m_private->glxPixmap(), GLX_FRONT_EXT, 0);
> +    return m_texture;

newline before return?

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:258
> +void GraphicsSurface::platformCopyToGLTexture(uint32_t target, uint32_t id, const IntRect& targetRect, const IntPoint& offset)
> +{
> +}

Add comment why not implemented?

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:260
> +void GraphicsSurface::platformCopyFromFramebuffer(uint32_t originFbo, const IntRect& sourceRect)

Do we always write out Framebuffer instead of just calling it FBO or so?

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:273
> +    m_private->makeCurrent();
> +    int width = m_size.width();
> +    int height = m_size.height();
> +    glPushAttrib(GL_ALL_ATTRIB_BITS);
> +    GLint oldFBO;
> +    glGetIntegerv(GL_FRAMEBUFFER_BINDING, &oldFBO);
> +    pGlBindFramebuffer(GL_READ_FRAMEBUFFER, originFbo);
> +    pGlBindFramebuffer(GL_DRAW_FRAMEBUFFER, m_private->glContext()->defaultFramebufferObject());
> +    pGlBlitFramebuffer(0, 0, width, height, 0, 0, width, height, GL_COLOR_BUFFER_BIT, GL_LINEAR);
> +    pGlBindFramebuffer(GL_FRAMEBUFFER, oldFBO);
> +    glPopAttrib();
> +    m_private->swapBuffers();

newline between logical units

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:285
> +    if (!initialize(surface->m_private))

maybe something more descriptive than initialize. maybe initializeGLXExtensions ?

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:320
> +char* GraphicsSurface::platformLock(const IntRect& rect, int* outputStride, LockOptions lockOptions)
> +{
> +    return 0;
> +}
> +
> +void GraphicsSurface::platformUnlock()
> +{
> +}

comments why those are not implemented?

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:330
> +    if (m_private)
> +        delete m_private;

no need for check

[Zeno Albisser]

Comment on attachment 151700 [details]
patch for review.

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

>> Source/WebCore/ChangeLog:16
>> +        Move creating GraphicsSurface instance into
> 
> Please add some indentations in the Changelog.

fixed.

>> Source/WebCore/Target.pri:4178
>> +        }
> 
> We need a config test for XComposite, and probably add XComposite in the bots.

added config test for Xcomposite.

>> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:68
>> +        refCount--;
> 
> --refCount

sure.

>> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:78
>> +    static QWindow* window;
> 
> Any reason why not to add s_* ?

coding style?

>> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:84
>> +QVector<int> qglxBuildSpec()
> 
> static

fixed.

>> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:120
>> +            delete m_glContext;
> 
> no need for null check

fixed.

>> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:130
>> +        }
> 
> same here

fixed.

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:224
> +static bool initialize(GraphicsSurfacePrivate* p)

I renamed this to resolveGLMethods.

>> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:226
>> +    if (pGlBlitFramebuffer && pGlBindFramebuffer && pGlXBindTexImageEXT && pGlXReleaseTexImageEXT)
> 
> This is a bit verbose. better use one of those, or have an "initialized" static variable.

Added a "static bool resolved" to return early in case all methods have previously been resolved successfully.

>> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:228
>> +    QOpenGLContext* glContext = p->glContext();
> 
> Add empty line

fixed

>> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:233
>> +    return pGlBlitFramebuffer && pGlBindFramebuffer && pGlXBindTexImageEXT && pGlXReleaseTexImageEXT;
> 
> I would also prefer a newline before the return here

ok - fixed.

>> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:246
>> +    glGenTextures(1, &m_texture);
> 
> Do you really need to delete and recreate the texture? If so, please add a comment.

You are right, i can actually reuse the existing one. And only generate a new one in case there is none.

>> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:253
>> +    return m_texture;
> 
> newline before return?

yes, sir.

>> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:258
>> +}
> 
> Add comment why not implemented?

added.

>> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:260
>> +void GraphicsSurface::platformCopyFromFramebuffer(uint32_t originFbo, const IntRect& sourceRect)
> 
> Do we always write out Framebuffer instead of just calling it FBO or so?

It is at least common in function names. And it is also how the prototype is called in GraphicsSurface.h. (even though i'm the one to blame for the naming there as well. ;) )

>> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:273
>> +    m_private->swapBuffers();
> 
> newline between logical units

ok.

>> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:285
>> +    if (!initialize(surface->m_private))
> 
> maybe something more descriptive than initialize. maybe initializeGLXExtensions ?

renamed to resolveGLMethods.

>> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:320
>> +}
> 
> comments why those are not implemented?

added.

>> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:330
>> +        delete m_private;
> 
> no need for check

removed.

>> Source/WebKit2/ChangeLog:13
>> +        in case the Handle contains a valid GraphicsSurface token.
> 
> Indent

fixed.

>> Source/WebKit2/UIProcess/LayerTreeCoordinatorProxy.cpp:71
>> +    key |= tileID;
> 
> put this in a different function

added "static inline uint64_t createUniqueKey(...)".

>> Tools/qmake/mkspecs/features/features.prf:174
>> +    haveQt(5):!win32-*: DEFINES += WTF_USE_GRAPHICS_SURFACE=1
> 
> i wouldn't say this is right; It wouldn't work in embedded Linux or even N9.

fixed with config test.

[Zeno Albisser]

Created attachment 151907 [details]
patch for review. - fixed issues as previously commented.

[Kenneth Rohde Christiansen]

Comment on attachment 151907 [details]
patch for review. - fixed issues as previously commented.

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

> Source/WebCore/WebCore.pri:209
> +contains(DEFINES, WTF_USE_GRAPHICS_SURFACE=1) {
> +    linux-*: LIBS += -lXcomposite
> +}
> +

This wont work on embedded linux will it?

[Zeno Albisser]

Comment on attachment 151907 [details]
patch for review. - fixed issues as previously commented.

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

>> Source/WebCore/WebCore.pri:209
>> +
> 
> This wont work on embedded linux will it?

If you look at features.prf further down, you will see that WTF_USE_GRAPHICS_SURFACE should only be set, in case there is an Xcomposite library. (unless manually enforced of course)

[Noam Rosenthal]

(In reply to comment #11)
> (From update of attachment 151907 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151907&action=review
> 
> >> Source/WebCore/WebCore.pri:209
> >> +
> > 
> > This wont work on embedded linux will it?
> 
> If you look at features.prf further down, you will see that WTF_USE_GRAPHICS_SURFACE should only be set, in case there is an Xcomposite library. (unless manually enforced of course)

Yes, but that's implicit... you should do something like DEFINES += HAVE_GLX=1 and then check for that define.

[Noam Rosenthal]

Comment on attachment 151907 [details]
patch for review. - fixed issues as previously commented.

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

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:193
> +            QCoreApplication::processEvents();

Hm.... this seems risky. Any other way to do it?

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:216
> +    QWindow* m_surface;
> +    QOpenGLContext* m_glContext;

Can you use OwnPtr here and avoid the delete calls?

> Source/WebKit2/UIProcess/LayerTreeCoordinatorProxy.cpp:64
> +static inline uint64_t createUniqueKey(int layerID, int tileID)

createLayerTileUniqueID

> Tools/qmake/mkspecs/features/features.prf:177
> +        linux-*:config_libXcomposite: DEFINES += WTF_USE_GRAPHICS_SURFACE=1

DEFINES += HAVE_GLX, maybe even HAVE_XCOMPOSITE

[Noam Rosenthal]

(In reply to comment #13)
> DEFINES += HAVE_GLX, maybe even HAVE_XCOMPOSITE
I mean HAVE_GLX=1

[Zeno Albisser]

Comment on attachment 151907 [details]
patch for review. - fixed issues as previously commented.

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

>>>> Source/WebCore/WebCore.pri:209
>>>> +
>>> 
>>> This wont work on embedded linux will it?
>> 
>> If you look at features.prf further down, you will see that WTF_USE_GRAPHICS_SURFACE should only be set, in case there is an Xcomposite library. (unless manually enforced of course)
> 
> Yes, but that's implicit... you should do something like DEFINES += HAVE_GLX=1 and then check for that define.

I'll change that.

>> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:193
>> +            QCoreApplication::processEvents();
> 
> Hm.... this seems risky. Any other way to do it?

I guess we can just do busy waiting. Which is kind of what we do anyway. But it seems that making the event loop process a few events would be the logical step.
In which way would that be risky?
I was thinking of using a timer to make sure that swapBuffers is executed after the surface is exposed. But that does not work either, because we need to block here and make sure that once we return the buffers have been swapped.
So i don't really see another option then actual waiting for the surface to be exposed.
This is only the problem for the very first frame, because the window creation is asynchronous.
Better ideas are welcome!

>> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:216
>> +    QOpenGLContext* m_glContext;
> 
> Can you use OwnPtr here and avoid the delete calls?

I'll see what i can do about these.

[Noam Rosenthal]

Comment on attachment 151907 [details]
patch for review. - fixed issues as previously commented.

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

>>> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:193
>>> +            QCoreApplication::processEvents();
>> 
>> Hm.... this seems risky. Any other way to do it?
> 
> I guess we can just do busy waiting. Which is kind of what we do anyway. But it seems that making the event loop process a few events would be the logical step.
> In which way would that be risky?
> I was thinking of using a timer to make sure that swapBuffers is executed after the surface is exposed. But that does not work either, because we need to block here and make sure that once we return the buffers have been swapped.
> So i don't really see another option then actual waiting for the surface to be exposed.
> This is only the problem for the very first frame, because the window creation is asynchronous.
> Better ideas are welcome!

If this only happens for the first frame I'm fine with it - please add a comment about that.

[Zeno Albisser]

Created attachment 151966 [details]
patch for review. - fixed issues as requested.

[Zeno Albisser]

Comment on attachment 151966 [details]
patch for review. - fixed issues as requested.

I'll rebase the change first.
Sorry for the noise.

[Zeno Albisser]

Created attachment 151974 [details]
patch for review. - rebased.

[WebKit Review Bot]

Attachment 151974 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:88:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:89:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:90:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:91:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Noam Rosenthal]

Comment on attachment 151974 [details]
patch for review. - rebased.

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

LGTM, see nitpick.

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:91
> +    spec[i++] = GLX_LEVEL;                          spec[i++] = 0;
> +    spec[i++] = GLX_DRAWABLE_TYPE;                  spec[i++] = GLX_PIXMAP_BIT | GLX_WINDOW_BIT;
> +    spec[i++] = GLX_BIND_TO_TEXTURE_TARGETS_EXT;    spec[i++] = GLX_TEXTURE_2D_BIT_EXT;
> +    spec[i++] = GLX_BIND_TO_TEXTURE_RGB_EXT;        spec[i++] = TRUE;

I'd rather just use a static array
static const int glxSpec[] = {
...
};

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceGLX.cpp:163
> +        QVector<int> attribList;
> +        attribList.append(GLX_TEXTURE_FORMAT_EXT);
> +        attribList.append(GLX_TEXTURE_FORMAT_RGB_EXT);
> +        attribList.append(GLX_TEXTURE_TARGET_EXT);
> +        attribList.append(GLX_TEXTURE_2D_EXT);
> +        attribList.append(0);

I'd rather use a static const array.

[Zeno Albisser]

Committed r122554: <http://trac.webkit.org/changeset/122554>

[Dongseong Hwang]

Hi, folks.

It is very grateful for UI_SIDE_COMPOSITING to support WebGL in linux now.

I have a question.

UI process binds a texture to the pixmap once, and then uses the texture.

uint32_t GraphicsSurface::platformGetTextureID()
{
    if (!m_texture) {
        glGenTextures(1, &m_texture);
        glBindTexture(GL_TEXTURE_2D, 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(m_private->display(), m_private->glxPixmap(), GLX_FRONT_EXT, 0);
    }

    return m_texture;
}

While UI process freely uses the texture which may point where is pixmap, web process swaps QWindow, which maybe change the offset which the pixmap handle points. I'm curious if it is safe. I think there maybe a unexpected behavior if UI process reads the texture while web process swaps QWindow.
I don't know well about glx. If my question is ridiculous, please understand me.

    void swapBuffers()
    {
        ...
        QOpenGLContext* glContext = QOpenGLContext::currentContext();
        if (m_surface && glContext) {
            GLint oldFBO;
            glGetIntegerv(GL_FRAMEBUFFER_BINDING, &oldFBO);
            pGlBindFramebuffer(GL_FRAMEBUFFER, glContext->defaultFramebufferObject());
            glContext->swapBuffers(m_surface.get());
            pGlBindFramebuffer(GL_FRAMEBUFFER, oldFBO);
        }
    }

[Zeno Albisser]

*** Bug 79169 has been marked as a duplicate of this bug. ***