GNOME Bugzilla – Bug 703518
Qt5/QtQuick 2.0 video element
Last modified: 2014-11-08 14:11:37 UTC
Created attachment 248287 [details] The backtrace of the resize bug The attached patches add a QtQuick 2.0 video element for qt-gstreamer. This is for now a first version and I think it needs still some improvements ... So feedback and a code review is very welcome. Note: At the moment there is a bug that need to be solved in my implementation. After resizing the video element it ends up in a segmentation fault. (See backtrace.log)
Created attachment 248288 [details] [review] Adds the QTGSTREAMER_QTQUICK2_INSTALL_DIR variable to the CMakeLists.txt
Created attachment 248289 [details] [review] Adds updatePaintNode signal/slot to the qtvideosink
Created attachment 248290 [details] [review] Adds the quickview video surface and the scenegraph nodes
Created attachment 248291 [details] [review] Adds the QtQuick 2.0 video element
Created attachment 248292 [details] [review] Adds a qmlplayer example
Created attachment 251797 [details] [review] Patch for the resize bug This patch solves the crash when the user resizes the application window. Further this patch changes the QtQuick2 imports base installation directory of the QtQuick2 QtGStreamer plugin from usr/lib/qt5/imports to usr/lib/qt5/qml. With this patch the QtQuick1 module of Qt5 is not necessarily needed to build the QtQuick2 QtGStreamer plugin only.
Impressive work Benjamin! Still I wonder how to integrate your work with an entirely custom pipeline that's for instance created via QGst::Parse::launch() for instance. Apparently, to get things rolling I'd just be needed to use QOpenGLContext instead of the deprecated QGLContext for Qt5 builds. After that's it's rather trivial to integrate gstqt5glvideosink with quick items. See the following attachements.
Created attachment 254840 [details] [review] Use QOpenGLContext instead of the QGLContext With Qt5 QGLContext is marked as "done". More importantly QtQuick2 is based upon the new QOpenGLContext. FIXME: Of course this patch still needs quite some cleanups, like conditional complication guards. It also might make sense to introduce an additional "openglcontext" property, instead of changing the type of "glcontext". Additionally one also has to decide how to deal with all those (for our purposes unrelated) classes that depend on a QGLWidget internally.
Created attachment 254841 [details] Minimal proof of concept that integrates QQ2 and QtGst via QOpelGLContext
Thanks for your comments Mathias. (In reply to comment #7) > Impressive work Benjamin! Still I wonder how to integrate your work with an > entirely custom pipeline that's for instance created via QGst::Parse::launch() > for instance. Thats right - at the moment I don't have an example in the BasicMediaPlayer how to use QGst::Parse::launch() with it. If I have some time I will add it ... > Apparently, to get things rolling I'd just be needed to use > QOpenGLContext instead of the deprecated QGLContext for Qt5 builds. After > that's it's rather trivial to integrate gstqt5glvideosink with quick items. See > the following attachements. Yes, the QGLContext needs to be changed for Qt5. But this change won't work for a Qt4 build of QtGStreamer then. I tried to integrate my changes with ifndef macros into the upstream QtGStreamer code. But it could further make sense to split QGStreamer in a Qt4 and a Qt5 version ...?!?
(In reply to comment #9) > Created an attachment (id=254841) [details] > Minimal proof of concept that integrates QQ2 and QtGst via QOpelGLContext I started the QQ2 video element implementation up on a "unofficial" QtGStreamer port from the Ubuntu phablet guys and in this version I created two QML elements QuickVideoItem (QQuickItem) and a PaintedVideoItem (QQuickPaintedItem). But I integrated just the QuickVideoItem implementation into the QtGStreamer upstream version. Your proof of concept look really similar to my implementation of the PaintedVideoItem. :) The only difference to your concept is that I have created a separate player QML element (BasicMediaPlayer) like in my current implementation of the QQ2 video element. The idea of the separate player QML element was that developer can create their own QML player element/plugin with a custom pipeline and media control implementation. In the end I haven't tested my PaintedVideoItem (QQuickPaintedItem) with the OpenGLContext property and the qtglvideosink.
Created attachment 255621 [details] [review] Aspect ratio fix when playing anamorphic video content This patch fixes the aspect ratio of the QtQuick2 video element when playing anamorphic video content. Further this patch makes the initial texture transparent.
So, let me do some generic comments first and then go into detail on each patch. In general, the approach is correct. I believe, though, that it could be shaped better. First, the VideoNode* classes are in the wrong place. They should be part of the element plugin imho and not part of the public API library. This would also remove the dependency that is introduced in this patch which makes qtvideosink link to QtGStreamerUi. The general design that was followed in the making of qtvideosink was to avoid using any QtGStreamer API, so that it can be used independently. Also, the VideoNode* classes share GL code & shaders with qtglvideosink. It would be nice if the code is actually shared instead of copied. Second, avoiding any qml2-related code in QtGStreamerUi would be better, as this library links with QtWidgets and there may be people wanting to use only Qt Quick and avoid distributing QtWidgets. For this, I would consider a new QtGStreamerQuick library. Third, the BasicMediaPlayer item... I'm not sure about it. I know this is perhaps useful for examples and tests, but I don't think this is what QtGStreamer is for. QtMultimedia and phonon also use playbin2 to do the same thing. If people just want a playbin pipeline, why not just use those APIs instead?
Review of attachment 248288 [details] [review]: ::: qt-gstreamer-orig/CMakeLists.txt @@ +159,3 @@ if (USE_QT5) set(QTGSTREAMER_QTQUICK1_INSTALL_DIR ${CMAKE_INSTALL_LIBDIR}/qt5/imports) + set(QTGSTREAMER_QTQUICK2_INSTALL_DIR ${QTGSTREAMER_QTQUICK1_INSTALL_DIR}) I think this is wrong. qml2 plugins are installed in a different location, afaict... ($QT_PREFIX/qml vs $QT_PREFIX/imports for qml1). To fix this, we also need some code in FindQt4or5.cmake to set QT_QML_DIR or something like that.
Review of attachment 248289 [details] [review]: I know the qtvideosink code is complicated with all those classes, but having this signal on all of the existing elements is not right. I would consider another element for this. I'll write some design notes later.
Review of attachment 248290 [details] [review]: I don't like this patch in general. 1) As I have said, I would prefer VideoNode* inside qtvideosink. 2) I'd rather not expose those Abstract* classes. AbstractVideoSurface makes sense maybe, but since we can't make it a superclass of GraphicsVideoSurface (to preserve ABI), it's useless. 3) There is a lot of commented out code and code with "still needed?" comments that should not be there. 4) What is the purpose of VideoNode_Generic? ::: qt-gstreamer-orig/src/QGst/Ui/quickviewvideosurface.h @@ +62,3 @@ + * Helper - returns true if the given orientation has the same aspect as the default (e.g. 180*n) + */ + inline bool isDefaultAspect(int o) { These inline methods could be in the .cpp
Review of attachment 248291 [details] [review]: The qml2 plugin export mechanism confuses me a bit, there are a lot of names involved... We have QtGStreamerQuick2.json, org.freedesktop.gstreamer.Qt5GStreamerQuick2-0.10, QtGStreamerQuick2 in the qmldir file, and in the example (later patch) it is loaded with "import QtGStreamer.Quick2 0.10". That is just confusing. I'd expect one name everywhere (Qt5GStreamerQuick2 ?). Also note that this patch should also modify the UNINSTALLED_IMPORTS_DIR in the qmlplayer example, as the directory is changing with this patch. ::: qt-gstreamer-orig/src/qml/quick2/CMakeLists.txt @@ +13,3 @@ + # On windows with gcc, cmake calls the binary libFOO.dll, but the Qt plugin loader + # does not remove the lib prefix when searching for .dlls, unlike what happens on unix + file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/qmldir "plugin libQtGStreamerQuick2") Apparently qmldir nowdays also requires a "module Foo" statement, otherwise it shows a warning.
Review of attachment 248292 [details] [review]: ::: qt-gstreamer-orig/examples/CMakeLists.txt @@ +36,3 @@ + + if (USE_QT5) + add_subdirectory(qmlplayer2) This should be under if (Qt4or5_Quick2_FOUND) or something like that... ::: qt-gstreamer-orig/examples/qmlplayer/qmlplayer.qml @@ -17,3 +17,3 @@ */ import QtQuick 1.0 -import QtGStreamer 0.10 +import QtGStreamer.QtQuick1 0.10 This is definitely wrong. It used to work with import QtGStreamer and it should still work the same way. We don't want to break existing applications. ::: qt-gstreamer-orig/examples/qmlplayer2/main.cpp @@ +23,3 @@ + +#ifndef QMLPLAYER_NO_OPENGL +# include <QtOpenGL/QGLWidget> No QGLWidget needed anymore ::: qt-gstreamer-orig/examples/qmlplayer2/qmlplayer2.pro @@ +21,3 @@ + +# link against QtDeclarative and QtOpenGL +QT += declarative opengl opengl not needed anymore
Review of attachment 251797 [details] [review]: Tbh, I don't fully understand this patch...
(In reply to comment #14) > Review of attachment 248288 [details] [review]: > > ::: qt-gstreamer-orig/CMakeLists.txt > @@ +159,3 @@ > if (USE_QT5) > set(QTGSTREAMER_QTQUICK1_INSTALL_DIR > ${CMAKE_INSTALL_LIBDIR}/qt5/imports) > + set(QTGSTREAMER_QTQUICK2_INSTALL_DIR > ${QTGSTREAMER_QTQUICK1_INSTALL_DIR}) > > I think this is wrong. qml2 plugins are installed in a different location, > afaict... ($QT_PREFIX/qml vs $QT_PREFIX/imports for qml1). To fix this, we also > need some code in FindQt4or5.cmake to set QT_QML_DIR or something like that. Yes, you are right and I had changed this with the "Patch for the resize bug" patch.
(In reply to comment #16) > Review of attachment 248290 [details] [review]: > > I don't like this patch in general. > > 1) As I have said, I would prefer VideoNode* inside qtvideosink. Feel free to shift it to the qtvideosink. When I started the implementation I was not sure where to place my additions, so I decided to put it in the QtGStreamerUI part. ;) > 2) I'd rather not expose those Abstract* classes. AbstractVideoSurface makes > sense maybe, but since we can't make it a superclass of GraphicsVideoSurface > (to preserve ABI), it's useless. I introduced this AbstractVideoSurface class because in my first implementation I also had a PaintedVideoSurface for a PaintedVideoItem (QQuickPaintedItem) QML element which uses the QPainter API. But I did not include it here. By using only the QuickVideoSurface this abstraction is not need. > 3) There is a lot of commented out code and code with "still needed?" comments > that should not be there. Yes there are some leftovers which needs to removed ... > 4) What is the purpose of VideoNode_Generic? I created the VideoNode_Generic to render an (initial) transparent texture before the video playback is started. Because without the first video frame I was not able to determine the video format (RGB, YUV). If you have a solution to render the initial node with one of the two other VideoNode_* class you can remove this class. > > ::: qt-gstreamer-orig/src/QGst/Ui/quickviewvideosurface.h > @@ +62,3 @@ > + * Helper - returns true if the given orientation has the same aspect as > the default (e.g. 180*n) > + */ > + inline bool isDefaultAspect(int o) { > > These inline methods could be in the .cpp Some leftovers from the QtMultimedia inspired code parts that I have used.
(In reply to comment #17) > Review of attachment 248291 [details] [review]: > > The qml2 plugin export mechanism confuses me a bit, there are a lot of names > involved... We have QtGStreamerQuick2.json, > org.freedesktop.gstreamer.Qt5GStreamerQuick2-0.10, QtGStreamerQuick2 in the > qmldir file, and in the example (later patch) it is loaded with "import > QtGStreamer.Quick2 0.10". That is just confusing. I'd expect one name > everywhere (Qt5GStreamerQuick2 ?). Yes one name makes sense. When I added my QtQuick2 plugin I created a folder for the QtQuick1 and a folder for the QtQuick2 plugin. This is why it results in the two imports in the qmlplayer examples. "import QtGStreamer.Quick1 0.10" (qmlplayer) "import QtGStreamer.Quick2 0.10" (qmlplayer2) > > Also note that this patch should also modify the UNINSTALLED_IMPORTS_DIR in the > qmlplayer example, as the directory is changing with this patch. > > ::: qt-gstreamer-orig/src/qml/quick2/CMakeLists.txt > @@ +13,3 @@ > + # On windows with gcc, cmake calls the binary libFOO.dll, but the Qt plugin > loader > + # does not remove the lib prefix when searching for .dlls, unlike what > happens on unix > + file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/qmldir "plugin libQtGStreamerQuick2") > > Apparently qmldir nowdays also requires a "module Foo" statement, otherwise it > shows a warning. Yes should be added then.
(In reply to comment #18) > Review of attachment 248292 [details] [review]: > > ::: qt-gstreamer-orig/examples/CMakeLists.txt > @@ +36,3 @@ > + > + if (USE_QT5) > + add_subdirectory(qmlplayer2) > > This should be under if (Qt4or5_Quick2_FOUND) or something like that... > > ::: qt-gstreamer-orig/examples/qmlplayer/qmlplayer.qml > @@ -17,3 +17,3 @@ > */ > import QtQuick 1.0 > -import QtGStreamer 0.10 > +import QtGStreamer.QtQuick1 0.10 > > This is definitely wrong. It used to work with import QtGStreamer and it should > still work the same way. We don't want to break existing applications. I fully agree with you! > > ::: qt-gstreamer-orig/examples/qmlplayer2/main.cpp > @@ +23,3 @@ > + > +#ifndef QMLPLAYER_NO_OPENGL > +# include <QtOpenGL/QGLWidget> > > No QGLWidget needed anymore > > ::: qt-gstreamer-orig/examples/qmlplayer2/qmlplayer2.pro > @@ +21,3 @@ > + > +# link against QtDeclarative and QtOpenGL > +QT += declarative opengl > > opengl not needed anymore Yes remove it.
(In reply to comment #19) > Review of attachment 251797 [details] [review]: > > Tbh, I don't fully understand this patch... Before this patch the qmlplayer2 example application has crashed after I have resized the application window several times. Before this patch I have wrote the video frame data in the bind() method of the VideoMaterial class to the OpenGL texture. I think this was too late and because of this the video frame data pointer was already invalid. I had to change the place at which time the video frame data was written to the OpenGL texture. As soon as I got the video frame data I stored it in the OpenGL texture. After this I didn't get any segmentation faults when resizing the application window.
I think this bug report can be closed, because the Qt5 QtQuick2 video element is now available in the qt-gstreamer Git repo master branch!?
So it seems. Is this bug obsolete? Are any of the patches still relevant?
Ok, the bug is fixed then, because all the needed Qt5 QtQuick2 Video element functionality was integrated by George on the basis of these patches.