GNOME Bugzilla – Bug 748858
opencv: cvequalizehist: add color support
Last modified: 2018-11-03 13:34:58 UTC
Created attachment 302826 [details] [review] patch for extending an input format The original "cvequalizehist" element only accepts a grayscale image as an input. However, the most images users want to handle are colored images. I extended its input format from grayscale to colored. Please find the attached patch. This patch was modified from d4703fa8806cf7d9cc778fb43370daebb37e70cd, and you can check it using the following command. gst-launch-1.0 videotestsrc pattern=13 ! tee name=t t. ! queue ! videoconvert ! xvimagesink t. ! queue ! cvequalizehist ! videoconvert ! xvimagesink Thank you.
Created attachment 302828 [details] [review] patch for extending an input format (2nd ed)
Comment on attachment 302826 [details] [review] patch for extending an input format Please remember to mark as obsolete patches that were replaced by newer versions.
Review of attachment 302828 [details] [review]: A nice addition, thanks for the patch. Some work is still needed, please see the comments in the review. ::: ext/opencv/gstcvequalizehist.c @@ +74,3 @@ + cvReleaseImage (&filter->plane_r); + cvReleaseImage (&filter->plane_g); + cvReleaseImage (&filter->plane_b); This seems far too late to release those. It should be released when going from PAUSED to READY state. I'd recommend implementing the stop method from GstBaseTransform. @@ +104,3 @@ + gst_caps_append (caps, gst_opencv_caps_from_cv_image_type (CV_8UC1)); + templ = gst_pad_template_new ("sink", GST_PAD_SINK, GST_PAD_ALWAYS, + gst_caps_ref (caps)); gst_pad_template_new doesn't take ownership of the caps so you don't need an extra ref here @@ +106,3 @@ + gst_caps_ref (caps)); + gst_element_class_add_pad_template (element_class, templ); + templ = gst_pad_template_new ("src", GST_PAD_SRC, GST_PAD_ALWAYS, caps); And then you need to unref the caps after this part. It might just be simpler to keep the static pad templates as they were and just add RGB and BGR to the list of supported video formats. @@ +127,3 @@ + filter->plane_r = cvCreateImage (cvSize (in_width, in_height), in_depth, 1); + filter->plane_g = cvCreateImage (cvSize (in_width, in_height), in_depth, 1); + filter->plane_b = cvCreateImage (cvSize (in_width, in_height), in_depth, 1); The caps can be changed during playback and you would leak the previous planes. You should release those before creating new ones.
Yay, bugzilla! Sorry for the spam, bugzilla gave me an error when submitting and stalled. Had to push 3 times to make it confirm my review but apparently it also submitted it 3 times despite erroring.
Created attachment 302955 [details] [review] patch for extending an input format (3rd ed) Thank you for your kind comments. I modified the patch according to your comments. Please find the attachment.
ping~
can anybody review this?
Comment on attachment 302955 [details] [review] patch for extending an input format (3rd ed) Thanks for the updated patch! Unfortunately it no longer applies and needs to be rebased on top of master (note that the file was renamed from .c to .cpp).
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/248.