Bug 56555 – [Qt] Enable GraphicsContext3D only when the window surface support OpenGL

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Jarkko Sakkinen]

This bug was cause by this change:
https://bugs.webkit.org/show_bug.cgi?id=56339

Viewport widget should not be stored into member variable because
it might not stay valid for the full life-cycle of GraphicsContext3D.

[Jarkko Sakkinen]

Created attachment 86043 [details]
Bug fix. See the ChangeLog entry for more verbose description.

[Jarkko Sakkinen]

Adding Simon and Benjamin to CC list because this in a way related to:
https://bugs.webkit.org/show_bug.cgi?id=40884

After some thinking I completely support your choice of not supporting any kind of software fallback at this point.

[Benjamin Poulain]

What about printing? Or rendering a particular frame to QImage? Wouldn't that disable the GraphicsContext3D for the following painting call?

[Jarkko Sakkinen]

You're right. In order to support those cases software fallback *is* needed. *Also*, there should be a check before allowing texture mapping in GraphicsContext3DImpl::paint() that painter->paintDevice() == m_viewportGLWidget. If not use software fallback.

I think, because of cases that you mentioned, you should reconsider reopening 40884 (QImage and printing). I'll revise this patch.

[Benjamin Poulain]

Comment on attachment 86043 [details]
Bug fix. See the ChangeLog entry for more verbose description.

I do not mind updating 40884.
Rejecting this for now.

[Jarkko Sakkinen]

Created attachment 86048 [details]
Revised fix. Allows software fallback.

[Benjamin Poulain]

Comment on attachment 86048 [details]
Revised fix. Allows software fallback.

Quick comment from a quick look: we are not limiting lines to 80 characters in WebKit, you don't have to split everything like this:
    PageClientQWidget* webPageClient
        = static_cast<PageClientQWidget*>(m_hostWindow->platformPageClient());

[Jarkko Sakkinen]

Created attachment 86062 [details]
Fixed indentation

[Jarkko Sakkinen]

Fixed indentation. This fixes now two issues:
- Crash does not occur when viewport is switched.
- Does only texture mapping if painter is associated to the viewport QGLWidget.

These are anyway valid fixes. Let's scope software fallback discussion out of this bug :)

[Benjamin Poulain]

Comment on attachment 86062 [details]
Fixed indentation

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

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:474
> +    PageClientQWidget* webPageClient
> +        = static_cast<PageClientQWidget*>(m_hostWindow->platformPageClient());

This should be on one line.

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:488
> +    PageClientQWidget* webPageClient
> +        = static_cast<PageClientQWidget*>(m_hostWindow->platformPageClient());

This should be on one line.

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:504
> +    QGLWidget* viewportGLWidget = getViewportGLWidget();
> +    if (testAcceleratedCompositing()
> +            && viewportGLWidget
> +            && viewportGLWidget == m_viewportGLWidget
> +            && viewportGLWidget == painter->device()) {

I would put those test as the first statements of the functions, and return if not true.

That way it is clear, if the initial condition are not correct, just return.

[Benjamin Poulain]

I r- just for style issues, the patch looks good to me otherwise.

[Benjamin Poulain]

Comment on attachment 86062 [details]
Fixed indentation

Thinking abou it...

Aren't the static_cast<PageClientQWidget*> wrong? You should use QWebPageClient. The return value of m_hostWindow->platformPageClient() is gonna be PageClientQGraphicsWidget for QGraphicsWebView.

[Jarkko Sakkinen]

Created attachment 86066 [details]
Fixed rest of the indentations.

After that last if-statement that you commented comes the software fallback. Therefore, it is handled that way.

[Jarkko Sakkinen]

Created attachment 86138 [details]
Check AC on creation of WebGLRenderingContext

See the ChangeLog entry for description.

[Kenneth Rohde Christiansen]

Comment on attachment 86138 [details]
Check AC on creation of WebGLRenderingContext

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

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:469
> +    QWebPageClient* webPageClient = m_hostWindow->platformPageClient();
> +
> +    QAbstractScrollArea* scrollArea = qobject_cast<QAbstractScrollArea*>(webPageClient->ownerWidget());

I would remove that unneeded newline

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:472
> +    if (scrollArea)
> +        return qobject_cast<QGLWidget*>(scrollArea->viewport());

Doesn't casting 0 return a 0? Just wondered if you could do "return qobject..."

[Jarkko Sakkinen]

The pointer scrollArea might be NULL. If it were, then scrollArea->viewport() would cause SEGFAULT.

[Jarkko Sakkinen]

Created attachment 86149 [details]
Removed unnecessary new line character

[Jarkko Sakkinen]

Created attachment 86150 [details]
Mistake in taking diff in last submit

[WebKit Commit Bot]

Comment on attachment 86150 [details]
Mistake in taking diff in last submit

Clearing flags on attachment: 86150

Committed r81468: <http://trac.webkit.org/changeset/81468>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.