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 775288 - opencv: base opencv video filter class does not fully implement the video filter contract
opencv: base opencv video filter class does not fully implement the video fil...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal enhancement
: 1.11.1
Assigned To: Nicolas Dufresne (ndufresne)
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-11-28 21:55 UTC by Philippe Renon
Modified: 2016-11-30 02:23 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
opencvfilter: Properly port to GstVideoFilter (8.13 KB, patch)
2016-11-29 03:02 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description Philippe Renon 2016-11-28 21:55:50 UTC
For isntance, the base opencv video filter implements directly some base transform vfuncs instead of equivalents from video filter.
Comment 1 Nicolas Dufresne (ndufresne) 2016-11-29 01:07:03 UTC
If i remember well, we had really good reason to do so. Would it be possible to elaborate please?
Comment 2 Nicolas Dufresne (ndufresne) 2016-11-29 03:02:05 UTC
Created attachment 340958 [details] [review]
opencvfilter: Properly port to GstVideoFilter

issue that I notice is that the frame stride is completly ignored. This is 
bad because the base class advertise GstVideoMeta support. Might have felt
guilty, but I completed the port to GstVideoFilter. Note, I omitted the
set_info() part, where we could add an optimized helper to prevent parsing
the caps into GstVideoInfo a second time. That didn't feel urgent enough for
tonight.

If anyone wonder why I'm not just merging this, well, it'S late, I'd like to
test a bit more then just running faceoverlay for 5s. The patch message:

opencvfilter: Properly port to GstVideoFilter

This is a subblass of VideoFilter but yet does not use any of it's
features. This also fixes issue in case the incoming images have custom
strides as the VideoMeta is no longer ignored.
Comment 3 Philippe Renon 2016-11-29 09:37:23 UTC
I have a patch of my own. Will take a look at yours and see if there is anything to keep in mine.

My patch also does a lot of renaming of local vars (the ones that hold the proper casted element...). Is that something worth keeping ?
Comment 4 Philippe Renon 2016-11-29 09:42:37 UTC
My patch also impacts all the opencv elements (except one or two that don't derive from gstopencvvideofilter): i.e. making them also closer to a video filter.
Comment 5 Philippe Renon 2016-11-29 09:45:29 UTC
Sorry for adding comments one by one... 

My patch will for instance pass GstVideoFrame in place of GstBuffer when calling the fclass->cv_trans_ip_func (transform, frame->buffer, transform->cvImage) vfunc.
Comment 6 Philippe Renon 2016-11-29 10:11:36 UTC
PS : please merge your patch. Looks good (and better than mine) ;)
Comment 7 Philippe Renon 2016-11-29 15:18:35 UTC
Minor nitpicks:

+  vfilter_class->transform_frame = gst_opencv_video_filter_transform_frame;
+  vfilter_class->transform_frame_ip = gst_opencv_video_filter_transform_frame_ip;
+  vfilter_class->set_info = gst_opencv_video_filter_set_info;
^ set_info should come first ? (same applies where the protorypes are declared and other places...).


+  transform->out_cvImage->imageData = (char *) outframe->data[0];
+  transform->out_cvImage->imageSize = outframe->info.size;
+  transform->out_cvImage->widthStep = outframe->info.stride[0];
^ there are macros to extract those items from a GstVideoFrames.


