[RFC v3] [RFC] v4l2: Support for multiple selections

[RFC v3] [RFC] v4l2: Support for multiple selections

[Ricardo Ribalda Delgado]

Extend v4l2 selection API to support multiple selection areas, this way
sensors that support multiple readout areas can work with multiple areas
of insterest.

The struct v4l2_selection and v4l2_subdev_selection has been extented
with a new field rectangles. If it is value is different than zero the
pr array is used instead of the r field.

A new structure v4l2_ext_rect has been defined, containing 4 reserved
fields for future improvements, as suggested by Hans.

Two helper functiona are also added, to help the drivers that support
a single selection.

This Patch ONLY adds the modification to the core. Once it is agreed, a
new version including changes on the drivers that handle the selection
api will come.

This patch includes all the comments by Hans Verkuil.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
v3:
-Changes on compat-ioctl32
-Remove checks on v4l2_selection_set_rect

 drivers/media/v4l2-core/v4l2-common.c         | 36 +++++++++++++++
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 63 +++++++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-ioctl.c          | 37 +++++++++++++---
 include/media/v4l2-common.h                   |  6 +++
 include/uapi/linux/v4l2-subdev.h              | 10 ++++-
 include/uapi/linux/videodev2.h                | 21 +++++++--
 6 files changed, 162 insertions(+), 11 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index a95e5e2..f60a2ce 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -886,3 +886,39 @@ void v4l2_get_timestamp(struct timeval *tv)
 	tv->tv_usec = ts.tv_nsec / NSEC_PER_USEC;
 }
 EXPORT_SYMBOL_GPL(v4l2_get_timestamp);
+
+int v4l2_selection_get_rect(const struct v4l2_selection *s,
+					struct v4l2_ext_rect *r)
+{
+	if (s->rectangles > 1)
+		return -EINVAL;
+	if (s->rectangles == 1) {
+		*r = s->pr[0];
+		return 0;
+	}
+	if (s->r.width < 0 || s->r.height < 0)
+		return -EINVAL;
+	r->left = s->r.left;
+	r->top = s->r.top;
+	r->width = s->r.width;
+	r->height = s->r.height;
+	memset(r->reserved, 0, sizeof(r->reserved));
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_selection_get_rect);
+
+void v4l2_selection_set_rect(struct v4l2_selection *s,
+				const struct v4l2_ext_rect *r)
+{
+	if (s->rectangles == 0) {
+		s->r.left = r->left;
+		s->r.top = r->top;
+		s->r.width = r->width;
+		s->r.height = r->height;
+		return;
+	}
+	s->pr[0] = *r;
+	s->rectangles = 1;
+	return;
+}
+EXPORT_SYMBOL_GPL(v4l2_selection_set_rect);
diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 8f7a6a4..36ed3c3 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -777,6 +777,54 @@ static int put_v4l2_subdev_edid32(struct v4l2_subdev_edid *kp, struct v4l2_subde
 	return 0;
 }
 
+struct v4l2_selection32 {
+	__u32			type;
+	__u32			target;
+	__u32                   flags;
+	union {
+		struct v4l2_rect        r;
+		compat_uptr_t           *pr;
+	};
+	__u32                   rectangles;
+	__u32                   reserved[8];
+} __packed;
+
+static int get_v4l2_selection32(struct v4l2_selection *kp,
+				struct v4l2_selection32 __user *up)
+{
+	if (!access_ok(VERIFY_READ, up, sizeof(struct v4l2_selection32))
+				|| (copy_from_user(kp, up, sizeof(*kp))))
+		return -EFAULT;
+
+	if (kp->rectangles) {
+		compat_uptr_t usr_ptr;
+		if (get_user(usr_ptr, &up->pr))
+			return -EFAULT;
+		kp->pr = compat_ptr(usr_ptr);
+	}
+
+	return 0;
+
+}
+
+static int put_v4l2_selection32(struct v4l2_selection *kp,
+				struct v4l2_selection32 __user *up)
+{
+	compat_uptr_t usr_ptr = 0;
+
+	if ((kp->rectangles) &&	get_user(usr_ptr, &up->pr))
+		return -EFAULT;
+
+	if (!access_ok(VERIFY_WRITE, up, sizeof(struct v4l2_selection32))
+				|| (copy_to_user(kp, up, sizeof(*kp))))
+		return -EFAULT;
+
+	if (kp->rectangles)
+		put_user(usr_ptr, &up->pr);
+
+	return 0;
+
+}
 
 #define VIDIOC_G_FMT32		_IOWR('V',  4, struct v4l2_format32)
 #define VIDIOC_S_FMT32		_IOWR('V',  5, struct v4l2_format32)
@@ -796,6 +844,8 @@ static int put_v4l2_subdev_edid32(struct v4l2_subdev_edid *kp, struct v4l2_subde
 #define	VIDIOC_DQEVENT32	_IOR ('V', 89, struct v4l2_event32)
 #define VIDIOC_CREATE_BUFS32	_IOWR('V', 92, struct v4l2_create_buffers32)
 #define VIDIOC_PREPARE_BUF32	_IOWR('V', 93, struct v4l2_buffer32)
+#define VIDIOC_G_SELECTION32	_IOWR('V', 94, struct v4l2_selection32)
+#define VIDIOC_S_SELECTION32	_IOWR('V', 95, struct v4l2_selection32)
 
 #define VIDIOC_OVERLAY32	_IOW ('V', 14, s32)
 #define VIDIOC_STREAMON32	_IOW ('V', 18, s32)
@@ -817,6 +867,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 		struct v4l2_event v2ev;
 		struct v4l2_create_buffers v2crt;
 		struct v4l2_subdev_edid v2edid;
+		struct v4l2_selection v2sel;
 		unsigned long vx;
 		int vi;
 	} karg;
@@ -851,6 +902,8 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	case VIDIOC_PREPARE_BUF32: cmd = VIDIOC_PREPARE_BUF; break;
 	case VIDIOC_SUBDEV_G_EDID32: cmd = VIDIOC_SUBDEV_G_EDID; break;
 	case VIDIOC_SUBDEV_S_EDID32: cmd = VIDIOC_SUBDEV_S_EDID; break;
+	case VIDIOC_G_SELECTION32: cmd = VIDIOC_G_SELECTION; break;
+	case VIDIOC_S_SELECTION32: cmd = VIDIOC_S_SELECTION; break;
 	}
 
 	switch (cmd) {
@@ -922,6 +975,11 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	case VIDIOC_DQEVENT:
 		compatible_arg = 0;
 		break;
+	case VIDIOC_G_SELECTION:
+	case VIDIOC_S_SELECTION:
+		err = get_v4l2_selection32(&karg.v2sel, up);
+		compatible_arg = 0;
+		break;
 	}
 	if (err)
 		return err;
@@ -994,6 +1052,11 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	case VIDIOC_ENUMINPUT:
 		err = put_v4l2_input32(&karg.v2i, up);
 		break;
+
+	case VIDIOC_G_SELECTION:
+	case VIDIOC_S_SELECTION:
+		err = put_v4l2_selection32(&karg.v2sel, up);
+		break;
 	}
 	return err;
 }
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 68e6b5e..aa1c2a4 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -572,11 +572,22 @@ static void v4l_print_crop(const void *arg, bool write_only)
 static void v4l_print_selection(const void *arg, bool write_only)
 {
 	const struct v4l2_selection *p = arg;
+	int i;
 
-	pr_cont("type=%s, target=%d, flags=0x%x, wxh=%dx%d, x,y=%d,%d\n",
-		prt_names(p->type, v4l2_type_names),
-		p->target, p->flags,
-		p->r.width, p->r.height, p->r.left, p->r.top);
+	if (p->rectangles == 0)
+		pr_cont("type=%s, target=%d, flags=0x%x, wxh=%dx%d, x,y=%d,%d\n",
+			prt_names(p->type, v4l2_type_names),
+			p->target, p->flags,
+			p->r.width, p->r.height, p->r.left, p->r.top);
+	else{
+		pr_cont("type=%s, target=%d, flags=0x%x rectangles=%d\n",
+			prt_names(p->type, v4l2_type_names),
+			p->target, p->flags, p->rectangles);
+		for (i = 0; i < p->rectangles; i++)
+			pr_cont("rectangle %d: wxh=%dx%d, x,y=%d,%d\n",
+				i, p->r.width, p->r.height,
+				p->r.left, p->r.top);
+	}
 }
 
 static void v4l_print_jpegcompression(const void *arg, bool write_only)
@@ -1692,7 +1703,9 @@ static int v4l_cropcap(const struct v4l2_ioctl_ops *ops,
 				struct file *file, void *fh, void *arg)
 {
 	struct v4l2_cropcap *p = arg;
-	struct v4l2_selection s = { .type = p->type };
+	struct v4l2_selection s = {
+		.type = p->type,
+	};
 	int ret;
 
 	if (ops->vidioc_cropcap)
@@ -2253,6 +2266,20 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
 		}
 		break;
 	}
+
+	case VIDIOC_G_SELECTION:
+	case VIDIOC_S_SELECTION: {
+		struct v4l2_selection *s = parg;
+
+		if (s->rectangles) {
+			*user_ptr = (void __user *)s->pr;
+			*kernel_ptr = (void *)&s->pr;
+			*array_size = sizeof(struct v4l2_ext_rect)
+				* s->rectangles;
+			ret = 1;
+		}
+		break;
+	}
 	}
 
 	return ret;
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index 015ff82..417ab82 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -216,4 +216,10 @@ struct v4l2_fract v4l2_calc_aspect_ratio(u8 hor_landscape, u8 vert_portrait);
 
 void v4l2_get_timestamp(struct timeval *tv);
 
+int v4l2_selection_get_rect(const struct v4l2_selection *s,
+					struct v4l2_ext_rect *r);
+
+void v4l2_selection_set_rect(struct v4l2_selection *s,
+					const struct v4l2_ext_rect *r);
+
 #endif /* V4L2_COMMON_H_ */
diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
index a33c4da..c02a886 100644
--- a/include/uapi/linux/v4l2-subdev.h
+++ b/include/uapi/linux/v4l2-subdev.h
@@ -133,6 +133,8 @@ struct v4l2_subdev_frame_interval_enum {
  *	    defined in v4l2-common.h; V4L2_SEL_TGT_* .
  * @flags: constraint flags, defined in v4l2-common.h; V4L2_SEL_FLAG_*.
  * @r: coordinates of the selection window
+ * @pr:		array of rectangles containing the selection windows
+ * @rectangles:	Number of rectangles in pr structure. If zero, r is used instead
  * @reserved: for future use, set to zero for now
  *
  * Hardware may use multiple helper windows to process a video stream.
@@ -144,8 +146,12 @@ struct v4l2_subdev_selection {
 	__u32 pad;
 	__u32 target;
 	__u32 flags;
-	struct v4l2_rect r;
-	__u32 reserved[8];
+	union {
+		struct v4l2_rect r;
+		struct v4l2_ext_rect *pr;
+	};
+	__u32 rectangles;
+	__u32 reserved[7];
 };
 
 struct v4l2_subdev_edid {
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 95ef455..a4a7902 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -211,6 +211,14 @@ struct v4l2_rect {
 	__s32   height;
 };
 
+struct v4l2_ext_rect {
+	__s32   left;
+	__s32   top;
+	__u32   width;
+	__u32   height;
+	__u32   reserved[4];
+};
+
 struct v4l2_fract {
 	__u32   numerator;
 	__u32   denominator;
@@ -804,6 +812,8 @@ struct v4l2_crop {
  *		defined in v4l2-common.h; V4L2_SEL_TGT_* .
  * @flags:	constraints flags, defined in v4l2-common.h; V4L2_SEL_FLAG_*.
  * @r:		coordinates of selection window
+ * @pr:		array of rectangles containing the selection windows
+ * @rectangles:	Number of rectangles in pr structure. If zero, r is used instead
  * @reserved:	for future use, rounds structure size to 64 bytes, set to zero
  *
  * Hardware may use multiple helper windows to process a video stream.
@@ -814,10 +824,13 @@ struct v4l2_selection {
 	__u32			type;
 	__u32			target;
 	__u32                   flags;
-	struct v4l2_rect        r;
-	__u32                   reserved[9];
-};
-
+	union {
+		struct v4l2_rect        r;
+		struct v4l2_ext_rect    *pr;
+	};
+	__u32                   rectangles;
+	__u32                   reserved[8];
+} __packed;
 
 /*
  *      A N A L O G   V I D E O   S T A N D A R D
-- 
1.8.4.rc3

Re: [RFC v3] [RFC] v4l2: Support for multiple selections

[Ricardo Ribalda Delgado]

Ping?

Any comment about this version? Shall I post a new one with the
modified device drivers?

Thanks!

On Tue, Oct 1, 2013 at 12:33 PM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> Extend v4l2 selection API to support multiple selection areas, this way
> sensors that support multiple readout areas can work with multiple areas
> of insterest.
>
> The struct v4l2_selection and v4l2_subdev_selection has been extented
> with a new field rectangles. If it is value is different than zero the
> pr array is used instead of the r field.
>
> A new structure v4l2_ext_rect has been defined, containing 4 reserved
> fields for future improvements, as suggested by Hans.
>
> Two helper functiona are also added, to help the drivers that support
> a single selection.
>
> This Patch ONLY adds the modification to the core. Once it is agreed, a
> new version including changes on the drivers that handle the selection
> api will come.
>
> This patch includes all the comments by Hans Verkuil.
>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
> v3:
> -Changes on compat-ioctl32
> -Remove checks on v4l2_selection_set_rect
>
>  drivers/media/v4l2-core/v4l2-common.c         | 36 +++++++++++++++
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 63 +++++++++++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-ioctl.c          | 37 +++++++++++++---
>  include/media/v4l2-common.h                   |  6 +++
>  include/uapi/linux/v4l2-subdev.h              | 10 ++++-
>  include/uapi/linux/videodev2.h                | 21 +++++++--
>  6 files changed, 162 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index a95e5e2..f60a2ce 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -886,3 +886,39 @@ void v4l2_get_timestamp(struct timeval *tv)
>         tv->tv_usec = ts.tv_nsec / NSEC_PER_USEC;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_get_timestamp);
> +
> +int v4l2_selection_get_rect(const struct v4l2_selection *s,
> +                                       struct v4l2_ext_rect *r)
> +{
> +       if (s->rectangles > 1)
> +               return -EINVAL;
> +       if (s->rectangles == 1) {
> +               *r = s->pr[0];
> +               return 0;
> +       }
> +       if (s->r.width < 0 || s->r.height < 0)
> +               return -EINVAL;
> +       r->left = s->r.left;
> +       r->top = s->r.top;
> +       r->width = s->r.width;
> +       r->height = s->r.height;
> +       memset(r->reserved, 0, sizeof(r->reserved));
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_selection_get_rect);
> +
> +void v4l2_selection_set_rect(struct v4l2_selection *s,
> +                               const struct v4l2_ext_rect *r)
> +{
> +       if (s->rectangles == 0) {
> +               s->r.left = r->left;
> +               s->r.top = r->top;
> +               s->r.width = r->width;
> +               s->r.height = r->height;
> +               return;
> +       }
> +       s->pr[0] = *r;
> +       s->rectangles = 1;
> +       return;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_selection_set_rect);
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index 8f7a6a4..36ed3c3 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -777,6 +777,54 @@ static int put_v4l2_subdev_edid32(struct v4l2_subdev_edid *kp, struct v4l2_subde
>         return 0;
>  }
>
> +struct v4l2_selection32 {
> +       __u32                   type;
> +       __u32                   target;
> +       __u32                   flags;
> +       union {
> +               struct v4l2_rect        r;
> +               compat_uptr_t           *pr;
> +       };
> +       __u32                   rectangles;
> +       __u32                   reserved[8];
> +} __packed;
> +
> +static int get_v4l2_selection32(struct v4l2_selection *kp,
> +                               struct v4l2_selection32 __user *up)
> +{
> +       if (!access_ok(VERIFY_READ, up, sizeof(struct v4l2_selection32))
> +                               || (copy_from_user(kp, up, sizeof(*kp))))
> +               return -EFAULT;
> +
> +       if (kp->rectangles) {
> +               compat_uptr_t usr_ptr;
> +               if (get_user(usr_ptr, &up->pr))
> +                       return -EFAULT;
> +               kp->pr = compat_ptr(usr_ptr);
> +       }
> +
> +       return 0;
> +
> +}
> +
> +static int put_v4l2_selection32(struct v4l2_selection *kp,
> +                               struct v4l2_selection32 __user *up)
> +{
> +       compat_uptr_t usr_ptr = 0;
> +
> +       if ((kp->rectangles) && get_user(usr_ptr, &up->pr))
> +               return -EFAULT;
> +
> +       if (!access_ok(VERIFY_WRITE, up, sizeof(struct v4l2_selection32))
> +                               || (copy_to_user(kp, up, sizeof(*kp))))
> +               return -EFAULT;
> +
> +       if (kp->rectangles)
> +               put_user(usr_ptr, &up->pr);
> +
> +       return 0;
> +
> +}
>
>  #define VIDIOC_G_FMT32         _IOWR('V',  4, struct v4l2_format32)
>  #define VIDIOC_S_FMT32         _IOWR('V',  5, struct v4l2_format32)
> @@ -796,6 +844,8 @@ static int put_v4l2_subdev_edid32(struct v4l2_subdev_edid *kp, struct v4l2_subde
>  #define        VIDIOC_DQEVENT32        _IOR ('V', 89, struct v4l2_event32)
>  #define VIDIOC_CREATE_BUFS32   _IOWR('V', 92, struct v4l2_create_buffers32)
>  #define VIDIOC_PREPARE_BUF32   _IOWR('V', 93, struct v4l2_buffer32)
> +#define VIDIOC_G_SELECTION32   _IOWR('V', 94, struct v4l2_selection32)
> +#define VIDIOC_S_SELECTION32   _IOWR('V', 95, struct v4l2_selection32)
>
>  #define VIDIOC_OVERLAY32       _IOW ('V', 14, s32)
>  #define VIDIOC_STREAMON32      _IOW ('V', 18, s32)
> @@ -817,6 +867,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>                 struct v4l2_event v2ev;
>                 struct v4l2_create_buffers v2crt;
>                 struct v4l2_subdev_edid v2edid;
> +               struct v4l2_selection v2sel;
>                 unsigned long vx;
>                 int vi;
>         } karg;
> @@ -851,6 +902,8 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>         case VIDIOC_PREPARE_BUF32: cmd = VIDIOC_PREPARE_BUF; break;
>         case VIDIOC_SUBDEV_G_EDID32: cmd = VIDIOC_SUBDEV_G_EDID; break;
>         case VIDIOC_SUBDEV_S_EDID32: cmd = VIDIOC_SUBDEV_S_EDID; break;
> +       case VIDIOC_G_SELECTION32: cmd = VIDIOC_G_SELECTION; break;
> +       case VIDIOC_S_SELECTION32: cmd = VIDIOC_S_SELECTION; break;
>         }
>
>         switch (cmd) {
> @@ -922,6 +975,11 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>         case VIDIOC_DQEVENT:
>                 compatible_arg = 0;
>                 break;
> +       case VIDIOC_G_SELECTION:
> +       case VIDIOC_S_SELECTION:
> +               err = get_v4l2_selection32(&karg.v2sel, up);
> +               compatible_arg = 0;
> +               break;
>         }
>         if (err)
>                 return err;
> @@ -994,6 +1052,11 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>         case VIDIOC_ENUMINPUT:
>                 err = put_v4l2_input32(&karg.v2i, up);
>                 break;
> +
> +       case VIDIOC_G_SELECTION:
> +       case VIDIOC_S_SELECTION:
> +               err = put_v4l2_selection32(&karg.v2sel, up);
> +               break;
>         }
>         return err;
>  }
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 68e6b5e..aa1c2a4 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -572,11 +572,22 @@ static void v4l_print_crop(const void *arg, bool write_only)
>  static void v4l_print_selection(const void *arg, bool write_only)
>  {
>         const struct v4l2_selection *p = arg;
> +       int i;
>
> -       pr_cont("type=%s, target=%d, flags=0x%x, wxh=%dx%d, x,y=%d,%d\n",
> -               prt_names(p->type, v4l2_type_names),
> -               p->target, p->flags,
> -               p->r.width, p->r.height, p->r.left, p->r.top);
> +       if (p->rectangles == 0)
> +               pr_cont("type=%s, target=%d, flags=0x%x, wxh=%dx%d, x,y=%d,%d\n",
> +                       prt_names(p->type, v4l2_type_names),
> +                       p->target, p->flags,
> +                       p->r.width, p->r.height, p->r.left, p->r.top);
> +       else{
> +               pr_cont("type=%s, target=%d, flags=0x%x rectangles=%d\n",
> +                       prt_names(p->type, v4l2_type_names),
> +                       p->target, p->flags, p->rectangles);
> +               for (i = 0; i < p->rectangles; i++)
> +                       pr_cont("rectangle %d: wxh=%dx%d, x,y=%d,%d\n",
> +                               i, p->r.width, p->r.height,
> +                               p->r.left, p->r.top);
> +       }
>  }
>
>  static void v4l_print_jpegcompression(const void *arg, bool write_only)
> @@ -1692,7 +1703,9 @@ static int v4l_cropcap(const struct v4l2_ioctl_ops *ops,
>                                 struct file *file, void *fh, void *arg)
>  {
>         struct v4l2_cropcap *p = arg;
> -       struct v4l2_selection s = { .type = p->type };
> +       struct v4l2_selection s = {
> +               .type = p->type,
> +       };
>         int ret;
>
>         if (ops->vidioc_cropcap)
> @@ -2253,6 +2266,20 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
>                 }
>                 break;
>         }
> +
> +       case VIDIOC_G_SELECTION:
> +       case VIDIOC_S_SELECTION: {
> +               struct v4l2_selection *s = parg;
> +
> +               if (s->rectangles) {
> +                       *user_ptr = (void __user *)s->pr;
> +                       *kernel_ptr = (void *)&s->pr;
> +                       *array_size = sizeof(struct v4l2_ext_rect)
> +                               * s->rectangles;
> +                       ret = 1;
> +               }
> +               break;
> +       }
>         }
>
>         return ret;
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index 015ff82..417ab82 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -216,4 +216,10 @@ struct v4l2_fract v4l2_calc_aspect_ratio(u8 hor_landscape, u8 vert_portrait);
>
>  void v4l2_get_timestamp(struct timeval *tv);
>
> +int v4l2_selection_get_rect(const struct v4l2_selection *s,
> +                                       struct v4l2_ext_rect *r);
> +
> +void v4l2_selection_set_rect(struct v4l2_selection *s,
> +                                       const struct v4l2_ext_rect *r);
> +
>  #endif /* V4L2_COMMON_H_ */
> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> index a33c4da..c02a886 100644
> --- a/include/uapi/linux/v4l2-subdev.h
> +++ b/include/uapi/linux/v4l2-subdev.h
> @@ -133,6 +133,8 @@ struct v4l2_subdev_frame_interval_enum {
>   *         defined in v4l2-common.h; V4L2_SEL_TGT_* .
>   * @flags: constraint flags, defined in v4l2-common.h; V4L2_SEL_FLAG_*.
>   * @r: coordinates of the selection window
> + * @pr:                array of rectangles containing the selection windows
> + * @rectangles:        Number of rectangles in pr structure. If zero, r is used instead
>   * @reserved: for future use, set to zero for now
>   *
>   * Hardware may use multiple helper windows to process a video stream.
> @@ -144,8 +146,12 @@ struct v4l2_subdev_selection {
>         __u32 pad;
>         __u32 target;
>         __u32 flags;
> -       struct v4l2_rect r;
> -       __u32 reserved[8];
> +       union {
> +               struct v4l2_rect r;
> +               struct v4l2_ext_rect *pr;
> +       };
> +       __u32 rectangles;
> +       __u32 reserved[7];
>  };
>
>  struct v4l2_subdev_edid {
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 95ef455..a4a7902 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -211,6 +211,14 @@ struct v4l2_rect {
>         __s32   height;
>  };
>
> +struct v4l2_ext_rect {
> +       __s32   left;
> +       __s32   top;
> +       __u32   width;
> +       __u32   height;
> +       __u32   reserved[4];
> +};
> +
>  struct v4l2_fract {
>         __u32   numerator;
>         __u32   denominator;
> @@ -804,6 +812,8 @@ struct v4l2_crop {
>   *             defined in v4l2-common.h; V4L2_SEL_TGT_* .
>   * @flags:     constraints flags, defined in v4l2-common.h; V4L2_SEL_FLAG_*.
>   * @r:         coordinates of selection window
> + * @pr:                array of rectangles containing the selection windows
> + * @rectangles:        Number of rectangles in pr structure. If zero, r is used instead
>   * @reserved:  for future use, rounds structure size to 64 bytes, set to zero
>   *
>   * Hardware may use multiple helper windows to process a video stream.
> @@ -814,10 +824,13 @@ struct v4l2_selection {
>         __u32                   type;
>         __u32                   target;
>         __u32                   flags;
> -       struct v4l2_rect        r;
> -       __u32                   reserved[9];
> -};
> -
> +       union {
> +               struct v4l2_rect        r;
> +               struct v4l2_ext_rect    *pr;
> +       };
> +       __u32                   rectangles;
> +       __u32                   reserved[8];
> +} __packed;
>
>  /*
>   *      A N A L O G   V I D E O   S T A N D A R D
> --
> 1.8.4.rc3
>



-- 
Ricardo Ribalda

Re: [RFC v3] [RFC] v4l2: Support for multiple selections

[Tomasz Stanislawski]

Hi Ricardo,
I am the designer of selection API. I hope I can help you a little.
I think that there are two issues mixed in 'Mulitple selections' topic.

Firstly, you described that you program a piece of hardware that is
capable of selecting 8 areas for scanning. Now you
are looking for userspace API to support such a feature.
The feature of posting multiple rectangle was proposed in this RFC.

Secondly, You introduced struct v4l2_ext_rect which is a future-proof
version of v4l2_rect.


I think that both issues should be solved in two separate patchsets.

Ad 1.
The selection of multiple scanning areas is a very driver-specific
feature, isn't it? I think that you do not need to introduce any abstract
interface. What would be other applications of the proposed interface?
Do you know other drivers that may need it? Sakari mentioned introduction
of private targets for selections. I like this idea. Just define:

#define V4L2_SEL_TGT_PRIVATE     0x80000000

All targets that are >= V4L2_SEL_TGT_PRIVATE are driver-specific.
Generic applications must not use them. Non-generic application
must check out the driver of video node before using selections
from private set. If some target becomes more useful and accepted
by more then one driver then it can be moved to generic API.
The good thing about private target is that enums from different
drivers can collide so the target space is not going to be trashed.

But how to deal with multiple rectangles?
I have an auxiliary question. Do you have to set all rectangles
at once? can you set up them one by one?

Anyway, first try to define something like this:

#define V4L2_SEL_TGT_XXX_SCANOUT0  V4L2_SEL_TGT_PRIVATE
#define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT  (V4L2_SEL_TGT_XXX_SCANOUT0 + 1)
#define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS  (V4L2_SEL_TGT_XXX_SCANOUT0 + 2)

#define V4L2_SEL_TGT_XXX_SCANOUT0  (V4L2_SEL_TGT_PRIVATE + 16)
...

-- OR-- parametrized macros similar to one below:

#define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n))

The application could setup all scanout areas one-by-one.
By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array.
The height of all consecutive area would be 0. This means disabling
them effectively.

The change of V4L2_SEL_TGT_XXX_SCANOUT0 would influence all consequtive
rectangle by shifting them down or resetting them to height 0.
Notice that as long as targets are driver specific you are free do define
interaction between the targets.

I hope that proposed solution is satisfactory.

BTW. I think that the HW trick you described is not cropping.
By cropping you select which part of sensor area is going
to be processed into compose rectangle in a buffer.

So technicaly you still insert the whole sensor area into the buffer.
Only part of the buffer is actually updated. So there is no cropping
(cropping area is equal to the whole array).

Notice, that every 'cropping' area goes to different part of a buffer.
So you would need also muliple targets for composing (!!!) and very long
chapter in V4L2 doc describing interactions between muliple-rectangle
crops and compositions. Good luck !!! :).
Currently it is a hell to understand and define interaction between
single crop, and compose and unfamous VIDIOC_S_FMT and m2m madness.

I strongly recommend to use private selections.
It will be much simpler to define, implement, and modify if needed.

BTW2. I think that the mulitple scanout areas can be modelled using
media device API. Sakari may know how to do this.


Ad 2. Extended rectangles.
It is a good idea because v4l2_rect lacks any place for extensions.
But before adding it to V4L2 API, I would like to know the examples
of actual applications. Please, point drivers that actually need it.

Other thing worth mentioning is reservation of few bits from
v4l2_selection::flags to describe the type of data used for
rectangle, v4l2_rect or v4l2_ext_rect. This way one can avoid
introducting v4l2_selection::rectangles field.

I hope you find this comments useful.

Regards,
Tomasz Stanislawski

Re: [RFC v3] [RFC] v4l2: Support for multiple selections

[Ricardo Ribalda Delgado]

Hello Tomasz

Sorry for the late reply, but I have been offline the last week due to
the conference.


On Thu, Oct 24, 2013 at 12:31 PM, Tomasz Stanislawski
<t.stanislaws@samsung.com> wrote:
> Hi Ricardo,
> I am the designer of selection API. I hope I can help you a little.
> I think that there are two issues mixed in 'Mulitple selections' topic.
>
> Firstly, you described that you program a piece of hardware that is
> capable of selecting 8 areas for scanning. Now you
> are looking for userspace API to support such a feature.
> The feature of posting multiple rectangle was proposed in this RFC.
>
> Secondly, You introduced struct v4l2_ext_rect which is a future-proof
> version of v4l2_rect.
>
>
> I think that both issues should be solved in two separate patchsets.
>
> Ad 1.
> The selection of multiple scanning areas is a very driver-specific
> feature, isn't it? I think that you do not need to introduce any abstract
> interface. What would be other applications of the proposed interface?

It is not driver specific. There are many sensors out there that
supports multiple window of interest, but today we are ignoring them
just because we dont have an api.

The main application would be industrial imaging, where less data to
read means more fps and therefore the system can run faster.

>From my field I can tell you that it is a hard requirement for
computer vision. And it is a feature that we cannot model through v4l2
controls.


> Do you know other drivers that may need it? Sakari mentioned introduction
> of private targets for selections. I like this idea. Just define:
>
> #define V4L2_SEL_TGT_PRIVATE     0x80000000
>
> All targets that are >= V4L2_SEL_TGT_PRIVATE are driver-specific.
> Generic applications must not use them. Non-generic application
> must check out the driver of video node before using selections
> from private set. If some target becomes more useful and accepted
> by more then one driver then it can be moved to generic API.
> The good thing about private target is that enums from different
> drivers can collide so the target space is not going to be trashed.
>

If you read the previous RFCs you will see that the approach you are
mentioning has been rejected.

The main issue is that you cannot set atomically all the rectangles.
Lets say that the configuration formed by rectangle A, B and C is
legal, but the configuration A and B is not allowed by the sensor. How
could you set the rectangles one by one?

> I have an auxiliary question. Do you have to set all rectangles
> at once? can you set up them one by one?

Also if you tell the driver what exact configuration you will need, it
will provide you with the closest possible confuration, that cannot be
done if you provide rectangle by rectangle.

> But how to deal with multiple rectangles?

Multiple rectangles is a desired feature, please take a look to the
presentation on the workshop.

>
> Anyway, first try to define something like this:
>
> #define V4L2_SEL_TGT_XXX_SCANOUT0  V4L2_SEL_TGT_PRIVATE
> #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT  (V4L2_SEL_TGT_XXX_SCANOUT0 + 1)
> #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS  (V4L2_SEL_TGT_XXX_SCANOUT0 + 2)
>
> #define V4L2_SEL_TGT_XXX_SCANOUT0  (V4L2_SEL_TGT_PRIVATE + 16)
> ...
>
> -- OR-- parametrized macros similar to one below:
>
> #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n))
>
> The application could setup all scanout areas one-by-one.
> By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array.
> The height of all consecutive area would be 0. This means disabling
> them effectively.

Lets say rectangle A + B + C +D is legal, and A +B is also legal. You
are in ABCD and you want to go to AB. How can you do it?

If yo dissable C or D, the configuration is ilegal and therefor the
driver will return -EINVAL. So once you are in ABCD you cannot go
back...


>
> The change of V4L2_SEL_TGT_XXX_SCANOUT0 would influence all consequtive
> rectangle by shifting them down or resetting them to height 0.
> Notice that as long as targets are driver specific you are free do define
> interaction between the targets.
>
> I hope that proposed solution is satisfactory.

As stated before, please follow the previous comments on the rfc,
specially the ones from Hans.

>
> BTW. I think that the HW trick you described is not cropping.
> By cropping you select which part of sensor area is going
> to be processed into compose rectangle in a buffer.

You are selecting part of the sensor, therefore you are cropping the image.

>
> So technicaly you still insert the whole sensor area into the buffer.

Only the lines/columns are read into the buffer.

> Only part of the buffer is actually updated. So there is no cropping
> (cropping area is equal to the whole array).
>
> Notice, that every 'cropping' area goes to different part of a buffer.
> So you would need also muliple targets for composing (!!!) and very long
> chapter in V4L2 doc describing interactions between muliple-rectangle
> crops and compositions. Good luck !!! :).

It is not that difficult to describe.

You need the same ammount of rectangles in cropping and in compossing.
Rectangle X in cropping will be mapped to rectangle X in compose.


> Currently it is a hell to understand and define interaction between
> single crop, and compose and unfamous VIDIOC_S_FMT and m2m madness.

On m2m devices we are only lacking a s_fmt on the first buffer, as we
have discussed on the workshop. I think we only lack a good reference
model in vivi.


>
> I strongly recommend to use private selections.
> It will be much simpler to define, implement, and modify if needed.

I think the private selections will lead to specific applications for
specific drivers and we cannot support all the configurations with
them. Also there is no way for an app to enumerate that capability.

>
> BTW2. I think that the mulitple scanout areas can be modelled using
> media device API. Sakari may know how to do this.

The areas are not read from the sensor. Therefore they can only be
proccessed on the sensor subdevice.

>
>
> Ad 2. Extended rectangles.
> It is a good idea because v4l2_rect lacks any place for extensions.
> But before adding it to V4L2 API, I would like to know the examples
> of actual applications. Please, point drivers that actually need it.

I have no need for it, but now that we are extending the selection API
it would be the perfect moment to add them.

They could describe properties of the sensor, like tracking ids.

>
> Other thing worth mentioning is reservation of few bits from
> v4l2_selection::flags to describe the type of data used for
> rectangle, v4l2_rect or v4l2_ext_rect. This way one can avoid
> introducting v4l2_selection::rectangles field.
>
> I hope you find this comments useful.


Regards

>
> Regards,
> Tomasz Stanislawski
>
>



-- 
Ricardo Ribalda

Re: [RFC v3] [RFC] v4l2: Support for multiple selections

[Sakari Ailus]

Hi Tomasz,

(Please also see the notes from the Media summit Hans posted.)

Tomasz Stanislawski wrote:
> Hi Ricardo,
> I am the designer of selection API. I hope I can help you a little.
> I think that there are two issues mixed in 'Mulitple selections' topic.
> 
> Firstly, you described that you program a piece of hardware that is
> capable of selecting 8 areas for scanning. Now you
> are looking for userspace API to support such a feature.
> The feature of posting multiple rectangle was proposed in this RFC.
> 
> Secondly, You introduced struct v4l2_ext_rect which is a future-proof
> version of v4l2_rect.
> 
> 
> I think that both issues should be solved in two separate patchsets.
> 
> Ad 1.
> The selection of multiple scanning areas is a very driver-specific
> feature, isn't it? I think that you do not need to introduce any abstract
> interface. What would be other applications of the proposed interface?
> Do you know other drivers that may need it? Sakari mentioned introduction
> of private targets for selections. I like this idea. Just define:
> 
> #define V4L2_SEL_TGT_PRIVATE     0x80000000

I think a private target would definitely make sense if this was
functionality that was present on a single sensor or two --- that's what
I actually proposed for this originally. But as far as I understand, it
is more common but just not present on sensors used in mobile devices.
So standardising this makes sense.

> All targets that are >= V4L2_SEL_TGT_PRIVATE are driver-specific.
> Generic applications must not use them. Non-generic application
> must check out the driver of video node before using selections
> from private set. If some target becomes more useful and accepted
> by more then one driver then it can be moved to generic API.
> The good thing about private target is that enums from different
> drivers can collide so the target space is not going to be trashed.
> 
> But how to deal with multiple rectangles?
> I have an auxiliary question. Do you have to set all rectangles
> at once? can you set up them one by one?
> 
> Anyway, first try to define something like this:
> 
> #define V4L2_SEL_TGT_XXX_SCANOUT0  V4L2_SEL_TGT_PRIVATE
> #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT  (V4L2_SEL_TGT_XXX_SCANOUT0 + 1)
> #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS  (V4L2_SEL_TGT_XXX_SCANOUT0 + 2)
> 
> #define V4L2_SEL_TGT_XXX_SCANOUT0  (V4L2_SEL_TGT_PRIVATE + 16)
> ...
> 
> -- OR-- parametrized macros similar to one below:
> 
> #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n))
> 
> The application could setup all scanout areas one-by-one.
> By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array.
> The height of all consecutive area would be 0. This means disabling
> them effectively.
> 
> The change of V4L2_SEL_TGT_XXX_SCANOUT0 would influence all consequtive
> rectangle by shifting them down or resetting them to height 0.
> Notice that as long as targets are driver specific you are free do define
> interaction between the targets.
> 
> I hope that proposed solution is satisfactory.
> 
> BTW. I think that the HW trick you described is not cropping.
> By cropping you select which part of sensor area is going
> to be processed into compose rectangle in a buffer.
> 
> So technicaly you still insert the whole sensor area into the buffer.
> Only part of the buffer is actually updated. So there is no cropping
> (cropping area is equal to the whole array).
> 
> Notice, that every 'cropping' area goes to different part of a buffer.
> So you would need also muliple targets for composing (!!!) and very long
> chapter in V4L2 doc describing interactions between muliple-rectangle
> crops and compositions. Good luck !!! :).

Multiple crop rectangles shouldn't make that more difficult than it was
since for the next step the image size is still a rectangle (it is just
composed of several smaller ones). Drivers supporting this will suffer,
though, but others shouldn't need to care.

The documentation must thus also be changed to say that the crop
rectangles must together form a rectangle if multiple rectangle support
is added.

I reckon Ricardo is looking forward to using this on V4L2 interface, but
I think support should also be added to the V4L2 sub-device interface.

> Currently it is a hell to understand and define interaction between
> single crop, and compose and unfamous VIDIOC_S_FMT and m2m madness.
> 
> I strongly recommend to use private selections.
> It will be much simpler to define, implement, and modify if needed.
> 
> BTW2. I think that the mulitple scanout areas can be modelled using
> media device API. Sakari may know how to do this.

The media device plays no part in subsystem specific matters such as
formats. This is relevant in the scope of V4L2 only, IMHO.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@iki.fi

Re: [RFC v3] [RFC] v4l2: Support for multiple selections

[Tomasz Stanislawski]

Hi Ricardo,
Sorry for a late reply. I've been 'offline' for the last two weeks.
Please refer to the comments below.

On 10/28/2013 11:46 PM, Ricardo Ribalda Delgado wrote:
> Hello Tomasz
> 
> Sorry for the late reply, but I have been offline the last week due to
> the conference.
> 
> 
> On Thu, Oct 24, 2013 at 12:31 PM, Tomasz Stanislawski
> <t.stanislaws@samsung.com> wrote:
>> Hi Ricardo,
>> I am the designer of selection API. I hope I can help you a little.
>> I think that there are two issues mixed in 'Mulitple selections' topic.
>>
>> Firstly, you described that you program a piece of hardware that is
>> capable of selecting 8 areas for scanning. Now you
>> are looking for userspace API to support such a feature.
>> The feature of posting multiple rectangle was proposed in this RFC.
>>
>> Secondly, You introduced struct v4l2_ext_rect which is a future-proof
>> version of v4l2_rect.
>>
>>
>> I think that both issues should be solved in two separate patchsets.
>>
>> Ad 1.
>> The selection of multiple scanning areas is a very driver-specific
>> feature, isn't it? I think that you do not need to introduce any abstract
>> interface. What would be other applications of the proposed interface?
> 
> It is not driver specific. There are many sensors out there that
> supports multiple window of interest, but today we are ignoring them
> just because we dont have an api.
> 
> The main application would be industrial imaging, where less data to
> read means more fps and therefore the system can run faster.
> 
> From my field I can tell you that it is a hard requirement for
> computer vision. And it is a feature that we cannot model through v4l2
> controls.
> 
> 

OK. So there is no need to implement this feature as a driver-specific API.
It can go automatically to generic API.

>> Do you know other drivers that may need it? Sakari mentioned introduction
>> of private targets for selections. I like this idea. Just define:
>>
>> #define V4L2_SEL_TGT_PRIVATE     0x80000000
>>
>> All targets that are >= V4L2_SEL_TGT_PRIVATE are driver-specific.
>> Generic applications must not use them. Non-generic application
>> must check out the driver of video node before using selections
>> from private set. If some target becomes more useful and accepted
>> by more then one driver then it can be moved to generic API.
>> The good thing about private target is that enums from different
>> drivers can collide so the target space is not going to be trashed.
>>
> 
> If you read the previous RFCs you will see that the approach you are
> mentioning has been rejected.
> 
> The main issue is that you cannot set atomically all the rectangles.
> Lets say that the configuration formed by rectangle A, B and C is
> legal, but the configuration A and B is not allowed by the sensor. How
> could you set the rectangles one by one?
> 

As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on.
So activation of rectangle B (setting height to non-zero value) will enable
rectangle C with some default size. Moreover disabling rectangle B (setting height to 0)
may disable rectangle C automatically. I do not follow what is the problem here?

Hmm. I think that the real problem is much different.
Not how to set up rectangles atomically but rather
how do anything non-trivial atomically in V4L2.

It would be very nice to have crop/compose and format configured at the same time.
However, current version of V4L2 API does not support that.

Setting multiple crop rectangles may help a bit for only this little case.
But how would like to set multicrop rectangles and multicompose rectangle atomically.
How to define which crop rectangle refers to which to which compose rectangle.

What to do if one would like to change only 3rd crop rectangle?

Introduce rectangle id into v4l2_ext_rect?
Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION?
Is it really scalable?

Multirectangle targets may seam to be a solution but I think it is not.

I think that atomic transactions are what is really needed in V4L2.
Other ideas like try-context from subdev API may also help.

It will be nice to have something like

VIDIOC_BEGIN
  VIDIOC_S_SELECTION (target = CROP)
  VIDIOC_S_SELECTION (target = COMPOSE)
  VIDIOC_S_FMT
  VIDIOC_S_CTRL
VIDIOC_COMMIT

and call a bunch of VIDIOC_G_* to see what really was applied.

>> I have an auxiliary question. Do you have to set all rectangles
>> at once? can you set up them one by one?
> 
> Also if you tell the driver what exact configuration you will need, it
> will provide you with the closest possible confuration, that cannot be

s/cannot be done/may not be 'doable'

> done if you provide rectangle by rectangle.
> 
>> But how to deal with multiple rectangles?
> 
> Multiple rectangles is a desired feature, please take a look to the
> presentation on the workshop.
> 

I agree that it may be useful. I just think that multirectangle selections
are needed to add support for such a feature.

>>
>> Anyway, first try to define something like this:
>>
>> #define V4L2_SEL_TGT_XXX_SCANOUT0  V4L2_SEL_TGT_PRIVATE
>> #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT  (V4L2_SEL_TGT_XXX_SCANOUT0 + 1)
>> #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS  (V4L2_SEL_TGT_XXX_SCANOUT0 + 2)
>>
>> #define V4L2_SEL_TGT_XXX_SCANOUT0  (V4L2_SEL_TGT_PRIVATE + 16)
>> ...
>>
>> -- OR-- parametrized macros similar to one below:
>>
>> #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n))
>>
>> The application could setup all scanout areas one-by-one.
>> By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array.
>> The height of all consecutive area would be 0. This means disabling
>> them effectively.
> 
> Lets say rectangle A + B + C +D is legal, and A +B is also legal. You
> are in ABCD and you want to go to AB. How can you do it?

Just set C to have height 0. It will disable D automatically.

BTW. How are you going to emulate S_CROP by selection API
if you must use at least two rectangles (A + B) ?

I suggest to use SCANOUT target in your first implementation.
Notice that as long you use something different from CROP and COMPOSE,
you may define interactions and dependencies any way you want, like
- if change of scanout area can affect composing area
- interaction with format and frame size
- check if scanout area can overlap, multi crops should be allowed to overlap

See if it will be sufficient.
See what kind of problems you encouter.

> 
> If yo dissable C or D, the configuration is ilegal and therefor the
> driver will return -EINVAL. So once you are in ABCD you cannot go
> back...
> 

The driver must not return EINVAL as long as it is possible to
adjust values passed by user to some valid configuration.
If configuration is fixed then VIDIOC_S_SELECTION works
effectively as VIDIOC_G_SELECTION.


> 
>>
>> The change of V4L2_SEL_TGT_XXX_SCANOUT0 would influence all consequtive
>> rectangle by shifting them down or resetting them to height 0.
>> Notice that as long as targets are driver specific you are free do define
>> interaction between the targets.
>>
>> I hope that proposed solution is satisfactory.
> 
> As stated before, please follow the previous comments on the rfc,
> specially the ones from Hans.
> 
>>
>> BTW. I think that the HW trick you described is not cropping.
>> By cropping you select which part of sensor area is going
>> to be processed into compose rectangle in a buffer.
> 
> You are selecting part of the sensor, therefore you are cropping the image.
> 

Yes, it is the same as long as you use only one rectangle.

If using more then 1 the situation may become more complex.
Currently a pair of composing and crop rectangles are used to setup scaling.

You have to introduce a new definition of scaling factor if multirect crops are introduced.

Moreover what happens if flipping or rotation is applied?
Currently only the content of the rectangle is rotated/flipped.
If you use multi rectangle then content of each rectangle must be
rotated/flipped separatelly. Are you sure that your hardware can do this?

What about layout of composing rectangle inside output buffer?
Should it reflect the layout of crop rectangle or all of them should be
independent.

Of course you can always say that everything is driver dependent :).

Anyway, adding multirectangle selections for crop/compose is openning Pandora's Box.

>>
>> So technicaly you still insert the whole sensor area into the buffer.
> 
> Only the lines/columns are read into the buffer.
> 

Yes, but where into a buffer?
Does you HW support setting destination for every crop rectangle?

>> Only part of the buffer is actually updated. So there is no cropping
>> (cropping area is equal to the whole array).
>>
>> Notice, that every 'cropping' area goes to different part of a buffer.
>> So you would need also muliple targets for composing (!!!) and very long
>> chapter in V4L2 doc describing interactions between muliple-rectangle
>> crops and compositions. Good luck !!! :).
> 
> It is not that difficult to describe.
> 
> You need the same ammount of rectangles in cropping and in compossing.
> Rectangle X in cropping will be mapped to rectangle X in compose.
> 
> 
>> Currently it is a hell to understand and define interaction between
>> single crop, and compose and unfamous VIDIOC_S_FMT and m2m madness.
> 
> On m2m devices we are only lacking a s_fmt on the first buffer, as we
> have discussed on the workshop. I think we only lack a good reference
> model in vivi.
> 
> 

I was not present on the workshop. Could you provide more details?

>>
>> I strongly recommend to use private selections.
>> It will be much simpler to define, implement, and modify if needed.
> 
> I think the private selections will lead to specific applications for
> specific drivers and we cannot support all the configurations with
> them. Also there is no way for an app to enumerate that capability.

call VIDIOC_G_SELECTION to check if a given SCANOUT is supported?
EINVAL means that it is not supported.

> 
>>
>> BTW2. I think that the mulitple scanout areas can be modelled using
>> media device API. Sakari may know how to do this.
> 
> The areas are not read from the sensor. Therefore they can only be
> proccessed on the sensor subdevice.
> 
>>
>>
>> Ad 2. Extended rectangles.
>> It is a good idea because v4l2_rect lacks any place for extensions.
>> But before adding it to V4L2 API, I would like to know the examples
>> of actual applications. Please, point drivers that actually need it.
> 
> I have no need for it, but now that we are extending the selection API
> it would be the perfect moment to add them.

The perfect moment for adding something is when it is needed.

The bad idea is preventing something from being added too early.

Regards,
Tomasz Stanislawski

> 
> They could describe properties of the sensor, like tracking ids.
> 
>>
>> Other thing worth mentioning is reservation of few bits from
>> v4l2_selection::flags to describe the type of data used for
>> rectangle, v4l2_rect or v4l2_ext_rect. This way one can avoid
>> introducting v4l2_selection::rectangles field.
>>
>> I hope you find this comments useful.
> 
> 
> Regards
> 
>>
>> Regards,
>> Tomasz Stanislawski
>>
>>
> 
> 
> 

Re: [RFC v3] [RFC] v4l2: Support for multiple selections

[Hans Verkuil]

Hi Tomasz,

On 11/12/13 15:54, Tomasz Stanislawski wrote:
> Hi Ricardo,
> Sorry for a late reply. I've been 'offline' for the last two weeks.
> Please refer to the comments below.
> 
> On 10/28/2013 11:46 PM, Ricardo Ribalda Delgado wrote:
>> Hello Tomasz
>>
>> Sorry for the late reply, but I have been offline the last week due to
>> the conference.
>>
>>
>> On Thu, Oct 24, 2013 at 12:31 PM, Tomasz Stanislawski
>> <t.stanislaws@samsung.com> wrote:
>>> Hi Ricardo,
>>> I am the designer of selection API. I hope I can help you a little.
>>> I think that there are two issues mixed in 'Mulitple selections' topic.
>>>
>>> Firstly, you described that you program a piece of hardware that is
>>> capable of selecting 8 areas for scanning. Now you
>>> are looking for userspace API to support such a feature.
>>> The feature of posting multiple rectangle was proposed in this RFC.
>>>
>>> Secondly, You introduced struct v4l2_ext_rect which is a future-proof
>>> version of v4l2_rect.
>>>
>>>
>>> I think that both issues should be solved in two separate patchsets.
>>>
>>> Ad 1.
>>> The selection of multiple scanning areas is a very driver-specific
>>> feature, isn't it? I think that you do not need to introduce any abstract
>>> interface. What would be other applications of the proposed interface?
>>
>> It is not driver specific. There are many sensors out there that
>> supports multiple window of interest, but today we are ignoring them
>> just because we dont have an api.
>>
>> The main application would be industrial imaging, where less data to
>> read means more fps and therefore the system can run faster.
>>
>> From my field I can tell you that it is a hard requirement for
>> computer vision. And it is a feature that we cannot model through v4l2
>> controls.
>>
>>
> 
> OK. So there is no need to implement this feature as a driver-specific API.
> It can go automatically to generic API.
> 
>>> Do you know other drivers that may need it? Sakari mentioned introduction
>>> of private targets for selections. I like this idea. Just define:
>>>
>>> #define V4L2_SEL_TGT_PRIVATE     0x80000000
>>>
>>> All targets that are >= V4L2_SEL_TGT_PRIVATE are driver-specific.
>>> Generic applications must not use them. Non-generic application
>>> must check out the driver of video node before using selections
>>> from private set. If some target becomes more useful and accepted
>>> by more then one driver then it can be moved to generic API.
>>> The good thing about private target is that enums from different
>>> drivers can collide so the target space is not going to be trashed.
>>>
>>
>> If you read the previous RFCs you will see that the approach you are
>> mentioning has been rejected.
>>
>> The main issue is that you cannot set atomically all the rectangles.
>> Lets say that the configuration formed by rectangle A, B and C is
>> legal, but the configuration A and B is not allowed by the sensor. How
>> could you set the rectangles one by one?
>>
> 
> As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on.
> So activation of rectangle B (setting height to non-zero value) will enable
> rectangle C with some default size. Moreover disabling rectangle B (setting height to 0)
> may disable rectangle C automatically. I do not follow what is the problem here?

The problem would be in a situation like this:

......
.AA.B.
......   -->   AAB
.C.DD.         CDD
......

A-D are the rectangles you want to select. They are cropped as shown on the
left and composed as shown on the right.

>From what Ricardo told me the resulting composed image typically must be
a proper rectangle without padding anywhere.

Trying to add rectangles one at a time breaks down when adding C because
the composition result is no longer a 'proper' rectangle. I don't see how
you can set something like that up one rectangle at a time.

It makes much more sense to set everything up at once.

BTW, what probably wasn't clear from Ricardo's explanation is that for every
crop rectangle you must have a corresponding compose rectangle so that you
know where to DMA it to.

Your next question will be that it is a real problem that you can't set
crop and compose simultaneously, and you are right about that. Read on for
that... :-)

> Hmm. I think that the real problem is much different.
> Not how to set up rectangles atomically but rather
> how do anything non-trivial atomically in V4L2.
> 
> It would be very nice to have crop/compose and format configured at the same time.
> However, current version of V4L2 API does not support that.
> 
> Setting multiple crop rectangles may help a bit for only this little case.
> But how would like to set multicrop rectangles and multicompose rectangle atomically.

Why can't we extend the selection API a bit? How about this:

#define V4L2_SEL_TGT_CROP_COMPOSE    0x0200

struct v4l2_selection {
        __u32                   type;
        __u32                   target;
        __u32                   flags;
	union {
	        struct v4l2_rect        r;
		struct v4l2_ext_rect    *pr;
	};
        __u32                   flags2;
	union {
	        struct v4l2_rect        r2;
		struct v4l2_ext_rect    *pr2;
	};
	__u32			rectangles;
        __u32                   reserved[3];
};

If the target is CROP_COMPOSE then flags & r define the crop rectangle, and
flags2 & r2 define the compose rectangle. That way you can set them both
atomically.

I would propose that the interaction with S_FMT is as follows: if S_FMT
defines a rectangle < then the current compose rectangle, then the
composition (and optionally crop) rectangle is reset. If it is >=, then
all is fine.

If a new compose rectangle doesn't fit in the S_FMT rectangle then it should
adapt the S_FMT rectangle to make it fit (as long as HW constraints are
obeyed of course).

This sequence will obey the rules of V4L2 as well: the last operation takes
precedence over older operations. So setting S_FMT allow the driver to
change cropping/composing to get as close to the desired format as possible.

> How to define which crop rectangle refers to which to which compose rectangle.

By setting the crop and compose selections at the same time and of the same
size you can map each crop selection to a compose selection. It's all atomic
as well.

> 
> What to do if one would like to change only 3rd crop rectangle?
> 
> Introduce rectangle id into v4l2_ext_rect?
> Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION?
> Is it really scalable?

Why not?

> 
> Multirectangle targets may seam to be a solution but I think it is not.
> 
> I think that atomic transactions are what is really needed in V4L2.
> Other ideas like try-context from subdev API may also help.
> 
> It will be nice to have something like
> 
> VIDIOC_BEGIN
>   VIDIOC_S_SELECTION (target = CROP)
>   VIDIOC_S_SELECTION (target = COMPOSE)
>   VIDIOC_S_FMT
>   VIDIOC_S_CTRL
> VIDIOC_COMMIT

I don't think S_FMT is needed here: it's something you set up at the
beginning and don't touch afterwards.

Wouldn't VIDIOC_S_SELECTION (target = CROP_COMPOSE) go a long way to
solving these atomicity problems?

Another nice feature that can be added to the selection API is to
add a field to refer to a frame sequence number or a v4l2_buffer
index (the latter is probably better): this would make it easy to
apply an atomic crop/compose change to easily implement things like
digital zoom or moving windows around. We could do something similar
for controls.

This would also solve the problem of assigning a per-buffer (or per-frame)
configuration, something that libcamera2/3 needs.

> 
> and call a bunch of VIDIOC_G_* to see what really was applied.
> 
>>> I have an auxiliary question. Do you have to set all rectangles
>>> at once? can you set up them one by one?
>>
>> Also if you tell the driver what exact configuration you will need, it
>> will provide you with the closest possible confuration, that cannot be
> 
> s/cannot be done/may not be 'doable'
> 
>> done if you provide rectangle by rectangle.
>>
>>> But how to deal with multiple rectangles?
>>
>> Multiple rectangles is a desired feature, please take a look to the
>> presentation on the workshop.
>>
> 
> I agree that it may be useful. I just think that multirectangle selections
> are needed to add support for such a feature.

I don't follow, isn't that what this proposal adds?

> 
>>>
>>> Anyway, first try to define something like this:
>>>
>>> #define V4L2_SEL_TGT_XXX_SCANOUT0  V4L2_SEL_TGT_PRIVATE
>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT  (V4L2_SEL_TGT_XXX_SCANOUT0 + 1)
>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS  (V4L2_SEL_TGT_XXX_SCANOUT0 + 2)
>>>
>>> #define V4L2_SEL_TGT_XXX_SCANOUT0  (V4L2_SEL_TGT_PRIVATE + 16)
>>> ...
>>>
>>> -- OR-- parametrized macros similar to one below:
>>>
>>> #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n))
>>>
>>> The application could setup all scanout areas one-by-one.
>>> By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array.
>>> The height of all consecutive area would be 0. This means disabling
>>> them effectively.
>>
>> Lets say rectangle A + B + C +D is legal, and A +B is also legal. You
>> are in ABCD and you want to go to AB. How can you do it?
> 
> Just set C to have height 0. It will disable D automatically.

It's the other direction that's the problem: how to go from A + B to
A + B + C + D if ABC is illegal.

> BTW. How are you going to emulate S_CROP by selection API
> if you must use at least two rectangles (A + B) ?

S_CROP will always just set one rectangle A. So if you had multiple
rectangles in your selection then after S_CROP you'll have only one.

I'm assuming that the hw will always support a single crop rectangle.
If not (extraordinarily unlikely), then S_CROP should just return an
error.

> I suggest to use SCANOUT target in your first implementation.
> Notice that as long you use something different from CROP and COMPOSE,
> you may define interactions and dependencies any way you want, like
> - if change of scanout area can affect composing area
> - interaction with format and frame size
> - check if scanout area can overlap, multi crops should be allowed to overlap

I see no reason for this. A CROP_COMPOSE target does all you need.

Note that to set up N rectangles where N differs from the current
rectangle count you do need to use CROP_COMPOSE. You can't setup CROP
and COMPOSE rectangles independently since having N crop rectangles
and M compose rectangles makes no sense. If you don't change the
number of rectangles, then you can still use CROP and COMPOSE as is.

> 
> See if it will be sufficient.
> See what kind of problems you encouter.
> 
>>
>> If yo dissable C or D, the configuration is ilegal and therefor the
>> driver will return -EINVAL. So once you are in ABCD you cannot go
>> back...
>>
> 
> The driver must not return EINVAL as long as it is possible to
> adjust values passed by user to some valid configuration.
> If configuration is fixed then VIDIOC_S_SELECTION works
> effectively as VIDIOC_G_SELECTION.
> 
> 
>>
>>>
>>> The change of V4L2_SEL_TGT_XXX_SCANOUT0 would influence all consequtive
>>> rectangle by shifting them down or resetting them to height 0.
>>> Notice that as long as targets are driver specific you are free do define
>>> interaction between the targets.
>>>
>>> I hope that proposed solution is satisfactory.
>>
>> As stated before, please follow the previous comments on the rfc,
>> specially the ones from Hans.
>>
>>>
>>> BTW. I think that the HW trick you described is not cropping.
>>> By cropping you select which part of sensor area is going
>>> to be processed into compose rectangle in a buffer.
>>
>> You are selecting part of the sensor, therefore you are cropping the image.
>>
> 
> Yes, it is the same as long as you use only one rectangle.
> 
> If using more then 1 the situation may become more complex.
> Currently a pair of composing and crop rectangles are used to setup scaling.
> 
> You have to introduce a new definition of scaling factor if multirect crops are introduced.
> 
> Moreover what happens if flipping or rotation is applied?
> Currently only the content of the rectangle is rotated/flipped.
> If you use multi rectangle then content of each rectangle must be
> rotated/flipped separatelly. Are you sure that your hardware can do this?
> 
> What about layout of composing rectangle inside output buffer?
> Should it reflect the layout of crop rectangle or all of them should be
> independent.
> 
> Of course you can always say that everything is driver dependent :).
> 
> Anyway, adding multirectangle selections for crop/compose is openning Pandora's Box.

I disagree. But it requires (and I hadn't realized that before) that you pass
all the crop and compose rectangles atomically.

> 
>>>
>>> So technicaly you still insert the whole sensor area into the buffer.
>>
>> Only the lines/columns are read into the buffer.
>>
> 
> Yes, but where into a buffer?
> Does you HW support setting destination for every crop rectangle?

>From what Ricardo told us during the meeting I got the impression that
it is basically just skipping the lines and columns that are not wanted.

So ...AAA..BB..C... just ends up as AAABBC
   .D....EEEEE.....                 DEEEEE

> 
>>> Only part of the buffer is actually updated. So there is no cropping
>>> (cropping area is equal to the whole array).
>>>
>>> Notice, that every 'cropping' area goes to different part of a buffer.
>>> So you would need also muliple targets for composing (!!!) and very long
>>> chapter in V4L2 doc describing interactions between muliple-rectangle
>>> crops and compositions. Good luck !!! :).
>>
>> It is not that difficult to describe.
>>
>> You need the same ammount of rectangles in cropping and in compossing.
>> Rectangle X in cropping will be mapped to rectangle X in compose.
>>
>>
>>> Currently it is a hell to understand and define interaction between
>>> single crop, and compose and unfamous VIDIOC_S_FMT and m2m madness.
>>
>> On m2m devices we are only lacking a s_fmt on the first buffer, as we
>> have discussed on the workshop. I think we only lack a good reference
>> model in vivi.
>>
>>
> 
> I was not present on the workshop. Could you provide more details?
> 
>>>
>>> I strongly recommend to use private selections.
>>> It will be much simpler to define, implement, and modify if needed.
>>
>> I think the private selections will lead to specific applications for
>> specific drivers and we cannot support all the configurations with
>> them. Also there is no way for an app to enumerate that capability.
> 
> call VIDIOC_G_SELECTION to check if a given SCANOUT is supported?
> EINVAL means that it is not supported.
> 
>>
>>>
>>> BTW2. I think that the mulitple scanout areas can be modelled using
>>> media device API. Sakari may know how to do this.
>>
>> The areas are not read from the sensor. Therefore they can only be
>> proccessed on the sensor subdevice.
>>
>>>
>>>
>>> Ad 2. Extended rectangles.
>>> It is a good idea because v4l2_rect lacks any place for extensions.
>>> But before adding it to V4L2 API, I would like to know the examples
>>> of actual applications. Please, point drivers that actually need it.
>>
>> I have no need for it, but now that we are extending the selection API
>> it would be the perfect moment to add them.
> 
> The perfect moment for adding something is when it is needed.

We need to add support for multiple rectangles, so we added a pointer.

> The bad idea is preventing something from being added too early.

But if we make it a pointer to v4l2_rect, then we can *never* add
additional information to each rectangle. Whenever we add new APIs
or features we try to allow for future extensions (with mixed
success...), so now is the time to do so for v4l2_rect.

Something like this:

struct v4l2_sel_rect {
	struct v4l2_rect r;
	__u32 reserved[4];
};

would be fine by me as well. It's an additional indirection, but it
also makes it easier to go from a v4l2_rect to a v4l2_sel_rect.

Regards,

	Hans

> 
> Regards,
> Tomasz Stanislawski
> 
>>
>> They could describe properties of the sensor, like tracking ids.
>>
>>>
>>> Other thing worth mentioning is reservation of few bits from
>>> v4l2_selection::flags to describe the type of data used for
>>> rectangle, v4l2_rect or v4l2_ext_rect. This way one can avoid
>>> introducting v4l2_selection::rectangles field.
>>>
>>> I hope you find this comments useful.
>>
>>
>> Regards
>>
>>>
>>> Regards,
>>> Tomasz Stanislawski
>>>
>>>
>>
>>
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC v3] [RFC] v4l2: Support for multiple selections

[Tomasz Stanislawski]

Hi Hans,

On 11/14/2013 11:18 AM, Hans Verkuil wrote:
> Hi Tomasz,
> 
> On 11/12/13 15:54, Tomasz Stanislawski wrote:
>> Hi Ricardo,
>> Sorry for a late reply. I've been 'offline' for the last two weeks.
>> Please refer to the comments below.
>>

[snip]

>>
>> As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on.
>> So activation of rectangle B (setting height to non-zero value) will enable
>> rectangle C with some default size. Moreover disabling rectangle B (setting height to 0)
>> may disable rectangle C automatically. I do not follow what is the problem here?
> 
> The problem would be in a situation like this:
> 
> ......
> .AA.B.
> ......   -->   AAB
> .C.DD.         CDD
> ......
> 
> A-D are the rectangles you want to select. They are cropped as shown on the
> left and composed as shown on the right.
> 
>>From what Ricardo told me the resulting composed image typically must be
> a proper rectangle without padding anywhere.
> 
> Trying to add rectangles one at a time breaks down when adding C because
> the composition result is no longer a 'proper' rectangle. I don't see how
> you can set something like that up one rectangle at a time.

I see the issue but I think that it is not a big problem.
Activating C forms a non-proper rectangle with A and B.
Therefore, driver must force enabling D to form a proper rectangle again.

I mean that instead of enlarging C to sum of width of A and B,
the driver can implicitly activate D to ensure that A,B,C,D form a proper rectangle.

The special target called V4L2_SEL_TGT_COMPOSE_PADDED was introduced
to inform application which part of a buffers if going to be modified
with some undefined value.

I see nothing against setting a padded rectangle for C to a rectangle that
covers C and D or even the whole ABCD rectangle.
I think it could be a great application for PADDED target.
The application could easily detect which part of a buffer are affected.

Even applications prepared to work with single crop devices
would still work after enabling multi crop mode.

The setup of rectangles my look like this.

************************************************

S_SELECTION(CROP0 = A)

crop           compose
----------------------
......
.AA...
......   -->   AA..
......         ....
......         ....

G_SELECTION(COMPOSE0)
  AA..
  ....
  ....


G_SELECTION(COMPOSE0_PADDED)
  AA..
  ....
  ....

************************************************

S_SELECTION(CROP1 = B)
......
.AA.B.
......   -->   AAB.
......         ....
......         ....

G_SELECTION(COMPOSE0)
  AA..
  ....
  ....

G_SELECTION(COMPOSE0_PADDED)
  AAA.
  ....
  ....

G_SELECTION(COMPOSE1)
  ..B.
  ....
  ....

G_SELECTION(COMPOSE1_PADDED)
  BBB.
  ....
  ....


************************************************

S_SELECTION(CROP2 = C) - D is activated implicitly
......
.AA.B.
.C.DD.   -->   AAB.
......         CDD.
......         ....

G_SELECTION(COMPOSE0_PADDED)

  AAA.
  AAA.
  ....

G_SELECTION(COMPOSE2)
  ....
  C...
  ....


G_SELECTION(COMPOSE2_PADDED)
  CCC.
  CCC.
  ....

G_SELECTION(COMPOSE3)
  ....
  .DD.
  ....


G_SELECTION(COMPOSE3_PADDED)
  DDD.
  DDD.
  ....

One may argue that all this logic is unnecessary after adding support
for multirect selections.
So, I kindly ask what should happen if someone call S_SELECTION
(in multirect mode) passing THREE rectangles A, B, and C (not D) ?

The driver must adjust rectangles to some valid value. So it can
increase width of C or implicitly activate D or disable C.
I think that the best solution is activating D because
it allows to set size of C to the value closest to requested one.
Therefore logic for implicit activation of D should be implemented anyway.

> 
> It makes much more sense to set everything up at once.

I agree that it is better to set everything at once.
But I strongly believe that transactions are the proper way to achieve that.

Not multirectangle selections.

It obfuscates API. It only pretends to fix a problem with applying
a part of configuration atomically.

> 
> BTW, what probably wasn't clear from Ricardo's explanation is that for every
> crop rectangle you must have a corresponding compose rectangle so that you
> know where to DMA it to.
> 
> Your next question will be that it is a real problem that you can't set
> crop and compose simultaneously, and you are right about that. Read on for
> that... :-)
> 
>> Hmm. I think that the real problem is much different.
>> Not how to set up rectangles atomically but rather
>> how do anything non-trivial atomically in V4L2.
>>
>> It would be very nice to have crop/compose and format configured at the same time.
>> However, current version of V4L2 API does not support that.
>>
>> Setting multiple crop rectangles may help a bit for only this little case.
>> But how would like to set multicrop rectangles and multicompose rectangle atomically.
> 
> Why can't we extend the selection API a bit? How about this:
> 
> #define V4L2_SEL_TGT_CROP_COMPOSE    0x0200
> 
> struct v4l2_selection {
>         __u32                   type;
>         __u32                   target;
>         __u32                   flags;
> 	union {
> 	        struct v4l2_rect        r;
> 		struct v4l2_ext_rect    *pr;
> 	};
>         __u32                   flags2;
> 	union {
> 	        struct v4l2_rect        r2;
> 		struct v4l2_ext_rect    *pr2;
> 	};
> 	__u32			rectangles;
>         __u32                   reserved[3];
> };
> 
> If the target is CROP_COMPOSE then flags & r define the crop rectangle, and
> flags2 & r2 define the compose rectangle. That way you can set them both
> atomically.

I do not like this idea because:
- mix crop and composing generating semi-related cropcompose
- obfuscates the API even more
- wastes a lot of reserved fields
- what if someone would like to use separate 'flags' for each rectangle
  ... this could be a nice feature anyway :)

This remains me the proposition from early days of selection API.

http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/28945

Refer to point 4 where v4l2_crop2 was mentioned.

> 
> I would propose that the interaction with S_FMT is as follows: if S_FMT
> defines a rectangle < then the current compose rectangle, then the
> composition (and optionally crop) rectangle is reset. If it is >=, then
> all is fine.

There is a issue here. Long time ago it was stated that S_FMT
should result in configuration where data are processed (scaled) into
the 'whole picture'.

It means that a COMPOSE rectangle should be reset after very S_FMT.

> 
> If a new compose rectangle doesn't fit in the S_FMT rectangle then it should
> adapt the S_FMT rectangle to make it fit (as long as HW constraints are
> obeyed of course).

What happens if REQBUFS was called between S_FMT and S_SELECTION?

> 
> This sequence will obey the rules of V4L2 as well: the last operation takes
> precedence over older operations. So setting S_FMT allow the driver to
> change cropping/composing to get as close to the desired format as possible.
> 
>> How to define which crop rectangle refers to which to which compose rectangle.
> 
> By setting the crop and compose selections at the same time and of the same
> size you can map each crop selection to a compose selection. It's all atomic
> as well.
> 

I think that adding an id field to v4l2_selection might be a good alternative
to introduction of numbered targets like V4L2_SEL_TGT_COMPOSE0, 1, ...

It will add support for all multirectangle features as cost of
sacrifice of single reserved field. Moreover id field might be
very useful for other selection targets like FOCUS or FACE.

Other idea might to using some bits of 'flags' field to carry rectangle id.

>>
>> What to do if one would like to change only 3rd crop rectangle?
>>
>> Introduce rectangle id into v4l2_ext_rect?
>> Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION?
>> Is it really scalable?
> 
> Why not?
> 

Because it is not scalable.
An application has to use 2 syscall instead of 1.
Unnecessary copying has to be performed.

>>
>> Multirectangle targets may seam to be a solution but I think it is not.
>>
>> I think that atomic transactions are what is really needed in V4L2.
>> Other ideas like try-context from subdev API may also help.
>>
>> It will be nice to have something like
>>
>> VIDIOC_BEGIN
>>   VIDIOC_S_SELECTION (target = CROP)
>>   VIDIOC_S_SELECTION (target = COMPOSE)
>>   VIDIOC_S_FMT
>>   VIDIOC_S_CTRL
>> VIDIOC_COMMIT
> 
> I don't think S_FMT is needed here: it's something you set up at the
> beginning and don't touch afterwards.

One could say the same about compose, crop, ctrls.
The problem are the interactions between those objects.
What is dependent on what. What can change what.
How to reach a valid state in all cases not preventing
some valid configurations from being unreachable.

> 
> Wouldn't VIDIOC_S_SELECTION (target = CROP_COMPOSE) go a long way to
> solving these atomicity problems?
> 
> Another nice feature that can be added to the selection API is to
> add a field to refer to a frame sequence number or a v4l2_buffer
> index (the latter is probably better): this would make it easy to
> apply an atomic crop/compose change to easily implement things like
> digital zoom or moving windows around. We could do something similar
> for controls.
> 
> This would also solve the problem of assigning a per-buffer (or per-frame)
> configuration, something that libcamera2/3 needs.
> 

Introduce transactions with frame sequence numbers.
I proposed this in Cambridge in 2012.

>>
>> and call a bunch of VIDIOC_G_* to see what really was applied.
>>
>>>> I have an auxiliary question. Do you have to set all rectangles
>>>> at once? can you set up them one by one?
>>>
>>> Also if you tell the driver what exact configuration you will need, it
>>> will provide you with the closest possible confuration, that cannot be
>>
>> s/cannot be done/may not be 'doable'
>>
>>> done if you provide rectangle by rectangle.
>>>
>>>> But how to deal with multiple rectangles?
>>>
>>> Multiple rectangles is a desired feature, please take a look to the
>>> presentation on the workshop.
>>>
>>
>> I agree that it may be useful. I just think that multirectangle selections
>> are needed to add support for such a feature.
> 
> I don't follow, isn't that what this proposal adds?
> 

s/are needed/are not needed/

Sorry for confusion.

>>
>>>>
>>>> Anyway, first try to define something like this:
>>>>
>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0  V4L2_SEL_TGT_PRIVATE
>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT  (V4L2_SEL_TGT_XXX_SCANOUT0 + 1)
>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS  (V4L2_SEL_TGT_XXX_SCANOUT0 + 2)
>>>>
>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0  (V4L2_SEL_TGT_PRIVATE + 16)
>>>> ...
>>>>
>>>> -- OR-- parametrized macros similar to one below:
>>>>
>>>> #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n))
>>>>
>>>> The application could setup all scanout areas one-by-one.
>>>> By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array.
>>>> The height of all consecutive area would be 0. This means disabling
>>>> them effectively.
>>>
>>> Lets say rectangle A + B + C +D is legal, and A +B is also legal. You
>>> are in ABCD and you want to go to AB. How can you do it?
>>
>> Just set C to have height 0. It will disable D automatically.
> 
> It's the other direction that's the problem: how to go from A + B to
> A + B + C + D if ABC is illegal.
> 
>> BTW. How are you going to emulate S_CROP by selection API
>> if you must use at least two rectangles (A + B) ?
> 
> S_CROP will always just set one rectangle A. So if you had multiple
> rectangles in your selection then after S_CROP you'll have only one.

So S_CROP should change only A.
What happens if someone already set device to work with 4 rectangles.
Then calls S_CROP that modifies A in such a way that ABCD no longer forms
a proper rectangle.

What should happen?
- ignore S_CROP, return previous settings, or
- modify B,C,D to form a proper rectangle, or
- disable B,C,D, or ?

> 
> I'm assuming that the hw will always support a single crop rectangle.
> If not (extraordinarily unlikely), then S_CROP should just return an
> error.

I agree that it is highly unlikely. I am just trying to figure out
what should happen when calling S_CROP or single-rectangle S_SELECTION
after calling multirect S_SELECTION.

> 
>> I suggest to use SCANOUT target in your first implementation.
>> Notice that as long you use something different from CROP and COMPOSE,
>> you may define interactions and dependencies any way you want, like
>> - if change of scanout area can affect composing area
>> - interaction with format and frame size
>> - check if scanout area can overlap, multi crops should be allowed to overlap
> 
> I see no reason for this. A CROP_COMPOSE target does all you need.

I think that CROP and COMPOSE is all you need.

> 
> Note that to set up N rectangles where N differs from the current
> rectangle count you do need to use CROP_COMPOSE. You can't setup CROP
> and COMPOSE rectangles independently since having N crop rectangles
> and M compose rectangles makes no sense. If you don't change the
> number of rectangles, then you can still use CROP and COMPOSE as is.
> 

I am afraid that you are trying to kill too many birds with one stone.
I think it is a very good moment to use bazooka.

Transactions enhanced with frame numbers will handle a lot of issues with V4L2.

Moreover it will allow to support some nice effects like:
- multiple exposures during HDR photography
- continuous online changes preview windows
- flash at a specific frame
- change format during video streaming to grab a high-resolution picture
- support multi rectangle cropping and composing :)

>>

[snip]


>>> I have no need for it, but now that we are extending the selection API
>>> it would be the perfect moment to add them.
>>
>> The perfect moment for adding something is when it is needed.
> 
> We need to add support for multiple rectangles, so we added a pointer.
> 
>> The bad idea is preventing something from being added too early.
> 
> But if we make it a pointer to v4l2_rect, then we can *never* add
> additional information to each rectangle. Whenever we add new APIs
> or features we try to allow for future extensions (with mixed
> success...), so now is the time to do so for v4l2_rect.
> 
> Something like this:
> 
> struct v4l2_sel_rect {
> 	struct v4l2_rect r;
> 	__u32 reserved[4];
> };
> 
> would be fine by me as well. It's an additional indirection, but it
> also makes it easier to go from a v4l2_rect to a v4l2_sel_rect.

We can always migrate to extended rectangle by modifying the structure to

struct v4l2_selection {
         __u32                   type;
         __u32                   target;
         __u32                   flags;
         union {
               struct v4l2_rect        r;
               struct v4l2_ext_rect    rext;
         };
         __u32                   reserved[X];
};

and adding V4L2_SEL_FLAG_EXT_RECT flag.

> 
> Regards,
> 
> 	Hans
> 
>>

Regards,
Tomasz Stanislawski

Re: [RFC v3] [RFC] v4l2: Support for multiple selections

[Ricardo Ribalda Delgado]

Hello Tomasz

Now is my time to say sorry for the delay, but i have been in holidays
and then I had a pile of work waiting on my desk :).



On Tue, Nov 12, 2013 at 3:54 PM, Tomasz Stanislawski
<t.stanislaws@samsung.com> wrote:
> Hi Ricardo,
> Sorry for a late reply. I've been 'offline' for the last two weeks.
> Please refer to the comments below.
>
> On 10/28/2013 11:46 PM, Ricardo Ribalda Delgado wrote:
>> Hello Tomasz
>>
>> Sorry for the late reply, but I have been offline the last week due to
>> the conference.
>>
>>
>> On Thu, Oct 24, 2013 at 12:31 PM, Tomasz Stanislawski
>> <t.stanislaws@samsung.com> wrote:
>>> Hi Ricardo,
>>> I am the designer of selection API. I hope I can help you a little.
>>> I think that there are two issues mixed in 'Mulitple selections' topic.
>>>
>>> Firstly, you described that you program a piece of hardware that is
>>> capable of selecting 8 areas for scanning. Now you
>>> are looking for userspace API to support such a feature.
>>> The feature of posting multiple rectangle was proposed in this RFC.
>>>
>>> Secondly, You introduced struct v4l2_ext_rect which is a future-proof
>>> version of v4l2_rect.
>>>
>>>
>>> I think that both issues should be solved in two separate patchsets.
>>>
>>> Ad 1.
>>> The selection of multiple scanning areas is a very driver-specific
>>> feature, isn't it? I think that you do not need to introduce any abstract
>>> interface. What would be other applications of the proposed interface?
>>
>> It is not driver specific. There are many sensors out there that
>> supports multiple window of interest, but today we are ignoring them
>> just because we dont have an api.
>>
>> The main application would be industrial imaging, where less data to
>> read means more fps and therefore the system can run faster.
>>
>> From my field I can tell you that it is a hard requirement for
>> computer vision. And it is a feature that we cannot model through v4l2
>> controls.
>>
>>
>
> OK. So there is no need to implement this feature as a driver-specific API.
> It can go automatically to generic API.
>
>>> Do you know other drivers that may need it? Sakari mentioned introduction
>>> of private targets for selections. I like this idea. Just define:
>>>
>>> #define V4L2_SEL_TGT_PRIVATE     0x80000000
>>>
>>> All targets that are >= V4L2_SEL_TGT_PRIVATE are driver-specific.
>>> Generic applications must not use them. Non-generic application
>>> must check out the driver of video node before using selections
>>> from private set. If some target becomes more useful and accepted
>>> by more then one driver then it can be moved to generic API.
>>> The good thing about private target is that enums from different
>>> drivers can collide so the target space is not going to be trashed.
>>>
>>
>> If you read the previous RFCs you will see that the approach you are
>> mentioning has been rejected.
>>
>> The main issue is that you cannot set atomically all the rectangles.
>> Lets say that the configuration formed by rectangle A, B and C is
>> legal, but the configuration A and B is not allowed by the sensor. How
>> could you set the rectangles one by one?
>>
>
> As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on.
> So activation of rectangle B (setting height to non-zero value) will enable
> rectangle C with some default size. Moreover disabling rectangle B (setting height to 0)
> may disable rectangle C automatically. I do not follow what is the problem here?
>

Lets say you want a configuration composed by 3 rectangles ABC and
there are no pair of rectangles with a legal configuration. You cannot
do step by step configuration.

Also lets say that your sensor requires that the total size of the
image is 700 lines on 3 rectanges and 500 on 4. You cannot do this
configuration step by step.


> Hmm. I think that the real problem is much different.
> Not how to set up rectangles atomically but rather
> how do anything non-trivial atomically in V4L2.
>
> It would be very nice to have crop/compose and format configured at the same time.
> However, current version of V4L2 API does not support that.
>
> Setting multiple crop rectangles may help a bit for only this little case.
> But how would like to set multicrop rectangles and multicompose rectangle atomically.
> How to define which crop rectangle refers to which to which compose rectangle.

The number of retangles in crop are the same number of rectables in the compose.

Crop[0] corresponds to compose[0], crop[1]to compose[1] and so on....

>
> What to do if one would like to change only 3rd crop rectangle?

You send the whole configuration. The same as today when the user only
wants to change the pixel format. He still have to send the size.

>
> Introduce rectangle id into v4l2_ext_rect?
> Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION?
> Is it really scalable?

 Why it is not scalable?It is much more scalable than 8 ioctls to set
8 rectangles.


>
> Multirectangle targets may seam to be a solution but I think it is not.
>
> I think that atomic transactions are what is really needed in V4L2.
> Other ideas like try-context from subdev API may also help.
>
> It will be nice to have something like
>
> VIDIOC_BEGIN
>   VIDIOC_S_SELECTION (target = CROP)
>   VIDIOC_S_SELECTION (target = COMPOSE)
>   VIDIOC_S_FMT
>   VIDIOC_S_CTRL
> VIDIOC_COMMIT
>
> and call a bunch of VIDIOC_G_* to see what really was applied.
>

This will trigger other isues. What we do if 2 programs starts two
transactions at the same time. We will keep a transaction array with
ALL the configurations? And what happens if there are 100?


>>> I have an auxiliary question. Do you have to set all rectangles
>>> at once? can you set up them one by one?
>>
>> Also if you tell the driver what exact configuration you will need, it
>> will provide you with the closest possible confuration, that cannot be
>
> s/cannot be done/may not be 'doable'

cannot be done. The driver cannot guess which rectangles will the user
set in the future.

>
>> done if you provide rectangle by rectangle.
>>
>>> But how to deal with multiple rectangles?
>>
>> Multiple rectangles is a desired feature, please take a look to the
>> presentation on the workshop.
>>
>
> I agree that it may be useful. I just think that multirectangle selections
> are needed to add support for such a feature.
>
>>>
>>> Anyway, first try to define something like this:
>>>
>>> #define V4L2_SEL_TGT_XXX_SCANOUT0  V4L2_SEL_TGT_PRIVATE
>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT  (V4L2_SEL_TGT_XXX_SCANOUT0 + 1)
>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS  (V4L2_SEL_TGT_XXX_SCANOUT0 + 2)
>>>
>>> #define V4L2_SEL_TGT_XXX_SCANOUT0  (V4L2_SEL_TGT_PRIVATE + 16)
>>> ...
>>>
>>> -- OR-- parametrized macros similar to one below:
>>>
>>> #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n))
>>>
>>> The application could setup all scanout areas one-by-one.
>>> By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array.
>>> The height of all consecutive area would be 0. This means disabling
>>> them effectively.
>>
>> Lets say rectangle A + B + C +D is legal, and A +B is also legal. You
>> are in ABCD and you want to go to AB. How can you do it?
>
> Just set C to have height 0. It will disable D automatically.

And what if I have ABCDE and I want to go to ABE?

As I said before the causistic of the sensors is inmense. Why limit ourselves?

>
> BTW. How are you going to emulate S_CROP by selection API
> if you must use at least two rectangles (A + B) ?


Simple, you dont.

>
> I suggest to use SCANOUT target in your first implementation.
> Notice that as long you use something different from CROP and COMPOSE,
> you may define interactions and dependencies any way you want, like
> - if change of scanout area can affect composing area
> - interaction with format and frame size
> - check if scanout area can overlap, multi crops should be allowed to overlap
>
> See if it will be sufficient.
> See what kind of problems you encouter.

On the process of the RFC I have used in our system all the
implementations. So far the only that has worked 100% is the solution
where the user present all the rectangles at once to the driver.

>
>>
>> If yo dissable C or D, the configuration is ilegal and therefor the
>> driver will return -EINVAL. So once you are in ABCD you cannot go
>> back...
>>
>
> The driver must not return EINVAL as long as it is possible to
> adjust values passed by user to some valid configuration.
> If configuration is fixed then VIDIOC_S_SELECTION works
> effectively as VIDIOC_G_SELECTION.
>
>
>>
>>>
>>> The change of V4L2_SEL_TGT_XXX_SCANOUT0 would influence all consequtive
>>> rectangle by shifting them down or resetting them to height 0.
>>> Notice that as long as targets are driver specific you are free do define
>>> interaction between the targets.
>>>
>>> I hope that proposed solution is satisfactory.
>>
>> As stated before, please follow the previous comments on the rfc,
>> specially the ones from Hans.
>>
>>>
>>> BTW. I think that the HW trick you described is not cropping.
>>> By cropping you select which part of sensor area is going
>>> to be processed into compose rectangle in a buffer.
>>
>> You are selecting part of the sensor, therefore you are cropping the image.
>>
>
> Yes, it is the same as long as you use only one rectangle.
>
> If using more then 1 the situation may become more complex.
> Currently a pair of composing and crop rectangles are used to setup scaling.
>
> You have to introduce a new definition of scaling factor if multirect crops are introduced.

The number of rectangles must be the same on the croping and on the
composing. The scaling factor is defined by the relation between
compose[x]/crop[x]


>
> Moreover what happens if flipping or rotation is applied?

Flipping is applied to the whole sensor.

> Currently only the content of the rectangle is rotated/flipped.
> If you use multi rectangle then content of each rectangle must be
> rotated/flipped separatelly. Are you sure that your hardware can do this?

In our hardware there is a mapping core that can map any pixel in any
location. So yes :)


>
> What about layout of composing rectangle inside output buffer?
> Should it reflect the layout of crop rectangle or all of them should be
> independent.
>
> Of course you can always say that everything is driver dependent :).
>
> Anyway, adding multirectangle selections for crop/compose is openning Pandora's Box.

I dont see why. When we defined the API we didn't consider the case of
multiples WOI, now we fix this.

>
>>>
>>> So technicaly you still insert the whole sensor area into the buffer.
>>
>> Only the lines/columns are read into the buffer.
>>
>
> Yes, but where into a buffer?
> Does you HW support setting destination for every crop rectangle?

The dma supports up to 256 destinations.

>
>>> Only part of the buffer is actually updated. So there is no cropping
>>> (cropping area is equal to the whole array).
>>>
>>> Notice, that every 'cropping' area goes to different part of a buffer.
>>> So you would need also muliple targets for composing (!!!) and very long
>>> chapter in V4L2 doc describing interactions between muliple-rectangle
>>> crops and compositions. Good luck !!! :).
>>
>> It is not that difficult to describe.
>>
>> You need the same ammount of rectangles in cropping and in compossing.
>> Rectangle X in cropping will be mapped to rectangle X in compose.
>>
>>
>>> Currently it is a hell to understand and define interaction between
>>> single crop, and compose and unfamous VIDIOC_S_FMT and m2m madness.
>>
>> On m2m devices we are only lacking a s_fmt on the first buffer, as we
>> have discussed on the workshop. I think we only lack a good reference
>> model in vivi.
>>
>>
>
> I was not present on the workshop. Could you provide more details?
>
>>>
>>> I strongly recommend to use private selections.
>>> It will be much simpler to define, implement, and modify if needed.
>>
>> I think the private selections will lead to specific applications for
>> specific drivers and we cannot support all the configurations with
>> them. Also there is no way for an app to enumerate that capability.
>
> call VIDIOC_G_SELECTION to check if a given SCANOUT is supported?
> EINVAL means that it is not supported.

And then we will have X zillions of SCANOUTS, that is really the Pandora box :)


>
>>
>>>
>>> BTW2. I think that the mulitple scanout areas can be modelled using
>>> media device API. Sakari may know how to do this.
>>
>> The areas are not read from the sensor. Therefore they can only be
>> proccessed on the sensor subdevice.
>>
>>>
>>>
>>> Ad 2. Extended rectangles.
>>> It is a good idea because v4l2_rect lacks any place for extensions.
>>> But before adding it to V4L2 API, I would like to know the examples
>>> of actual applications. Please, point drivers that actually need it.
>>
>> I have no need for it, but now that we are extending the selection API
>> it would be the perfect moment to add them.
>
> The perfect moment for adding something is when it is needed.

If we add the extended rectangles. Why using a structure that we
cannot extend in the future?

>
> The bad idea is preventing something from being added too early.
>
> Regards,
> Tomasz Stanislawski
>
>>
>> They could describe properties of the sensor, like tracking ids.
>>
>>>
>>> Other thing worth mentioning is reservation of few bits from
>>> v4l2_selection::flags to describe the type of data used for
>>> rectangle, v4l2_rect or v4l2_ext_rect. This way one can avoid
>>> introducting v4l2_selection::rectangles field.
>>>
>>> I hope you find this comments useful.
>>
>>
>> Regards
>>
>>>
>>> Regards,
>>> Tomasz Stanislawski
>>>
>>>
>>
>>
>>
>

Regards

-- 
Ricardo Ribalda

Re: [RFC v3] [RFC] v4l2: Support for multiple selections

[Ricardo Ribalda Delgado]

Hello Tomasz and Hans

On Thu, Nov 14, 2013 at 4:40 PM, Tomasz Stanislawski
<t.stanislaws@samsung.com> wrote:
> Hi Hans,
>
> On 11/14/2013 11:18 AM, Hans Verkuil wrote:
>> Hi Tomasz,
>>
>> On 11/12/13 15:54, Tomasz Stanislawski wrote:
>>> Hi Ricardo,
>>> Sorry for a late reply. I've been 'offline' for the last two weeks.
>>> Please refer to the comments below.
>>>
>
> [snip]
>
>>>
>>> As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on.
>>> So activation of rectangle B (setting height to non-zero value) will enable
>>> rectangle C with some default size. Moreover disabling rectangle B (setting height to 0)
>>> may disable rectangle C automatically. I do not follow what is the problem here?
>>
>> The problem would be in a situation like this:
>>
>> ......
>> .AA.B.
>> ......   -->   AAB
>> .C.DD.         CDD
>> ......
>>
>> A-D are the rectangles you want to select. They are cropped as shown on the
>> left and composed as shown on the right.
>>
>>>From what Ricardo told me the resulting composed image typically must be
>> a proper rectangle without padding anywhere.
>>
>> Trying to add rectangles one at a time breaks down when adding C because
>> the composition result is no longer a 'proper' rectangle. I don't see how
>> you can set something like that up one rectangle at a time.
>
> I see the issue but I think that it is not a big problem.
> Activating C forms a non-proper rectangle with A and B.
> Therefore, driver must force enabling D to form a proper rectangle again.
>
> I mean that instead of enlarging C to sum of width of A and B,
> the driver can implicitly activate D to ensure that A,B,C,D form a proper rectangle.

I think this will lead to X different drivers with X different behaviours.

>
> The special target called V4L2_SEL_TGT_COMPOSE_PADDED was introduced
> to inform application which part of a buffers if going to be modified
> with some undefined value.
>
> I see nothing against setting a padded rectangle for C to a rectangle that
> covers C and D or even the whole ABCD rectangle.
> I think it could be a great application for PADDED target.
> The application could easily detect which part of a buffer are affected.
>
> Even applications prepared to work with single crop devices
> would still work after enabling multi crop mode.
>
> The setup of rectangles my look like this.
>
> ************************************************
>
> S_SELECTION(CROP0 = A)
>
> crop           compose
> ----------------------
> ......
> .AA...
> ......   -->   AA..
> ......         ....
> ......         ....
>
> G_SELECTION(COMPOSE0)
>   AA..
>   ....
>   ....
>
>
> G_SELECTION(COMPOSE0_PADDED)
>   AA..
>   ....
>   ....
>
> ************************************************
>
> S_SELECTION(CROP1 = B)
> ......
> .AA.B.
> ......   -->   AAB.
> ......         ....
> ......         ....
>
> G_SELECTION(COMPOSE0)
>   AA..
>   ....
>   ....
>
> G_SELECTION(COMPOSE0_PADDED)
>   AAA.
>   ....
>   ....
>
> G_SELECTION(COMPOSE1)
>   ..B.
>   ....
>   ....
>
> G_SELECTION(COMPOSE1_PADDED)
>   BBB.
>   ....
>   ....
>
>
> ************************************************
>
> S_SELECTION(CROP2 = C) - D is activated implicitly
> ......
> .AA.B.
> .C.DD.   -->   AAB.
> ......         CDD.
> ......         ....
>
> G_SELECTION(COMPOSE0_PADDED)
>
>   AAA.
>   AAA.
>   ....
>
> G_SELECTION(COMPOSE2)
>   ....
>   C...
>   ....
>
>
> G_SELECTION(COMPOSE2_PADDED)
>   CCC.
>   CCC.
>   ....
>
> G_SELECTION(COMPOSE3)
>   ....
>   .DD.
>   ....
>
>
> G_SELECTION(COMPOSE3_PADDED)
>   DDD.
>   DDD.
>   ....
>
> One may argue that all this logic is unnecessary after adding support
> for multirect selections.
> So, I kindly ask what should happen if someone call S_SELECTION
> (in multirect mode) passing THREE rectangles A, B, and C (not D) ?
>
> The driver must adjust rectangles to some valid value. So it can
> increase width of C or implicitly activate D or disable C.
> I think that the best solution is activating D because
> it allows to set size of C to the value closest to requested one.
> Therefore logic for implicit activation of D should be implemented anyway.

You are assuming that the user will send you the rectangles in an
order that "makes sense", but it is not always the case. On the other
hand if you have all the rectangles, you can ALWAYS make the better
decisition.

>
>>
>> It makes much more sense to set everything up at once.
>
> I agree that it is better to set everything at once.
> But I strongly believe that transactions are the proper way to achieve that.
>
> Not multirectangle selections.
>

As I said before the transaction is something easier said than made.
What happens if multiple users starts a transaction? What happen if in
a transaction of 10 itmes, item 7 needs a readjusment by the driver?

The selection API cannot cover a type of selection, therefore it
should be fixed. Especially when it is something as easy as the
propossed RFC.


> It obfuscates API. It only pretends to fix a problem with applying
> a part of configuration atomically.
>
>>
>> BTW, what probably wasn't clear from Ricardo's explanation is that for every
>> crop rectangle you must have a corresponding compose rectangle so that you
>> know where to DMA it to.
>>
>> Your next question will be that it is a real problem that you can't set
>> crop and compose simultaneously, and you are right about that. Read on for
>> that... :-)
>>
>>> Hmm. I think that the real problem is much different.
>>> Not how to set up rectangles atomically but rather
>>> how do anything non-trivial atomically in V4L2.
>>>
>>> It would be very nice to have crop/compose and format configured at the same time.
>>> However, current version of V4L2 API does not support that.
>>>
>>> Setting multiple crop rectangles may help a bit for only this little case.
>>> But how would like to set multicrop rectangles and multicompose rectangle atomically.
>>
>> Why can't we extend the selection API a bit? How about this:
>>
>> #define V4L2_SEL_TGT_CROP_COMPOSE    0x0200
>>
>> struct v4l2_selection {
>>         __u32                   type;
>>         __u32                   target;
>>         __u32                   flags;
>>       union {
>>               struct v4l2_rect        r;
>>               struct v4l2_ext_rect    *pr;
>>       };
>>         __u32                   flags2;
>>       union {
>>               struct v4l2_rect        r2;
>>               struct v4l2_ext_rect    *pr2;
>>       };
>>       __u32                   rectangles;
>>         __u32                   reserved[3];
>> };
>>
>> If the target is CROP_COMPOSE then flags & r define the crop rectangle, and
>> flags2 & r2 define the compose rectangle. That way you can set them both
>> atomically.
>
> I do not like this idea because:
> - mix crop and composing generating semi-related cropcompose
Now looking with some perspective the selection API, dont you think
that the crop and compose are related enough to be set at the same
time?

> - obfuscates the API even more
Agreed, but we can find better naming.

> - wastes a lot of reserved fields
Only 2

> - what if someone would like to use separate 'flags' for each rectangle
>   ... this could be a nice feature anyway :)
Then he uses the ext_rect :P

>
> This remains me the proposition from early days of selection API.
>
> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/28945
>
> Refer to point 4 where v4l2_crop2 was mentioned.
>
>>
>> I would propose that the interaction with S_FMT is as follows: if S_FMT
>> defines a rectangle < then the current compose rectangle, then the
>> composition (and optionally crop) rectangle is reset. If it is >=, then
>> all is fine.
>
> There is a issue here. Long time ago it was stated that S_FMT
> should result in configuration where data are processed (scaled) into
> the 'whole picture'.
>
> It means that a COMPOSE rectangle should be reset after very S_FMT.
>
>>
>> If a new compose rectangle doesn't fit in the S_FMT rectangle then it should
>> adapt the S_FMT rectangle to make it fit (as long as HW constraints are
>> obeyed of course).
>
> What happens if REQBUFS was called between S_FMT and S_SELECTION?
>
>>
>> This sequence will obey the rules of V4L2 as well: the last operation takes
>> precedence over older operations. So setting S_FMT allow the driver to
>> change cropping/composing to get as close to the desired format as possible.
>>
>>> How to define which crop rectangle refers to which to which compose rectangle.
>>
>> By setting the crop and compose selections at the same time and of the same
>> size you can map each crop selection to a compose selection. It's all atomic
>> as well.
>>
>
> I think that adding an id field to v4l2_selection might be a good alternative
> to introduction of numbered targets like V4L2_SEL_TGT_COMPOSE0, 1, ...
>
> It will add support for all multirectangle features as cost of
> sacrifice of single reserved field. Moreover id field might be
> very useful for other selection targets like FOCUS or FACE.
>
> Other idea might to using some bits of 'flags' field to carry rectangle id.
>
>>>
>>> What to do if one would like to change only 3rd crop rectangle?
>>>
>>> Introduce rectangle id into v4l2_ext_rect?
>>> Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION?
>>> Is it really scalable?
>>
>> Why not?
>>
>
> Because it is not scalable.
> An application has to use 2 syscall instead of 1.
> Unnecessary copying has to be performed.

And setting 8 rectangles will mean 8 iocts instead of 2.

The user can also keep a copy of what has been set and there will only
be 1 ioctl.

>
>>>
>>> Multirectangle targets may seam to be a solution but I think it is not.
>>>
>>> I think that atomic transactions are what is really needed in V4L2.
>>> Other ideas like try-context from subdev API may also help.
>>>
>>> It will be nice to have something like
>>>
>>> VIDIOC_BEGIN
>>>   VIDIOC_S_SELECTION (target = CROP)
>>>   VIDIOC_S_SELECTION (target = COMPOSE)
>>>   VIDIOC_S_FMT
>>>   VIDIOC_S_CTRL
>>> VIDIOC_COMMIT
>>
>> I don't think S_FMT is needed here: it's something you set up at the
>> beginning and don't touch afterwards.
>
> One could say the same about compose, crop, ctrls.
> The problem are the interactions between those objects.
> What is dependent on what. What can change what.
> How to reach a valid state in all cases not preventing
> some valid configurations from being unreachable.
>
>>
>> Wouldn't VIDIOC_S_SELECTION (target = CROP_COMPOSE) go a long way to
>> solving these atomicity problems?
>>
>> Another nice feature that can be added to the selection API is to
>> add a field to refer to a frame sequence number or a v4l2_buffer
>> index (the latter is probably better): this would make it easy to
>> apply an atomic crop/compose change to easily implement things like
>> digital zoom or moving windows around. We could do something similar
>> for controls.
>>
>> This would also solve the problem of assigning a per-buffer (or per-frame)
>> configuration, something that libcamera2/3 needs.
>>
>
> Introduce transactions with frame sequence numbers.
> I proposed this in Cambridge in 2012.
>
>>>
>>> and call a bunch of VIDIOC_G_* to see what really was applied.
>>>
>>>>> I have an auxiliary question. Do you have to set all rectangles
>>>>> at once? can you set up them one by one?
>>>>
>>>> Also if you tell the driver what exact configuration you will need, it
>>>> will provide you with the closest possible confuration, that cannot be
>>>
>>> s/cannot be done/may not be 'doable'
>>>
>>>> done if you provide rectangle by rectangle.
>>>>
>>>>> But how to deal with multiple rectangles?
>>>>
>>>> Multiple rectangles is a desired feature, please take a look to the
>>>> presentation on the workshop.
>>>>
>>>
>>> I agree that it may be useful. I just think that multirectangle selections
>>> are needed to add support for such a feature.
>>
>> I don't follow, isn't that what this proposal adds?
>>
>
> s/are needed/are not needed/
>
> Sorry for confusion.
>
>>>
>>>>>
>>>>> Anyway, first try to define something like this:
>>>>>
>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0  V4L2_SEL_TGT_PRIVATE
>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT  (V4L2_SEL_TGT_XXX_SCANOUT0 + 1)
>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS  (V4L2_SEL_TGT_XXX_SCANOUT0 + 2)
>>>>>
>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0  (V4L2_SEL_TGT_PRIVATE + 16)
>>>>> ...
>>>>>
>>>>> -- OR-- parametrized macros similar to one below:
>>>>>
>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n))
>>>>>
>>>>> The application could setup all scanout areas one-by-one.
>>>>> By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array.
>>>>> The height of all consecutive area would be 0. This means disabling
>>>>> them effectively.
>>>>
>>>> Lets say rectangle A + B + C +D is legal, and A +B is also legal. You
>>>> are in ABCD and you want to go to AB. How can you do it?
>>>
>>> Just set C to have height 0. It will disable D automatically.
>>
>> It's the other direction that's the problem: how to go from A + B to
>> A + B + C + D if ABC is illegal.
>>
>>> BTW. How are you going to emulate S_CROP by selection API
>>> if you must use at least two rectangles (A + B) ?
>>
>> S_CROP will always just set one rectangle A. So if you had multiple
>> rectangles in your selection then after S_CROP you'll have only one.
>
> So S_CROP should change only A.
> What happens if someone already set device to work with 4 rectangles.
> Then calls S_CROP that modifies A in such a way that ABCD no longer forms
> a proper rectangle.
>
> What should happen?
> - ignore S_CROP, return previous settings, or
> - modify B,C,D to form a proper rectangle, or
> - disable B,C,D, or ?

It will be the same as setting a multicrop with 1 rectangle.

>
>>
>> I'm assuming that the hw will always support a single crop rectangle.
>> If not (extraordinarily unlikely), then S_CROP should just return an
>> error.
>
> I agree that it is highly unlikely. I am just trying to figure out
> what should happen when calling S_CROP or single-rectangle S_SELECTION
> after calling multirect S_SELECTION.

The sensor is configured with 1 rectangle.

>
>>
>>> I suggest to use SCANOUT target in your first implementation.
>>> Notice that as long you use something different from CROP and COMPOSE,
>>> you may define interactions and dependencies any way you want, like
>>> - if change of scanout area can affect composing area
>>> - interaction with format and frame size
>>> - check if scanout area can overlap, multi crops should be allowed to overlap
>>
>> I see no reason for this. A CROP_COMPOSE target does all you need.
>
> I think that CROP and COMPOSE is all you need.
>
>>
>> Note that to set up N rectangles where N differs from the current
>> rectangle count you do need to use CROP_COMPOSE. You can't setup CROP
>> and COMPOSE rectangles independently since having N crop rectangles
>> and M compose rectangles makes no sense. If you don't change the
>> number of rectangles, then you can still use CROP and COMPOSE as is.
>>
>
> I am afraid that you are trying to kill too many birds with one stone.
> I think it is a very good moment to use bazooka.
>
> Transactions enhanced with frame numbers will handle a lot of issues with V4L2.
>
> Moreover it will allow to support some nice effects like:
> - multiple exposures during HDR photography
> - continuous online changes preview windows
> - flash at a specific frame
> - change format during video streaming to grab a high-resolution picture
> - support multi rectangle cropping and composing :)

Even with transactions I humble believe that we should have a
multirectangle call. :)


>
>>>
>
> [snip]
>
>
>>>> I have no need for it, but now that we are extending the selection API
>>>> it would be the perfect moment to add them.
>>>
>>> The perfect moment for adding something is when it is needed.
>>
>> We need to add support for multiple rectangles, so we added a pointer.
>>
>>> The bad idea is preventing something from being added too early.
>>
>> But if we make it a pointer to v4l2_rect, then we can *never* add
>> additional information to each rectangle. Whenever we add new APIs
>> or features we try to allow for future extensions (with mixed
>> success...), so now is the time to do so for v4l2_rect.
>>
>> Something like this:
>>
>> struct v4l2_sel_rect {
>>       struct v4l2_rect r;
>>       __u32 reserved[4];
>> };
>>
>> would be fine by me as well. It's an additional indirection, but it
>> also makes it easier to go from a v4l2_rect to a v4l2_sel_rect.
>
> We can always migrate to extended rectangle by modifying the structure to
>
> struct v4l2_selection {
>          __u32                   type;
>          __u32                   target;
>          __u32                   flags;
>          union {
>                struct v4l2_rect        r;
>                struct v4l2_ext_rect    rext;
>          };
>          __u32                   reserved[X];
> };
>
> and adding V4L2_SEL_FLAG_EXT_RECT flag.

Dont you think this is a bit confusing?



>
>>
>> Regards,
>>
>>       Hans
>>
>>>
>
> Regards,
> Tomasz Stanislawski
>

Regards

-- 
Ricardo Ribalda

Re: [RFC v3] [RFC] v4l2: Support for multiple selections

[Tomasz Stanislawski]

Hi Ricardo,
Please refer to the comments below.

On 12/10/2013 09:37 AM, Ricardo Ribalda Delgado wrote:
> Hello Tomasz
> 
> Now is my time to say sorry for the delay, but i have been in holidays
> and then I had a pile of work waiting on my desk :).
> 

No problem.

> 
> 
> On Tue, Nov 12, 2013 at 3:54 PM, Tomasz Stanislawski
> <t.stanislaws@samsung.com> wrote:
>> Hi Ricardo,
>> Sorry for a late reply. I've been 'offline' for the last two weeks.
>> Please refer to the comments below.

[snip]

>> As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on.
>> So activation of rectangle B (setting height to non-zero value) will enable
>> rectangle C with some default size. Moreover disabling rectangle B (setting height to 0)
>> may disable rectangle C automatically. I do not follow what is the problem here?
>>
> 
> Lets say you want a configuration composed by 3 rectangles ABC and
> there are no pair of rectangles with a legal configuration. You cannot
> do step by step configuration.
> 
> Also lets say that your sensor requires that the total size of the
> image is 700 lines on 3 rectanges and 500 on 4. You cannot do this
> configuration step by step.
> 
> 

Please, give more details. I do not think that situation is that problematic.
Is it even a practical limitation of hardware?

>> Hmm. I think that the real problem is much different.
>> Not how to set up rectangles atomically but rather
>> how do anything non-trivial atomically in V4L2.
>>
>> It would be very nice to have crop/compose and format configured at the same time.
>> However, current version of V4L2 API does not support that.
>>
>> Setting multiple crop rectangles may help a bit for only this little case.
>> But how would like to set multicrop rectangles and multicompose rectangle atomically.
>> How to define which crop rectangle refers to which to which compose rectangle.
> 
> The number of retangles in crop are the same number of rectables in the compose.
> 
> Crop[0] corresponds to compose[0], crop[1]to compose[1] and so on....
> 

I mean something different.
While using multirectangle selection, one still must use two
VIDIOC_S_SELECTION calls. One to set crop rectangles,
second one to set compose rectangles.
So you cannot set crop and compose atomically, anyway.

Hans proposed some extension to support atomic setup
of both properties. However I think that it is a little overengineered.

I still does not solve problems with flipping and rotations, which may
have a huge impact on mulitrect cropping/composing limitations.

>>
>> What to do if one would like to change only 3rd crop rectangle?
> 
> You send the whole configuration. The same as today when the user only
> wants to change the pixel format. He still have to send the size.
> 
>>
>> Introduce rectangle id into v4l2_ext_rect?
>> Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION?
>> Is it really scalable?
> 
>  Why it is not scalable?It is much more scalable than 8 ioctls to set
> 8 rectangles.
> 
> 

One has to copy_from_user/copy_to_user an arbitrary amount of data.
One cannot say "I would like to change rectangle number 5".
Using single S_SELECTION for 1 rectangle is only one ioctl need to be called.

When using multirect selections a kernel must copy
all data to user (completely needlessly) during G_SELECTION.
next, the kernel must load all rectangles back to kernel space
(only one rectangle is actually needed to be copied).

It is quite probable that copying 8 rectangle will NOT cause any
performance issues. But copying 256 rectangles in both directions
might cause an observable slowdown.

>>
>> Multirectangle targets may seam to be a solution but I think it is not.
>>
>> I think that atomic transactions are what is really needed in V4L2.
>> Other ideas like try-context from subdev API may also help.
>>
>> It will be nice to have something like
>>
>> VIDIOC_BEGIN
>>   VIDIOC_S_SELECTION (target = CROP)
>>   VIDIOC_S_SELECTION (target = COMPOSE)
>>   VIDIOC_S_FMT
>>   VIDIOC_S_CTRL
>> VIDIOC_COMMIT
>>
>> and call a bunch of VIDIOC_G_* to see what really was applied.
>>
> 
> This will trigger other isues. What we do if 2 programs starts two
> transactions at the same time. We will keep a transaction array with
> ALL the configurations? And what happens if there are 100?
> 
> 

There are multiple ways to solve the problem.
One options would be making VIDIOC_BEGIN a blocking operation
so only one thread can access the critical section.

Other way would be creation of temporary configuration for
each file handle and applying it after VIDIOC_COMMIT.
The VIDIOC_COMMIT would he handled in a driver under a mutex.

There is a huge space for inventions here.

>>>> I have an auxiliary question. Do you have to set all rectangles
>>>> at once? can you set up them one by one?
>>>
>>> Also if you tell the driver what exact configuration you will need, it
>>> will provide you with the closest possible confuration, that cannot be
>>
>> s/cannot be done/may not be 'doable'
> 
> cannot be done. The driver cannot guess which rectangles will the user
> set in the future.
> 
>>
>>> done if you provide rectangle by rectangle.
>>>
>>>> But how to deal with multiple rectangles?
>>>
>>> Multiple rectangles is a desired feature, please take a look to the
>>> presentation on the workshop.
>>>
>>
>> I agree that it may be useful. I just think that multirectangle selections
>> are needed to add support for such a feature.
>>
>>>>
>>>> Anyway, first try to define something like this:
>>>>
>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0  V4L2_SEL_TGT_PRIVATE
>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT  (V4L2_SEL_TGT_XXX_SCANOUT0 + 1)
>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS  (V4L2_SEL_TGT_XXX_SCANOUT0 + 2)
>>>>
>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0  (V4L2_SEL_TGT_PRIVATE + 16)
>>>> ...
>>>>
>>>> -- OR-- parametrized macros similar to one below:
>>>>
>>>> #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n))
>>>>
>>>> The application could setup all scanout areas one-by-one.
>>>> By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array.
>>>> The height of all consecutive area would be 0. This means disabling
>>>> them effectively.
>>>
>>> Lets say rectangle A + B + C +D is legal, and A +B is also legal. You
>>> are in ABCD and you want to go to AB. How can you do it?
>>
>> Just set C to have height 0. It will disable D automatically.
> 
> And what if I have ABCDE and I want to go to ABE?
> 
> As I said before the causistic of the sensors is inmense. Why limit ourselves?
> 

Please provide some example with detailed description.

>>
>> BTW. How are you going to emulate S_CROP by selection API
>> if you must use at least two rectangles (A + B) ?
> 
> 
> Simple, you dont.
> 

Sorry but I do not follow.
Driver must setup a configuration that is the most similar to user's request.
It is not allowed to return EINVAL. The selections are allowed to
return -ERANGE if rectangle is not consistent with constraints.
However, using constraints is optional, so applications that uses
constraints expects that ioctl might fail.

If HW supports crop the it should allow user to set crop.
Support for one rectangle can be emulated using 2 rectangles.
So I see no reason to give up support for single rectangle crop.

>>
>> I suggest to use SCANOUT target in your first implementation.
>> Notice that as long you use something different from CROP and COMPOSE,
>> you may define interactions and dependencies any way you want, like
>> - if change of scanout area can affect composing area
>> - interaction with format and frame size
>> - check if scanout area can overlap, multi crops should be allowed to overlap
>>
>> See if it will be sufficient.
>> See what kind of problems you encouter.
> 
> On the process of the RFC I have used in our system all the
> implementations. So far the only that has worked 100% is the solution
> where the user present all the rectangles at once to the driver.
> 

What were the problems with "the other" implementations?
Maybe they could be fixed/improved to support
the multirect feature without those huge API extensions.
Please provide more practical details.
I mean detailed HW limitations, and application needs.

>>
>>>
>>> If yo dissable C or D, the configuration is ilegal and therefor the
>>> driver will return -EINVAL. So once you are in ABCD you cannot go
>>> back...
>>>
>>
>> The driver must not return EINVAL as long as it is possible to
>> adjust values passed by user to some valid configuration.
>> If configuration is fixed then VIDIOC_S_SELECTION works
>> effectively as VIDIOC_G_SELECTION.
>>
>>
>>>
>>>>
>>>> The change of V4L2_SEL_TGT_XXX_SCANOUT0 would influence all consequtive
>>>> rectangle by shifting them down or resetting them to height 0.
>>>> Notice that as long as targets are driver specific you are free do define
>>>> interaction between the targets.
>>>>
>>>> I hope that proposed solution is satisfactory.
>>>
>>> As stated before, please follow the previous comments on the rfc,
>>> specially the ones from Hans.
>>>
>>>>
>>>> BTW. I think that the HW trick you described is not cropping.
>>>> By cropping you select which part of sensor area is going
>>>> to be processed into compose rectangle in a buffer.
>>>
>>> You are selecting part of the sensor, therefore you are cropping the image.
>>>
>>
>> Yes, it is the same as long as you use only one rectangle.
>>
>> If using more then 1 the situation may become more complex.
>> Currently a pair of composing and crop rectangles are used to setup scaling.
>>
>> You have to introduce a new definition of scaling factor if multirect crops are introduced.
> 
> The number of rectangles must be the same on the croping and on the
> composing. The scaling factor is defined by the relation between
> compose[x]/crop[x]
> 
> 

What happens if scaling factor must be the same for all rectangles due to HW limitations?
How to choose the proper scaling factor is crop and compose rectangles
are passed in separate ioctls?

>>
>> Moreover what happens if flipping or rotation is applied?
> 
> Flipping is applied to the whole sensor.
> 
>> Currently only the content of the rectangle is rotated/flipped.
>> If you use multi rectangle then content of each rectangle must be
>> rotated/flipped separatelly. Are you sure that your hardware can do this?
> 
> In our hardware there is a mapping core that can map any pixel in any
> location. So yes :)
> 
> 

So your HW is so sophisticated that using single rectangle selections
(with small tweaks) might be good enough to configure your hardware.
Please provide more details.

>>
>> What about layout of composing rectangle inside output buffer?
>> Should it reflect the layout of crop rectangle or all of them should be
>> independent.
>>
>> Of course you can always say that everything is driver dependent :).
>>
>> Anyway, adding multirectangle selections for crop/compose is openning Pandora's Box.
> 
> I dont see why. When we defined the API we didn't consider the case of
> multiples WOI, now we fix this.
> 

Adding multiple instances of crop rectangle is not an issue in selection API.
This API was designed to allow new targets (or ids per target) to be added.

The real problem is passing all rectangles at one time.
Using mutlirect selection may help a bit in your special case
at a cost of API obfuscation.
It still will not solve problems with interactions between
format/crop/compose/flipping/rotation.

Again. I think that transactions are the solution to many of those problems.
I would be much more generic solutions than all hypothetical extensions
to selections.

>>
>>>>
>>>> So technicaly you still insert the whole sensor area into the buffer.
>>>
>>> Only the lines/columns are read into the buffer.
>>>
>>
>> Yes, but where into a buffer?
>> Does you HW support setting destination for every crop rectangle?
> 
> The dma supports up to 256 destinations.
> 

Nice. So may encounter performance issues while trying to change
one rectangle using S_SELECTION :).

>>
>>>> Only part of the buffer is actually updated. So there is no cropping
>>>> (cropping area is equal to the whole array).
>>>>
>>>> Notice, that every 'cropping' area goes to different part of a buffer.
>>>> So you would need also muliple targets for composing (!!!) and very long
>>>> chapter in V4L2 doc describing interactions between muliple-rectangle
>>>> crops and compositions. Good luck !!! :).
>>>
>>> It is not that difficult to describe.
>>>
>>> You need the same ammount of rectangles in cropping and in compossing.
>>> Rectangle X in cropping will be mapped to rectangle X in compose.
>>>
>>>
>>>> Currently it is a hell to understand and define interaction between
>>>> single crop, and compose and unfamous VIDIOC_S_FMT and m2m madness.
>>>
>>> On m2m devices we are only lacking a s_fmt on the first buffer, as we
>>> have discussed on the workshop. I think we only lack a good reference
>>> model in vivi.
>>>
>>>
>>
>> I was not present on the workshop. Could you provide more details?
>>
>>>>
>>>> I strongly recommend to use private selections.
>>>> It will be much simpler to define, implement, and modify if needed.
>>>
>>> I think the private selections will lead to specific applications for
>>> specific drivers and we cannot support all the configurations with
>>> them. Also there is no way for an app to enumerate that capability.
>>
>> call VIDIOC_G_SELECTION to check if a given SCANOUT is supported?
>> EINVAL means that it is not supported.
> 
> And then we will have X zillions of SCANOUTS, that is really the Pandora box :)
> 
> 

Not exactly. I preferred to introduce a bunch of SCANOUT targets to
avoid wasting reserved fields in struct v4l2_selection. There is
a plenty of unused bits in v4l2_selection::target. However, it is still
ok to introduce v4l2_selection::index field. The API is still pretty nice.

I preferred to use new target called SCANOUTs to avoid mixing
this multirectangle feature is crop/compose interactions.
This fear might be considered was little premature (or even paranoid).

I just wanted to open this "Pandora's Box" in same safer
and less demanding environment.
First trying to implement everything using some
non-conflicting hw-specific target called SCANOUT.
If it'd worked than one would make it an alias to CROP in
a newer version of the kernel.
Making it a part of main API will force you to support
its functionality (even erroneous) in all future versions of kernel
with no possibility to change it.

>>
>>>
>>>>
>>>> BTW2. I think that the mulitple scanout areas can be modelled using
>>>> media device API. Sakari may know how to do this.
>>>
>>> The areas are not read from the sensor. Therefore they can only be
>>> proccessed on the sensor subdevice.
>>>
>>>>
>>>>
>>>> Ad 2. Extended rectangles.
>>>> It is a good idea because v4l2_rect lacks any place for extensions.
>>>> But before adding it to V4L2 API, I would like to know the examples
>>>> of actual applications. Please, point drivers that actually need it.
>>>
>>> I have no need for it, but now that we are extending the selection API
>>> it would be the perfect moment to add them.
>>
>> The perfect moment for adding something is when it is needed.
> 
> If we add the extended rectangles. Why using a structure that we
> cannot extend in the future?
> 

Basically, because it is good enough.
Introducting extended rectangles breaks symmetry between
singlerect and multirect selections.
v4l2_rect is used in other structures like v4l2_clip, v4l2_crop, v4l2_window, etc.
Why not enhancing them with this new extended rectangle?
Moreover, it violates "Rule of Least Surprise" from "Unix Philosophy".
Why setting number of rectangles changes the structure type.

Please, first find a real application that really cannot be implemented
without introducing extended rectangle before extending the API.

The extended rectangles are an orthogonal feature to multirect selections
and can be added later.

Regards,

Tomasz Stanislawski

Re: [RFC v3] [RFC] v4l2: Support for multiple selections

[Hans Verkuil]

Hi Sylwester,

On 12/19/2013 12:09 PM, Tomasz Stanislawski wrote:
> Hi Ricardo,
> Please refer to the comments below.
> 
> On 12/10/2013 09:37 AM, Ricardo Ribalda Delgado wrote:
>> Hello Tomasz
>>
>> Now is my time to say sorry for the delay, but i have been in holidays
>> and then I had a pile of work waiting on my desk :).
>>
> 
> No problem.
> 
>>
>>
>> On Tue, Nov 12, 2013 at 3:54 PM, Tomasz Stanislawski
>> <t.stanislaws@samsung.com> wrote:
>>> Hi Ricardo,
>>> Sorry for a late reply. I've been 'offline' for the last two weeks.
>>> Please refer to the comments below.
> 
> [snip]
> 
>>> As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on.
>>> So activation of rectangle B (setting height to non-zero value) will enable
>>> rectangle C with some default size. Moreover disabling rectangle B (setting height to 0)
>>> may disable rectangle C automatically. I do not follow what is the problem here?
>>>
>>
>> Lets say you want a configuration composed by 3 rectangles ABC and
>> there are no pair of rectangles with a legal configuration. You cannot
>> do step by step configuration.
>>
>> Also lets say that your sensor requires that the total size of the
>> image is 700 lines on 3 rectanges and 500 on 4. You cannot do this
>> configuration step by step.
>>
>>
> 
> Please, give more details. I do not think that situation is that problematic.
> Is it even a practical limitation of hardware?
> 
>>> Hmm. I think that the real problem is much different.
>>> Not how to set up rectangles atomically but rather
>>> how do anything non-trivial atomically in V4L2.
>>>
>>> It would be very nice to have crop/compose and format configured at the same time.
>>> However, current version of V4L2 API does not support that.
>>>
>>> Setting multiple crop rectangles may help a bit for only this little case.
>>> But how would like to set multicrop rectangles and multicompose rectangle atomically.
>>> How to define which crop rectangle refers to which to which compose rectangle.
>>
>> The number of retangles in crop are the same number of rectables in the compose.
>>
>> Crop[0] corresponds to compose[0], crop[1]to compose[1] and so on....
>>
> 
> I mean something different.
> While using multirectangle selection, one still must use two
> VIDIOC_S_SELECTION calls. One to set crop rectangles,
> second one to set compose rectangles.
> So you cannot set crop and compose atomically, anyway.
> 
> Hans proposed some extension to support atomic setup
> of both properties. However I think that it is a little overengineered.

I disagree. I implemented it in vivi and it turned out to be quite easy.

For the record: I'm talking about this RFC:

http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/71822

> I still does not solve problems with flipping and rotations, which may
> have a huge impact on mulitrect cropping/composing limitations.

My proposal will make that much easier as well since flipping, rotating,
cropping and composing are all controls/properties that can be set
atomically (a control cluster). So drivers can create a single function
that can handle all the relationships in one place, and applications can
set all of these with one VIDIOC_S_EXT_CTRLS call.

> 
>>>
>>> What to do if one would like to change only 3rd crop rectangle?
>>
>> You send the whole configuration. The same as today when the user only
>> wants to change the pixel format. He still have to send the size.
>>
>>>
>>> Introduce rectangle id into v4l2_ext_rect?
>>> Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION?
>>> Is it really scalable?
>>
>>  Why it is not scalable?It is much more scalable than 8 ioctls to set
>> 8 rectangles.
>>
>>
> 
> One has to copy_from_user/copy_to_user an arbitrary amount of data.
> One cannot say "I would like to change rectangle number 5".
> Using single S_SELECTION for 1 rectangle is only one ioctl need to be called.
> 
> When using multirect selections a kernel must copy
> all data to user (completely needlessly) during G_SELECTION.
> next, the kernel must load all rectangles back to kernel space
> (only one rectangle is actually needed to be copied).
> 
> It is quite probable that copying 8 rectangle will NOT cause any
> performance issues. But copying 256 rectangles in both directions
> might cause an observable slowdown.

Adding matrix support is part of my proposal (i.e. allowing for 2D-arrays
of properties/controls. An array is just a 1xN matrix).

Originally I thought it would be a good idea to prefix the matrix with a
struct v4l2_rect where the application could set which sub-matrix it
wants to get/set. E.g. if the full 2D array is 10x20, then you could just
obtain a 2x2 matrix at position 5x4.

While that works, I've decided to drop that. It is quite cumbersome and I
am not convinced of its usefulness. Should it be needed in the future, then
it can be added (a flag would need to be added in struct v4l2_ext_control to
tell the framework that the matrix is preceeded by a struct v4l2_rect).

But before I would do that I would have to see a real use-case where this
is a clear performance issue. Adding support for this would increase the code
and add a performance penalty for the non-matrix controls/properties.

> 
>>>
>>> Multirectangle targets may seam to be a solution but I think it is not.
>>>
>>> I think that atomic transactions are what is really needed in V4L2.
>>> Other ideas like try-context from subdev API may also help.
>>>
>>> It will be nice to have something like
>>>
>>> VIDIOC_BEGIN
>>>   VIDIOC_S_SELECTION (target = CROP)
>>>   VIDIOC_S_SELECTION (target = COMPOSE)
>>>   VIDIOC_S_FMT
>>>   VIDIOC_S_CTRL
>>> VIDIOC_COMMIT
>>>
>>> and call a bunch of VIDIOC_G_* to see what really was applied.
>>>
>>
>> This will trigger other isues. What we do if 2 programs starts two
>> transactions at the same time. We will keep a transaction array with
>> ALL the configurations? And what happens if there are 100?
>>
>>
> 
> There are multiple ways to solve the problem.
> One options would be making VIDIOC_BEGIN a blocking operation
> so only one thread can access the critical section.
> 
> Other way would be creation of temporary configuration for
> each file handle and applying it after VIDIOC_COMMIT.
> The VIDIOC_COMMIT would he handled in a driver under a mutex.
> 
> There is a huge space for inventions here.

Using the control framework for this turns out to be very nice. All the
support for atomic configuration is already there. Implementing begin/commit
ioctls would be very hard, and it would also be difficult to tie into the
configuration store concept. Not to mention that it would basically duplicate
what the control framework already does w.r.t. handling atomicity.

I actually looked at what would be needed to do something like that, but
I realized very quickly that in reality the control framework did almost
everything I needed, and that it was much easier to add the missing pieces
there then to try and invent something completely new.

I hope to post a patch series for review before Christmas (fingers crossed).

Personally I am quite happy with it because it adds powerful new features
with a minimum amount of API (only one new ioctl: VIDIOC_QUERY_EXT_CTRL) and
code churn.

Regards,

	Hans

> 
>>>>> I have an auxiliary question. Do you have to set all rectangles
>>>>> at once? can you set up them one by one?
>>>>
>>>> Also if you tell the driver what exact configuration you will need, it
>>>> will provide you with the closest possible confuration, that cannot be
>>>
>>> s/cannot be done/may not be 'doable'
>>
>> cannot be done. The driver cannot guess which rectangles will the user
>> set in the future.
>>
>>>
>>>> done if you provide rectangle by rectangle.
>>>>
>>>>> But how to deal with multiple rectangles?
>>>>
>>>> Multiple rectangles is a desired feature, please take a look to the
>>>> presentation on the workshop.
>>>>
>>>
>>> I agree that it may be useful. I just think that multirectangle selections
>>> are needed to add support for such a feature.
>>>
>>>>>
>>>>> Anyway, first try to define something like this:
>>>>>
>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0  V4L2_SEL_TGT_PRIVATE
>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT  (V4L2_SEL_TGT_XXX_SCANOUT0 + 1)
>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS  (V4L2_SEL_TGT_XXX_SCANOUT0 + 2)
>>>>>
>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0  (V4L2_SEL_TGT_PRIVATE + 16)
>>>>> ...
>>>>>
>>>>> -- OR-- parametrized macros similar to one below:
>>>>>
>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n))
>>>>>
>>>>> The application could setup all scanout areas one-by-one.
>>>>> By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array.
>>>>> The height of all consecutive area would be 0. This means disabling
>>>>> them effectively.
>>>>
>>>> Lets say rectangle A + B + C +D is legal, and A +B is also legal. You
>>>> are in ABCD and you want to go to AB. How can you do it?
>>>
>>> Just set C to have height 0. It will disable D automatically.
>>
>> And what if I have ABCDE and I want to go to ABE?
>>
>> As I said before the causistic of the sensors is inmense. Why limit ourselves?
>>
> 
> Please provide some example with detailed description.
> 
>>>
>>> BTW. How are you going to emulate S_CROP by selection API
>>> if you must use at least two rectangles (A + B) ?
>>
>>
>> Simple, you dont.
>>
> 
> Sorry but I do not follow.
> Driver must setup a configuration that is the most similar to user's request.
> It is not allowed to return EINVAL. The selections are allowed to
> return -ERANGE if rectangle is not consistent with constraints.
> However, using constraints is optional, so applications that uses
> constraints expects that ioctl might fail.
> 
> If HW supports crop the it should allow user to set crop.
> Support for one rectangle can be emulated using 2 rectangles.
> So I see no reason to give up support for single rectangle crop.
> 
>>>
>>> I suggest to use SCANOUT target in your first implementation.
>>> Notice that as long you use something different from CROP and COMPOSE,
>>> you may define interactions and dependencies any way you want, like
>>> - if change of scanout area can affect composing area
>>> - interaction with format and frame size
>>> - check if scanout area can overlap, multi crops should be allowed to overlap
>>>
>>> See if it will be sufficient.
>>> See what kind of problems you encouter.
>>
>> On the process of the RFC I have used in our system all the
>> implementations. So far the only that has worked 100% is the solution
>> where the user present all the rectangles at once to the driver.
>>
> 
> What were the problems with "the other" implementations?
> Maybe they could be fixed/improved to support
> the multirect feature without those huge API extensions.
> Please provide more practical details.
> I mean detailed HW limitations, and application needs.
> 
>>>
>>>>
>>>> If yo dissable C or D, the configuration is ilegal and therefor the
>>>> driver will return -EINVAL. So once you are in ABCD you cannot go
>>>> back...
>>>>
>>>
>>> The driver must not return EINVAL as long as it is possible to
>>> adjust values passed by user to some valid configuration.
>>> If configuration is fixed then VIDIOC_S_SELECTION works
>>> effectively as VIDIOC_G_SELECTION.
>>>
>>>
>>>>
>>>>>
>>>>> The change of V4L2_SEL_TGT_XXX_SCANOUT0 would influence all consequtive
>>>>> rectangle by shifting them down or resetting them to height 0.
>>>>> Notice that as long as targets are driver specific you are free do define
>>>>> interaction between the targets.
>>>>>
>>>>> I hope that proposed solution is satisfactory.
>>>>
>>>> As stated before, please follow the previous comments on the rfc,
>>>> specially the ones from Hans.
>>>>
>>>>>
>>>>> BTW. I think that the HW trick you described is not cropping.
>>>>> By cropping you select which part of sensor area is going
>>>>> to be processed into compose rectangle in a buffer.
>>>>
>>>> You are selecting part of the sensor, therefore you are cropping the image.
>>>>
>>>
>>> Yes, it is the same as long as you use only one rectangle.
>>>
>>> If using more then 1 the situation may become more complex.
>>> Currently a pair of composing and crop rectangles are used to setup scaling.
>>>
>>> You have to introduce a new definition of scaling factor if multirect crops are introduced.
>>
>> The number of rectangles must be the same on the croping and on the
>> composing. The scaling factor is defined by the relation between
>> compose[x]/crop[x]
>>
>>
> 
> What happens if scaling factor must be the same for all rectangles due to HW limitations?
> How to choose the proper scaling factor is crop and compose rectangles
> are passed in separate ioctls?
> 
>>>
>>> Moreover what happens if flipping or rotation is applied?
>>
>> Flipping is applied to the whole sensor.
>>
>>> Currently only the content of the rectangle is rotated/flipped.
>>> If you use multi rectangle then content of each rectangle must be
>>> rotated/flipped separatelly. Are you sure that your hardware can do this?
>>
>> In our hardware there is a mapping core that can map any pixel in any
>> location. So yes :)
>>
>>
> 
> So your HW is so sophisticated that using single rectangle selections
> (with small tweaks) might be good enough to configure your hardware.
> Please provide more details.
> 
>>>
>>> What about layout of composing rectangle inside output buffer?
>>> Should it reflect the layout of crop rectangle or all of them should be
>>> independent.
>>>
>>> Of course you can always say that everything is driver dependent :).
>>>
>>> Anyway, adding multirectangle selections for crop/compose is openning Pandora's Box.
>>
>> I dont see why. When we defined the API we didn't consider the case of
>> multiples WOI, now we fix this.
>>
> 
> Adding multiple instances of crop rectangle is not an issue in selection API.
> This API was designed to allow new targets (or ids per target) to be added.
> 
> The real problem is passing all rectangles at one time.
> Using mutlirect selection may help a bit in your special case
> at a cost of API obfuscation.
> It still will not solve problems with interactions between
> format/crop/compose/flipping/rotation.
> 
> Again. I think that transactions are the solution to many of those problems.
> I would be much more generic solutions than all hypothetical extensions
> to selections.
> 
>>>
>>>>>
>>>>> So technicaly you still insert the whole sensor area into the buffer.
>>>>
>>>> Only the lines/columns are read into the buffer.
>>>>
>>>
>>> Yes, but where into a buffer?
>>> Does you HW support setting destination for every crop rectangle?
>>
>> The dma supports up to 256 destinations.
>>
> 
> Nice. So may encounter performance issues while trying to change
> one rectangle using S_SELECTION :).
> 
>>>
>>>>> Only part of the buffer is actually updated. So there is no cropping
>>>>> (cropping area is equal to the whole array).
>>>>>
>>>>> Notice, that every 'cropping' area goes to different part of a buffer.
>>>>> So you would need also muliple targets for composing (!!!) and very long
>>>>> chapter in V4L2 doc describing interactions between muliple-rectangle
>>>>> crops and compositions. Good luck !!! :).
>>>>
>>>> It is not that difficult to describe.
>>>>
>>>> You need the same ammount of rectangles in cropping and in compossing.
>>>> Rectangle X in cropping will be mapped to rectangle X in compose.
>>>>
>>>>
>>>>> Currently it is a hell to understand and define interaction between
>>>>> single crop, and compose and unfamous VIDIOC_S_FMT and m2m madness.
>>>>
>>>> On m2m devices we are only lacking a s_fmt on the first buffer, as we
>>>> have discussed on the workshop. I think we only lack a good reference
>>>> model in vivi.
>>>>
>>>>
>>>
>>> I was not present on the workshop. Could you provide more details?
>>>
>>>>>
>>>>> I strongly recommend to use private selections.
>>>>> It will be much simpler to define, implement, and modify if needed.
>>>>
>>>> I think the private selections will lead to specific applications for
>>>> specific drivers and we cannot support all the configurations with
>>>> them. Also there is no way for an app to enumerate that capability.
>>>
>>> call VIDIOC_G_SELECTION to check if a given SCANOUT is supported?
>>> EINVAL means that it is not supported.
>>
>> And then we will have X zillions of SCANOUTS, that is really the Pandora box :)
>>
>>
> 
> Not exactly. I preferred to introduce a bunch of SCANOUT targets to
> avoid wasting reserved fields in struct v4l2_selection. There is
> a plenty of unused bits in v4l2_selection::target. However, it is still
> ok to introduce v4l2_selection::index field. The API is still pretty nice.
> 
> I preferred to use new target called SCANOUTs to avoid mixing
> this multirectangle feature is crop/compose interactions.
> This fear might be considered was little premature (or even paranoid).
> 
> I just wanted to open this "Pandora's Box" in same safer
> and less demanding environment.
> First trying to implement everything using some
> non-conflicting hw-specific target called SCANOUT.
> If it'd worked than one would make it an alias to CROP in
> a newer version of the kernel.
> Making it a part of main API will force you to support
> its functionality (even erroneous) in all future versions of kernel
> with no possibility to change it.
> 
>>>
>>>>
>>>>>
>>>>> BTW2. I think that the mulitple scanout areas can be modelled using
>>>>> media device API. Sakari may know how to do this.
>>>>
>>>> The areas are not read from the sensor. Therefore they can only be
>>>> proccessed on the sensor subdevice.
>>>>
>>>>>
>>>>>
>>>>> Ad 2. Extended rectangles.
>>>>> It is a good idea because v4l2_rect lacks any place for extensions.
>>>>> But before adding it to V4L2 API, I would like to know the examples
>>>>> of actual applications. Please, point drivers that actually need it.
>>>>
>>>> I have no need for it, but now that we are extending the selection API
>>>> it would be the perfect moment to add them.
>>>
>>> The perfect moment for adding something is when it is needed.
>>
>> If we add the extended rectangles. Why using a structure that we
>> cannot extend in the future?
>>
> 
> Basically, because it is good enough.
> Introducting extended rectangles breaks symmetry between
> singlerect and multirect selections.
> v4l2_rect is used in other structures like v4l2_clip, v4l2_crop, v4l2_window, etc.
> Why not enhancing them with this new extended rectangle?
> Moreover, it violates "Rule of Least Surprise" from "Unix Philosophy".
> Why setting number of rectangles changes the structure type.
> 
> Please, first find a real application that really cannot be implemented
> without introducing extended rectangle before extending the API.
> 
> The extended rectangles are an orthogonal feature to multirect selections
> and can be added later.
> 
> Regards,
> 
> Tomasz Stanislawski
> 

Re: [RFC v3] [RFC] v4l2: Support for multiple selections

[Ricardo Ribalda Delgado]

Hello Tomasz

Thanks for your comments.

On Thu, Dec 19, 2013 at 12:09 PM, Tomasz Stanislawski
<t.stanislaws@samsung.com> wrote:
> Hi Ricardo,
> Please refer to the comments below.
>
> On 12/10/2013 09:37 AM, Ricardo Ribalda Delgado wrote:
>> Hello Tomasz
>>
>> Now is my time to say sorry for the delay, but i have been in holidays
>> and then I had a pile of work waiting on my desk :).
>>
>
> No problem.
>
>>
>>
>> On Tue, Nov 12, 2013 at 3:54 PM, Tomasz Stanislawski
>> <t.stanislaws@samsung.com> wrote:
>>> Hi Ricardo,
>>> Sorry for a late reply. I've been 'offline' for the last two weeks.
>>> Please refer to the comments below.
>
> [snip]
>
>>> As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on.
>>> So activation of rectangle B (setting height to non-zero value) will enable
>>> rectangle C with some default size. Moreover disabling rectangle B (setting height to 0)
>>> may disable rectangle C automatically. I do not follow what is the problem here?
>>>
>>
>> Lets say you want a configuration composed by 3 rectangles ABC and
>> there are no pair of rectangles with a legal configuration. You cannot
>> do step by step configuration.
>>
>> Also lets say that your sensor requires that the total size of the
>> image is 700 lines on 3 rectanges and 500 on 4. You cannot do this
>> configuration step by step.
>>
>>
>
> Please, give more details. I do not think that situation is that problematic.
> Is it even a practical limitation of hardware?

We have an fpga camera that uses X cycles to read a sensor. That X is
FIXED. Lets say it is 1000 lines time

The sensor is read linearly from a programmable address A. for a lines

You can program also address B for b lines

Jumping from the area A to the area B needs j lines time.

a+b+j must be equal to 1000.

On the multiselect, the user sets A and B, the constrains are verified
and the system is set.

On your proposal

Area A is set, but because the size is <1000, the driver changes the
size to 1000, probably also changing the initial line a.

Area B is set, the driver needs to resize the area A too, but the
initial line a (what the user wants) has been replaced by a driver
chossen a


This is a very simple example with 2 areas, extrapolate it to 8 areas.


>
>>> Hmm. I think that the real problem is much different.
>>> Not how to set up rectangles atomically but rather
>>> how do anything non-trivial atomically in V4L2.
>>>
>>> It would be very nice to have crop/compose and format configured at the same time.
>>> However, current version of V4L2 API does not support that.
>>>
>>> Setting multiple crop rectangles may help a bit for only this little case.
>>> But how would like to set multicrop rectangles and multicompose rectangle atomically.
>>> How to define which crop rectangle refers to which to which compose rectangle.
>>
>> The number of retangles in crop are the same number of rectables in the compose.
>>
>> Crop[0] corresponds to compose[0], crop[1]to compose[1] and so on....
>>
>
> I mean something different.
> While using multirectangle selection, one still must use two
> VIDIOC_S_SELECTION calls. One to set crop rectangles,
> second one to set compose rectangles.
> So you cannot set crop and compose atomically, anyway.

No, but I can set all the crop rectangles at once.

The driver then finds out a compatible configuration of  composing.

The user replaces that configuration (if needed) with a new compose.

Exactly the same as today with one rectangle..

>
> Hans proposed some extension to support atomic setup
> of both properties. However I think that it is a little overengineered.
>
> I still does not solve problems with flipping and rotations, which may
> have a huge impact on mulitrect cropping/composing limitations.

Flip and rotation is affected to whole sensor not to the specific rectangles.

>
>>>
>>> What to do if one would like to change only 3rd crop rectangle?
>>
>> You send the whole configuration. The same as today when the user only
>> wants to change the pixel format. He still have to send the size.
>>
>>>
>>> Introduce rectangle id into v4l2_ext_rect?
>>> Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION?
>>> Is it really scalable?
>>
>>  Why it is not scalable?It is much more scalable than 8 ioctls to set
>> 8 rectangles.
>>
>>
>
> One has to copy_from_user/copy_to_user an arbitrary amount of data.
> One cannot say "I would like to change rectangle number 5".
> Using single S_SELECTION for 1 rectangle is only one ioctl need to be called.

If the user have saved the output of the previous s_selection he can
change a specific rectangle with one s_selection call

On the other hand he can even change all the rectangles with one
s_selection, on your example the user has to make N s_selection calls.

>
> When using multirect selections a kernel must copy
> all data to user (completely needlessly) during G_SELECTION.
> next, the kernel must load all rectangles back to kernel space
> (only one rectangle is actually needed to be copied).
>
> It is quite probable that copying 8 rectangle will NOT cause any
> performance issues. But copying 256 rectangles in both directions
> might cause an observable slowdown.

256 small ioctls are at least one order of mangniture slower that 1
ioctl with 256 times more memory send.

Also bare in mind that the most probable case takes 2 or 3 rectangles, not 256.


>
>>>
>>> Multirectangle targets may seam to be a solution but I think it is not.
>>>
>>> I think that atomic transactions are what is really needed in V4L2.
>>> Other ideas like try-context from subdev API may also help.
>>>
>>> It will be nice to have something like
>>>
>>> VIDIOC_BEGIN
>>>   VIDIOC_S_SELECTION (target = CROP)
>>>   VIDIOC_S_SELECTION (target = COMPOSE)
>>>   VIDIOC_S_FMT
>>>   VIDIOC_S_CTRL
>>> VIDIOC_COMMIT
>>>
>>> and call a bunch of VIDIOC_G_* to see what really was applied.
>>>
>>
>> This will trigger other isues. What we do if 2 programs starts two
>> transactions at the same time. We will keep a transaction array with
>> ALL the configurations? And what happens if there are 100?
>>
>>
>
> There are multiple ways to solve the problem.
> One options would be making VIDIOC_BEGIN a blocking operation
> so only one thread can access the critical section.

So, an app opens /dev/video0 , calls vidioc_begin, and no other user can use it.

>
> Other way would be creation of temporary configuration for
> each file handle and applying it after VIDIOC_COMMIT.
> The VIDIOC_COMMIT would he handled in a driver under a mutex.

So a driver has to anyway save a multirectangle structure in his
memory. The app will have to implement common algorithtms like
rectangle collide, rectangle reorder, max line counting....

IMHO: Even with transaction a multirectangle set is a good idea.

>
> There is a huge space for inventions here.
>
>>>>> I have an auxiliary question. Do you have to set all rectangles
>>>>> at once? can you set up them one by one?
>>>>
>>>> Also if you tell the driver what exact configuration you will need, it
>>>> will provide you with the closest possible confuration, that cannot be
>>>
>>> s/cannot be done/may not be 'doable'
>>
>> cannot be done. The driver cannot guess which rectangles will the user
>> set in the future.
>>
>>>
>>>> done if you provide rectangle by rectangle.
>>>>
>>>>> But how to deal with multiple rectangles?
>>>>
>>>> Multiple rectangles is a desired feature, please take a look to the
>>>> presentation on the workshop.
>>>>
>>>
>>> I agree that it may be useful. I just think that multirectangle selections
>>> are needed to add support for such a feature.
>>>
>>>>>
>>>>> Anyway, first try to define something like this:
>>>>>
>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0  V4L2_SEL_TGT_PRIVATE
>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT  (V4L2_SEL_TGT_XXX_SCANOUT0 + 1)
>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS  (V4L2_SEL_TGT_XXX_SCANOUT0 + 2)
>>>>>
>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0  (V4L2_SEL_TGT_PRIVATE + 16)
>>>>> ...
>>>>>
>>>>> -- OR-- parametrized macros similar to one below:
>>>>>
>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n))
>>>>>
>>>>> The application could setup all scanout areas one-by-one.
>>>>> By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array.
>>>>> The height of all consecutive area would be 0. This means disabling
>>>>> them effectively.
>>>>
>>>> Lets say rectangle A + B + C +D is legal, and A +B is also legal. You
>>>> are in ABCD and you want to go to AB. How can you do it?
>>>
>>> Just set C to have height 0. It will disable D automatically.
>>
>> And what if I have ABCDE and I want to go to ABE?
>>
>> As I said before the causistic of the sensors is inmense. Why limit ourselves?
>>
>
> Please provide some example with detailed description.

We gave 3 rectangles

A: 0-10
B: 20-30
C: 40-50

we want a setup with

A: 0-10
C: 40-50

If I remove B with your proposal also C is removed, so I have to
create it again.

Also take a look to the very first example, there are sensors that
require that the total amount of lines to be X


>
>>>
>>> BTW. How are you going to emulate S_CROP by selection API
>>> if you must use at least two rectangles (A + B) ?
>>
>>
>> Simple, you dont.
>>
>
> Sorry but I do not follow.
> Driver must setup a configuration that is the most similar to user's request.
> It is not allowed to return EINVAL. The selections are allowed to
> return -ERANGE if rectangle is not consistent with constraints.
> However, using constraints is optional, so applications that uses
> constraints expects that ioctl might fail.
>
> If HW supports crop the it should allow user to set crop.
> Support for one rectangle can be emulated using 2 rectangles.
> So I see no reason to give up support for single rectangle crop.
>

And what if the rectangless cannot be contiguous?

I think that if the HW requires more than 1 rectangle S/G_CROP must
not be implemented in the driver.


>>>
>>> I suggest to use SCANOUT target in your first implementation.
>>> Notice that as long you use something different from CROP and COMPOSE,
>>> you may define interactions and dependencies any way you want, like
>>> - if change of scanout area can affect composing area
>>> - interaction with format and frame size
>>> - check if scanout area can overlap, multi crops should be allowed to overlap
>>>
>>> See if it will be sufficient.
>>> See what kind of problems you encouter.
>>
>> On the process of the RFC I have used in our system all the
>> implementations. So far the only that has worked 100% is the solution
>> where the user present all the rectangles at once to the driver.
>>
>
> What were the problems with "the other" implementations?
> Maybe they could be fixed/improved to support
> the multirect feature without those huge API extensions.
> Please provide more practical details.
> I mean detailed HW limitations, and application needs.

It is mainly the first example. The driver can take more educated
guesses of what the user needs if it the user gives as much
information as possible. There were some setups what could not be
achieved by setting the rectangles one by one

>
>>>
>>>>
>>>> If yo dissable C or D, the configuration is ilegal and therefor the
>>>> driver will return -EINVAL. So once you are in ABCD you cannot go
>>>> back...
>>>>
>>>
>>> The driver must not return EINVAL as long as it is possible to
>>> adjust values passed by user to some valid configuration.
>>> If configuration is fixed then VIDIOC_S_SELECTION works
>>> effectively as VIDIOC_G_SELECTION.
>>>
>>>
>>>>
>>>>>
>>>>> The change of V4L2_SEL_TGT_XXX_SCANOUT0 would influence all consequtive
>>>>> rectangle by shifting them down or resetting them to height 0.
>>>>> Notice that as long as targets are driver specific you are free do define
>>>>> interaction between the targets.
>>>>>
>>>>> I hope that proposed solution is satisfactory.
>>>>
>>>> As stated before, please follow the previous comments on the rfc,
>>>> specially the ones from Hans.
>>>>
>>>>>
>>>>> BTW. I think that the HW trick you described is not cropping.
>>>>> By cropping you select which part of sensor area is going
>>>>> to be processed into compose rectangle in a buffer.
>>>>
>>>> You are selecting part of the sensor, therefore you are cropping the image.
>>>>
>>>
>>> Yes, it is the same as long as you use only one rectangle.
>>>
>>> If using more then 1 the situation may become more complex.
>>> Currently a pair of composing and crop rectangles are used to setup scaling.
>>>
>>> You have to introduce a new definition of scaling factor if multirect crops are introduced.
>>
>> The number of rectangles must be the same on the croping and on the
>> composing. The scaling factor is defined by the relation between
>> compose[x]/crop[x]
>>
>>
>
> What happens if scaling factor must be the same for all rectangles due to HW limitations?
> How to choose the proper scaling factor is crop and compose rectangles
> are passed in separate ioctls?

How do we solve that today? Why it is different?

>
>>>
>>> Moreover what happens if flipping or rotation is applied?
>>
>> Flipping is applied to the whole sensor.
>>
>>> Currently only the content of the rectangle is rotated/flipped.
>>> If you use multi rectangle then content of each rectangle must be
>>> rotated/flipped separatelly. Are you sure that your hardware can do this?
>>
>> In our hardware there is a mapping core that can map any pixel in any
>> location. So yes :)
>>
>>
>
> So your HW is so sophisticated that using single rectangle selections
> (with small tweaks) might be good enough to configure your hardware.
> Please provide more details.

We have a working window A. Every pixel (x,y) can be remapped to any
pixel in the range (0,y-A) (MAX_X,y+A).

So we still need multirectangles.

Also, the core if after the sensor, and the best benefit is gained if
the data is NOT read from the sensor.


>
>>>
>>> What about layout of composing rectangle inside output buffer?
>>> Should it reflect the layout of crop rectangle or all of them should be
>>> independent.
>>>
>>> Of course you can always say that everything is driver dependent :).
>>>
>>> Anyway, adding multirectangle selections for crop/compose is openning Pandora's Box.
>>
>> I dont see why. When we defined the API we didn't consider the case of
>> multiples WOI, now we fix this.
>>
>
> Adding multiple instances of crop rectangle is not an issue in selection API.
> This API was designed to allow new targets (or ids per target) to be added.
>
> The real problem is passing all rectangles at one time.
> Using mutlirect selection may help a bit in your special case
> at a cost of API obfuscation.
> It still will not solve problems with interactions between
> format/crop/compose/flipping/rotation.
>
> Again. I think that transactions are the solution to many of those problems.
> I would be much more generic solutions than all hypothetical extensions
> to selections.
>
>>>
>>>>>
>>>>> So technicaly you still insert the whole sensor area into the buffer.
>>>>
>>>> Only the lines/columns are read into the buffer.
>>>>
>>>
>>> Yes, but where into a buffer?
>>> Does you HW support setting destination for every crop rectangle?
>>
>> The dma supports up to 256 destinations.
>>
>
> Nice. So may encounter performance issues while trying to change
> one rectangle using S_SELECTION :).

Less that trying to change all the rectangles at once :P

>
>>>
>>>>> Only part of the buffer is actually updated. So there is no cropping
>>>>> (cropping area is equal to the whole array).
>>>>>
>>>>> Notice, that every 'cropping' area goes to different part of a buffer.
>>>>> So you would need also muliple targets for composing (!!!) and very long
>>>>> chapter in V4L2 doc describing interactions between muliple-rectangle
>>>>> crops and compositions. Good luck !!! :).
>>>>
>>>> It is not that difficult to describe.
>>>>
>>>> You need the same ammount of rectangles in cropping and in compossing.
>>>> Rectangle X in cropping will be mapped to rectangle X in compose.
>>>>
>>>>
>>>>> Currently it is a hell to understand and define interaction between
>>>>> single crop, and compose and unfamous VIDIOC_S_FMT and m2m madness.
>>>>
>>>> On m2m devices we are only lacking a s_fmt on the first buffer, as we
>>>> have discussed on the workshop. I think we only lack a good reference
>>>> model in vivi.
>>>>
>>>>
>>>
>>> I was not present on the workshop. Could you provide more details?
>>>
>>>>>
>>>>> I strongly recommend to use private selections.
>>>>> It will be much simpler to define, implement, and modify if needed.
>>>>
>>>> I think the private selections will lead to specific applications for
>>>> specific drivers and we cannot support all the configurations with
>>>> them. Also there is no way for an app to enumerate that capability.
>>>
>>> call VIDIOC_G_SELECTION to check if a given SCANOUT is supported?
>>> EINVAL means that it is not supported.
>>
>> And then we will have X zillions of SCANOUTS, that is really the Pandora box :)
>>
>>
>
> Not exactly. I preferred to introduce a bunch of SCANOUT targets to
> avoid wasting reserved fields in struct v4l2_selection. There is
> a plenty of unused bits in v4l2_selection::target. However, it is still
> ok to introduce v4l2_selection::index field. The API is still pretty nice.
>
> I preferred to use new target called SCANOUTs to avoid mixing
> this multirectangle feature is crop/compose interactions.
> This fear might be considered was little premature (or even paranoid).
>
> I just wanted to open this "Pandora's Box" in same safer
> and less demanding environment.
> First trying to implement everything using some
> non-conflicting hw-specific target called SCANOUT.
> If it'd worked than one would make it an alias to CROP in
> a newer version of the kernel.
> Making it a part of main API will force you to support
> its functionality (even erroneous) in all future versions of kernel
> with no possibility to change it.
>
>>>
>>>>
>>>>>
>>>>> BTW2. I think that the mulitple scanout areas can be modelled using
>>>>> media device API. Sakari may know how to do this.
>>>>
>>>> The areas are not read from the sensor. Therefore they can only be
>>>> proccessed on the sensor subdevice.
>>>>
>>>>>
>>>>>
>>>>> Ad 2. Extended rectangles.
>>>>> It is a good idea because v4l2_rect lacks any place for extensions.
>>>>> But before adding it to V4L2 API, I would like to know the examples
>>>>> of actual applications. Please, point drivers that actually need it.
>>>>
>>>> I have no need for it, but now that we are extending the selection API
>>>> it would be the perfect moment to add them.
>>>
>>> The perfect moment for adding something is when it is needed.
>>
>> If we add the extended rectangles. Why using a structure that we
>> cannot extend in the future?
>>
>
> Basically, because it is good enough.
> Introducting extended rectangles breaks symmetry between
> singlerect and multirect selections.
> v4l2_rect is used in other structures like v4l2_clip, v4l2_crop, v4l2_window, etc.
> Why not enhancing them with this new extended rectangle?
> Moreover, it violates "Rule of Least Surprise" from "Unix Philosophy".
> Why setting number of rectangles changes the structure type.
>
> Please, first find a real application that really cannot be implemented
> without introducing extended rectangle before extending the API.
>
> The extended rectangles are an orthogonal feature to multirect selections
> and can be added later.
>
> Regards,
>
> Tomasz Stanislawski
>


I am ok with not adding the ext_rect, but I really think we need the
multirect, even with transactions.

The multiple rectangle via id uses the same amount of reserved fields
but requires more ioctl calls.

The multiple rectangle will be more deterministic that setting one
rectangle at a time, because the driver has to take decisions for
every rectangle set.

With multiple rectangle we can create a bunch of function helpers that
can be used by the device driver, instead of reinventing the wheel
every-time.

Multirectangle is just the equivalent of multi control set/get.

Regards!


-- 
Ricardo Ribalda

Re: [RFC v3] [RFC] v4l2: Support for multiple selections

[Hans Verkuil]

On 12/19/2013 12:45 PM, Hans Verkuil wrote:
> Hi Sylwester,

That should be Tomasz, of course :-)

	Hans

Re: [RFC v3] [RFC] v4l2: Support for multiple selections

[Tomasz Stanislawski]

Hi Ricardo,

On 12/10/2013 10:46 AM, Ricardo Ribalda Delgado wrote:
> Hello Tomasz and Hans
> 
> On Thu, Nov 14, 2013 at 4:40 PM, Tomasz Stanislawski
> <t.stanislaws@samsung.com> wrote:
>> Hi Hans,
>>
>> On 11/14/2013 11:18 AM, Hans Verkuil wrote:
>>> Hi Tomasz,
>>>
>>> On 11/12/13 15:54, Tomasz Stanislawski wrote:
>>>> Hi Ricardo,
>>>> Sorry for a late reply. I've been 'offline' for the last two weeks.
>>>> Please refer to the comments below.
>>>>
>>
>> [snip]
>>
>>>>
>>>> As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on.
>>>> So activation of rectangle B (setting height to non-zero value) will enable
>>>> rectangle C with some default size. Moreover disabling rectangle B (setting height to 0)
>>>> may disable rectangle C automatically. I do not follow what is the problem here?
>>>
>>> The problem would be in a situation like this:
>>>
>>> ......
>>> .AA.B.
>>> ......   -->   AAB
>>> .C.DD.         CDD
>>> ......
>>>
>>> A-D are the rectangles you want to select. They are cropped as shown on the
>>> left and composed as shown on the right.
>>>
>>> >From what Ricardo told me the resulting composed image typically must be
>>> a proper rectangle without padding anywhere.
>>>
>>> Trying to add rectangles one at a time breaks down when adding C because
>>> the composition result is no longer a 'proper' rectangle. I don't see how
>>> you can set something like that up one rectangle at a time.
>>
>> I see the issue but I think that it is not a big problem.
>> Activating C forms a non-proper rectangle with A and B.
>> Therefore, driver must force enabling D to form a proper rectangle again.
>>
>> I mean that instead of enlarging C to sum of width of A and B,
>> the driver can implicitly activate D to ensure that A,B,C,D form a proper rectangle.
> 
> I think this will lead to X different drivers with X different behaviours.
> 

I sadly have to agree. All drivers behave differently.
But, is it really an issue?

As I understand a developer is responsible to assure that
future versions of driver works the same (in practical context)
when used by old applications.

The driver is allowed to interpret/adjust user calls.
This includes even ignoring them.

>>
>> The special target called V4L2_SEL_TGT_COMPOSE_PADDED was introduced
>> to inform application which part of a buffers if going to be modified
>> with some undefined value.
>>
>> I see nothing against setting a padded rectangle for C to a rectangle that
>> covers C and D or even the whole ABCD rectangle.
>> I think it could be a great application for PADDED target.
>> The application could easily detect which part of a buffer are affected.
>>
>> Even applications prepared to work with single crop devices
>> would still work after enabling multi crop mode.
>>
>> The setup of rectangles my look like this.
>>
>> ************************************************
>>
>> S_SELECTION(CROP0 = A)
>>
>> crop           compose
>> ----------------------
>> ......
>> .AA...
>> ......   -->   AA..
>> ......         ....
>> ......         ....
>>
>> G_SELECTION(COMPOSE0)
>>   AA..
>>   ....
>>   ....
>>
>>
>> G_SELECTION(COMPOSE0_PADDED)
>>   AA..
>>   ....
>>   ....
>>
>> ************************************************
>>
>> S_SELECTION(CROP1 = B)
>> ......
>> .AA.B.
>> ......   -->   AAB.
>> ......         ....
>> ......         ....
>>
>> G_SELECTION(COMPOSE0)
>>   AA..
>>   ....
>>   ....
>>
>> G_SELECTION(COMPOSE0_PADDED)
>>   AAA.
>>   ....
>>   ....
>>
>> G_SELECTION(COMPOSE1)
>>   ..B.
>>   ....
>>   ....
>>
>> G_SELECTION(COMPOSE1_PADDED)
>>   BBB.
>>   ....
>>   ....
>>
>>
>> ************************************************
>>
>> S_SELECTION(CROP2 = C) - D is activated implicitly
>> ......
>> .AA.B.
>> .C.DD.   -->   AAB.
>> ......         CDD.
>> ......         ....
>>
>> G_SELECTION(COMPOSE0_PADDED)
>>
>>   AAA.
>>   AAA.
>>   ....
>>
>> G_SELECTION(COMPOSE2)
>>   ....
>>   C...
>>   ....
>>
>>
>> G_SELECTION(COMPOSE2_PADDED)
>>   CCC.
>>   CCC.
>>   ....
>>
>> G_SELECTION(COMPOSE3)
>>   ....
>>   .DD.
>>   ....
>>
>>
>> G_SELECTION(COMPOSE3_PADDED)
>>   DDD.
>>   DDD.
>>   ....
>>
>> One may argue that all this logic is unnecessary after adding support
>> for multirect selections.
>> So, I kindly ask what should happen if someone call S_SELECTION
>> (in multirect mode) passing THREE rectangles A, B, and C (not D) ?
>>
>> The driver must adjust rectangles to some valid value. So it can
>> increase width of C or implicitly activate D or disable C.
>> I think that the best solution is activating D because
>> it allows to set size of C to the value closest to requested one.
>> Therefore logic for implicit activation of D should be implemented anyway.
> 
> You are assuming that the user will send you the rectangles in an
> order that "makes sense", but it is not always the case. On the other
> hand if you have all the rectangles, you can ALWAYS make the better
> decisition.
> 

Yes. I assume that a user will send rectangles in the right order.
It stated that changing order of V4L2 operations may result
is different configuration. So I do not see a problem that
a specific configuration can be achieved on specific hardware
only if operations are performed in specific order.

> On the other
> hand if you have all the rectangles, you can ALWAYS make the better
> decisition.

I agree completely.
To make a best decision you must have "all rectangles". It means both
cropping/composing and format which is also dependent of S_CROP (see V4L2 doc).
Moreover, having controls like rotation and flipping may be helpful.

So passing only crop rectangles is only a part of information needed to make
a best decision. Therefore, multirectangle selections are only a partial solution
to the problem of configuration. It is a workaround for the specific feature
of some hardware. Therefore, before merging multirect selections you must PROVE
that the existing solutions are not satisfactory in your case.
That you truly cannot use singlerect selections to configure
your piece of hardware (not some abstract HW).
You cannot say that it is impossible because you do not know how to do it.

I just want to avoid loosing time and obfuscating the API for a feature
that will not be even sufficient for future needs.

>>
>>>
>>> It makes much more sense to set everything up at once.
>>
>> I agree that it is better to set everything at once.
>> But I strongly believe that transactions are the proper way to achieve that.
>>
>> Not multirectangle selections.
>>
> 
> As I said before the transaction is something easier said than made.
> What happens if multiple users starts a transaction? What happen if in
> a transaction of 10 itmes, item 7 needs a readjusment by the driver?
> 

It is a very good question.
I have some ideas about transactions.
First, that there should some form of temporary configuration buffer.
This buffer could be singular. In such a case VIDIOC_BEGIN would
be a blocking ioctl.
Or every handle/thread could have its own configuration buffer.
The buffer would be processed at VIDIOC_COMMIT.
The values would be adjusted against HW limitations, current configuration
and the buffer itself.
Finally, all properties would be applied to HW.

As I said in previous email. There is a huge space for ideas.

> The selection API cannot cover a type of selection, therefore it
> should be fixed. Especially when it is something as easy as the
> propossed RFC.

No. It is by no means easy. It just pretends to be easy.
There is a very complex logic hidden behind this extension.

Just add v4l2_selection::index if you want to avoid utilizing bits
from v4l2_selection::target. Index is nice because 0 would
refer to first and usually only one crop rectangle.
Those are relatively simple to add with few lines of patch.

You can even use CROP target instead of SCANOUT.
You will responsible for fighting with nightmare of
backward compatibility and messy business logic hidden
in driver code.

I am strongly against passing all rectangles at the same time.
The simultaneous configuration should be a separate part of
V4L2 API.

> 
> 
>> It obfuscates API. It only pretends to fix a problem with applying
>> a part of configuration atomically.
>>
>>>
>>> BTW, what probably wasn't clear from Ricardo's explanation is that for every
>>> crop rectangle you must have a corresponding compose rectangle so that you
>>> know where to DMA it to.
>>>
>>> Your next question will be that it is a real problem that you can't set
>>> crop and compose simultaneously, and you are right about that. Read on for
>>> that... :-)
>>>
>>>> Hmm. I think that the real problem is much different.
>>>> Not how to set up rectangles atomically but rather
>>>> how do anything non-trivial atomically in V4L2.
>>>>
>>>> It would be very nice to have crop/compose and format configured at the same time.
>>>> However, current version of V4L2 API does not support that.
>>>>
>>>> Setting multiple crop rectangles may help a bit for only this little case.
>>>> But how would like to set multicrop rectangles and multicompose rectangle atomically.
>>>
>>> Why can't we extend the selection API a bit? How about this:
>>>
>>> #define V4L2_SEL_TGT_CROP_COMPOSE    0x0200
>>>
>>> struct v4l2_selection {
>>>         __u32                   type;
>>>         __u32                   target;
>>>         __u32                   flags;
>>>       union {
>>>               struct v4l2_rect        r;
>>>               struct v4l2_ext_rect    *pr;
>>>       };
>>>         __u32                   flags2;
>>>       union {
>>>               struct v4l2_rect        r2;
>>>               struct v4l2_ext_rect    *pr2;
>>>       };
>>>       __u32                   rectangles;
>>>         __u32                   reserved[3];
>>> };
>>>
>>> If the target is CROP_COMPOSE then flags & r define the crop rectangle, and
>>> flags2 & r2 define the compose rectangle. That way you can set them both
>>> atomically.
>>
>> I do not like this idea because:
>> - mix crop and composing generating semi-related cropcompose
> Now looking with some perspective the selection API, dont you think
> that the crop and compose are related enough to be set at the same
> time?
> 

Technically everything in V4L2 is more or less related.

I expect that in future we may see Frankenstein's monster that
sets up multirectangle crops, compose, format and some important controls
using single ioctl with a huge struct.
Probably, this would sufficient ... for some time.

I am simply tired of all those "almost solutions".
The API should be modular. It should be made of simple
parts that do one thing well.

>> - obfuscates the API even more
> Agreed, but we can find better naming.
> 
>> - wastes a lot of reserved fields
> Only 2

There are new fields named flags2 (1 word), second union (4 words),
rectangles (1 word). So 6 extra words are wasted.

> 
>> - what if someone would like to use separate 'flags' for each rectangle
>>   ... this could be a nice feature anyway :)
> Then he uses the ext_rect :P
> 

Or use only single rectangle selections.

>>
>> This remains me the proposition from early days of selection API.
>>
>> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/28945
>>
>> Refer to point 4 where v4l2_crop2 was mentioned.
>>
>>>
>>> I would propose that the interaction with S_FMT is as follows: if S_FMT
>>> defines a rectangle < then the current compose rectangle, then the
>>> composition (and optionally crop) rectangle is reset. If it is >=, then
>>> all is fine.
>>
>> There is a issue here. Long time ago it was stated that S_FMT
>> should result in configuration where data are processed (scaled) into
>> the 'whole picture'.
>>
>> It means that a COMPOSE rectangle should be reset after very S_FMT.
>>
>>>
>>> If a new compose rectangle doesn't fit in the S_FMT rectangle then it should
>>> adapt the S_FMT rectangle to make it fit (as long as HW constraints are
>>> obeyed of course).
>>
>> What happens if REQBUFS was called between S_FMT and S_SELECTION?
>>
>>>
>>> This sequence will obey the rules of V4L2 as well: the last operation takes
>>> precedence over older operations. So setting S_FMT allow the driver to
>>> change cropping/composing to get as close to the desired format as possible.
>>>
>>>> How to define which crop rectangle refers to which to which compose rectangle.
>>>
>>> By setting the crop and compose selections at the same time and of the same
>>> size you can map each crop selection to a compose selection. It's all atomic
>>> as well.
>>>
>>
>> I think that adding an id field to v4l2_selection might be a good alternative
>> to introduction of numbered targets like V4L2_SEL_TGT_COMPOSE0, 1, ...
>>
>> It will add support for all multirectangle features as cost of
>> sacrifice of single reserved field. Moreover id field might be
>> very useful for other selection targets like FOCUS or FACE.
>>
>> Other idea might to using some bits of 'flags' field to carry rectangle id.
>>
>>>>
>>>> What to do if one would like to change only 3rd crop rectangle?
>>>>
>>>> Introduce rectangle id into v4l2_ext_rect?
>>>> Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION?
>>>> Is it really scalable?
>>>
>>> Why not?
>>>
>>
>> Because it is not scalable.
>> An application has to use 2 syscall instead of 1.
>> Unnecessary copying has to be performed.
> 
> And setting 8 rectangles will mean 8 iocts instead of 2.

The very similar amount of data is copied in both cases.

sizeof(v4l2_selection) + N * sizeof (v4l2_ext_rect)
	OR
N * sizeof(v4l2_selection)

> 
> The user can also keep a copy of what has been set and there will only
> be 1 ioctl.
> 

Notice that user must update this copy every time S_SELECTION is called.
Moreover it must always use this array whenever calling S_SELECTION.

If one use single-rectangle selections there is no need to keep all
those bookkeeping data. Nor assure its consistency.

>>
>>>>
>>>> Multirectangle targets may seam to be a solution but I think it is not.
>>>>
>>>> I think that atomic transactions are what is really needed in V4L2.
>>>> Other ideas like try-context from subdev API may also help.
>>>>
>>>> It will be nice to have something like
>>>>
>>>> VIDIOC_BEGIN
>>>>   VIDIOC_S_SELECTION (target = CROP)
>>>>   VIDIOC_S_SELECTION (target = COMPOSE)
>>>>   VIDIOC_S_FMT
>>>>   VIDIOC_S_CTRL
>>>> VIDIOC_COMMIT
>>>
>>> I don't think S_FMT is needed here: it's something you set up at the
>>> beginning and don't touch afterwards.
>>
>> One could say the same about compose, crop, ctrls.
>> The problem are the interactions between those objects.
>> What is dependent on what. What can change what.
>> How to reach a valid state in all cases not preventing
>> some valid configurations from being unreachable.
>>
>>>
>>> Wouldn't VIDIOC_S_SELECTION (target = CROP_COMPOSE) go a long way to
>>> solving these atomicity problems?
>>>
>>> Another nice feature that can be added to the selection API is to
>>> add a field to refer to a frame sequence number or a v4l2_buffer
>>> index (the latter is probably better): this would make it easy to
>>> apply an atomic crop/compose change to easily implement things like
>>> digital zoom or moving windows around. We could do something similar
>>> for controls.
>>>
>>> This would also solve the problem of assigning a per-buffer (or per-frame)
>>> configuration, something that libcamera2/3 needs.
>>>
>>
>> Introduce transactions with frame sequence numbers.
>> I proposed this in Cambridge in 2012.
>>
>>>>
>>>> and call a bunch of VIDIOC_G_* to see what really was applied.
>>>>
>>>>>> I have an auxiliary question. Do you have to set all rectangles
>>>>>> at once? can you set up them one by one?
>>>>>
>>>>> Also if you tell the driver what exact configuration you will need, it
>>>>> will provide you with the closest possible confuration, that cannot be
>>>>
>>>> s/cannot be done/may not be 'doable'
>>>>
>>>>> done if you provide rectangle by rectangle.
>>>>>
>>>>>> But how to deal with multiple rectangles?
>>>>>
>>>>> Multiple rectangles is a desired feature, please take a look to the
>>>>> presentation on the workshop.
>>>>>
>>>>
>>>> I agree that it may be useful. I just think that multirectangle selections
>>>> are needed to add support for such a feature.
>>>
>>> I don't follow, isn't that what this proposal adds?
>>>
>>
>> s/are needed/are not needed/
>>
>> Sorry for confusion.
>>
>>>>
>>>>>>
>>>>>> Anyway, first try to define something like this:
>>>>>>
>>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0  V4L2_SEL_TGT_PRIVATE
>>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT  (V4L2_SEL_TGT_XXX_SCANOUT0 + 1)
>>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS  (V4L2_SEL_TGT_XXX_SCANOUT0 + 2)
>>>>>>
>>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0  (V4L2_SEL_TGT_PRIVATE + 16)
>>>>>> ...
>>>>>>
>>>>>> -- OR-- parametrized macros similar to one below:
>>>>>>
>>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n))
>>>>>>
>>>>>> The application could setup all scanout areas one-by-one.
>>>>>> By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array.
>>>>>> The height of all consecutive area would be 0. This means disabling
>>>>>> them effectively.
>>>>>
>>>>> Lets say rectangle A + B + C +D is legal, and A +B is also legal. You
>>>>> are in ABCD and you want to go to AB. How can you do it?
>>>>
>>>> Just set C to have height 0. It will disable D automatically.
>>>
>>> It's the other direction that's the problem: how to go from A + B to
>>> A + B + C + D if ABC is illegal.
>>>
>>>> BTW. How are you going to emulate S_CROP by selection API
>>>> if you must use at least two rectangles (A + B) ?
>>>
>>> S_CROP will always just set one rectangle A. So if you had multiple
>>> rectangles in your selection then after S_CROP you'll have only one.
>>
>> So S_CROP should change only A.
>> What happens if someone already set device to work with 4 rectangles.
>> Then calls S_CROP that modifies A in such a way that ABCD no longer forms
>> a proper rectangle.
>>
>> What should happen?
>> - ignore S_CROP, return previous settings, or
>> - modify B,C,D to form a proper rectangle, or
>> - disable B,C,D, or ?
> 
> It will be the same as setting a multicrop with 1 rectangle.
> 

It not the answer to my question.
I expect that your answer refers to "disable B,C,D" variant, doesn't it?

>>
>>>
>>> I'm assuming that the hw will always support a single crop rectangle.
>>> If not (extraordinarily unlikely), then S_CROP should just return an
>>> error.
>>
>> I agree that it is highly unlikely. I am just trying to figure out
>> what should happen when calling S_CROP or single-rectangle S_SELECTION
>> after calling multirect S_SELECTION.
> 
> The sensor is configured with 1 rectangle.
> 
>>
>>>
>>>> I suggest to use SCANOUT target in your first implementation.
>>>> Notice that as long you use something different from CROP and COMPOSE,
>>>> you may define interactions and dependencies any way you want, like
>>>> - if change of scanout area can affect composing area
>>>> - interaction with format and frame size
>>>> - check if scanout area can overlap, multi crops should be allowed to overlap
>>>
>>> I see no reason for this. A CROP_COMPOSE target does all you need.
>>
>> I think that CROP and COMPOSE is all you need.
>>
>>>
>>> Note that to set up N rectangles where N differs from the current
>>> rectangle count you do need to use CROP_COMPOSE. You can't setup CROP
>>> and COMPOSE rectangles independently since having N crop rectangles
>>> and M compose rectangles makes no sense. If you don't change the
>>> number of rectangles, then you can still use CROP and COMPOSE as is.
>>>
>>
>> I am afraid that you are trying to kill too many birds with one stone.
>> I think it is a very good moment to use bazooka.
>>
>> Transactions enhanced with frame numbers will handle a lot of issues with V4L2.
>>
>> Moreover it will allow to support some nice effects like:
>> - multiple exposures during HDR photography
>> - continuous online changes preview windows
>> - flash at a specific frame
>> - change format during video streaming to grab a high-resolution picture
>> - support multi rectangle cropping and composing :)
> 
> Even with transactions I humble believe that we should have a
> multirectangle call. :)
> 
> 

I humbly believe that single rectangle selections are sufficient.
Please, try to convince me that I am wrong.
If I was sure that this multirect feature cannot be implemented
using single-rect selections I will support this extension.
Even if this feature is not future-proof as mentioned earlier.

We may let transactions to be a part of V4L3 :).

By now, I am still not convinced.

>>
>>>>
>>
>> [snip]
>>
>>
>>>>> I have no need for it, but now that we are extending the selection API
>>>>> it would be the perfect moment to add them.
>>>>
>>>> The perfect moment for adding something is when it is needed.
>>>
>>> We need to add support for multiple rectangles, so we added a pointer.
>>>
>>>> The bad idea is preventing something from being added too early.
>>>
>>> But if we make it a pointer to v4l2_rect, then we can *never* add
>>> additional information to each rectangle. Whenever we add new APIs
>>> or features we try to allow for future extensions (with mixed
>>> success...), so now is the time to do so for v4l2_rect.
>>>
>>> Something like this:
>>>
>>> struct v4l2_sel_rect {
>>>       struct v4l2_rect r;
>>>       __u32 reserved[4];
>>> };
>>>
>>> would be fine by me as well. It's an additional indirection, but it
>>> also makes it easier to go from a v4l2_rect to a v4l2_sel_rect.
>>
>> We can always migrate to extended rectangle by modifying the structure to
>>
>> struct v4l2_selection {
>>          __u32                   type;
>>          __u32                   target;
>>          __u32                   flags;
>>          union {
>>                struct v4l2_rect        r;
>>                struct v4l2_ext_rect    rext;
>>          };
>>          __u32                   reserved[X];
>> };
>>
>> and adding V4L2_SEL_FLAG_EXT_RECT flag.
> 
> Dont you think this is a bit confusing?
> 
> 
> 

Why? It is much less confusing then union type
dependent on value of v4l2_selection::rectangles.

You can use pointer to struct v4l2_ext_rect
instead of composition. Then only 1 bit could
be used to support extended rectangles.

Regards,
Tomasz Stanislawski

Re: [RFC v3] [RFC] v4l2: Support for multiple selections

[Ricardo Ribalda Delgado]

Hello Tomasz

On Thu, Dec 19, 2013 at 1:34 PM, Tomasz Stanislawski
<t.stanislaws@samsung.com> wrote:
> Hi Ricardo,
>
> On 12/10/2013 10:46 AM, Ricardo Ribalda Delgado wrote:
>> Hello Tomasz and Hans
>>
>> On Thu, Nov 14, 2013 at 4:40 PM, Tomasz Stanislawski
>> <t.stanislaws@samsung.com> wrote:
>>> Hi Hans,
>>>
>>> On 11/14/2013 11:18 AM, Hans Verkuil wrote:
>>>> Hi Tomasz,
>>>>
>>>> On 11/12/13 15:54, Tomasz Stanislawski wrote:
>>>>> Hi Ricardo,
>>>>> Sorry for a late reply. I've been 'offline' for the last two weeks.
>>>>> Please refer to the comments below.
>>>>>
>>>
>>> [snip]
>>>
>>>>>
>>>>> As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on.
>>>>> So activation of rectangle B (setting height to non-zero value) will enable
>>>>> rectangle C with some default size. Moreover disabling rectangle B (setting height to 0)
>>>>> may disable rectangle C automatically. I do not follow what is the problem here?
>>>>
>>>> The problem would be in a situation like this:
>>>>
>>>> ......
>>>> .AA.B.
>>>> ......   -->   AAB
>>>> .C.DD.         CDD
>>>> ......
>>>>
>>>> A-D are the rectangles you want to select. They are cropped as shown on the
>>>> left and composed as shown on the right.
>>>>
>>>> >From what Ricardo told me the resulting composed image typically must be
>>>> a proper rectangle without padding anywhere.
>>>>
>>>> Trying to add rectangles one at a time breaks down when adding C because
>>>> the composition result is no longer a 'proper' rectangle. I don't see how
>>>> you can set something like that up one rectangle at a time.
>>>
>>> I see the issue but I think that it is not a big problem.
>>> Activating C forms a non-proper rectangle with A and B.
>>> Therefore, driver must force enabling D to form a proper rectangle again.
>>>
>>> I mean that instead of enlarging C to sum of width of A and B,
>>> the driver can implicitly activate D to ensure that A,B,C,D form a proper rectangle.
>>
>> I think this will lead to X different drivers with X different behaviours.
>>
>
> I sadly have to agree. All drivers behave differently.
> But, is it really an issue?

Well, I would say that we have APIs to abstract HW from SW. I expect
the same behaviour on every mouse I connect on my computer :)

>
> As I understand a developer is responsible to assure that
> future versions of driver works the same (in practical context)
> when used by old applications.
>
> The driver is allowed to interpret/adjust user calls.
> This includes even ignoring them.
>
>>>
>>> The special target called V4L2_SEL_TGT_COMPOSE_PADDED was introduced
>>> to inform application which part of a buffers if going to be modified
>>> with some undefined value.
>>>
>>> I see nothing against setting a padded rectangle for C to a rectangle that
>>> covers C and D or even the whole ABCD rectangle.
>>> I think it could be a great application for PADDED target.
>>> The application could easily detect which part of a buffer are affected.
>>>
>>> Even applications prepared to work with single crop devices
>>> would still work after enabling multi crop mode.
>>>
>>> The setup of rectangles my look like this.
>>>
>>> ************************************************
>>>
>>> S_SELECTION(CROP0 = A)
>>>
>>> crop           compose
>>> ----------------------
>>> ......
>>> .AA...
>>> ......   -->   AA..
>>> ......         ....
>>> ......         ....
>>>
>>> G_SELECTION(COMPOSE0)
>>>   AA..
>>>   ....
>>>   ....
>>>
>>>
>>> G_SELECTION(COMPOSE0_PADDED)
>>>   AA..
>>>   ....
>>>   ....
>>>
>>> ************************************************
>>>
>>> S_SELECTION(CROP1 = B)
>>> ......
>>> .AA.B.
>>> ......   -->   AAB.
>>> ......         ....
>>> ......         ....
>>>
>>> G_SELECTION(COMPOSE0)
>>>   AA..
>>>   ....
>>>   ....
>>>
>>> G_SELECTION(COMPOSE0_PADDED)
>>>   AAA.
>>>   ....
>>>   ....
>>>
>>> G_SELECTION(COMPOSE1)
>>>   ..B.
>>>   ....
>>>   ....
>>>
>>> G_SELECTION(COMPOSE1_PADDED)
>>>   BBB.
>>>   ....
>>>   ....
>>>
>>>
>>> ************************************************
>>>
>>> S_SELECTION(CROP2 = C) - D is activated implicitly
>>> ......
>>> .AA.B.
>>> .C.DD.   -->   AAB.
>>> ......         CDD.
>>> ......         ....
>>>
>>> G_SELECTION(COMPOSE0_PADDED)
>>>
>>>   AAA.
>>>   AAA.
>>>   ....
>>>
>>> G_SELECTION(COMPOSE2)
>>>   ....
>>>   C...
>>>   ....
>>>
>>>
>>> G_SELECTION(COMPOSE2_PADDED)
>>>   CCC.
>>>   CCC.
>>>   ....
>>>
>>> G_SELECTION(COMPOSE3)
>>>   ....
>>>   .DD.
>>>   ....
>>>
>>>
>>> G_SELECTION(COMPOSE3_PADDED)
>>>   DDD.
>>>   DDD.
>>>   ....
>>>
>>> One may argue that all this logic is unnecessary after adding support
>>> for multirect selections.
>>> So, I kindly ask what should happen if someone call S_SELECTION
>>> (in multirect mode) passing THREE rectangles A, B, and C (not D) ?
>>>
>>> The driver must adjust rectangles to some valid value. So it can
>>> increase width of C or implicitly activate D or disable C.
>>> I think that the best solution is activating D because
>>> it allows to set size of C to the value closest to requested one.
>>> Therefore logic for implicit activation of D should be implemented anyway.
>>
>> You are assuming that the user will send you the rectangles in an
>> order that "makes sense", but it is not always the case. On the other
>> hand if you have all the rectangles, you can ALWAYS make the better
>> decisition.
>>
>
> Yes. I assume that a user will send rectangles in the right order.
> It stated that changing order of V4L2 operations may result
> is different configuration. So I do not see a problem that
> a specific configuration can be achieved on specific hardware
> only if operations are performed in specific order.

What is the right order? Why the software should know anything about
the hardware to procure a proper image?

>
>> On the other
>> hand if you have all the rectangles, you can ALWAYS make the better
>> decisition.
>
> I agree completely.
> To make a best decision you must have "all rectangles". It means both
> cropping/composing and format which is also dependent of S_CROP (see V4L2 doc).
> Moreover, having controls like rotation and flipping may be helpful.
>
> So passing only crop rectangles is only a part of information needed to make
> a best decision. Therefore, multirectangle selections are only a partial solution
> to the problem of configuration. It is a workaround for the specific feature
> of some hardware. Therefore, before merging multirect selections you must PROVE
> that the existing solutions are not satisfactory in your case.
> That you truly cannot use singlerect selections to configure
> your piece of hardware (not some abstract HW).
> You cannot say that it is impossible because you do not know how to do it.

Please take a look to the example from my previous mail, which is a real camera.

If the total size depends of the number of rectangles I can not see
how to do it right.


>
> I just want to avoid loosing time and obfuscating the API for a feature
> that will not be even sufficient for future needs.
>
>>>
>>>>
>>>> It makes much more sense to set everything up at once.
>>>
>>> I agree that it is better to set everything at once.
>>> But I strongly believe that transactions are the proper way to achieve that.
>>>
>>> Not multirectangle selections.
>>>
>>
>> As I said before the transaction is something easier said than made.
>> What happens if multiple users starts a transaction? What happen if in
>> a transaction of 10 itmes, item 7 needs a readjusment by the driver?
>>
>
> It is a very good question.
> I have some ideas about transactions.
> First, that there should some form of temporary configuration buffer.
> This buffer could be singular. In such a case VIDIOC_BEGIN would
> be a blocking ioctl.
> Or every handle/thread could have its own configuration buffer.
> The buffer would be processed at VIDIOC_COMMIT.
> The values would be adjusted against HW limitations, current configuration
> and the buffer itself.
> Finally, all properties would be applied to HW.
>
> As I said in previous email. There is a huge space for ideas.

But even with transactions we need a way to define multiple rectangles.

If we provide common api we will be able to reuse helper functions and
have similar behaviour on all the drivers.
>
>> The selection API cannot cover a type of selection, therefore it
>> should be fixed. Especially when it is something as easy as the
>> propossed RFC.
>
> No. It is by no means easy. It just pretends to be easy.
> There is a very complex logic hidden behind this extension.

Where is the complexity?

>
> Just add v4l2_selection::index if you want to avoid utilizing bits
> from v4l2_selection::target. Index is nice because 0 would
> refer to first and usually only one crop rectangle.
> Those are relatively simple to add with few lines of patch.

The current rfc is only 150 lines of code, including doc.

>
> You can even use CROP target instead of SCANOUT.
> You will responsible for fighting with nightmare of
> backward compatibility and messy business logic hidden
> in driver code.
>
> I am strongly against passing all rectangles at the same time.
> The simultaneous configuration should be a separate part of
> V4L2 API.

What I want to explain is that we are not setting rectangle0,
rectangle1, rectangle2... we are setting the readout configuration
which is atomic. It would be the same as separate the gain in
gain_bit0, gain_bit1, gain_bit2...

>
>>
>>
>>> It obfuscates API. It only pretends to fix a problem with applying
>>> a part of configuration atomically.
>>>
>>>>
>>>> BTW, what probably wasn't clear from Ricardo's explanation is that for every
>>>> crop rectangle you must have a corresponding compose rectangle so that you
>>>> know where to DMA it to.
>>>>
>>>> Your next question will be that it is a real problem that you can't set
>>>> crop and compose simultaneously, and you are right about that. Read on for
>>>> that... :-)
>>>>
>>>>> Hmm. I think that the real problem is much different.
>>>>> Not how to set up rectangles atomically but rather
>>>>> how do anything non-trivial atomically in V4L2.
>>>>>
>>>>> It would be very nice to have crop/compose and format configured at the same time.
>>>>> However, current version of V4L2 API does not support that.
>>>>>
>>>>> Setting multiple crop rectangles may help a bit for only this little case.
>>>>> But how would like to set multicrop rectangles and multicompose rectangle atomically.
>>>>
>>>> Why can't we extend the selection API a bit? How about this:
>>>>
>>>> #define V4L2_SEL_TGT_CROP_COMPOSE    0x0200
>>>>
>>>> struct v4l2_selection {
>>>>         __u32                   type;
>>>>         __u32                   target;
>>>>         __u32                   flags;
>>>>       union {
>>>>               struct v4l2_rect        r;
>>>>               struct v4l2_ext_rect    *pr;
>>>>       };
>>>>         __u32                   flags2;
>>>>       union {
>>>>               struct v4l2_rect        r2;
>>>>               struct v4l2_ext_rect    *pr2;
>>>>       };
>>>>       __u32                   rectangles;
>>>>         __u32                   reserved[3];
>>>> };
>>>>
>>>> If the target is CROP_COMPOSE then flags & r define the crop rectangle, and
>>>> flags2 & r2 define the compose rectangle. That way you can set them both
>>>> atomically.
>>>
>>> I do not like this idea because:
>>> - mix crop and composing generating semi-related cropcompose
>> Now looking with some perspective the selection API, dont you think
>> that the crop and compose are related enough to be set at the same
>> time?
>>
>
> Technically everything in V4L2 is more or less related.
>
> I expect that in future we may see Frankenstein's monster that
> sets up multirectangle crops, compose, format and some important controls
> using single ioctl with a huge struct.
> Probably, this would sufficient ... for some time.
>
> I am simply tired of all those "almost solutions".
> The API should be modular. It should be made of simple
> parts that do one thing well.

Agree, but every ioctl must do something complete.

>
>>> - obfuscates the API even more
>> Agreed, but we can find better naming.
>>
>>> - wastes a lot of reserved fields
>> Only 2
>
> There are new fields named flags2 (1 word), second union (4 words),
> rectangles (1 word). So 6 extra words are wasted.
>
>>
>>> - what if someone would like to use separate 'flags' for each rectangle
>>>   ... this could be a nice feature anyway :)
>> Then he uses the ext_rect :P
>>
>
> Or use only single rectangle selections.
>
>>>
>>> This remains me the proposition from early days of selection API.
>>>
>>> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/28945
>>>
>>> Refer to point 4 where v4l2_crop2 was mentioned.
>>>
>>>>
>>>> I would propose that the interaction with S_FMT is as follows: if S_FMT
>>>> defines a rectangle < then the current compose rectangle, then the
>>>> composition (and optionally crop) rectangle is reset. If it is >=, then
>>>> all is fine.
>>>
>>> There is a issue here. Long time ago it was stated that S_FMT
>>> should result in configuration where data are processed (scaled) into
>>> the 'whole picture'.
>>>
>>> It means that a COMPOSE rectangle should be reset after very S_FMT.
>>>
>>>>
>>>> If a new compose rectangle doesn't fit in the S_FMT rectangle then it should
>>>> adapt the S_FMT rectangle to make it fit (as long as HW constraints are
>>>> obeyed of course).
>>>
>>> What happens if REQBUFS was called between S_FMT and S_SELECTION?
>>>
>>>>
>>>> This sequence will obey the rules of V4L2 as well: the last operation takes
>>>> precedence over older operations. So setting S_FMT allow the driver to
>>>> change cropping/composing to get as close to the desired format as possible.
>>>>
>>>>> How to define which crop rectangle refers to which to which compose rectangle.
>>>>
>>>> By setting the crop and compose selections at the same time and of the same
>>>> size you can map each crop selection to a compose selection. It's all atomic
>>>> as well.
>>>>
>>>
>>> I think that adding an id field to v4l2_selection might be a good alternative
>>> to introduction of numbered targets like V4L2_SEL_TGT_COMPOSE0, 1, ...
>>>
>>> It will add support for all multirectangle features as cost of
>>> sacrifice of single reserved field. Moreover id field might be
>>> very useful for other selection targets like FOCUS or FACE.
>>>
>>> Other idea might to using some bits of 'flags' field to carry rectangle id.
>>>
>>>>>
>>>>> What to do if one would like to change only 3rd crop rectangle?
>>>>>
>>>>> Introduce rectangle id into v4l2_ext_rect?
>>>>> Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION?
>>>>> Is it really scalable?
>>>>
>>>> Why not?
>>>>
>>>
>>> Because it is not scalable.
>>> An application has to use 2 syscall instead of 1.
>>> Unnecessary copying has to be performed.
>>
>> And setting 8 rectangles will mean 8 iocts instead of 2.
>
> The very similar amount of data is copied in both cases.
>
> sizeof(v4l2_selection) + N * sizeof (v4l2_ext_rect)
>         OR
> N * sizeof(v4l2_selection)

But 8 times more iocts, and ioctl N can change a rectangle set in ioct
N-1. So in order to set up my readout I will probably have to

1) set rectangle 0
2) set retangle 1
3) get rectangle 0, to make sure it is still ok
4) set rectangle 2
5) get rectangle 1
6) get rectangle 0 ....

>
>>
>> The user can also keep a copy of what has been set and there will only
>> be 1 ioctl.
>>
>
> Notice that user must update this copy every time S_SELECTION is called.
> Moreover it must always use this array whenever calling S_SELECTION.
>
> If one use single-rectangle selections there is no need to keep all
> those bookkeeping data. Nor assure its consistency.

It is in your example where the user needs to keep a whole copy to
avoid N ioctsl. In the propossed api if the user doesnt want to keep a
copy he only needs 1 ioclt.


>
>>>
>>>>>
>>>>> Multirectangle targets may seam to be a solution but I think it is not.
>>>>>
>>>>> I think that atomic transactions are what is really needed in V4L2.
>>>>> Other ideas like try-context from subdev API may also help.
>>>>>
>>>>> It will be nice to have something like
>>>>>
>>>>> VIDIOC_BEGIN
>>>>>   VIDIOC_S_SELECTION (target = CROP)
>>>>>   VIDIOC_S_SELECTION (target = COMPOSE)
>>>>>   VIDIOC_S_FMT
>>>>>   VIDIOC_S_CTRL
>>>>> VIDIOC_COMMIT
>>>>
>>>> I don't think S_FMT is needed here: it's something you set up at the
>>>> beginning and don't touch afterwards.
>>>
>>> One could say the same about compose, crop, ctrls.
>>> The problem are the interactions between those objects.
>>> What is dependent on what. What can change what.
>>> How to reach a valid state in all cases not preventing
>>> some valid configurations from being unreachable.
>>>
>>>>
>>>> Wouldn't VIDIOC_S_SELECTION (target = CROP_COMPOSE) go a long way to
>>>> solving these atomicity problems?
>>>>
>>>> Another nice feature that can be added to the selection API is to
>>>> add a field to refer to a frame sequence number or a v4l2_buffer
>>>> index (the latter is probably better): this would make it easy to
>>>> apply an atomic crop/compose change to easily implement things like
>>>> digital zoom or moving windows around. We could do something similar
>>>> for controls.
>>>>
>>>> This would also solve the problem of assigning a per-buffer (or per-frame)
>>>> configuration, something that libcamera2/3 needs.
>>>>
>>>
>>> Introduce transactions with frame sequence numbers.
>>> I proposed this in Cambridge in 2012.
>>>
>>>>>
>>>>> and call a bunch of VIDIOC_G_* to see what really was applied.
>>>>>
>>>>>>> I have an auxiliary question. Do you have to set all rectangles
>>>>>>> at once? can you set up them one by one?
>>>>>>
>>>>>> Also if you tell the driver what exact configuration you will need, it
>>>>>> will provide you with the closest possible confuration, that cannot be
>>>>>
>>>>> s/cannot be done/may not be 'doable'
>>>>>
>>>>>> done if you provide rectangle by rectangle.
>>>>>>
>>>>>>> But how to deal with multiple rectangles?
>>>>>>
>>>>>> Multiple rectangles is a desired feature, please take a look to the
>>>>>> presentation on the workshop.
>>>>>>
>>>>>
>>>>> I agree that it may be useful. I just think that multirectangle selections
>>>>> are needed to add support for such a feature.
>>>>
>>>> I don't follow, isn't that what this proposal adds?
>>>>
>>>
>>> s/are needed/are not needed/
>>>
>>> Sorry for confusion.
>>>
>>>>>
>>>>>>>
>>>>>>> Anyway, first try to define something like this:
>>>>>>>
>>>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0  V4L2_SEL_TGT_PRIVATE
>>>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT  (V4L2_SEL_TGT_XXX_SCANOUT0 + 1)
>>>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS  (V4L2_SEL_TGT_XXX_SCANOUT0 + 2)
>>>>>>>
>>>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0  (V4L2_SEL_TGT_PRIVATE + 16)
>>>>>>> ...
>>>>>>>
>>>>>>> -- OR-- parametrized macros similar to one below:
>>>>>>>
>>>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n))
>>>>>>>
>>>>>>> The application could setup all scanout areas one-by-one.
>>>>>>> By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array.
>>>>>>> The height of all consecutive area would be 0. This means disabling
>>>>>>> them effectively.
>>>>>>
>>>>>> Lets say rectangle A + B + C +D is legal, and A +B is also legal. You
>>>>>> are in ABCD and you want to go to AB. How can you do it?
>>>>>
>>>>> Just set C to have height 0. It will disable D automatically.
>>>>
>>>> It's the other direction that's the problem: how to go from A + B to
>>>> A + B + C + D if ABC is illegal.
>>>>
>>>>> BTW. How are you going to emulate S_CROP by selection API
>>>>> if you must use at least two rectangles (A + B) ?
>>>>
>>>> S_CROP will always just set one rectangle A. So if you had multiple
>>>> rectangles in your selection then after S_CROP you'll have only one.
>>>
>>> So S_CROP should change only A.
>>> What happens if someone already set device to work with 4 rectangles.
>>> Then calls S_CROP that modifies A in such a way that ABCD no longer forms
>>> a proper rectangle.
>>>
>>> What should happen?
>>> - ignore S_CROP, return previous settings, or
>>> - modify B,C,D to form a proper rectangle, or
>>> - disable B,C,D, or ?
>>
>> It will be the same as setting a multicrop with 1 rectangle.
>>
>
> It not the answer to my question.
> I expect that your answer refers to "disable B,C,D" variant, doesn't it?

s_crop should be the equivalent of s_selection with n_rect=0


>
>>>
>>>>
>>>> I'm assuming that the hw will always support a single crop rectangle.
>>>> If not (extraordinarily unlikely), then S_CROP should just return an
>>>> error.
>>>
>>> I agree that it is highly unlikely. I am just trying to figure out
>>> what should happen when calling S_CROP or single-rectangle S_SELECTION
>>> after calling multirect S_SELECTION.
>>
>> The sensor is configured with 1 rectangle.
>>
>>>
>>>>
>>>>> I suggest to use SCANOUT target in your first implementation.
>>>>> Notice that as long you use something different from CROP and COMPOSE,
>>>>> you may define interactions and dependencies any way you want, like
>>>>> - if change of scanout area can affect composing area
>>>>> - interaction with format and frame size
>>>>> - check if scanout area can overlap, multi crops should be allowed to overlap
>>>>
>>>> I see no reason for this. A CROP_COMPOSE target does all you need.
>>>
>>> I think that CROP and COMPOSE is all you need.
>>>
>>>>
>>>> Note that to set up N rectangles where N differs from the current
>>>> rectangle count you do need to use CROP_COMPOSE. You can't setup CROP
>>>> and COMPOSE rectangles independently since having N crop rectangles
>>>> and M compose rectangles makes no sense. If you don't change the
>>>> number of rectangles, then you can still use CROP and COMPOSE as is.
>>>>
>>>
>>> I am afraid that you are trying to kill too many birds with one stone.
>>> I think it is a very good moment to use bazooka.
>>>
>>> Transactions enhanced with frame numbers will handle a lot of issues with V4L2.
>>>
>>> Moreover it will allow to support some nice effects like:
>>> - multiple exposures during HDR photography
>>> - continuous online changes preview windows
>>> - flash at a specific frame
>>> - change format during video streaming to grab a high-resolution picture
>>> - support multi rectangle cropping and composing :)
>>
>> Even with transactions I humble believe that we should have a
>> multirectangle call. :)
>>
>>
>
> I humbly believe that single rectangle selections are sufficient.
> Please, try to convince me that I am wrong.
> If I was sure that this multirect feature cannot be implemented
> using single-rect selections I will support this extension.
> Even if this feature is not future-proof as mentioned earlier.

-Configurations where the number of lines depends on the number of rectangles.
-Configurations where the valid start of rectangles depends on the
number of rectangles


>
> We may let transactions to be a part of V4L3 :).
>
> By now, I am still not convinced.
>
>>>
>>>>>
>>>
>>> [snip]
>>>
>>>
>>>>>> I have no need for it, but now that we are extending the selection API
>>>>>> it would be the perfect moment to add them.
>>>>>
>>>>> The perfect moment for adding something is when it is needed.
>>>>
>>>> We need to add support for multiple rectangles, so we added a pointer.
>>>>
>>>>> The bad idea is preventing something from being added too early.
>>>>
>>>> But if we make it a pointer to v4l2_rect, then we can *never* add
>>>> additional information to each rectangle. Whenever we add new APIs
>>>> or features we try to allow for future extensions (with mixed
>>>> success...), so now is the time to do so for v4l2_rect.
>>>>
>>>> Something like this:
>>>>
>>>> struct v4l2_sel_rect {
>>>>       struct v4l2_rect r;
>>>>       __u32 reserved[4];
>>>> };
>>>>
>>>> would be fine by me as well. It's an additional indirection, but it
>>>> also makes it easier to go from a v4l2_rect to a v4l2_sel_rect.
>>>
>>> We can always migrate to extended rectangle by modifying the structure to
>>>
>>> struct v4l2_selection {
>>>          __u32                   type;
>>>          __u32                   target;
>>>          __u32                   flags;
>>>          union {
>>>                struct v4l2_rect        r;
>>>                struct v4l2_ext_rect    rext;
>>>          };
>>>          __u32                   reserved[X];
>>> };
>>>
>>> and adding V4L2_SEL_FLAG_EXT_RECT flag.
>>
>> Dont you think this is a bit confusing?
>>
>>
>>
>
> Why? It is much less confusing then union type
> dependent on value of v4l2_selection::rectangles.
>
> You can use pointer to struct v4l2_ext_rect
> instead of composition. Then only 1 bit could
> be used to support extended rectangles.
>
> Regards,
> Tomasz Stanislawski

Best Regards

-- 
Ricardo Ribalda

Re: [RFC v3] [RFC] v4l2: Support for multiple selections

[Tomasz Stanislawski]

Hi Hans,
We've misunderstood. When I was saying 'overengineered'
I did not mean your RFC.
I was taking about this:

#define V4L2_SEL_TGT_CROP_COMPOSE    0x0200

struct v4l2_selection {
        __u32                   type;
        __u32                   target;
        __u32                   flags;
	union {
	        struct v4l2_rect        r;
		struct v4l2_ext_rect    *pr;
	};
        __u32                   flags2;
	union {
	        struct v4l2_rect        r2;
		struct v4l2_ext_rect    *pr2;
	};
	__u32			rectangles;
        __u32                   reserved[3];
};

This structure looks scary to me :).

> 
> I disagree. I implemented it in vivi and it turned out to be quite easy.
> 
> For the record: I'm talking about this RFC:
> 
> http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/71822
> 
>> I still does not solve problems with flipping and rotations, which may
>> have a huge impact on mulitrect cropping/composing limitations.
> 
> My proposal will make that much easier as well since flipping, rotating,
> cropping and composing are all controls/properties that can be set
> atomically (a control cluster). So drivers can create a single function
> that can handle all the relationships in one place, and applications can
> set all of these with one VIDIOC_S_EXT_CTRLS call.
> 

I think that your idea is quite good. Solve atomic configuration
in a different part of API (control cluster), not by making
properties larger.

As I said, there are multiple way to handle atomic configuration.
Using control API is one of them. Quite nice BTW :)

Regards,
Tomasz Stanislawski

Re: [RFC v3] [RFC] v4l2: Support for multiple selections

[Hans Verkuil]

On 12/19/2013 02:30 PM, Tomasz Stanislawski wrote:
> Hi Hans,
> We've misunderstood. When I was saying 'overengineered'
> I did not mean your RFC.
> I was taking about this:
> 
> #define V4L2_SEL_TGT_CROP_COMPOSE    0x0200
> 
> struct v4l2_selection {
>         __u32                   type;
>         __u32                   target;
>         __u32                   flags;
> 	union {
> 	        struct v4l2_rect        r;
> 		struct v4l2_ext_rect    *pr;
> 	};
>         __u32                   flags2;
> 	union {
> 	        struct v4l2_rect        r2;
> 		struct v4l2_ext_rect    *pr2;
> 	};
> 	__u32			rectangles;
>         __u32                   reserved[3];
> };
> 
> This structure looks scary to me :).

Ah, yes. Implementing this as properties works much better.

And multi-selection can be done simply by making an array of crop and
compose rectangles.

Regards,

	Hans

> 
>>
>> I disagree. I implemented it in vivi and it turned out to be quite easy.
>>
>> For the record: I'm talking about this RFC:
>>
>> http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/71822
>>
>>> I still does not solve problems with flipping and rotations, which may
>>> have a huge impact on mulitrect cropping/composing limitations.
>>
>> My proposal will make that much easier as well since flipping, rotating,
>> cropping and composing are all controls/properties that can be set
>> atomically (a control cluster). So drivers can create a single function
>> that can handle all the relationships in one place, and applications can
>> set all of these with one VIDIOC_S_EXT_CTRLS call.
>>
> 
> I think that your idea is quite good. Solve atomic configuration
> in a different part of API (control cluster), not by making
> properties larger.
> 
> As I said, there are multiple way to handle atomic configuration.
> Using control API is one of them. Quite nice BTW :)
> 
> Regards,
> Tomasz Stanislawski
> 
>