GNOME Bugzilla – Bug 747606
v4l2: adding cropping and compose functionality
Last modified: 2018-11-03 14:59:44 UTC
This patch adds cropping and composition to the v4l2src. The reason for limiting the patch to the source is the lack of a v4l2 output device to test on. The device available also only supports cropping, so composition also requires additional testing This patch has been developed with the current master branch, commit id: 8cfebfec8c6ca7a47dd064c8f5d3e587973f31a1
Created attachment 301261 [details] [review] Patch for adding cropping and compose using selection api
To use the cropping and composition, a structure is expected, for example: v4l2src crop=crop,left=100,flag=g ! videoconvert ! ximagesink All fields ommitted are filled with the default values. The structure names are expected to be crop for the cropping structure and compose for the composition structure. The flag paramter can also be set. It expects a string with l for the V4L2_SEL_FLAG_LE flag, g for the V4L2_SEL_FLAG_GE flag and kc for the V4L2_SEL_FLAG_KEEP_CONFIG flag
Created attachment 302219 [details] [review] Patch to not force the device open to fail if the selection api is not available
Review of attachment 301261 [details] [review]: Sorry for taking ages to review this. Note that v4l2_calls.h was merged into gstv4l2object.h, so this patch won't apply any more. ::: sys/v4l2/gstv4l2object.c @@ +3822,3 @@ + type = V4L2_BUF_TYPE_VIDEO_OUTPUT; + else + type = obj->type; Hans removed this stupidity, you can just use obj->type on new kernels. @@ +3838,3 @@ + + /* get the cropping bounds */ + if (v4l2_ioctl (obj->video_fd, VIDIOC_G_SELECTION, &bounds) < 0) { Did you mean to do something with the bounds ? @@ +3885,3 @@ + type = V4L2_BUF_TYPE_VIDEO_OUTPUT; + else + type = obj->type; Just use obj->type. @@ +3901,3 @@ + + /* get the compose bounds */ + if (v4l2_ioctl (obj->video_fd, VIDIOC_G_SELECTION, &bounds) < 0) { Same question, I don't see anything using the values in bounds.
Review of attachment 302219 [details] [review]: ::: sys/v4l2/v4l2_calls.c @@ +695,2 @@ /* test for the selection api */ + gst_v4l2_get_selection_capabilities (v4l2object); Yes, and sqash this into the first patch please.
Since it has been a while and since then I have been using a revised version, I have spend some time to rewrite the revised version for the latest version of gstreamer. All the core functionality is in the first patch, with 2 extra patches introducing commonly used features that may need some rework for pushing upstream. Any feedback is much appreciated :)
Created attachment 368153 [details] [review] Core cropping-compose functionality
Created attachment 368154 [details] [review] Centering cropping property
Created attachment 368155 [details] [review] Selection set signal
Review of attachment 368153 [details] [review]: I think we need to step back design nicer properties. Here's some possible solution, hopefully this will open the discussion. ::: sys/v4l2/gstv4l2object.c @@ +440,3 @@ + * Video for Linux API + * An example command line input: + * section=crop,top=150,flags= The SELECTION API in V4L2 is terribly loaded and complicated, it fits badly inside a single property. To help reflect that, there is no way a new user will manage to use your example here to set that property, since a GstStructure have a name, which in your case seems ignored, so syntax is "whatever/section=crop,..." which is terribly ugly (just like the extra-control property). As per the spec, to detect if the driver supports scaling, you need to be able to read back the value. This implementation does not allow this, which means you can only hardcode for your hardware. Overall, I think it would be much nicer to split into multiple properties, which then can be read/write. like: - "crop-rectangle" - "compose-rectangle" (or render-rectangle) And then you follow videosink path for render-rectangle property, which uses a list to represent the rectangle (removing the ugly name/ from GstStruct syntax). In general, a rectangle has the coordinate preceding the size. Then the naming is all equivalent, our preferred naming is: <x, y, width, height> But it's the same as "left, top, width, height" or "x1, y1, x2, y2". The exception being videocrop (left, top, right, bottom) which is hard to use. @@ +442,3 @@ + * section=crop,top=150,flags= + * + * Since: 1.13.x We never set Since marker on dev version. 1.14, even though I doubt this will be ready for that one. ::: sys/v4l2/gstv4l2src.c @@ +634,3 @@ gst_object_unref (allocator); + gst_v4l2_object_try_set_selection (src->v4l2object); Return value ?
Review of attachment 368154 [details] [review]: ::: sys/v4l2/gstv4l2object.c @@ +457,3 @@ + */ + g_object_class_install_property (gobject_class, PROP_CENTER_CROP, + g_param_spec_boolean ("hw-center-crop", "Hardware Center Crop", Doc and property name miss-match. In the widget libraries this would be more flexitly, you basically choose the gravity / direction. Like top-left, center-left, center, center-right, or using using NORTH/SOUTH/EAST/WEST, etc. This sound much superior. You also need to document the interection with the crop/render rectangles. If the hardware can scale, will this scale ? What the default ? How does this interact with the output format selected in caps ?
Review of attachment 368155 [details] [review]: ::: sys/v4l2/gstv4l2object.c @@ +472,3 @@ + + /** + * GstV4l2Src::selection-set: This is miss-leading, as it will be added to all objects.
-- 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-good/issues/179.