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 774223 - opencv: move base opencv filter class and utilities to gst-libs
opencv: move base opencv filter class and utilities to gst-libs
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal enhancement
: 1.11.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-11-10 17:50 UTC by Philippe Renon
Modified: 2016-11-30 18:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
opencv: move base opencv filter class and utilities to gst-libs (13.17 KB, patch)
2016-11-10 17:50 UTC, Philippe Renon
none Details | Review
opencv: move base opencv filter class and utilities to gst-libs (12.78 KB, patch)
2016-11-14 17:00 UTC, Philippe Renon
committed Details | Review
opencv: forward declare opencv types to avoid exposing them in the API (3.38 KB, patch)
2016-11-16 09:57 UTC, Philippe Renon
none Details | Review
opencv: forward declare opencv types to avoid exposing them in the API (2.55 KB, patch)
2016-11-21 16:21 UTC, Philippe Renon
committed Details | Review
Work in progress meson build for OpenCV (3.28 KB, patch)
2016-11-21 16:47 UTC, Luis de Bethencourt
none Details | Review
This patch will succesfully build the opencv plugins with meson (3.63 KB, patch)
2016-11-22 15:17 UTC, Luis de Bethencourt
committed Details | Review

Description Philippe Renon 2016-11-10 17:50:04 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.
Comment 1 Philippe Renon 2016-11-14 10:15:01 UTC
Patch has a minot issue (headers are installed in base instead of opencv).
Will fix that.

Note that I did not handle meson...
Comment 2 Luis de Bethencourt 2016-11-14 11:30:48 UTC
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 :)
Comment 3 Philippe Renon 2016-11-14 11:42:41 UTC
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 ?
Comment 4 Sebastian Dröge (slomo) 2016-11-14 11:51:52 UTC
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 :)
Comment 5 Philippe Renon 2016-11-14 17:00:07 UTC
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.
Comment 6 Luis de Bethencourt 2016-11-14 18:35:41 UTC
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
Comment 7 Luis de Bethencourt 2016-11-14 18:38:37 UTC
Thanks Philippe
Comment 8 Tim-Philipp Müller 2016-11-14 18:47:11 UTC
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'.)
Comment 9 Tim-Philipp Müller 2016-11-14 18:48:01 UTC
And the meson build will need to be updated for this change too.
Comment 10 Sebastian Dröge (slomo) 2016-11-14 19:01:34 UTC
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.
Comment 11 Tim-Philipp Müller 2016-11-14 19:12:45 UTC
Yes, I think it's a good thing to expose, still think we should have a closer look.
Comment 12 Philippe Renon 2016-11-14 21:19:55 UTC
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.
Comment 13 Philippe Renon 2016-11-14 21:25:27 UTC
PS: The patch is field tested. I am using it to build statically registered elements that depend on gst-libs/opencv.
Comment 14 Philippe Renon 2016-11-14 21:35:07 UTC
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* ? ;)
Comment 15 Luis de Bethencourt 2016-11-15 10:28:23 UTC
I am updating the meson build for this today. It is a good reason to play with meson.
Comment 16 Tim-Philipp Müller 2016-11-15 10:38:55 UTC
We probably also need to provide a pkg-config .pc file if this is to be used by developers :)
Comment 17 Luis de Bethencourt 2016-11-15 12:08:01 UTC
Roger that Tim :)
Comment 18 Philippe Renon 2016-11-15 16:04:58 UTC
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 ?
Comment 19 Luis de Bethencourt 2016-11-15 17:00:57 UTC
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.
Comment 20 Luis de Bethencourt 2016-11-15 17:07:26 UTC
I have been working on adding meson build for opencv. Hit a wall and asking Nirbheek for some help. Will keep you guys updated.
Comment 21 Philippe Renon 2016-11-16 09:57:18 UTC
Created attachment 340001 [details] [review]
opencv: forward declare opencv types to avoid exposing them in the API
Comment 22 Philippe Renon 2016-11-16 11:09:20 UTC
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).
Comment 23 Tim-Philipp Müller 2016-11-16 11:24:26 UTC
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.
Comment 24 Philippe Renon 2016-11-16 11:51:50 UTC
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.
Comment 25 Philippe Renon 2016-11-19 11:07:23 UTC
Could the last patch be merged ? It cleans the API of opencv references.

I have more commits on top of that patch...
Comment 26 Luis de Bethencourt 2016-11-21 14:12:51 UTC
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.
Comment 27 Luis de Bethencourt 2016-11-21 14:15:25 UTC
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?
Comment 28 Philippe Renon 2016-11-21 14:26:45 UTC
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...
Comment 29 Philippe Renon 2016-11-21 16:21:32 UTC
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
Comment 30 Luis de Bethencourt 2016-11-21 16:40:52 UTC
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 :)
Comment 31 Luis de Bethencourt 2016-11-21 16:47:31 UTC
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);
                                                               ^
Comment 32 Philippe Renon 2016-11-21 16:54:33 UTC
I'll take a look.
Comment 33 Philippe Renon 2016-11-21 16:55:26 UTC
Please ignore last comment. Did not realize it was about meson.
Comment 34 Philippe Renon 2016-11-21 17:06:15 UTC
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.
Comment 35 Philippe Renon 2016-11-21 18:25:30 UTC
Correction to the last comment : it is HAVE_OPENCV2_HIGHGUI_HIGHGUI_C_H that you need to have defined (not HAVE_HIGHGUI)
Comment 36 Luis de Bethencourt 2016-11-22 15:17:24 UTC
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?
Comment 37 Philippe Renon 2016-11-22 15:25:40 UTC
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.
Comment 38 Luis de Bethencourt 2016-11-22 15:46:27 UTC
(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 :)
Comment 39 Luis de Bethencourt 2016-11-26 17:58:16 UTC
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
Comment 40 Luis de Bethencourt 2016-11-26 17:58:47 UTC
Review of attachment 340437 [details] [review]:

Pushed
Comment 41 Sebastian Dröge (slomo) 2016-11-27 09:58:12 UTC
https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=7488a8fb3544634a1629ce0c804dc3c3ab4314ed

The plugin should be optional too
Comment 42 Luis de Bethencourt 2016-11-27 12:06:25 UTC
Thanks Sebastian! :)
Comment 43 Nicolas Dufresne (ndufresne) 2016-11-30 02:51:01 UTC
What's left here ?
Comment 44 Philippe Renon 2016-11-30 10:53:58 UTC
Nothing I guess.
Comment 45 Tim-Philipp Müller 2016-11-30 18:55:20 UTC
Ok, let's deal with any remaining issue into new bugs then. Thanks!