After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 767553 - qmlglsink: Add Wayland support
qmlglsink: Add Wayland support
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal blocker
: 1.8.3
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-06-12 07:49 UTC by Haihua Hu
Modified: 2016-08-15 17:10 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
qmlgisink: Fix cannot create egl context on wayland (1.75 KB, patch)
2016-06-12 07:55 UTC, Haihua Hu
needs-work Details | Review
qmlgisink: Add Wayland support (3.16 KB, patch)
2016-06-14 02:25 UTC, Haihua Hu
reviewed Details | Review
Remove path calculate into configure.ac (2.77 KB, patch)
2016-06-14 06:50 UTC, Haihua Hu
committed Details | Review
config.log (416.28 KB, text/plain)
2016-06-20 20:52 UTC, Tim-Philipp Müller
  Details
qmlglsink: Fix build error when don't have QPA installed. (2.54 KB, patch)
2016-06-27 10:32 UTC, Haihua Hu
reviewed Details | Review

Description Haihua Hu 2016-06-12 07:49:56 UTC
Don't use gstgldisplay to get wayland display. Should use QPA on wayland to get wayland display for QT.

By the way, Is there a common way to include QPA in gst-plugins-bad for qt plugin build? Just #include <QtGui/5.6.1/QtGui/qpa/qplatformnativeinterface.h> is not good when version changed.
Comment 1 Haihua Hu 2016-06-12 07:55:07 UTC
Created attachment 329631 [details] [review]
qmlgisink: Fix cannot create egl context on wayland
Comment 2 Sebastian Dröge (slomo) 2016-06-12 08:06:49 UTC
Review of attachment 329631 [details] [review]:

::: ext/qt/qtitem.cc
@@ +33,3 @@
 #include <QtQuick/QQuickWindow>
 #include <QtQuick/QSGSimpleTextureNode>
+#include <QtGui/5.6.1/QtGui/qpa/qplatformnativeinterface.h>

This should work as #include <QtGui/gpa/...>. You will have to set up things correctly in configure.ac for that

@@ +160,3 @@
+    QPlatformNativeInterface *native =
+              QGuiApplication::platformNativeInterface();
+    ret = (GstGLDisplayWayland *)g_object_new (GST_TYPE_GL_DISPLAY_WAYLAND, NULL);

Why don't we have a gst_gl_display_wayland_new()? Might make sense to add :)

@@ +162,3 @@
+    ret = (GstGLDisplayWayland *)g_object_new (GST_TYPE_GL_DISPLAY_WAYLAND, NULL);
+    ret->display = (struct wl_display *)
+              native->nativeResourceForWindow("display", NULL);

