560537 - Implement Top level GL layer manager for Qt widget port

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Qt, defect)

Description

[Oleg Romashin (:romaxa)]

If I understood correctly, to make Qt widget rendering with GL accelerated layers we should have:
1) QGLWidget backend enabled for toplevel QGraphicsView.
2) Add some detection for accelerated mode (MOZ_QT_GL=1)
3) Fix glContext wrappers (gfx/layers/opengl) when MOZ_WIDGET_QT defined, and make sure that it works and does not conflict with QT gl context.

I'll add some simple QGLWidget enabler, and layers automanager setup.
The rest of work need to be done later.
Fix me if I'm wrong here.

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: simple layers setup

Comment 2

[Oleg Romashin (:romaxa)]

Attachment: This seems to work fine with patch from bug 562409

Comment 3

[Oleg Romashin (:romaxa)]

Attachment: Better patch

Comment 4

[Oleg Romashin (:romaxa)]

Attachment: Switch offscreen buffer creation to platform->CreateOffscreenSurface

Use platform optimized offscreen for widget cached buffer surface

Comment 5

[Oleg Romashin (:romaxa)]

Attachment: Add DIRECT_X rendering mode, for GL layers rendering

With GL rendering I think we don't want offscreen buffer anymore, and default target for layer manager should be QWidget - XWindow.

Comment 6

[Joe Drew (not getting mail)]

Comment on attachment 449295 [details] [diff] [review]
Better patch

Bas should look at this too. In particular I'm not sure about DoPaint - don't we need an AutoLayerManagerSetup in the LAYERS_OPENGL path?

Comment 7

[Bas Schouten (:bas.schouten)]

(In reply to comment #6)
> (From update of attachment 449295 [details] [diff] [review])
> Bas should look at this too. In particular I'm not sure about DoPaint - don't
> we need an AutoLayerManagerSetup in the LAYERS_OPENGL path?

We do not, AutoLayerManagerSetup is only for BasicLayerManagers. Its results are even unpredictable for others since it will just static_cast to BasicLayerManager*.

Comment 8

[Oleg Romashin (:romaxa)]

Comment on attachment 449639 [details] [diff] [review]
Add DIRECT_X rendering mode, for GL layers rendering

Make rendering mode directly to QWidget (XSurface), and allow to avoid memory copy

Comment 9

[Joe Drew (not getting mail)]

Comment on attachment 449637 [details] [diff] [review]
Switch offscreen buffer creation to platform->CreateOffscreenSurface

I like this patch a lot.

Comment 10

[Oleg Romashin (:romaxa)]

Comment on attachment 449637 [details] [diff] [review]
Switch offscreen buffer creation to platform->CreateOffscreenSurface

This patch moved to separate bug 571431

Comment 11

[Oleg Romashin (:romaxa)]

Attachment: Tested on GLX layers QT desktop, works with patches from 565833

Comment 12

[Oleg Romashin (:romaxa)]

Attachment: Make EGL provider use GL context created by Qt

Comment 13

[Oleg Romashin (:romaxa)]

Attachment: Tested with GLX, EGL, QT-GL context

For Qt we have multiple QGraphicsWidget's but all of them painted to the same X-Window (QGraphicsView viewport).

It is easier to create GLContext using Qt API, and adopt it to mozilla::gl provider code.

Comment 14

[Oleg Romashin (:romaxa)]

Attachment: Updated to trunk, fixed conflicts with GL/QGL defines, works fine on maemo device

Comment 15

[Oleg Romashin (:romaxa)]

Attachment: Make EGL provider use GL context created by Qt

Less QT ifdefs for QT-GL context creation.

Comment 16

[Joe Drew (not getting mail)]

Comment on attachment 451488 [details] [diff] [review]
Updated to trunk, fixed conflicts with GL/QGL defines, works fine on maemo device


>diff -r 7f9aa069f7b4 widget/src/qt/nsWindow.cpp

>+#define GLfloat GLdouble
>+#define GLdouble GLfloat

This looks very wrong.

Other than that looks ok though.

Comment 17

[Joe Drew (not getting mail)]

Comment on attachment 451554 [details] [diff] [review]
Make EGL provider use GL context created by Qt


>diff -r b48e3441abcb gfx/thebes/src/GLContextProviderEGL.cpp

>+#define GLfloat GLdouble
>+#define GLdouble GLfloat

This looks very wrong.

>     ~GLContextEGL()
>     {
>+        // If mGLWidget defined then Widget owning GL context

"If mGLWidget is non-null, then we've been given it by the GL context provider, and it's managed by the widget implementation. In this case, We can't destroy our contexts."

>+        if (mGLWidget)
>+            return;
>+
>         sEGLLibrary.fDestroyContext(mDisplay, mContext);
> 	sEGLLibrary.fDestroySurface(mDisplay, mSurface);

While you're in here, can you fix the indentation of those two lines?

>     }

> already_AddRefed<GLContext>
> GLContextProvider::CreateForWindow(nsIWidget *aWidget)

>+        // All Qt nsIWidget's have the same X-Window surface
>+        // And EGL not allowing to create multiple GL context for the same window
>+        // we should be able to create GL context for QGV viewport once, and reuse it for all child widgets
>+        NS_ERROR("Need special GLContext implementation for QT widgets structure");

This should probably be an NS_RUNTIME_ABORT, unless we're sure that returning nsnull from this function will be handled properly.

>+        return nsnull;

Comment 18

[Oleg Romashin (:romaxa)]

> 
> >+        if (mGLWidget)
> >+            return;
> >+
> >         sEGLLibrary.fDestroyContext(mDisplay, mContext);
> > 	sEGLLibrary.fDestroySurface(mDisplay, mSurface);
> 
> While you're in here, can you fix the indentation of those two lines?

probably it is better to attach another patch which is fixing only indent for this file.

> >+        NS_ERROR("Need special GLContext implementation for QT widgets structure");
> 
> This should probably be an NS_RUNTIME_ABORT, unless we're sure that returning
> nsnull from this function will be handled properly.
> 
> >+        return nsnull;

returning nsnull for this function will switch to software rendering, and nothing serious should happen..

Comment 19

[Oleg Romashin (:romaxa)]

Attachment: Qt Top level GL, fixed review comments

Comment 20

[Oleg Romashin (:romaxa)]

Attachment: Fix indent for EGL provider

Comment 21

[Oleg Romashin (:romaxa)]

Attachment: Updated to fixed indent, fixed joe comments

Comment 22

[Oleg Romashin (:romaxa)]

Attachment: qgl.h conflicts with GLDefs.h fix

Comment 23

[Bas Schouten (:bas.schouten)]

Comment on attachment 452801 [details] [diff] [review]
Updated to fixed indent, fixed joe comments

Please take into account that a GLContext can theoretically outlive its Widget.

Comment 24

[Bas Schouten (:bas.schouten)]

Comment on attachment 452800 [details] [diff] [review]
Fix indent for EGL provider

I believe indentation fixes don't require review :-) But this looks good to me!

Comment 25

[Bas Schouten (:bas.schouten)]

I'm not a peer for widget so I'm not quite sure if I should be reviewing the widget patch!

Comment 26

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 452799 [details] [diff] [review]
Qt Top level GL, fixed review comments

don't really know much qt these days, but this looks ok

Comment 27

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/18b10e10399d
http://hg.mozilla.org/mozilla-central/rev/ddd29edb23b1
http://hg.mozilla.org/mozilla-central/rev/ccc830f57678
http://hg.mozilla.org/mozilla-central/rev/e6008f023c11