+  ret = fclass->cv_trans_ip_func (transform, frame->buffer, transform->cvImage);
^ my patch will take care of renaming the vfuncs to proper names (i.e. cv_transform_frame_ip) and passing GstVideoFrame in place of GstBuffer, which will impact all elements deriving from gstopencvvideofilter.
Comment 8 Nicolas Dufresne (ndufresne) 2016-11-29 15:50:36 UTC
(In reply to Philippe Renon from comment #7)
> Minor nitpicks:
> 
> +  vfilter_class->transform_frame = gst_opencv_video_filter_transform_frame;
> +  vfilter_class->transform_frame_ip =
> gst_opencv_video_filter_transform_frame_ip;
> +  vfilter_class->set_info = gst_opencv_video_filter_set_info;
> ^ set_info should come first ? (same applies where the protorypes are
> declared and other places...).

Ok.

> 
> 
> +  transform->out_cvImage->imageData = (char *) outframe->data[0];
> +  transform->out_cvImage->imageSize = outframe->info.size;
> +  transform->out_cvImage->widthStep = outframe->info.stride[0];
> ^ there are macros to extract those items from a GstVideoFrames.

I know, it's been many many years, and I still don't see the point of using them ;-P

> 
> +  ret = fclass->cv_trans_ip_func (transform, frame->buffer,
> transform->cvImage);
> ^ my patch will take care of renaming the vfuncs to proper names (i.e.
> cv_transform_frame_ip) and passing GstVideoFrame in place of GstBuffer,
> which will impact all elements deriving from gstopencvvideofilter.

This is quite intrusive. What do we gain from that ? I guess I need to go over the elements to see. I would be tempted to just remove the GstBuffer if possible, and only have IplImage in there.
Comment 9 Philippe Renon 2016-11-29 15:54:32 UTC
> > 
> > +  ret = fclass->cv_trans_ip_func (transform, frame->buffer,
> > transform->cvImage);
> > ^ my patch will take care of renaming the vfuncs to proper names (i.e.
> > cv_transform_frame_ip) and passing GstVideoFrame in place of GstBuffer,
> > which will impact all elements deriving from gstopencvvideofilter.
> 
> This is quite intrusive. What do we gain from that ? I guess I need to go
> over the elements to see. I would be tempted to just remove the GstBuffer if
> possible, and only have IplImage in there.

Some elements get a TIMESTAMP from the buffer. Same and more can be gotten from a video frame.
Comment 10 Nicolas Dufresne (ndufresne) 2016-11-29 16:14:04 UTC
Element that uses the GstBuffer:

cvlaplace/cvsobel/edgedetect/
- it already have the mapped buffer in outimage
- ignores GstVideoMeta
- ignored buffer_map return value.

facedetect/motioncells:
- The VideoFilter ensure the buffer is writable
+ Fair use of the timestamps to post message

tmplatematch:
- The usage of buf make no sense at all

So the only use I found is for timestamps, we could just pass that. Random calls to make-writable are really odd (looks like porting errors, from 0.10 to 1.0). Conclusion there is a lot more cleanup to do. I'll do the small changes you proposed.  If you feel like attaching patches to create an interesting cleanup collection, that would be nice. Please, create small patch. One big intrusive patch is really annoying to review.

In general, I believe if we provide less, it will avoid wrong usage. Clearly someone didn't know what he was doing. Now, this is arguably a baseclass in a library, so flexibility is also important. But let's cleanup up all the usage to the buffer to be limited to timestamp as it's supposed. This will also remove couple of useless copies. Then we can probably think about what should the library interface be. I don't think we'll be moving those to -good in this cycle.
Comment 11 Philippe Renon 2016-11-29 16:42:45 UTC
Great analysis. Matches what I found and cleaned up while doing my single massive patch.

I'll take over once you are done and will propose small incremental patches.

Please also consider merging related patches in :
- https://bugzilla.gnome.org/show_bug.cgi?id=774576
and
- https://bugzilla.gnome.org/show_bug.cgi?id=772822
Comment 12 Philippe Renon 2016-11-29 17:01:44 UTC
About naming:

1/ Some elements are prefixed by cv (cvlaplace, cvsobel) while others are not (edgedetect, facedetect). What would be the preferred convention ? Don't worry, if I rename them it will be one of the last patch...

2/ When passing a GstOpencvVideoFilter element as an argument, the argument name should not be trans or what not. I suggest to use cvfilter (same as vfilter for a GstVideoFilter).

3/ Most concrete elements, do something like this:
    GstCvSobel *filter = GST_CV_SOBEL (base);
According to 2/ it should be:
    GstCvSobel *filter = GST_CV_SOBEL (cvfilter);
And better (?) :
    GstCvSobel *cvsobel = GST_CV_SOBEL (base);
I am not sure what the current/recommended convention is.
Variable names can get quite long for some elements (GstTemplateMatch, ...)
Comment 13 Nicolas Dufresne (ndufresne) 2016-11-29 17:21:53 UTC
(In reply to Philippe Renon from comment #11)
> Great analysis. Matches what I found and cleaned up while doing my single
> massive patch.
> 
> I'll take over once you are done and will propose small incremental patches.
> 
> Please also consider merging related patches in :
> - https://bugzilla.gnome.org/show_bug.cgi?id=774576
> and
> - https://bugzilla.gnome.org/show_bug.cgi?id=772822

Adding to my ToDo.

(In reply to Philippe Renon from comment #12)
> About naming:
> 
> 1/ Some elements are prefixed by cv (cvlaplace, cvsobel) while others are
> not (edgedetect, facedetect). What would be the preferred convention ? Don't
> worry, if I rename them it will be one of the last patch...

I personally prefer prefixing everything with cv, but it's not like we have any other element that do edgetectect or facedetect. Might make users angry if we rename. It's something we need to discuss and decide before a future move to -good.

> 
> 2/ When passing a GstOpencvVideoFilter element as an argument, the argument
> name should not be trans or what not. I suggest to use cvfilter (same as
> vfilter for a GstVideoFilter).

This is cosmetic, but I agree, while at it, we should normalise. We could have:
  - trans: for GstBaseTransform
  - filter: For GstVideoFilter
  - cvfilter: for GstOpencvVideoFilter

While at it (assuming we go ahead and break the API), we could rename from GstOpencvVideoFilter to GstOpencvFilter. OpenCV is about video so it's redundant.

> 
> 3/ Most concrete elements, do something like this:
>     GstCvSobel *filter = GST_CV_SOBEL (base);
> According to 2/ it should be:
>     GstCvSobel *filter = GST_CV_SOBEL (cvfilter);
> And better (?) :
>     GstCvSobel *cvsobel = GST_CV_SOBEL (base);

This makes absolutely no difference. The value of the pointer is exactly the same. The inheritance in GObject is just a C cast combined with a type check. If you are certain that the type is right and require performance you could just do:
      GstCvSobel *filter = (GstCvSobel *) base;
Comment 14 Philippe Renon 2016-11-29 17:54:48 UTC
> > 3/ Most concrete elements, do something like this:
> >     GstCvSobel *filter = GST_CV_SOBEL (base);
> > According to 2/ it should be:
> >     GstCvSobel *filter = GST_CV_SOBEL (cvfilter);
> > And better (?) :
> >     GstCvSobel *cvsobel = GST_CV_SOBEL (base);
> 
> This makes absolutely no difference. The value of the pointer is exactly the
> same. The inheritance in GObject is just a C cast combined with a type
> check. If you are certain that the type is right and require performance you
> could just do:
>       GstCvSobel *filter = (GstCvSobel *) base;

My point was not about correctness or performance, but plain naming again.

An example of how things are currently:

static GstFlowReturn
gst_cv_sobel_transform (GstOpencvVideoFilter * base, GstBuffer * buf,
    IplImage * img, GstBuffer * outbuf, IplImage * outimg)
{
  GstCvSobel *filter = GST_CV_SOBEL (base);
  ...

Good elements would have cvfilter instead of base and the cvsobel instead of filter. What's the consensus.
I my massive patch (that I'll drop) I went ahead and renamed all "filter" to element names (shortening some). But the changes it entails are cumbersome in places. Line width were changed down the line, requiring many edits to keep them under 80 chars. Btw 80 chars..., come on... ;)
Comment 15 Nicolas Dufresne (ndufresne) 2016-11-29 18:52:32 UTC
Ok, I have an argument against, we usally want to use generic name for the local object. I prefer using "self", but other can use generic names like filter. The point is that it helps reusing the code elsewhere, in cases you need similar but different code.
Comment 16 Philippe Renon 2016-11-29 20:06:54 UTC
(In reply to Nicolas Dufresne (stormer) from comment #15)
> Ok, I have an argument against, we usally want to use generic name for the
> local object. I prefer using "self", but other can use generic names like
> filter. The point is that it helps reusing the code elsewhere, in cases you
> need similar but different code.

Ok. Point taken. I agree on the generic name approach.
- Makes it easier to scan code and move it around.
- Does not ask you to be creative where it brings little value.
- Generic names are usually shorter...

I felt quite uneasy with the result of the non generic approach.

I'll try self or leave filter.
Comment 17 Tim-Philipp Müller 2016-11-29 20:22:13 UTC
I think it'd be best not to change these things gratuitously unless you're rewriting the code anyway, everything else is just noise and makes it harder to figure out the history of code sections later down the road. (Personally I dislike 'self' and 'this' ;))
Comment 18 Nicolas Dufresne (ndufresne) 2016-11-29 21:10:51 UTC
Agreed, we should focus on fixing the incorrect code or API. (don't worry, this code is C++ so we cannot use 'this' ;-P)
Comment 19 Nicolas Dufresne (ndufresne) 2016-11-30 02:22:05 UTC
Attachment 340958 [details] pushed as ed38776 - opencvfilter: Properly port to GstVideoFilter