Re: [PATCH 1/4] v4l: add support for selection api

[Tomasz Stanislawski <t.stanislaws@samsung.com>]

Hi Hans,

On 09/27/2011 10:28 AM, Hans Verkuil wrote:
> Here is my 'better late than never' review :-)
>
> On Wednesday, August 31, 2011 14:28:20 Tomasz Stanislawski wrote:
>> This patch introduces new api for a precise control of cropping and composing
>> features for video devices. The new ioctls are VIDIOC_S_SELECTION and
>> VIDIOC_G_SELECTION.
>>
>> Signed-off-by: Tomasz Stanislawski<t.stanislaws@samsung.com>
>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>> ---
>>   drivers/media/video/v4l2-compat-ioctl32.c |    2 +
>>   drivers/media/video/v4l2-ioctl.c          |   28 +++++++++++++++++
>>   include/linux/videodev2.h                 |   46 +++++++++++++++++++++++++++++
>>   include/media/v4l2-ioctl.h                |    4 ++
>>   4 files changed, 80 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
>> index 61979b7..f3b9d15 100644
>> --- a/drivers/media/video/v4l2-compat-ioctl32.c
>> +++ b/drivers/media/video/v4l2-compat-ioctl32.c
>> @@ -927,6 +927,8 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
>>   	case VIDIOC_CROPCAP:
>>   	case VIDIOC_G_CROP:
>>   	case VIDIOC_S_CROP:
>> +	case VIDIOC_G_SELECTION:
>> +	case VIDIOC_S_SELECTION:
>>   	case VIDIOC_G_JPEGCOMP:
>>   	case VIDIOC_S_JPEGCOMP:
>>   	case VIDIOC_QUERYSTD:
>> diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
>> index 002ce13..6e02b45 100644
>> --- a/drivers/media/video/v4l2-ioctl.c
>> +++ b/drivers/media/video/v4l2-ioctl.c
>> @@ -225,6 +225,8 @@ static const char *v4l2_ioctls[] = {
>>   	[_IOC_NR(VIDIOC_CROPCAP)]          = "VIDIOC_CROPCAP",
>>   	[_IOC_NR(VIDIOC_G_CROP)]           = "VIDIOC_G_CROP",
>>   	[_IOC_NR(VIDIOC_S_CROP)]           = "VIDIOC_S_CROP",
>> +	[_IOC_NR(VIDIOC_G_SELECTION)]      = "VIDIOC_G_SELECTION",
>> +	[_IOC_NR(VIDIOC_S_SELECTION)]      = "VIDIOC_S_SELECTION",
>>   	[_IOC_NR(VIDIOC_G_JPEGCOMP)]       = "VIDIOC_G_JPEGCOMP",
>>   	[_IOC_NR(VIDIOC_S_JPEGCOMP)]       = "VIDIOC_S_JPEGCOMP",
>>   	[_IOC_NR(VIDIOC_QUERYSTD)]         = "VIDIOC_QUERYSTD",
>> @@ -1714,6 +1716,32 @@ static long __video_do_ioctl(struct file *file,
>>   		ret = ops->vidioc_s_crop(file, fh, p);
>>   		break;
>>   	}
>> +	case VIDIOC_G_SELECTION:
>> +	{
>> +		struct v4l2_selection *p = arg;
>> +
>> +		if (!ops->vidioc_g_selection)
>> +			break;
>> +
>> +		dbgarg(cmd, "type=%s\n", prt_names(p->type, v4l2_type_names));
>> +
>> +		ret = ops->vidioc_g_selection(file, fh, p);
>> +		if (!ret)
>> +			dbgrect(vfd, "",&p->r);
>> +		break;
>> +	}
>> +	case VIDIOC_S_SELECTION:
>> +	{
>> +		struct v4l2_selection *p = arg;
>> +
>> +		if (!ops->vidioc_s_selection)
>> +			break;
>
> Here you should insert this code so that this ioctl handles the priority check
> correctly:
>
> 		if (ret_prio) {
> 			ret = ret_prio;
> 			break;
> 		}
>

OK

>> +		dbgarg(cmd, "type=%s\n", prt_names(p->type, v4l2_type_names));
>> +		dbgrect(vfd, "",&p->r);
>> +
>> +		ret = ops->vidioc_s_selection(file, fh, p);
>> +		break;
>> +	}
>>   	case VIDIOC_CROPCAP:
>>   	{
>>   		struct v4l2_cropcap *p = arg;
>> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
>> index fca24cc..b7471fe 100644
>> --- a/include/linux/videodev2.h
>> +++ b/include/linux/videodev2.h
>> @@ -738,6 +738,48 @@ struct v4l2_crop {
>>   	struct v4l2_rect        c;
>>   };
>>
>> +/* Hints for adjustments of selection rectangle */
>> +#define V4L2_SEL_SIZE_GE	0x00000001
>> +#define V4L2_SEL_SIZE_LE	0x00000002
>> +
>> +/* Selection targets */
>> +
>> +/* current cropping area */
>> +#define V4L2_SEL_CROP_ACTIVE		0
>> +/* default cropping area */
>> +#define V4L2_SEL_CROP_DEFAULT		1
>> +/* cropping bounds */
>> +#define V4L2_SEL_CROP_BOUNDS		2
>> +/* current composing area */
>> +#define V4L2_SEL_COMPOSE_ACTIVE		256
>> +/* default composing area */
>> +#define V4L2_SEL_COMPOSE_DEFAULT	257
>> +/* composing bounds */
>> +#define V4L2_SEL_COMPOSE_BOUNDS		258
>> +/* current composing area plus all padding pixels */
>> +#define V4L2_SEL_COMPOSE_PADDED		259
>> +
>> +/**
>> + * struct v4l2_selection - selection info
>> + * @type:	buffer type (do not use *_MPLANE types)
>
> Why can't I use MPLANE types?
>

Because the selection has nothing to do with "multiplanar" stuff. The 
VIDIOC_{S/G}_CROP does not use it. Therefore the selection should not 
use it either. Only memory referring ioctl should use *_MPLANE types. I 
would really like to avoid forcing developers to implement this 
non-intuitive and confusing business logic like it happened to 
VIDIOC_S_FMT, and VIDIOC_STREAM{ON/OFF} ioctls.

>> + * @target:	selection target, used to choose one of possible rectangles
>> + * @flags:	constraints flags
>> + * @r:		coordinates of selection window
>> + * @reserved:	for future use, rounds structure size to 64 bytes, set to zero
>> + *
>> + * Hardware may use multiple helper window to process a video stream.
>> + * The structure is used to exchange this selection areas between
>> + * an application and a driver.
>> + */
>> +struct v4l2_selection {
>> +	__u32			type;
>> +	__u32			target;
>> +	__u32                   flags;
>> +	struct v4l2_rect        r;
>> +	__u32                   reserved[9];
>> +};
>> +
>> +
>>   /*
>>    *      A N A L O G   V I D E O   S T A N D A R D
>>    */
>> @@ -2182,6 +2224,10 @@ struct v4l2_dbg_chip_ident {
>>   #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct v4l2_event_subscription)
>>   #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91, struct v4l2_event_subscription)
>>
>> +/* Experimental crop/compose API */
>> +#define VIDIOC_G_SELECTION	_IOWR('V', 92, struct v4l2_selection)
>> +#define VIDIOC_S_SELECTION	_IOWR('V', 93, struct v4l2_selection)
>
> So we decided not to implement a TRY_SELECTION yet? I'm fine with that, BTW.
>

OK. This ioctl is needed for advanced cropping/composing negotiations.
I think that the constraints would be sufficient for simple use cases.

Best regards,
Tomasz Stanislawski

>> +
>>   /* Reminder: when adding new ioctls please add support for them to
>>      drivers/media/video/v4l2-compat-ioctl32.c as well! */
>>
>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
>> index dd9f1e7..9dd6e18 100644
>> --- a/include/media/v4l2-ioctl.h
>> +++ b/include/media/v4l2-ioctl.h
>> @@ -194,6 +194,10 @@ struct v4l2_ioctl_ops {
>>   					struct v4l2_crop *a);
>>   	int (*vidioc_s_crop)           (struct file *file, void *fh,
>>   					struct v4l2_crop *a);
>> +	int (*vidioc_g_selection)      (struct file *file, void *fh,
>> +					struct v4l2_selection *s);
>> +	int (*vidioc_s_selection)      (struct file *file, void *fh,
>> +					struct v4l2_selection *s);
>>   	/* Compression ioctls */
>>   	int (*vidioc_g_jpegcomp)       (struct file *file, void *fh,
>>   					struct v4l2_jpegcompression *a);
>>
>