Bug 37070 – [Qt] WebGL is not visible when QGLWidget viewport is used

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

[Jarkko Sakkinen]

Create shared OpenGL context so that WebGL view can be blitted directly without slow memory copy in the way.

[Jarkko Sakkinen]

This I attend to solve this issue. Basic idea is that every document instance needs pointer to the "main" GL context.

Common:
- FrameLoaderClientQt: add pointer for QGLContext of either graphics engine or QGLWidget viewport.
- Frame: In costructor grab pointer of QGLContext from FrameLoaderClientQt and store.
- Document: In costructor grab pointer of QGLContext from Frame and store.
- HTMLCanvasElement gets QGLContext from document and forwards it to GraphicsContext3D through WebGLRenderingContext.

QGLWidget: Easy, ask context pointer from the widget.
OpenGL graphicssystem: QPixmap painter creation actives the associated GL context.

[Jarkko Sakkinen]

Plan that requires only changes to GraphicsContext3D.

- Add pointer to the WebGLRenderingContext to construction parameters of GraphicsContext3D.
- In GraphicsContext3D constructor grab pointer through:
 * WebGLRenderingContext::canvas() -> HTMLCanvasElement 
 * HTMLCanvasElement::document() -> Document
 * Document::frameView() -> Widget (FrameView inherits from Widgets)
 * Widget::platformWidget() -> QWebPageClient
 * QWebPageClient::ownerWidget() -> QWidget/QGLWidget

Owner context can then be acquired in GraphicsContext3D. I'll create patch asap.

[Jarkko Sakkinen]

Created attachment 53049 [details]
Creates shared OpenGL context with QGLWidget viewport. This fill effectively also allow direct blitting instead of glReadPixels().

Getting rid of glReadPixels is being worked on in the bug https://bugs.webkit.org/show_bug.cgi?id=35388.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:46:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:286:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 2 in 4 files


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

[Jarkko Sakkinen]

Created attachment 53090 [details]
Fixed style issues.

[Jarkko Sakkinen]

NOTE: If this patch is to be cherry picked into the release branch it should be done only after https://bugs.webkit.org/show_bug.cgi?id=37412 is completed for which I submit patch during upcoming week. I'll do resource sandboxing after this patch gets to the upstream. It's the next logical step and finalisation for OpenGL context handling.

[Simon Hausmann]

Kenneth, I recall I mentioned this yesterday briefly during the seesions. It seems Jarkko made a patch in the meantime :)

What do you think about passing through the canvas element here? It would really help us.

[Kenneth Russell]

