Created attachment 317185 [details] [review]
Support video crop when convert frame

Created attachment 317186 [details] [review]
Support video crop when convert frame

Update the patch.

Review of attachment 317186 [details] [review]:

In theory videoscale and videoconvert could also do the cropping nowadays. GstVideoConverter has support for it. But making them do here if both converters don't have to convert everything might not be easy, needs to be checked

::: gst-libs/gst/video/convertframe.c
@@ +121,3 @@
   GST_DEBUG ("creating elements");
   if (!create_element ("appsrc", &src, &error) ||
+      !create_element ("videocrop", &vcrop, &error) ||

videocrop is in gst-plugins-good, so the usage of it here should be optional

Created attachment 317188 [details] [review]
Support video crop when convert frame

Update patch based on review. Changed videocrop to optional element.

Review of attachment 317188 [details] [review]:

::: gst-libs/gst/video/convertframe.c
@@ +119,3 @@
 
+  if (!create_element ("videocrop", &vcrop, &error))
+    GST_DEBUG ("No videocrop element, can't process crop.");

Not a problem if no crop meta but see below

@@ +140,3 @@
+    gst_bin_add_many (GST_BIN (pipeline), src, vcrop, csp, vscale, sink, NULL);
+  else
+    gst_bin_add_many (GST_BIN (pipeline), src, csp, vscale, sink, NULL);

Maybe keep the old add_many() line and then only add a
if (vcrop) gst_bin_add (GST_BIN (pipeline), vcrop)

@@ +152,3 @@
+    GST_DEBUG ("crop meta [x,y,width,height]: %d %d %d %d", cmeta->x,
+        cmeta->y, cmeta->width, cmeta->height);
+  }

if (!vcrop && cmeta) this should probably print a g_warning()

Created attachment 317191 [details] [review]
Support video crop when convert frame

Update based on review.

Review of attachment 317191 [details] [review]:

::: gst-libs/gst/video/convertframe.c
@@ +122,3 @@
+    if (cmeta)
+      g_warning ("buffer has crop meta, but no videocrop element. ");
+      g_warning ("can't process crop.");

Put this in a single g_warning() and prefix it with where this comes from, e.g.

g_warning("gst_video_convert_sample: Buffer has crop metadata but videocrop element is not found. Cropping will be disabled")

@@ +142,3 @@
   GST_DEBUG ("adding elements");
   gst_bin_add_many (GST_BIN (pipeline), src, csp, vscale, sink, NULL);
+  if (vcrop) gst_bin_add (GST_BIN (pipeline), vcrop);

You should also run gst-indent on your .c files before you submit patches :) A line break is missing here

@@ +163,3 @@
+      goto link_failed;
+
+    GST_DEBUG ("linking vcrop->csp");

It might be necessary to have another videoconvert before videocrop too

Created attachment 317208 [details] [review]
Support video crop when convert frame.

Update patch based on review.

As the videoconvert library now support convert/scale/crop in one pass, can't we just update videoconvert to support all this, and then we don't need multiple elements/pass here ?

commit a476145797d3ea2d3d1467a48b18fc5000a3b8a1
Author: Song Bing <b06498@freescale.com>
Date:   Fri Dec 11 21:42:00 2015 +0800

    video/convertframe: Add crop meta support via videocrop
    
    https://bugzilla.gnome.org/show_bug.cgi?id=759329

(In reply to Nicolas Dufresne (stormer) from comment #9)
> As the videoconvert library now support convert/scale/crop in one pass,
> can't we just update videoconvert to support all this, and then we don't
> need multiple elements/pass here ?

See comment #3 :) I was going to open a new bug for that, the videoconvert refactoring is going to be some more work though.

The other question I just noticed is... why does it use the CROP meta at all if downstream is an appsink? appsink is not going to report that it can CROP, so putting a CROP meta on the buffers would be wrong anyway.

The video buffer which has CROP meta is get from video sink. The video sink can handle CROP meta. Thumbnail function will get the last-buffer from the video sink and convert it to user's desired format. So we should consider the CROP meta to get right thumbnail.

Makes sense