If i remember well, we had really good reason to do so. Would it be possible to elaborate please?

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.

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 ?

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.

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.

PS : please merge your patch. Looks good (and better than mine) ;)

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.

(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.

> > 
> > +  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.

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.

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

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, ...)

(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;

> > 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... ;)

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.

(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.

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' ;))

Agreed, we should focus on fixing the incorrect code or API. (don't worry, this code is C++ so we cannot use 'this' ;-P)

Attachment 340958 [details] pushed as ed38776 - opencvfilter: Properly port to GstVideoFilter