Comment on attachment 53090 [details]
Fixed style issues.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 6a38cff..55153cf 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,20 @@
> [phone]  Jarkko Sakkinen  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Qt] WebGL is not visible when QGLWidget viewport is used 
> +        https://bugs.webkit.org/show_bug.cgi?id=37070
> +
> +        * html/canvas/WebGLRenderingContext.cpp:
> +        (WebCore::WebGLRenderingContext::create):
> +        * platform/graphics/GraphicsContext3D.h:
> +        * platform/graphics/qt/GraphicsContext3DQt.cpp:
> +        (WebCore::GraphicsContext3DInternal::GraphicsContext3DInternal):
> +        (WebCore::GraphicsContext3DInternal::~GraphicsContext3DInternal):
> +        (WebCore::GraphicsContext3DInternal::getOwnerGLWidget):
> +        (WebCore::GraphicsContext3D::create):
> +        (WebCore::GraphicsContext3D::GraphicsContext3D):
> +
>  2010-04-09  Tasuku Suzuki  <[e-mail]>
>  
>          Reviewed by Simon Hausmann.
> diff --git a/WebCore/html/canvas/WebGLRenderingContext.cpp b/WebCore/html/canvas/WebGLRenderingContext.cpp
> index bde49ad..94e9bb6 100644
> --- a/WebCore/html/canvas/WebGLRenderingContext.cpp
> +++ b/WebCore/html/canvas/WebGLRenderingContext.cpp
> @@ -72,7 +72,12 @@ private:
>  
>  PassOwnPtr<WebGLRenderingContext> WebGLRenderingContext::create(HTMLCanvasElement* canvas, WebGLContextAttributes* attrs)
>  {
> +#if PLATFORM(QT)
> +    OwnPtr<GraphicsContext3D> context(GraphicsContext3D::create(attrs->attributes(), canvas));
> +#else
>      OwnPtr<GraphicsContext3D> context(GraphicsContext3D::create(attrs->attributes()));
> +#endif
> +
>      if (!context)
>          return 0;
>          
> diff --git a/WebCore/platform/graphics/GraphicsContext3D.h b/WebCore/platform/graphics/GraphicsContext3D.h
> index 96f77d9..4f8d1b6 100644
> --- a/WebCore/platform/graphics/GraphicsContext3D.h
> +++ b/WebCore/platform/graphics/GraphicsContext3D.h
> @@ -49,7 +49,7 @@ const Platform3DObject NullPlatform3DObject = 0;
>  
>  typedef void* PlatformGraphicsContext3D;
>  const  PlatformGraphicsContext3D NullPlatformGraphicsContext3D = 0;
> -typedef int Platform3DObject;
> +typedef GLuint Platform3DObject;
>  const Platform3DObject NullPlatform3DObject = 0;
>  #else
>  typedef void* PlatformGraphicsContext3D;
> @@ -73,6 +73,7 @@ namespace WebCore {
>      class WebGLTexture;
>      class Image;
>      class ImageData;
> +    class HTMLCanvasElement;
>  
>      struct ActiveInfo {
>          String name;
> @@ -410,7 +411,11 @@ namespace WebCore {
>              bool premultipliedAlpha;
>          };
>  
> +#if PLATFORM(QT)
> +        static PassOwnPtr<GraphicsContext3D> create(Attributes attrs, HTMLCanvasElement* canvas);
> +#else
>          static PassOwnPtr<GraphicsContext3D> create(Attributes attrs);
> +#endif
>          virtual ~GraphicsContext3D();
>  
>  #if PLATFORM(MAC)
> @@ -685,7 +690,11 @@ namespace WebCore {
>          void synthesizeGLError(unsigned long error);
>  
>      private:        
> +#if PLATFORM(QT)
> +        GraphicsContext3D(Attributes attrs, HTMLCanvasElement* canvas);
> +#else 
>          GraphicsContext3D(Attributes attrs);
> +#endif
>  
>          // Helpers for texture uploading.
>          void premultiplyAlpha(unsigned char* rgbaData, int numPixels);
> diff --git a/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp b/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp
> index caa805c..ae42739 100644
> --- a/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp
> +++ b/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp
> @@ -21,10 +21,13 @@
>  #include "GraphicsContext3D.h"
>  
>  #include "CanvasObject.h"
> +#include "FrameView.h"
>  #include "GraphicsContext.h"
>  #include "HTMLCanvasElement.h"
> +#include "HostWindow.h"
>  #include "ImageBuffer.h"
>  #include "NotImplemented.h"
> +#include "QWebPageClient.h"
>  #include "WebGLActiveInfo.h"
>  #include "WebGLArray.h"
>  #include "WebGLBuffer.h"
> @@ -37,6 +40,8 @@
>  #include "WebGLShader.h"
>  #include "WebGLTexture.h"
>  #include "WebGLUnsignedByteArray.h"
> +#include "Widget.h"
> +#include <QGraphicsView>
>  #include <wtf/UnusedParam.h>
>  #include <wtf/text/CString.h>
>  
> @@ -147,13 +152,11 @@ typedef void (APIENTRY* glVertexAttribPointerType) (GLuint, GLint, GLenum, GLboo
>  
>  class GraphicsContext3DInternal {
>  public:
> -    GraphicsContext3DInternal(GraphicsContext3D::Attributes attrs);
> +    GraphicsContext3DInternal(GraphicsContext3D::Attributes attrs, HTMLCanvasElement* canvas);
>      ~GraphicsContext3DInternal();
>  
>      bool isContextValid() { return m_contextValid; }
>  
> -
> -
>      glActiveTextureType activeTexture;
>      glAttachShaderType attachShader;
>      glBindAttribLocationType bindAttribLocation;
> @@ -255,6 +258,7 @@ public:
>  
>  private:
>  
> +    QGLWidget* getOwnerGLWidget(QWebPageClient* webPageClient);
>      void* getProcAddress(const String& proc);
>      bool m_contextValid;
>  };
> @@ -265,7 +269,7 @@ private:
>  #define GET_PROC_ADDRESS(Proc) reinterpret_cast<Proc##Type>(getProcAddress(#Proc));
>  #endif
>   
> -GraphicsContext3DInternal::GraphicsContext3DInternal(GraphicsContext3D::Attributes attrs)
> +GraphicsContext3DInternal::GraphicsContext3DInternal(GraphicsContext3D::Attributes attrs, HTMLCanvasElement* canvas)
>      : m_attrs(attrs)
>      , m_glWidget(0)
>      , m_texture(0)
> @@ -274,24 +278,33 @@ GraphicsContext3DInternal::GraphicsContext3DInternal(GraphicsContext3D::Attribut
>      , m_depthBuffer(0)
>      , m_contextValid(true)
>  {
> -    m_attrs.alpha = true;
> -    m_attrs.depth = true;
> -    m_attrs.stencil = false;
> -    m_attrs.antialias = false;
> -    m_attrs.premultipliedAlpha = true;
> +    QWebPageClient* webPageClient = canvas->document()->view()->root()->hostWindow()->platformPageClient();

Passing the HTMLCanvasElement* is a layering violation. Eric Seidel and Adam Treat point out that the HostWindow* can and should be passed instead. The chain of calls above should be done in higher-level code.

Also, please make the change to WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp and WebKit/chromium/src/GraphicsContext3D.cpp, and use an UNUSED_PARAM for the incoming HostView* on those platforms.

> +    QGLWidget* ownerGLWidget  = getOwnerGLWidget(webPageClient);
>  
> -    QGLFormat format;
> +    if (ownerGLWidget) 
> +        m_glWidget = new QGLWidget(0, ownerGLWidget);
> +    else {
> +        QGLFormat format;
> +        format.setDepth(true);
> +        format.setSampleBuffers(true);
> +        format.setStencil(false);
>  
> -    format.setDepth(true);
> -    format.setSampleBuffers(true);
> -    format.setStencil(false);
> +        m_glWidget = new QGLWidget(format);
> +    }
>  
> -    m_glWidget = new QGLWidget(format);
>      if (!m_glWidget->isValid()) {
>          LOG_ERROR("GraphicsContext3D: QGLWidget does not have a valid context");
>          m_contextValid = false;
>          return;
>      }
> + 
> +    QGLFormat format = m_glWidget->format();
> +
> +    m_attrs.alpha = format.alpha();
> +    m_attrs.depth = format.depth();
> +    m_attrs.stencil = format.stencil();
> +    m_attrs.antialias = false;
> +    m_attrs.premultipliedAlpha = true;
>  
>      m_glWidget->makeCurrent();
>  
> @@ -428,7 +441,16 @@ GraphicsContext3DInternal::GraphicsContext3DInternal(GraphicsContext3D::Attribut
>  GraphicsContext3DInternal::~GraphicsContext3DInternal()
>  {
>      delete m_glWidget;
> -    m_glWidget = 0;
> +}
> +
> +QGLWidget* GraphicsContext3DInternal::getOwnerGLWidget(QWebPageClient* webPageClient)
> +{
> +    QGraphicsView* ownerGraphicsView = qobject_cast<QGraphicsView*>(webPageClient->ownerWidget());
> +
> +    if (ownerGraphicsView) 
> +        return qobject_cast<QGLWidget*>(ownerGraphicsView->viewport());
> +
> +    return 0;
>  }
>  
>  void* GraphicsContext3DInternal::getProcAddress(const String& proc)
> @@ -448,14 +470,14 @@ void* GraphicsContext3DInternal::getProcAddress(const String& proc)
>      return 0;
>  }
>  
> -PassOwnPtr<GraphicsContext3D> GraphicsContext3D::create(GraphicsContext3D::Attributes attrs)
> +PassOwnPtr<GraphicsContext3D> GraphicsContext3D::create(GraphicsContext3D::Attributes attrs, HTMLCanvasElement* canvas)
>  {
> -    OwnPtr<GraphicsContext3D> context(new GraphicsContext3D(attrs));
> +    OwnPtr<GraphicsContext3D> context(new GraphicsContext3D(attrs, canvas));
>      return context->m_internal ? context.release() : 0;
>  }
>  
> -GraphicsContext3D::GraphicsContext3D(GraphicsContext3D::Attributes attrs)
> -    : m_internal(new GraphicsContext3DInternal(attrs))
> +GraphicsContext3D::GraphicsContext3D(GraphicsContext3D::Attributes attrs, HTMLCanvasElement* canvas)
> +    : m_internal(new GraphicsContext3DInternal(attrs, canvas))
>  {
>      if (!m_internal->isContextValid()) 
>          m_internal = 0;

[Jarkko Sakkinen]

Ok, I'll make those changes, thanks for input!

[Jarkko Sakkinen]

Created attachment 53267 [details]
HostWindow parameter

[Kenneth Russell]

Comment on attachment 53267 [details]
HostWindow parameter

Looks good overall. One minor issue.

> diff --git a/WebKit/chromium/src/GraphicsContext3D.cpp b/WebKit/chromium/src/GraphicsContext3D.cpp
> index 9fbe899..19bff11 100644
> --- a/WebKit/chromium/src/GraphicsContext3D.cpp
> +++ b/WebKit/chromium/src/GraphicsContext3D.cpp
> @@ -1044,22 +1044,25 @@ rt GraphicsContext3D::name(t1 a1, t2 a2, t3 a3, t4 a4, t5 a5, t6 a6, t7 a7, t8 a
>      return m_internal->name(a1, a2, a3, a4, a5, a6, a7, a8, a9);   \
>  }
>  
> -GraphicsContext3D::GraphicsContext3D(GraphicsContext3D::Attributes attrs)
> +GraphicsContext3D::GraphicsContext3D(GraphicsContext3D::Attributes attrs, HostWindow* hostWindow)
>  {
> +    UNUSED_PARAM(hostWindow);
>  }
>  
>  GraphicsContext3D::~GraphicsContext3D()
>  {
>  }
>  
> -PassOwnPtr<GraphicsContext3D> GraphicsContext3D::create(GraphicsContext3D::Attributes attrs)
> +PassOwnPtr<GraphicsContext3D> GraphicsContext3D::create(GraphicsContext3D::Attributes attrs, HostWindow* hostWindow)
>  {
> +    UNUSED_PARAM(hostWindow);
> +

This UNUSED_PARAM is incorrect. The one in the constructor is correct.

Note that I'm not a WebKit reviewer. CC'ing a couple of other people who may be able to provide r+ for subsequent patch.

[Jarkko Sakkinen]

Created attachment 53319 [details]
Removed UNUSED_PARAM from create method added by mistake

[Kenneth Russell]

Comment on attachment 53319 [details]
Removed UNUSED_PARAM from create method added by mistake

Looks good to me.

[Simon Hausmann]

Comment on attachment 53319 [details]
Removed UNUSED_PARAM from create method added by mistake



>  typedef void* PlatformGraphicsContext3D;
>  const  PlatformGraphicsContext3D NullPlatformGraphicsContext3D = 0;
> -typedef int Platform3DObject;
> +typedef GLuint Platform3DObject;

The ChangeLog does not explain why this is needed.

> -    m_attrs.alpha = true;
> -    m_attrs.depth = true;
> -    m_attrs.stencil = false;
> -    m_attrs.antialias = false;
> -    m_attrs.premultipliedAlpha = true;
> +    QWebPageClient* webPageClient = hostWindow->platformPageClient();
> +    QGLWidget* ownerGLWidget  = getOwnerGLWidget(webPageClient);
>  
> -    QGLFormat format;
> +    if (ownerGLWidget) 
> +        m_glWidget = new QGLWidget(0, ownerGLWidget);
> +    else {
> +        QGLFormat format;
> +        format.setDepth(true);
> +        format.setSampleBuffers(true);
> +        format.setStencil(false);
>  
> -    format.setDepth(true);
> -    format.setSampleBuffers(true);
> -    format.setStencil(false);
> +        m_glWidget = new QGLWidget(format);
> +    }

I would like to see the removal of QGLWidget altogether if possible. Not in this patch though :)


> +QGLWidget* GraphicsContext3DInternal::getOwnerGLWidget(QWebPageClient* webPageClient)
> +{
> +    QGraphicsView* ownerGraphicsView = qobject_cast<QGraphicsView*>(webPageClient->ownerWidget());
> +
> +    if (ownerGraphicsView) 
> +        return qobject_cast<QGLWidget*>(ownerGraphicsView->viewport());
> +
> +    return 0;

viewport() is not a function of QGraphicsView but its base-class QAbstractScrollArea. So the qobject_cast should cast towards QAbstractScrollArea instead of QGraphicsView.

I suggest something along these lines:

QWidget* viewport = webPageClient->ownerWidget();
if (QAbstractScrollArea* scrollArea = qobject_cast<QAbstractScrollArea*>(viewport))
    viewport = scrollArea->viewport();

return qobject_cast<QGLWidget*>(viewport);

[Jarkko Sakkinen]

1. Oops, I think I forgot to clean up that change while experimenting AC and this patch. Not related in anyway to this patch. Will clean it up :)
2. I think I'll investigate that after AC integration is complete.
3. Ok, I'll do the necessary modifications.

[Jarkko Sakkinen]

Created attachment 53522 [details]
Review changes proposed

[Jarkko Sakkinen]

Created attachment 53523 [details]
Fixed ChangeLog entries.

[Jarkko Sakkinen]

Comment for not using QGLWidget would not be necessary why it does exist? For example, QGraphicsView would not need then QGLWidget viewport then in any case. My academic guess is that there is strong dependence for OpenGL QPaintDevice/QPaintEngine built into QGLContext. 

For shared context case (i.e. we have QGLWidget viewport), it might be possible to create just shared QGLContext (haven't got this work in reality though) but then the implementation would require either instance of QGLWidget or QGLContext and I think that is uglier than current approach. But anyway, this goes beyond scope of this bug.

[Jarkko Sakkinen]

Forgot "could be turned around" from the first sentence :)

[Jarkko Sakkinen]

This thread mostly confirms everything mentioned: http://lists.trolltech.com/qt-interest/2008-08/thread00046-0.html

[Simon Hausmann]

(In reply to comment #20)
> This thread mostly confirms everything mentioned:
> http://lists.trolltech.com/qt-interest/2008-08/thread00046-0.html

Thanks, that's a good reference indeed. I think Trond's statement gives clarity: "Just use the QGLPixelBuffer class for offscreen rendering."

[WebKit Commit Bot]

Comment on attachment 53523 [details]
Fixed ChangeLog entries.

Clearing flags on attachment: 53523

Committed r57747: <http://trac.webkit.org/changeset/57747>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.