GNOME Bugzilla – Bug 767553
qmlglsink: Add Wayland support
Last modified: 2016-08-15 17:10:15 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.
Created attachment 329631 [details] [review] qmlgisink: Fix cannot create egl context on wayland
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
Also please update the commit message. You're adding Wayland support here, not fixing anything that was broken before :)
(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
(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?
(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.
Created attachment 329744 [details] [review] qmlgisink: Add Wayland support
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
(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.
Created attachment 329757 [details] [review] Remove path calculate into configure.ac
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
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.
Are you building with or without wayland?
It depends what you mean exactly. Probably without. I just ran autogen.sh.
(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.
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).
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?
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.
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 Wayland (currently) AFAIU
(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.
Created attachment 330425 [details] [review] qmlglsink: Fix build error when don't have QPA installed.
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
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?
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