And that could take this in the constructor
Comment 3 Sebastian Dröge (slomo) 2016-06-12 08:10:25 UTC
Also please update the commit message. You're adding Wayland support here, not fixing anything that was broken before :)
Comment 4 Haihua Hu 2016-06-12 08:21:33 UTC
(In reply to Sebastian Dröge (slomo) from comment #2)
> Review of attachment 329631 [details] [review] [review]:
> 
> ::: ext/qt/qtitem.cc
> @@ +33,3 @@
>  #include <QtQuick/QQuickWindow>
>  #include <QtQuick/QSGSimpleTextureNode>
> +#include <QtGui/5.6.1/QtGui/qpa/qplatformnativeinterface.h>
> 
> This should work as #include <QtGui/gpa/...>. You will have to set up things
> correctly in configure.ac for that
> 

But I cannot found gpa folder in QtGui, it cannot work on my side. What should i set in configure.ac?

> @@ +160,3 @@
> +    QPlatformNativeInterface *native =
> +              QGuiApplication::platformNativeInterface();
> +    ret = (GstGLDisplayWayland *)g_object_new (GST_TYPE_GL_DISPLAY_WAYLAND,
> NULL);
> 
> Why don't we have a gst_gl_display_wayland_new()? Might make sense to add :)
> 

gst_gl_display_wayland_new() will use wl_display_connect to get wayland display. It is not correct on wayland for qt implement. 

> @@ +162,3 @@
> +    ret = (GstGLDisplayWayland *)g_object_new (GST_TYPE_GL_DISPLAY_WAYLAND,
> NULL);
> +    ret->display = (struct wl_display *)
> +              native->nativeResourceForWindow("display", NULL);
> 
> And that could take this in the constructor

It is just for qtsink, not compatible for other GL plugin
Comment 5 Sebastian Dröge (slomo) 2016-06-12 08:59:16 UTC
(In reply to Haihua Hu from comment #4)
> (In reply to Sebastian Dröge (slomo) from comment #2)
> > Review of attachment 329631 [details] [review] [review] [review]:
> > 
> > ::: ext/qt/qtitem.cc
> > @@ +33,3 @@
> >  #include <QtQuick/QQuickWindow>
> >  #include <QtQuick/QSGSimpleTextureNode>
> > +#include <QtGui/5.6.1/QtGui/qpa/qplatformnativeinterface.h>
> > 
> > This should work as #include <QtGui/gpa/...>. You will have to set up things
> > correctly in configure.ac for that
> > 
> 
> But I cannot found gpa folder in QtGui, it cannot work on my side. What
> should i set in configure.ac?

There should be a pkg-config file for this path, which seems to be Qt5PlatformSupport.pc?

> > @@ +160,3 @@
> > +    QPlatformNativeInterface *native =
> > +              QGuiApplication::platformNativeInterface();
> > +    ret = (GstGLDisplayWayland *)g_object_new (GST_TYPE_GL_DISPLAY_WAYLAND,
> > NULL);
> > 
> > Why don't we have a gst_gl_display_wayland_new()? Might make sense to add :)
> > 
> 
> gst_gl_display_wayland_new() will use wl_display_connect to get wayland
> display. It is not correct on wayland for qt implement. 
> 
> > @@ +162,3 @@
> > +    ret = (GstGLDisplayWayland *)g_object_new (GST_TYPE_GL_DISPLAY_WAYLAND,
> > NULL);
> > +    ret->display = (struct wl_display *)
> > +              native->nativeResourceForWindow("display", NULL);
> > 
> > And that could take this in the constructor
> 
> It is just for qtsink, not compatible for other GL plugin


Isn't this the same as gst_gl_display_wayland_new_with_display(), as used by gtkglsink for example?
Comment 6 Haihua Hu 2016-06-12 09:09:54 UTC
(In reply to Sebastian Dröge (slomo) from comment #5)
> (In reply to Haihua Hu from comment #4)
> > (In reply to Sebastian Dröge (slomo) from comment #2)
> > > Review of attachment 329631 [details] [review] [review] [review] [review]:
> > > 
> > > ::: ext/qt/qtitem.cc
> > > @@ +33,3 @@
> > >  #include <QtQuick/QQuickWindow>
> > >  #include <QtQuick/QSGSimpleTextureNode>
> > > +#include <QtGui/5.6.1/QtGui/qpa/qplatformnativeinterface.h>
> > > 
> > > This should work as #include <QtGui/gpa/...>. You will have to set up things
> > > correctly in configure.ac for that
> > > 
> > 
> > But I cannot found gpa folder in QtGui, it cannot work on my side. What
> > should i set in configure.ac?
> 
> There should be a pkg-config file for this path, which seems to be
> Qt5PlatformSupport.pc?

I will work on it.

> 
> > > @@ +160,3 @@
> > > +    QPlatformNativeInterface *native =
> > > +              QGuiApplication::platformNativeInterface();
> > > +    ret = (GstGLDisplayWayland *)g_object_new (GST_TYPE_GL_DISPLAY_WAYLAND,
> > > NULL);
> > > 
> > > Why don't we have a gst_gl_display_wayland_new()? Might make sense to add :)
> > > 
> > 
> > gst_gl_display_wayland_new() will use wl_display_connect to get wayland
> > display. It is not correct on wayland for qt implement. 
> > 
> > > @@ +162,3 @@
> > > +    ret = (GstGLDisplayWayland *)g_object_new (GST_TYPE_GL_DISPLAY_WAYLAND,
> > > NULL);
> > > +    ret->display = (struct wl_display *)
> > > +              native->nativeResourceForWindow("display", NULL);
> > > 
> > > And that could take this in the constructor
> > 
> > It is just for qtsink, not compatible for other GL plugin
> 
> 
> Isn't this the same as gst_gl_display_wayland_new_with_display(), as used by
> gtkglsink for example?

It seems this api is what i want to use, Let me have a try.
Comment 7 Haihua Hu 2016-06-14 02:25:51 UTC
Created attachment 329744 [details] [review]
qmlgisink: Add Wayland support
Comment 8 Sebastian Dröge (slomo) 2016-06-14 06:24:09 UTC
Review of attachment 329744 [details] [review]:

Except for the include path this looks good

::: ext/qt/Makefile.am
@@ +27,3 @@
 	-I$(top_srcdir)/gst-libs \
 	-I$(top_builddir)/gst-libs \
+	-I$(QT_INCLUDE_PATH)/QtGui/$(QT_VERSION)/QtGui \

There is really no pkg-config file that contains this include path anywhere? It might still make sense to calculate this path in configure.ac then instead of doing that here
Comment 9 Haihua Hu 2016-06-14 06:27:02 UTC
(In reply to Sebastian Dröge (slomo) from comment #8)
> Review of attachment 329744 [details] [review] [review]:
> 
> Except for the include path this looks good
> 
> ::: ext/qt/Makefile.am
> @@ +27,3 @@
>  	-I$(top_srcdir)/gst-libs \
>  	-I$(top_builddir)/gst-libs \
> +	-I$(QT_INCLUDE_PATH)/QtGui/$(QT_VERSION)/QtGui \
> 
> There is really no pkg-config file that contains this include path anywhere?
> It might still make sense to calculate this path in configure.ac then
> instead of doing that here

No, there is no pkg-config file for QPA, QPA is still private in Qt5, I will remove this into configure.ac.
Comment 10 Haihua Hu 2016-06-14 06:50:07 UTC
Created attachment 329757 [details] [review]
Remove path calculate into configure.ac
Comment 11 Matthew Waters (ystreet00) 2016-06-15 15:51:26 UTC
commit 39034063045af3bb91775fcf3ea0bf35fe6de68f
Author: Haihua Hu <jared.hu@nxp.com>
Date:   Sun Jun 12 15:35:28 2016 +0800

    qmlglsink: Add Wayland support
    
    Don't use gstgldisplay to get wayland display. Should use QPA on wayland
    to get wayland display for QT.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=767553
Comment 12 Tim-Philipp Müller 2016-06-17 14:31:00 UTC
This breaks the build for me:

cc1plus: error: /usr/include/x86_64-linux-gnu/qt5/QtGui/5.5.1/QtGui: No such file or directory [-Werror=missing-include-dirs]
qtitem.cc:35:42: fatal error: qpa/qplatformnativeinterface.h: No such file or directory


(I didn't have qtbase5-private-dev installed)

Just removing the include makes it at least compile though.
Comment 13 Matthew Waters (ystreet00) 2016-06-19 07:48:22 UTC
Are you building with or without wayland?
Comment 14 Tim-Philipp Müller 2016-06-19 08:57:17 UTC
It depends what you mean exactly. Probably without. I just ran autogen.sh.
Comment 15 Haihua Hu 2016-06-20 02:21:36 UTC
(In reply to Tim-Philipp Müller from comment #14)
> It depends what you mean exactly. Probably without. I just ran autogen.sh.

Sorry for not getting to this message earlier. Could you share you configure log with me? If you don't have qtbase5-private-dev installed, it will not include qt build i think.
Comment 16 Tim-Philipp Müller 2016-06-20 20:52:24 UTC
Created attachment 330103 [details]
config.log

Here you go. Are you sure the plugin should be disabled if I don't have qtbase5-private-dev installed, seeing that it compiles fine if I comment out that qplatform* include? (not sure if it actually works, but it used to, on X11).
Comment 17 Sebastian Dröge (slomo) 2016-06-21 06:51:02 UTC
The problem is probably this:
https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/configure.ac#n2870

It does that in any case, not only if Wayland support (a few lines below) is found. I also remember seeing somewhere that QPA was going to go away, maybe Matthew said that in some other bug?
Comment 18 Matthew Waters (ystreet00) 2016-06-21 06:58:29 UTC
Adding extra includes is not the problem, the problem is that the qpa headers aren't installed on debian at all without some extra package being installed.

QPA also isn't going to disappear, some of the other pkg-config files however were like Qt5PlatformSupport and some Qt5EGL one were fleeting for a release.
Comment 19 Tim-Philipp Müller 2016-06-21 08:30:17 UTC
Ok, so:

Is the qpa header required for all cases?

-> if yes, we should check for its existance and disable all qt stuff if not available

-> if not, we should check for its existance and add a #define HAVE_QT_QPLATFORMWHATEVER_HEADER and wrap the include in that.
Comment 20 Sebastian Dröge (slomo) 2016-06-21 08:53:08 UTC
Only Wayland (currently) AFAIU
Comment 21 Haihua Hu 2016-06-27 02:21:02 UTC
(In reply to Tim-Philipp Müller from comment #19)
> Ok, so:
> 
> Is the qpa header required for all cases?
> 
> -> if yes, we should check for its existance and disable all qt stuff if not
> available
> 
> -> if not, we should check for its existance and add a #define
> HAVE_QT_QPLATFORMWHATEVER_HEADER and wrap the include in that.

Only for wayland, I will try to fix this issue.
Comment 22 Haihua Hu 2016-06-27 10:32:40 UTC
Created attachment 330425 [details] [review]
qmlglsink: Fix build error when don't have QPA installed.
Comment 23 Matthew Waters (ystreet00) 2016-06-27 12:47:31 UTC
Pushed after rebasing on master

commit d60b071474330f986d8de0098ecbecb6f9fc26d3
Author: Haihua Hu <jared.hu@nxp.com>
Date:   Mon Jun 27 18:15:08 2016 +0800

    qmlglsink: Fix build error when don't have QPA installed.
    
    Check header file existance and wrap the header file include
    in the necessary #ifdef to avoid build error.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=767553
Comment 24 Sebastian Dröge (slomo) 2016-06-27 12:52:47 UTC
Review of attachment 330425 [details] [review]:

::: configure.ac
@@ +2870,3 @@
+          AC_SUBST(QPA_INCLUDE_PATH)
+          HAVE_QT_QPA_HEADER="yes"
+        ], [AC_MSG_NOTICE([Cannot find QPA])])

Wouldn't it be better to just move this into the if GST_GL_HAVE_WINDOW_WAYLAND ... below?
Comment 25 Tim-Philipp Müller 2016-07-01 18:58:52 UTC
commit 013eaee06b066a5613b5f830d9b3c087a39096bd
Author: Tim-Philipp Müller <tim@centricular.com>
Date:   Fri Jul 1 19:22:32 2016 +0100

    qt: fix build some more when QPA is not available
    
    Compiler would complain about include directory that didn't
    exist because QPA_INCLUDE_PATH gets subst-ed regardless
    (and if it didn't we'd have just an empty -I argument).
    
    https://bugzilla.gnome.org/show_bug.cgi?id=767553