GNOME Bugzilla – Bug 774223
opencv: move base opencv filter class and utilities to gst-libs
Last modified: 2016-11-30 18:55:20 UTC
Created attachment 339534 [details] [review] opencv: move base opencv filter class and utilities to gst-libs This patch moves gstopencvvideofilter and gstopencvutils to gst-libs making it possible to write opencv based plugins outside of the main opencv plugin.
Patch has a minot issue (headers are installed in base instead of opencv). Will fix that. Note that I did not handle meson...
This is a great idea! You are missing something to be able to use <gst/video/video.h> in gstopencvutils.h In file included from gstopencvutils.cpp:26:0: gstopencvutils.h:30:29: fatal error: gst/video/video.h: No such file or directory compilation terminated. A few small fixes as well. -libgstopencv_la_CFLAGS = $(GST_PLUGINS_BASE_CFLAGS) $(GST_BASE_CFLAGS) \ - $(GST_CFLAGS) $(OPENCV_CFLAGS) \ The change here does nothing but restructure the content, please keep it as it is to keep a clean git history. Related to the above. When you add one element at the beginning of the list, keep the rest structured as before. If there are multiple in one line it is because it is easier to read. Keep the lines in gst-libs/gst/opencv/Makefile.am inside the 80 character limit. #libgstopencv_@GST_API_VERSION@_la_CFLAGS Why the commented out section in gst-libs/gst/opencv/Makefile.am? +EXTRA_DIST = adds a white line Add the bugzilla URL in the last line of the commit, since we now have a bug thread. Maybe mention that this patch requires cleaning the autotools crumbs of the previous builds to work. Thanks :)
Strange that compilation fails for you (i.e. missing video.h): works for me. Where should I mention that the patch requires cleaning the autotools crumbs ?
Review of attachment 339534 [details] [review]: ::: gst-libs/gst/opencv/Makefile.am @@ +7,3 @@ +libgstopencv_@GST_API_VERSION@_la_CXXFLAGS = $(GST_CXXFLAGS) $(OPENCV_CFLAGS) + +#libgstopencv_@GST_API_VERSION@_la_CFLAGS = $(GST_CFLAGS) $(OPENCV_CFLAGS) \ It fails for Luis because he uses gst-uninstalled... and you're missing the GST_BASE_CFLAGS, GST_PLUGINS_BASE_CFLAGS, (and _LIBS) here. Check which parts you need and add them :)
Created attachment 339821 [details] [review] opencv: move base opencv filter class and utilities to gst-libs New patch that takes into account review comments and hopefully builds in uninstalled mode.
Review of attachment 339821 [details] [review]: commit 5594c7e53eb95f93cbecac33b83609e70d7a3ce5 Author: Philippe Renon <philippe_renon@yahoo.fr> Date: Thu Nov 10 18:42:29 2016 +0100 opencv: move base opencv filter class and utilities to gst-libs https://bugzilla.gnome.org/show_bug.cgi?id=774223
Thanks Philippe
Re-opening. I think we want to review this thoroughly before the release. a) API exposed b) opencv dependency and implications on API/ABI stability if any c) if it should be in gst-libs/ext then (We have so far not exposed gst libs that depends on external libraries that are not 'system libraries'.)
And the meson build will need to be updated for this change too.
I think it's generally a good idea to expose this as a library. Many people were trying to integrate OpenCV things into their own code, and then implemented worse code than what we had in the opencv plugin already. Until OpenCV has a stable API, we should keep this in -bad and have it explicitly marked as unstable API too. The headers use types from OpenCV.
Yes, I think it's a good thing to expose, still think we should have a closer look.
Some notes: 1/ The opencv plugin and gst-libs library will be compiled only if opencv is detected. 2/ The opencv plugin not only depends on opencv but also on opencv_contrib while the gst-libs/opencv depends only on opencv. On a related note please, take a look at https://bugzilla.gnome.org/show_bug.cgi?id=772822 The opencv plugin will fail to compile if opencv is detected but opencv_contrib is not present. The opencv_contrib is built along opencv, so some opencv distributions have it and others not (cf. https://github.com/opencv/opencv_contrib/blob/master/README.md). 3/ Concerning meson, this patch does not make the situation worse as the opencv plugin was not yet ported to it. 4/ Last, and less relevant, there are issues in gstopencvutils. One of them is that it does not support "sparse" video formats well. Some opencv elements declare "BGRx" as supported it does not work well. Related to that I opened https://bugzilla.gnome.org/show_bug.cgi?id=773433 for which I will show a use case on top of this patch.
PS: The patch is field tested. I am using it to build statically registered elements that depend on gst-libs/opencv.
About the API exposing opencv types: Only IplImage* is exposed. There are two IplImage pointers that are aliases of GstBuffer data. Relevant code in the implementation looks like this: if (!gst_buffer_map (inbuf, &in_info, GST_MAP_READ)) goto inbuf_map_failed; transform->cvImage->imageData = (char *) in_info.data; Where transform->cvImage is a IplImage*. Is it possible to hide the two IplImage pointers behind some gst abstraction ? GstBuffer* ? ;)
I am updating the meson build for this today. It is a good reason to play with meson.
We probably also need to provide a pkg-config .pc file if this is to be used by developers :)
Roger that Tim :)
By forward declaring IplImage in gstopencvvideofilter.h it is possible to rid the API of all opencv references. Works fine on my end. Should I update the patch or submit an additional one ?
Philippe, I already committed the patch. Please submit an additional one on top of current git master. This is better than reverting and pushing an updated version.
I have been working on adding meson build for opencv. Hit a wall and asking Nirbheek for some help. Will keep you guys updated.
Created attachment 340001 [details] [review] opencv: forward declare opencv types to avoid exposing them in the API
Should I move the opencv from gst-libs/gst to gst-libs/ext. I did not realize at the time that gst-libs/ext was an option (plugins-bad didi not have gst-libs/ext yet).
Let's leave it where it is for now,especially if the plan is to not include the opencv header in the gst header (although I don't know if that's sustainable in the long run as we add more stuff). We can always move it later.
Yes, sustainability is the issue here. Enums for instance will be an issue. PS : The last patch added to this issue removes all opencv headers from gst headers.
Could the last patch be merged ? It cleans the API of opencv references. I have more commits on top of that patch...
Philippe, If you have more commits on top of that patch. Feel free to share them here. Meanwhile you use git format-patch to have them number-ordered, you can upload as many as you want here.
Review of attachment 340001 [details] [review]: Patch content looks good. Before I commit, there are a few changes that are just style changes (removing or adding empty lines). Can you resend without these code style changes?
The code style changes were intentional. Is it a "no no" to mix structural and style changes in a single patch ? I don't mind doing whatever is requested ;) Just let me know. The other patches are not related to this issue...
Created attachment 340437 [details] [review] opencv: forward declare opencv types to avoid exposing them in the API new version of patch without unrelated format changes
Review of attachment 340437 [details] [review]: commit d130a19c8991eaed824f22d0b1a1bd138a07e49f Author: Philippe Renon <philippe_renon@yahoo.fr> Date: Mon Nov 21 17:19:46 2016 +0100 opencv: forward declare opencv types Forward declare opencv types to avoid exposing them in the API. https://bugzilla.gnome.org/show_bug.cgi?id=774223 We prefer avoiding style changes since they pollute the git log history. Thanks Philippe :)
Created attachment 340453 [details] [review] Work in progress meson build for OpenCV Not working yet, it complains about some macros. Can anybody help? ../subprojects/gst-plugins-bad/ext/opencv/gsttemplatematch.cpp: In function ‘void gst_template_match_load_template(GstTemplateMatch*, gchar*)’: ../subprojects/gst-plugins-bad/ext/opencv/gsttemplatematch.cpp:181:44: error: ‘CV_LOAD_IMAGE_COLOR’ was not declared in this scope newTemplateImage = cvLoadImage (templ, CV_LOAD_IMAGE_COLOR); ^~~~~~~~~~~~~~~~~~~ ../subprojects/gst-plugins-bad/ext/opencv/gsttemplatematch.cpp:181:63: error: ‘cvLoadImage’ was not declared in this scope newTemplateImage = cvLoadImage (templ, CV_LOAD_IMAGE_COLOR); ^
I'll take a look.
Please ignore last comment. Did not realize it was about meson.
The missing types are in opencv2/imgcodecs/imgcodecs_c.h They get included indirectly from gsttemplatematch.h through: #ifdef HAVE_HIGHGUI_H #include <highgui.h> // includes highGUI definitions #endif #ifdef HAVE_OPENCV2_HIGHGUI_HIGHGUI_C_H #include <opencv2/highgui/highgui_c.h> // includes highGUI definitions #endif The errors you get mean that neither HAVE_HIGHGUI_H nor HAVE_OPENCV2_HIGHGUI_HIGHGUI_C_H are defined.
Correction to the last comment : it is HAVE_OPENCV2_HIGHGUI_HIGHGUI_C_H that you need to have defined (not HAVE_HIGHGUI)
Created attachment 340523 [details] [review] This patch will succesfully build the opencv plugins with meson Remaining thing is to check the existence of the OPENCV_PATH_NAME, but meson has no facility to check if a file exists yet. So nothing convenient to replicate: configure.ac: if test -d "$PKG_CONFIG_SYSROOT_DIR/$OPENCV_PREFIX/share/opencv/"; then Any comments? Any fixes before it is ready to push?
Related to meson is the opencv_contrib related issues : https://bugzilla.gnome.org/show_bug.cgi?id=772822 I guess it can be addressed later.
(In reply to Philippe Renon from comment #37) > Related to meson is the opencv_contrib related issues : > https://bugzilla.gnome.org/show_bug.cgi?id=772822 > > I guess it can be addressed later. Yeah. I marked it as ToDo to fix that once meson supports it. For now defaulting to the most common path is better than having no opencv elements in meson builds :)
Review of attachment 340523 [details] [review]: commit de99cf0de1fb332b2e757d027ec8c35eb1ac5811 Author: Luis de Bethencourt <luisbg@osg.samsung.com> Date: Tue Nov 15 16:57:20 2016 +0000 opencv: Enable in meson build https://bugzilla.gnome.org/show_bug.cgi?id=774223
Review of attachment 340437 [details] [review]: Pushed
https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=7488a8fb3544634a1629ce0c804dc3c3ab4314ed The plugin should be optional too
Thanks Sebastian! :)
What's left here ?
Nothing I guess.
Ok, let's deal with any remaining issue into new bugs then. Thanks!