Implement Top level GL layer manager for Qt widget port

RESOLVED FIXED

Status

defect
RESOLVED FIXED
10 years ago
3 years ago

People

(Reporter: romaxa, Assigned: romaxa)

Tracking

Trunk
x86
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 10 obsolete attachments)

10.51 KB, patch
vlad
: review+
Details | Diff | Splinter Review
7.53 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
4.47 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
1.90 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
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.
Posted patch simple layers setup (obsolete) — Splinter Review
Assignee: nobody → romaxa
Attachment #440221 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #449245 - Flags: review?(dougt)
Posted patch Better patch (obsolete) — Splinter Review
Attachment #449245 - Attachment is obsolete: true
Attachment #449295 - Flags: review?(dougt)
Attachment #449245 - Flags: review?(dougt)
Attachment #449295 - Flags: review?(dougt) → review?(joe)
Use platform optimized offscreen for widget cached buffer surface
With GL rendering I think we don't want offscreen buffer anymore, and default target for layer manager should be QWidget - XWindow.
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?
Attachment #449295 - Flags: superreview?(bas.schouten)
Attachment #449295 - Flags: review?(joe)
Attachment #449295 - Flags: review+
(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*.
Attachment #449637 - Flags: review?(joe)
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
Attachment #449639 - Flags: review?(joe)
Comment on attachment 449637 [details] [diff] [review]
Switch offscreen buffer creation to platform->CreateOffscreenSurface

I like this patch a lot.
Attachment #449637 - Flags: review?(joe) → review+
Attachment #449639 - Flags: review?(joe) → review+
Comment on attachment 449637 [details] [diff] [review]
Switch offscreen buffer creation to platform->CreateOffscreenSurface

This patch moved to separate bug 571431
Attachment #449295 - Attachment is obsolete: true
Attachment #449637 - Attachment is obsolete: true
Attachment #449639 - Attachment is obsolete: true
Attachment #449295 - Flags: superreview?(bas.schouten)
Attachment #450987 - Flags: review?(joe)
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.
Attachment #450630 - Attachment is obsolete: true
Attachment #450988 - Flags: review?(joe)
Attachment #450988 - Attachment is obsolete: true
Attachment #451488 - Flags: review?(joe)
Attachment #450988 - Flags: review?(joe)
Less QT ifdefs for QT-GL context creation.
Attachment #450987 - Attachment is obsolete: true
Attachment #451554 - Flags: review?(joe)
Attachment #450987 - Flags: review?(joe)
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.
Attachment #451488 - Flags: review?(joe) → review-
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;
Attachment #451554 - Flags: review?(joe) → review-
> 
> >+        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..
Attachment #451488 - Attachment is obsolete: true
Attachment #452799 - Flags: review?(bas.schouten)
Attachment #452800 - Flags: review?(bas.schouten)
Attachment #451554 - Attachment is obsolete: true
Attachment #452801 - Flags: review?(bas.schouten)
Attachment #452948 - Flags: review?(bas.schouten)
Attachment #452948 - Flags: review?(bas.schouten) → review+
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.
Attachment #452801 - Flags: review?(bas.schouten) → review+
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!
Attachment #452800 - Flags: review?(bas.schouten) → review+
I'm not a peer for widget so I'm not quite sure if I should be reviewing the widget patch!
Attachment #452799 - Flags: review?(bas.schouten) → review?(vladimir)
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
Attachment #452799 - Flags: review?(vladimir) → review+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.