Bug 37070 - [Qt] WebGL is not visible when QGLWidget viewport is used
Summary: [Qt] WebGL is not visible when QGLWidget viewport is used
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-04 01:38 PDT by Jarkko Sakkinen
Modified: 2010-04-16 14:39 PDT (History)
6 users (show)

See Also:


Attachments
Creates shared OpenGL context with QGLWidget viewport. This fill effectively also allow direct blitting instead of glReadPixels(). (7.86 KB, patch)
2010-04-10 12:41 PDT, Jarkko Sakkinen
no flags Details | Formatted Diff | Diff
Fixed style issues. (7.83 KB, patch)
2010-04-11 08:04 PDT, Jarkko Sakkinen
no flags Details | Formatted Diff | Diff
HostWindow parameter (11.49 KB, patch)
2010-04-13 11:32 PDT, Jarkko Sakkinen
no flags Details | Formatted Diff | Diff
Removed UNUSED_PARAM from create method added by mistake (11.41 KB, patch)
2010-04-13 22:45 PDT, Jarkko Sakkinen
hausmann: review-
hausmann: commit-queue-
Details | Formatted Diff | Diff
Review changes proposed (11.04 KB, patch)
2010-04-16 01:00 PDT, Jarkko Sakkinen
no flags Details | Formatted Diff | Diff
Fixed ChangeLog entries. (11.04 KB, patch)
2010-04-16 01:06 PDT, Jarkko Sakkinen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jarkko Sakkinen 2010-04-04 01:38:54 PDT
Create shared OpenGL context so that WebGL view can be blitted directly without slow memory copy in the way.
Comment 1 Jarkko Sakkinen 2010-04-09 01:14:30 PDT
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.
Comment 2 Jarkko Sakkinen 2010-04-09 02:58:42 PDT
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.
Comment 3 Jarkko Sakkinen 2010-04-10 12:41:58 PDT
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.
Comment 4 WebKit Review Bot 2010-04-10 12:48:49 PDT
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.
Comment 5 Jarkko Sakkinen 2010-04-11 08:04:40 PDT
Created attachment 53090 [details]
Fixed style issues.
Comment 6 Jarkko Sakkinen 2010-04-11 12:55:26 PDT
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.
Comment 7 Simon Hausmann 2010-04-13 07:06:00 PDT
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.
Comment 8 Kenneth Russell 2010-04-13 09:42:09 PDT
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 @@
> +2010-04-10  Jarkko Sakkinen  <jarkko.j.sakkinen@gmail.com>
> +
> +        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  <tasuku.suzuki@nokia.com>
>  
>          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;
Comment 9 Jarkko Sakkinen 2010-04-13 10:44:58 PDT
Ok, I'll make those changes, thanks for input!
Comment 10 Jarkko Sakkinen 2010-04-13 11:32:02 PDT
Created attachment 53267 [details]
HostWindow parameter
Comment 11 Kenneth Russell 2010-04-13 16:44:09 PDT
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.
Comment 12 Jarkko Sakkinen 2010-04-13 22:45:18 PDT
Created attachment 53319 [details]
Removed UNUSED_PARAM from create method added by mistake
Comment 13 Kenneth Russell 2010-04-14 01:10:05 PDT
Comment on attachment 53319 [details]
Removed UNUSED_PARAM from create method added by mistake

Looks good to me.
Comment 14 Simon Hausmann 2010-04-15 11:46:03 PDT
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);
Comment 15 Jarkko Sakkinen 2010-04-15 14:55:53 PDT
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.
Comment 16 Jarkko Sakkinen 2010-04-16 01:00:04 PDT
Created attachment 53522 [details]
Review changes proposed
Comment 17 Jarkko Sakkinen 2010-04-16 01:06:25 PDT
Created attachment 53523 [details]
Fixed ChangeLog entries.
Comment 18 Jarkko Sakkinen 2010-04-16 03:02:23 PDT
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.
Comment 19 Jarkko Sakkinen 2010-04-16 03:03:26 PDT
Forgot "could be turned around" from the first sentence :)
Comment 20 Jarkko Sakkinen 2010-04-16 03:46:57 PDT
This thread mostly confirms everything mentioned: http://lists.trolltech.com/qt-interest/2008-08/thread00046-0.html
Comment 21 Simon Hausmann 2010-04-16 13:46:31 PDT
(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."
Comment 22 WebKit Commit Bot 2010-04-16 14:39:46 PDT
Comment on attachment 53523 [details]
Fixed ChangeLog entries.

Clearing flags on attachment: 53523

Committed r57747: <http://trac.webkit.org/changeset/57747>
Comment 23 WebKit Commit Bot 2010-04-16 14:39:52 PDT
All reviewed patches have been landed.  Closing bug.