RFC> multi-crop (was: Multiple Rectangle cropping)

RFC> multi-crop (was: Multiple Rectangle cropping)

[Ricardo Ribalda Delgado]

Hello

 I am working porting a industrial camera driver to v4l. So far I have
been able to describe most of the old functionality with v4l
equivalents. The only thing that I am missing is multi cropping.

The sensor (both a cmosis and a ccd chips) supports skipping lines
from up to 8 regions. This increases the readout speed up to 50%,
which is critical for the application.

Unfortunately I have no way to describe multiple cropping areas in
v4l. I am thinking about creating a new API/extending and old one for
this.

Any suggestion before I start? Have you faced also this problem? How
did you solve it?

I am planning to go to the Edinburgh mini summit, maybe we could add
this to the agenda (if you consider that it is worth the time, of
course)

Thanks

-- 
Ricardo Ribalda

Re: RFC> multi-crop (was: Multiple Rectangle cropping)

[Sylwester Nawrocki]

On 09/05/2013 11:10 PM, Ricardo Ribalda Delgado wrote:
> Hello

Hi,

>   I am working porting a industrial camera driver to v4l. So far I have
> been able to describe most of the old functionality with v4l
> equivalents. The only thing that I am missing is multi cropping.
>
> The sensor (both a cmosis and a ccd chips) supports skipping lines
> from up to 8 regions. This increases the readout speed up to 50%,
> which is critical for the application.
>
> Unfortunately I have no way to describe multiple cropping areas in
> v4l. I am thinking about creating a new API/extending and old one for
> this.
>
> Any suggestion before I start? Have you faced also this problem? How
> did you solve it?

A similar issue has been raised during discussions on the camera auto
focus rectangle selection API. While defining need selection targets [1]
it was also proposed to convert one of the struct v4l2_selection reserved
fields into an index field, which would indicate one rectangle of some
set of rectangles supported by a driver. Then there could be a v4l2
bitmask control to determine which rectangles are currently valid/in use.

Would something like this be relevant to your problem ?

> I am planning to go to the Edinburgh mini summit, maybe we could add
> this to the agenda (if you consider that it is worth the time, of
> course)

It definitely sounds like a good topic to discuss at the mini summit,
unless it gets resolved until then. ;-)

[1] http://www.spinics.net/lists/linux-media/msg64499.html

--
Regards,
Sylwester

Re: RFC> multi-crop (was: Multiple Rectangle cropping)

[Ricardo Ribalda Delgado]

Hi Sylvester

Thanks for your response

Unfortunately, the v4l2_crop dont have any reserved field :(

struct v4l2_crop {
__u32 type; /* enum v4l2_buf_type */
struct v4l2_rect        c;
};

And changing that ABI I dont think is an option.

What about a new call: G/S_READOUT .that uses a modified
v4l2_selection as you propose?

That call selects the readable areas from the sensor.

The new structure could be something like:

#define SELECTION_BITMAP 0xffffffff
#define SELECTION_RESET 0xfffffffe
#define SELECTION_MAX_AREAS 32
struct v4l2_selection {
__u32 type;
__u32 target;
__u32                   flags;
union {
   struct v4l2_rect        r;
   __u32 bitmap;
};
__u32 n;
__u32                   reserved[8];
};

n chooses the readout area to choose, up to 32.

When n is == 0xffffffff the user wants to change the bitmap that
selects which areas are enabled.
  When the bitmap is 0x0 all the sensor is read.
  When the bitmap is 0x5 the readout area 0 and 2 are enabled.

When the bitmap is set to a value !=0, the driver checks if the
combination of readout areas is supported by the sensor/readout logic
and returns -EINVAL if not.

The g/s_crop API still works as usual.

Any comment on this? Of course the names should be better chosen, this
is just a declaration of intentions.

Cheers!



On Thu, Sep 5, 2013 at 11:44 PM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
> On 09/05/2013 11:10 PM, Ricardo Ribalda Delgado wrote:
>>
>> Hello
>
>
> Hi,
>
>
>>   I am working porting a industrial camera driver to v4l. So far I have
>> been able to describe most of the old functionality with v4l
>> equivalents. The only thing that I am missing is multi cropping.
>>
>> The sensor (both a cmosis and a ccd chips) supports skipping lines
>> from up to 8 regions. This increases the readout speed up to 50%,
>> which is critical for the application.
>>
>> Unfortunately I have no way to describe multiple cropping areas in
>> v4l. I am thinking about creating a new API/extending and old one for
>> this.
>>
>> Any suggestion before I start? Have you faced also this problem? How
>> did you solve it?
>
>
> A similar issue has been raised during discussions on the camera auto
> focus rectangle selection API. While defining need selection targets [1]
> it was also proposed to convert one of the struct v4l2_selection reserved
> fields into an index field, which would indicate one rectangle of some
> set of rectangles supported by a driver. Then there could be a v4l2
> bitmask control to determine which rectangles are currently valid/in use.
>
> Would something like this be relevant to your problem ?
>
>
>> I am planning to go to the Edinburgh mini summit, maybe we could add
>> this to the agenda (if you consider that it is worth the time, of
>> course)
>
>
> It definitely sounds like a good topic to discuss at the mini summit,
> unless it gets resolved until then. ;-)
>
> [1] http://www.spinics.net/lists/linux-media/msg64499.html
>
> --
> Regards,
> Sylwester



-- 
Ricardo Ribalda

Re: RFC> multi-crop (was: Multiple Rectangle cropping)

[Sakari Ailus]

Hi Ricardo,

On Fri, Sep 06, 2013 at 10:30:18AM +0200, Ricardo Ribalda Delgado wrote:
> Hi Sylvester
> 
> Thanks for your response
> 
> Unfortunately, the v4l2_crop dont have any reserved field :(

Don't worry about v4lw_crop. we have selections now. :-)

> struct v4l2_crop {
> __u32 type; /* enum v4l2_buf_type */
> struct v4l2_rect        c;
> };
> 
> And changing that ABI I dont think is an option.
> 
> What about a new call: G/S_READOUT .that uses a modified
> v4l2_selection as you propose?

Could this functionality be added to the ex$sting selection API, with a
possible extension in a for of a new field, say, "id" to tell which one is
being changed?

> That call selects the readable areas from the sensor.
> 
> The new structure could be something like:
> 
> #define SELECTION_BITMAP 0xffffffff
> #define SELECTION_RESET 0xfffffffe
> #define SELECTION_MAX_AREAS 32
> struct v4l2_selection {
> __u32 type;
> __u32 target;
> __u32                   flags;
> union {
>    struct v4l2_rect        r;
>    __u32 bitmap;
> };
> __u32 n;
> __u32                   reserved[8];
> };
> 
> n chooses the readout area to choose, up to 32.
> 
> When n is == 0xffffffff the user wants to change the bitmap that
> selects which areas are enabled.
>   When the bitmap is 0x0 all the sensor is read.
>   When the bitmap is 0x5 the readout area 0 and 2 are enabled.
> 
> When the bitmap is set to a value !=0, the driver checks if the
> combination of readout areas is supported by the sensor/readout logic
> and returns -EINVAL if not.
> 
> The g/s_crop API still works as usual.
> 
> Any comment on this? Of course the names should be better chosen, this
> is just a declaration of intentions.

I think the functionality you're describing is highly peculiar. I have to
say that, as Sylwester noted, it bears resemblance to the AF windows so the
solution could be same as well.

I think earlier on (say perhaps a year and a half) ago it was proposed to
use bitmask controls with selections to tell which IDs are valid. What would
you think about that?

It's also always possible to use private controls, too.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: RFC> multi-crop

[Sylwester Nawrocki]

Hi All,

On 09/10/2013 11:41 PM, Sakari Ailus wrote:
> Hi Ricardo,
>
> On Fri, Sep 06, 2013 at 10:30:18AM +0200, Ricardo Ribalda Delgado wrote:
>> Hi Sylvester
>>
>> Thanks for your response
>>
>> Unfortunately, the v4l2_crop dont have any reserved field :(
>
> Don't worry about v4lw_crop. we have selections now. :-)

True, I belive no possibility of extending struct v4l2_crop was one of
the reasons why the selections API has benn introduced. The selections
API provides superset of functionality of the original cropping API and
G/S_CROP/CROPCAP ioctls should be considered deprecated.

>> struct v4l2_crop {
>> __u32 type; /* enum v4l2_buf_type */
>> struct v4l2_rect        c;
>> };
>>
>> And changing that ABI I dont think is an option.
>>
>> What about a new call: G/S_READOUT .that uses a modified
>> v4l2_selection as you propose?
>
> Could this functionality be added to the ex$sting selection API, with a
> possible extension in a for of a new field, say, "id" to tell which one is
> being changed?

+1, that was my idea as well.

>> That call selects the readable areas from the sensor.
>>
>> The new structure could be something like:
>>
>> #define SELECTION_BITMAP 0xffffffff
>> #define SELECTION_RESET 0xfffffffe
>> #define SELECTION_MAX_AREAS 32
>> struct v4l2_selection {
>> __u32 type;
>> __u32 target;
>> __u32                   flags;
>> union {
>>     struct v4l2_rect        r;
>>     __u32 bitmap;
>> };
>> __u32 n;
>> __u32                   reserved[8];
>> };
>>
>> n chooses the readout area to choose, up to 32.
>>
>> When n is == 0xffffffff the user wants to change the bitmap that
>> selects which areas are enabled.
>>    When the bitmap is 0x0 all the sensor is read.
>>    When the bitmap is 0x5 the readout area 0 and 2 are enabled.
>>
>> When the bitmap is set to a value !=0, the driver checks if the
>> combination of readout areas is supported by the sensor/readout logic
>> and returns -EINVAL if not.

Would the supported combinations vary at run-time, depending on some
configuration parameters. Or would it be rather fixed and known at device
initialization time ?

>> The g/s_crop API still works as usual.
>>
>> Any comment on this? Of course the names should be better chosen, this
>> is just a declaration of intentions.
>
> I think the functionality you're describing is highly peculiar. I have to
> say that, as Sylwester noted, it bears resemblance to the AF windows so the
> solution could be same as well.
>
> I think earlier on (say perhaps a year and a half) ago it was proposed to
> use bitmask controls with selections to tell which IDs are valid. What would
> you think about that?

My feelings are that using a bitmask control to select sub-windows would
be far more flexible than embedding the mask field in struct v4l2_selection.
If there is more than 32 windows needed the control API could be extended,
at least for up to 64-bit it seems not a big deal.
And an "id" member of struct v4l2_selection would be generic and could be
used for other purposes as well.

> It's also always possible to use private controls, too.

--
Regards,
Sylwester

Re: RFC> multi-crop

[Ricardo Ribalda Delgado]

Hello

Thanks for your comments

On Wed, Sep 11, 2013 at 12:05 AM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
> Hi All,
>
> On 09/10/2013 11:41 PM, Sakari Ailus wrote:
>>
>> Hi Ricardo,
>>
>> On Fri, Sep 06, 2013 at 10:30:18AM +0200, Ricardo Ribalda Delgado wrote:
>>>
>>> Hi Sylvester
>>>
>>> Thanks for your response
>>>
>>> Unfortunately, the v4l2_crop dont have any reserved field :(
>>
>>
>> Don't worry about v4lw_crop. we have selections now. :-)
>
>
> True, I belive no possibility of extending struct v4l2_crop was one of
> the reasons why the selections API has benn introduced. The selections
> API provides superset of functionality of the original cropping API and
> G/S_CROP/CROPCAP ioctls should be considered deprecated.
>

We should update the doc to tell the user that the crop api will be
superseded soon by the selection api.

>>> struct v4l2_crop {
>>> __u32 type; /* enum v4l2_buf_type */
>>> struct v4l2_rect        c;
>>> };
>>>
>>> And changing that ABI I dont think is an option.
>>>
>>> What about a new call: G/S_READOUT .that uses a modified
>>> v4l2_selection as you propose?
>>
>>
>> Could this functionality be added to the ex$sting selection API, with a
>> possible extension in a for of a new field, say, "id" to tell which one is
>> being changed?
>
>
> +1, that was my idea as well.
>

This looks like the right way to do it. I only see an issue. Lets say
we have a hw that needs all the selection to have the same width.

The user creates selection 0, 1 and 2 with width 100. Now he wants
them to be 200 witdh.

He changes selection 0, but because the driver returns -EINVAL (or
resizes the selection to 100, to fit the old selections).

So the user cannot change the size anymore :(

>>> That call selects the readable areas from the sensor.
>>>
>>> The new structure could be something like:
>>>
>>> #define SELECTION_BITMAP 0xffffffff
>>> #define SELECTION_RESET 0xfffffffe
>>> #define SELECTION_MAX_AREAS 32
>>> struct v4l2_selection {
>>> __u32 type;
>>> __u32 target;
>>> __u32                   flags;
>>> union {
>>>     struct v4l2_rect        r;
>>>     __u32 bitmap;
>>> };
>>> __u32 n;
>>> __u32                   reserved[8];
>>> };
>>>
>>> n chooses the readout area to choose, up to 32.
>>>
>>> When n is == 0xffffffff the user wants to change the bitmap that
>>> selects which areas are enabled.
>>>    When the bitmap is 0x0 all the sensor is read.
>>>    When the bitmap is 0x5 the readout area 0 and 2 are enabled.
>>>
>>> When the bitmap is set to a value !=0, the driver checks if the
>>> combination of readout areas is supported by the sensor/readout logic
>>> and returns -EINVAL if not.
>
>
> Would the supported combinations vary at run-time, depending on some
> configuration parameters. Or would it be rather fixed and known at device
> initialization time ?
>

The combinations vary at runtime, depending on the size and what the
user wants to readout.

>>> The g/s_crop API still works as usual.
>>>
>>> Any comment on this? Of course the names should be better chosen, this
>>> is just a declaration of intentions.
>>
>>
>> I think the functionality you're describing is highly peculiar. I have to
>> say that, as Sylwester noted, it bears resemblance to the AF windows so
>> the
>> solution could be same as well.
>>
>> I think earlier on (say perhaps a year and a half) ago it was proposed to
>> use bitmask controls with selections to tell which IDs are valid. What
>> would
>> you think about that?
>
>
> My feelings are that using a bitmask control to select sub-windows would
> be far more flexible than embedding the mask field in struct v4l2_selection.
> If there is more than 32 windows needed the control API could be extended,
> at least for up to 64-bit it seems not a big deal.
> And an "id" member of struct v4l2_selection would be generic and could be
> used for other purposes as well.

I agree here too.

>
>> It's also always possible to use private controls, too.
>
>
> --
> Regards,
> Sylwester


Thanks for your comments


-- 
Ricardo Ribalda

Re: RFC> multi-crop (was: Multiple Rectangle cropping)

[Sakari Ailus]

Hi Ricardo,

On Fri, Sep 06, 2013 at 10:30:18AM +0200, Ricardo Ribalda Delgado wrote:
> Any comment on this? Of course the names should be better chosen, this
> is just a declaration of intentions.

I forgot to ask one question: what's the behaviour of cropping on different
regions? Are the regions located on particular line or what?

Contrary to the case with AF rectaangles, I see fewer possibilities for
standardising the behaviour of multiple crop rectanges which decreases the
value of a generic interface: even if the interface is generic but you have
no idea what it'd actually do you wouldn't gain much.

For this reason it might also make sense to use a private IOCTL (and not a
control) to support the functionality. Or private selections (which we don't
have yet).

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: RFC> multi-crop (was: Multiple Rectangle cropping)

[Ricardo Ribalda Delgado]

Hello Sakari

On Wed, Sep 11, 2013 at 12:35 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> Hi Ricardo,
>
> On Fri, Sep 06, 2013 at 10:30:18AM +0200, Ricardo Ribalda Delgado wrote:
>> Any comment on this? Of course the names should be better chosen, this
>> is just a declaration of intentions.
>
> I forgot to ask one question: what's the behaviour of cropping on different
> regions? Are the regions located on particular line or what?

The purpose of this is to increase the framerate on high speed cameras.

Lets say you are controlling the speed on a highway. You have a camera
looking at the road. The fps will determine the accuracy of your
meassurement. If you could skip the reservation between both lanes you
could increase the fps.

Also imaging a high speed ballistics camera. You have a reference
plate on rows 1-10, and you are studing a sample on rows 400-500. You
dont want to read rows 11-399, because you will reduce your framerate

Finally If you have an scenario where you want to process a specific
part of the image at 1mm per pixel and the rest at 10mm per pixel.

>
> Contrary to the case with AF rectaangles, I see fewer possibilities for
> standardising the behaviour of multiple crop rectanges which decreases the
> value of a generic interface: even if the interface is generic but you have
> no idea what it'd actually do you wouldn't gain much.
>
> For this reason it might also make sense to use a private IOCTL (and not a
> control) to support the functionality. Or private selections (which we don't
> have yet).

I like the idea of adding an id field to the selection api, and then a
private control with a bitmask selection with selections are active.

If we can agree on the value of this we could define a standard
control. If not, we leave the door open to the drivers to define their
own.


Thanks for you feedback!

-- 
Ricardo Ribalda

[PATCH] RFC: Support for multiple selections

[Ricardo Ribalda Delgado]

A new id field is added to the struct selection. On devices that
supports multiple sections this id indicate which of the selection to
modify.

A new control V4L2_CID_SELECTION_BITMASK selects which of the selections
are used, if the control is set to zero the default rectangles are used.

This is needed in cases where the user has to change multiple selections
at the same time to get a valid combination.

On devices where the control V4L2_CID_SELECTION_BITMASK does not exist,
the id field is ignored

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 Documentation/DocBook/media/v4l/controls.xml | 6 ++++++
 drivers/media/v4l2-core/v4l2-ctrls.c         | 2 ++
 drivers/media/v4l2-core/v4l2-ioctl.c         | 4 ++--
 include/uapi/linux/v4l2-controls.h           | 4 +++-
 include/uapi/linux/videodev2.h               | 5 ++++-
 5 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
index c2fc9ec..a8c84bb 100644
--- a/Documentation/DocBook/media/v4l/controls.xml
+++ b/Documentation/DocBook/media/v4l/controls.xml
@@ -338,6 +338,12 @@ minimum value disables backlight compensation.</entry>
 		  coefficients determined by <constant>V4L2_CID_COLORFX_CBCR</constant>
 		  control.</entry>
 		</row>
+		<row>
+		  <entry><constant>V4L2_CID_SELECTION_BITMASK</constant>&nbsp;</entry>
+		  <entry>For drivers that support multiple selections. This control
+		  determine which selections are enabled. If it is set to zero the default
+		  crop and compose rectangle are used.</entry>
+		</row>
 	      </tbody>
 	    </entrytbl>
 	  </row>
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index fccd08b..fa3bfc2 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -599,6 +599,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:	return "Min Number of Output Buffers";
 	case V4L2_CID_ALPHA_COMPONENT:		return "Alpha Component";
 	case V4L2_CID_COLORFX_CBCR:		return "Color Effects, CbCr";
+	case V4L2_CID_SELECTION_BITMASK:	return "Selection Bitmask";
 
 	/* MPEG controls */
 	/* Keep the order of the 'case's the same as in videodev2.h! */
@@ -957,6 +958,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_DV_TX_RXSENSE:
 	case V4L2_CID_DV_TX_EDID_PRESENT:
 	case V4L2_CID_DV_RX_POWER_PRESENT:
+	case V4L2_CID_SELECTION_BITMASK:
 		*type = V4L2_CTRL_TYPE_BITMASK;
 		break;
 	case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE:
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 68e6b5e..ca177bb 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -573,9 +573,9 @@ static void v4l_print_selection(const void *arg, bool write_only)
 {
 	const struct v4l2_selection *p = arg;
 
-	pr_cont("type=%s, target=%d, flags=0x%x, wxh=%dx%d, x,y=%d,%d\n",
+	pr_cont("type=%s, target=%d, flags=0x%x, id=%d,wxh=%dx%d, x,y=%d,%d\n",
 		prt_names(p->type, v4l2_type_names),
-		p->target, p->flags,
+		p->target, p->flags, p->id,
 		p->r.width, p->r.height, p->r.left, p->r.top);
 }
 
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index e90a88a..ca44338 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -138,8 +138,10 @@ enum v4l2_colorfx {
 #define V4L2_CID_ALPHA_COMPONENT		(V4L2_CID_BASE+41)
 #define V4L2_CID_COLORFX_CBCR			(V4L2_CID_BASE+42)
 
+#define V4L2_CID_SELECTION_BITMASK		(V4L2_CID_BASE+43)
+
 /* last CID + 1 */
-#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+43)
+#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+44)
 
 /* USER-class private control IDs */
 
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 0e80472..4f68196 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -806,6 +806,8 @@ struct v4l2_crop {
  * @target:	Selection target, used to choose one of possible rectangles;
  *		defined in v4l2-common.h; V4L2_SEL_TGT_* .
  * @flags:	constraints flags, defined in v4l2-common.h; V4L2_SEL_FLAG_*.
+ * @id:		Selection to change, for devices that supports multiple,
+ *		set to zero otherwise.
  * @r:		coordinates of selection window
  * @reserved:	for future use, rounds structure size to 64 bytes, set to zero
  *
@@ -818,7 +820,8 @@ struct v4l2_selection {
 	__u32			target;
 	__u32                   flags;
 	struct v4l2_rect        r;
-	__u32                   reserved[9];
+	__u32			id;
+	__u32                   reserved[8];
 };
 
 
-- 
1.8.4.rc3

Re: [PATCH] RFC: Support for multiple selections

[Hans Verkuil]

Hi Ricardo,

On 09/11/2013 10:30 AM, Ricardo Ribalda Delgado wrote:
> A new id field is added to the struct selection. On devices that
> supports multiple sections this id indicate which of the selection to
> modify.
> 
> A new control V4L2_CID_SELECTION_BITMASK selects which of the selections
> are used, if the control is set to zero the default rectangles are used.
> 
> This is needed in cases where the user has to change multiple selections
> at the same time to get a valid combination.
> 
> On devices where the control V4L2_CID_SELECTION_BITMASK does not exist,
> the id field is ignored

This feels like a hack to me. A big problem I have with using a control here
is that with a control you can't specify for which selection target it is.

If you want to set multiple rectangles, why not just pass them directly? E.g.:

struct v4l2_selection {
        __u32                   type;
        __u32                   target;
        __u32                   flags;
	union {
	        struct v4l2_rect        r;
		struct v4l2_rect	*pr;
	};
	__u32			rectangles;
        __u32                   reserved[8];
};

If rectangles > 1, then pr is used.

It's a bit more work to add this to the core code (VIDIOC_SUBDEV_G/S_SELECTION
should probably be changed at the same time and you have to fix existing drivers
to check/set the new rectangles field), but it scales much better.

Regards,

	Hans

> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  Documentation/DocBook/media/v4l/controls.xml | 6 ++++++
>  drivers/media/v4l2-core/v4l2-ctrls.c         | 2 ++
>  drivers/media/v4l2-core/v4l2-ioctl.c         | 4 ++--
>  include/uapi/linux/v4l2-controls.h           | 4 +++-
>  include/uapi/linux/videodev2.h               | 5 ++++-
>  5 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
> index c2fc9ec..a8c84bb 100644
> --- a/Documentation/DocBook/media/v4l/controls.xml
> +++ b/Documentation/DocBook/media/v4l/controls.xml
> @@ -338,6 +338,12 @@ minimum value disables backlight compensation.</entry>
>  		  coefficients determined by <constant>V4L2_CID_COLORFX_CBCR</constant>
>  		  control.</entry>
>  		</row>
> +		<row>
> +		  <entry><constant>V4L2_CID_SELECTION_BITMASK</constant>&nbsp;</entry>
> +		  <entry>For drivers that support multiple selections. This control
> +		  determine which selections are enabled. If it is set to zero the default
> +		  crop and compose rectangle are used.</entry>
> +		</row>
>  	      </tbody>
>  	    </entrytbl>
>  	  </row>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index fccd08b..fa3bfc2 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -599,6 +599,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:	return "Min Number of Output Buffers";
>  	case V4L2_CID_ALPHA_COMPONENT:		return "Alpha Component";
>  	case V4L2_CID_COLORFX_CBCR:		return "Color Effects, CbCr";
> +	case V4L2_CID_SELECTION_BITMASK:	return "Selection Bitmask";
>  
>  	/* MPEG controls */
>  	/* Keep the order of the 'case's the same as in videodev2.h! */
> @@ -957,6 +958,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_DV_TX_RXSENSE:
>  	case V4L2_CID_DV_TX_EDID_PRESENT:
>  	case V4L2_CID_DV_RX_POWER_PRESENT:
> +	case V4L2_CID_SELECTION_BITMASK:
>  		*type = V4L2_CTRL_TYPE_BITMASK;
>  		break;
>  	case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE:
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 68e6b5e..ca177bb 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -573,9 +573,9 @@ static void v4l_print_selection(const void *arg, bool write_only)
>  {
>  	const struct v4l2_selection *p = arg;
>  
> -	pr_cont("type=%s, target=%d, flags=0x%x, wxh=%dx%d, x,y=%d,%d\n",
> +	pr_cont("type=%s, target=%d, flags=0x%x, id=%d,wxh=%dx%d, x,y=%d,%d\n",
>  		prt_names(p->type, v4l2_type_names),
> -		p->target, p->flags,
> +		p->target, p->flags, p->id,
>  		p->r.width, p->r.height, p->r.left, p->r.top);
>  }
>  
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index e90a88a..ca44338 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -138,8 +138,10 @@ enum v4l2_colorfx {
>  #define V4L2_CID_ALPHA_COMPONENT		(V4L2_CID_BASE+41)
>  #define V4L2_CID_COLORFX_CBCR			(V4L2_CID_BASE+42)
>  
> +#define V4L2_CID_SELECTION_BITMASK		(V4L2_CID_BASE+43)
> +
>  /* last CID + 1 */
> -#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+43)
> +#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+44)
>  
>  /* USER-class private control IDs */
>  
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 0e80472..4f68196 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -806,6 +806,8 @@ struct v4l2_crop {
>   * @target:	Selection target, used to choose one of possible rectangles;
>   *		defined in v4l2-common.h; V4L2_SEL_TGT_* .
>   * @flags:	constraints flags, defined in v4l2-common.h; V4L2_SEL_FLAG_*.
> + * @id:		Selection to change, for devices that supports multiple,
> + *		set to zero otherwise.
>   * @r:		coordinates of selection window
>   * @reserved:	for future use, rounds structure size to 64 bytes, set to zero
>   *
> @@ -818,7 +820,8 @@ struct v4l2_selection {
>  	__u32			target;
>  	__u32                   flags;
>  	struct v4l2_rect        r;
> -	__u32                   reserved[9];
> +	__u32			id;
> +	__u32                   reserved[8];
>  };
>  
>  
> 

Re: [PATCH] RFC: Support for multiple selections

[Ricardo Ribalda Delgado]

Hi Hans

Thanks for your feedback

On Wed, Sep 11, 2013 at 11:04 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> Hi Ricardo,
>
> On 09/11/2013 10:30 AM, Ricardo Ribalda Delgado wrote:
>> A new id field is added to the struct selection. On devices that
>> supports multiple sections this id indicate which of the selection to
>> modify.
>>
>> A new control V4L2_CID_SELECTION_BITMASK selects which of the selections
>> are used, if the control is set to zero the default rectangles are used.
>>
>> This is needed in cases where the user has to change multiple selections
>> at the same time to get a valid combination.
>>
>> On devices where the control V4L2_CID_SELECTION_BITMASK does not exist,
>> the id field is ignored
>
> This feels like a hack to me. A big problem I have with using a control here
> is that with a control you can't specify for which selection target it is.
>

I am not sure that I understand what you mean here.

If you set the control to 0x1 you are using selection 0, if you set
the control to 0x5, you are using selection 0 and 2.


> If you want to set multiple rectangles, why not just pass them directly? E.g.:
>
> struct v4l2_selection {
>         __u32                   type;
>         __u32                   target;
>         __u32                   flags;
>         union {
>                 struct v4l2_rect        r;
>                 struct v4l2_rect        *pr;
>         };
>         __u32                   rectangles;
>         __u32                   reserved[8];
> };
>
> If rectangles > 1, then pr is used.
>

The structure is passed in a ioctl and I dont think that it is a good
idea that you let the kernel get/set a memory address not encapsulated
in it. I can see that it could lead to security breaches if there is a
mistake on the handling.

> It's a bit more work to add this to the core code (VIDIOC_SUBDEV_G/S_SELECTION
> should probably be changed at the same time and you have to fix existing drivers
> to check/set the new rectangles field), but it scales much better.

Also, we would be broking the ABI. Rectangles is not a mandatory
field, and has a value != 0.

What we could do is leave the V4L2_CID_SELECTION_BITMASK  out of the
api, but keep the id field on the structure, so the user can define a
private control to do whatever he needs with the id field, wekeep the
ABI compatibility and no big changes are needed.


Thaks!

>
> Regards,
>
>         Hans
>
>>

Re: [PATCH] RFC: Support for multiple selections

[Hans Verkuil]

Hi Ricardo,

On 09/11/2013 11:34 AM, Ricardo Ribalda Delgado wrote:
> Hi Hans
> 
> Thanks for your feedback
> 
> On Wed, Sep 11, 2013 at 11:04 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> Hi Ricardo,
>>
>> On 09/11/2013 10:30 AM, Ricardo Ribalda Delgado wrote:
>>> A new id field is added to the struct selection. On devices that
>>> supports multiple sections this id indicate which of the selection to
>>> modify.
>>>
>>> A new control V4L2_CID_SELECTION_BITMASK selects which of the selections
>>> are used, if the control is set to zero the default rectangles are used.
>>>
>>> This is needed in cases where the user has to change multiple selections
>>> at the same time to get a valid combination.
>>>
>>> On devices where the control V4L2_CID_SELECTION_BITMASK does not exist,
>>> the id field is ignored
>>
>> This feels like a hack to me. A big problem I have with using a control here
>> is that with a control you can't specify for which selection target it is.
>>
> 
> I am not sure that I understand what you mean here.
> 
> If you set the control to 0x1 you are using selection 0, if you set
> the control to 0x5, you are using selection 0 and 2.

If you look here:

http://hverkuil.home.xs4all.nl/spec/media.html#v4l2-selection-targets

you see that a selection has a 'target': i.e. does the selection define a crop
rectangle, a compose rectange, a default crop rectangle, etc.

You are extending the API to support multiple rectangles per target and using
a control to select which rectangles are active (if I understand correctly).
But that control does not (and cannot) specify for which target the rectangles
should be activated.

> 
> 
>> If you want to set multiple rectangles, why not just pass them directly? E.g.:
>>
>> struct v4l2_selection {
>>         __u32                   type;
>>         __u32                   target;
>>         __u32                   flags;
>>         union {
>>                 struct v4l2_rect        r;
>>                 struct v4l2_rect        *pr;
>>         };
>>         __u32                   rectangles;
>>         __u32                   reserved[8];
>> };
>>
>> If rectangles > 1, then pr is used.
>>
> 
> The structure is passed in a ioctl and I dont think that it is a good
> idea that you let the kernel get/set a memory address not encapsulated
> in it. I can see that it could lead to security breaches if there is a
> mistake on the handling.

It's used in lots of places. It's OK, provided you check the memory carefully.
See e.g. how VIDIOC_G/S_EXT_CTRLS is handled. Usually the memory checks are done
in the v4l2 core and the driver doesn't need to take care of it.

>> It's a bit more work to add this to the core code (VIDIOC_SUBDEV_G/S_SELECTION
>> should probably be changed at the same time and you have to fix existing drivers
>> to check/set the new rectangles field), but it scales much better.
> 
> Also, we would be broking the ABI. Rectangles is not a mandatory
> field, and has a value != 0.

The spec clearly states that:

"The flags and reserved fields of struct v4l2_selection are ignored and they must
 be filled with zeros."

Any app not doing that is not obeying the API and hence it is an application bug.

It's standard practice inside the V4L2 API to handle reserved fields in that way,
as it provides a mechanism to extend the API safely in the future.

> 
> What we could do is leave the V4L2_CID_SELECTION_BITMASK  out of the
> api, but keep the id field on the structure, so the user can define a
> private control to do whatever he needs with the id field, wekeep the
> ABI compatibility and no big changes are needed.

I really don't like that. It artificially limits the number of rectangles to 32
and makes the API just cumbersome to use.

The changes aren't difficult, just a bit tedious, and the end result does exactly
what you want and, I think, is also very useful for things like face detection or
object detection in general where a list of rectangles (or points, which is just a
1x1 rectangle) has to be returned in an atomic manner.

One thing that I am not certain about is whether v4l2_rect should be used when
specifying multiple rectangles. I can imagine that rectangles have an additional
type field (e.g. for face detection you might have rectangles for the left eye,
right eye and mouth).

Regards,

	Hans

Re: [PATCH] RFC: Support for multiple selections

[Ricardo Ribalda Delgado]

Hello Hans

On Wed, Sep 11, 2013 at 12:49 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> Hi Ricardo,
>
> On 09/11/2013 11:34 AM, Ricardo Ribalda Delgado wrote:
>> Hi Hans
>>
>> Thanks for your feedback
>>
>> On Wed, Sep 11, 2013 at 11:04 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>> Hi Ricardo,
>>>
>>> On 09/11/2013 10:30 AM, Ricardo Ribalda Delgado wrote:
>>>> A new id field is added to the struct selection. On devices that
>>>> supports multiple sections this id indicate which of the selection to
>>>> modify.
>>>>
>>>> A new control V4L2_CID_SELECTION_BITMASK selects which of the selections
>>>> are used, if the control is set to zero the default rectangles are used.
>>>>
>>>> This is needed in cases where the user has to change multiple selections
>>>> at the same time to get a valid combination.
>>>>
>>>> On devices where the control V4L2_CID_SELECTION_BITMASK does not exist,
>>>> the id field is ignored
>>>
>>> This feels like a hack to me. A big problem I have with using a control here
>>> is that with a control you can't specify for which selection target it is.
>>>
>>
>> I am not sure that I understand what you mean here.
>>
>> If you set the control to 0x1 you are using selection 0, if you set
>> the control to 0x5, you are using selection 0 and 2.
>
> If you look here:
>
> http://hverkuil.home.xs4all.nl/spec/media.html#v4l2-selection-targets
>
> you see that a selection has a 'target': i.e. does the selection define a crop
> rectangle, a compose rectange, a default crop rectangle, etc.
>
> You are extending the API to support multiple rectangles per target and using
> a control to select which rectangles are active (if I understand correctly).
> But that control does not (and cannot) specify for which target the rectangles
> should be activated.

I want to have N crop rectangles and N compose rectangles. Every crop
rectangle has associated one compose rectangle.

This will fit multiple purposes ie:

- swap two areas of an image,
- multiple readout zones
- different decimation per area....


>
>>
>>
>>> If you want to set multiple rectangles, why not just pass them directly? E.g.:
>>>
>>> struct v4l2_selection {
>>>         __u32                   type;
>>>         __u32                   target;
>>>         __u32                   flags;
>>>         union {
>>>                 struct v4l2_rect        r;
>>>                 struct v4l2_rect        *pr;
>>>         };
>>>         __u32                   rectangles;
>>>         __u32                   reserved[8];
>>> };
>>>
>>> If rectangles > 1, then pr is used.
>>>
>>
>> The structure is passed in a ioctl and I dont think that it is a good
>> idea that you let the kernel get/set a memory address not encapsulated
>> in it. I can see that it could lead to security breaches if there is a
>> mistake on the handling.
>
> It's used in lots of places. It's OK, provided you check the memory carefully.
> See e.g. how VIDIOC_G/S_EXT_CTRLS is handled. Usually the memory checks are done
> in the v4l2 core and the driver doesn't need to take care of it.
>

I agree IFF the v4l2 core does the checks. One question raises me: how
does the user knows how big is the structure he has to allocate for
g_selection? Do we have to make a new ioctl g_n_selections?

>>> It's a bit more work to add this to the core code (VIDIOC_SUBDEV_G/S_SELECTION
>>> should probably be changed at the same time and you have to fix existing drivers
>>> to check/set the new rectangles field), but it scales much better.
>>
>> Also, we would be broking the ABI. Rectangles is not a mandatory
>> field, and has a value != 0.
>
> The spec clearly states that:
>
> "The flags and reserved fields of struct v4l2_selection are ignored and they must
>  be filled with zeros."
>
> Any app not doing that is not obeying the API and hence it is an application bug.
>
> It's standard practice inside the V4L2 API to handle reserved fields in that way,
> as it provides a mechanism to extend the API safely in the future.
>

That is what I mean, the current programs are writing a 0 on that
field, but now they are required to write a 1, so the API is broken.
Maybe we should describe:
if rectangles is 0, the field r is used (legacy), otherwise the pr,
even for 1 (multi rectangle).

>>
>> What we could do is leave the V4L2_CID_SELECTION_BITMASK  out of the
>> api, but keep the id field on the structure, so the user can define a
>> private control to do whatever he needs with the id field, wekeep the
>> ABI compatibility and no big changes are needed.
>
> I really don't like that. It artificially limits the number of rectangles to 32
> and makes the API just cumbersome to use.

I wasn't seeing the 32 rectangles as a limitation, but if you think
that it is limiting, then the solution that you provide looks good.

>
> The changes aren't difficult, just a bit tedious, and the end result does exactly
> what you want and, I think, is also very useful for things like face detection or
> object detection in general where a list of rectangles (or points, which is just a
> 1x1 rectangle) has to be returned in an atomic manner.
>
> One thing that I am not certain about is whether v4l2_rect should be used when
> specifying multiple rectangles. I can imagine that rectangles have an additional
> type field (e.g. for face detection you might have rectangles for the left eye,
> right eye and mouth).

That is where the id field was handy :), you could say rectangle
id=LEFT_EYE and then have private controls for removing red component
from rectangles which id is *_EYE.

My approach was:
You use the s/g selection api to describe rectangles (input and
output) and then you use bitmap controls to do things on the
rectangles: compose,remove red eye, track.....

>
> Regards,
>
>         Hans

So

If the 32 rectangle limitation is a nogo for you, then I would suggest:

struct v4l2_selection {
         __u32                   type;
         __u32                   target;
         __u32                   flags;
          union {
                 struct v4l2_rect        r;
                 struct v4l2_rect        *pr;
         };
         __u32                   rectangles; //if 0, r is used,
otherwise pr with rectangle components
         __u32                   id;//0 for compose/crop , N->driver
dependent (face tracking....)
        __u32                   reserved[7];
};

But:
 -memory handling has to be done in the core
 -we have to provide the user a way to know how many rectangles are in use.


If the 32 rectangle limitiation is acceptable I still like my first proposal.
But:
32 rectangles means 32 ioctls.


Thanks for your feedback and promptly response :)

-- 
Ricardo Ribalda

Re: [PATCH] RFC: Support for multiple selections

[Hans Verkuil]

Hi Ricardo,

On 09/11/2013 02:13 PM, Ricardo Ribalda Delgado wrote:
> Hello Hans
> 
> On Wed, Sep 11, 2013 at 12:49 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> Hi Ricardo,
>>
>> On 09/11/2013 11:34 AM, Ricardo Ribalda Delgado wrote:
>>> Hi Hans
>>>
>>> Thanks for your feedback
>>>
>>> On Wed, Sep 11, 2013 at 11:04 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>> Hi Ricardo,
>>>>
>>>> On 09/11/2013 10:30 AM, Ricardo Ribalda Delgado wrote:
>>>>> A new id field is added to the struct selection. On devices that
>>>>> supports multiple sections this id indicate which of the selection to
>>>>> modify.
>>>>>
>>>>> A new control V4L2_CID_SELECTION_BITMASK selects which of the selections
>>>>> are used, if the control is set to zero the default rectangles are used.
>>>>>
>>>>> This is needed in cases where the user has to change multiple selections
>>>>> at the same time to get a valid combination.
>>>>>
>>>>> On devices where the control V4L2_CID_SELECTION_BITMASK does not exist,
>>>>> the id field is ignored
>>>>
>>>> This feels like a hack to me. A big problem I have with using a control here
>>>> is that with a control you can't specify for which selection target it is.
>>>>
>>>
>>> I am not sure that I understand what you mean here.
>>>
>>> If you set the control to 0x1 you are using selection 0, if you set
>>> the control to 0x5, you are using selection 0 and 2.
>>
>> If you look here:
>>
>> http://hverkuil.home.xs4all.nl/spec/media.html#v4l2-selection-targets
>>
>> you see that a selection has a 'target': i.e. does the selection define a crop
>> rectangle, a compose rectange, a default crop rectangle, etc.
>>
>> You are extending the API to support multiple rectangles per target and using
>> a control to select which rectangles are active (if I understand correctly).
>> But that control does not (and cannot) specify for which target the rectangles
>> should be activated.
> 
> I want to have N crop rectangles and N compose rectangles. Every crop
> rectangle has associated one compose rectangle.
> 
> This will fit multiple purposes ie:
> 
> - swap two areas of an image,
> - multiple readout zones
> - different decimation per area....
> 
> 
>>
>>>
>>>
>>>> If you want to set multiple rectangles, why not just pass them directly? E.g.:
>>>>
>>>> struct v4l2_selection {
>>>>         __u32                   type;
>>>>         __u32                   target;
>>>>         __u32                   flags;
>>>>         union {
>>>>                 struct v4l2_rect        r;
>>>>                 struct v4l2_rect        *pr;
>>>>         };
>>>>         __u32                   rectangles;
>>>>         __u32                   reserved[8];
>>>> };
>>>>
>>>> If rectangles > 1, then pr is used.
>>>>
>>>
>>> The structure is passed in a ioctl and I dont think that it is a good
>>> idea that you let the kernel get/set a memory address not encapsulated
>>> in it. I can see that it could lead to security breaches if there is a
>>> mistake on the handling.
>>
>> It's used in lots of places. It's OK, provided you check the memory carefully.
>> See e.g. how VIDIOC_G/S_EXT_CTRLS is handled. Usually the memory checks are done
>> in the v4l2 core and the driver doesn't need to take care of it.
>>
> 
> I agree IFF the v4l2 core does the checks. One question raises me: how
> does the user knows how big is the structure he has to allocate for
> g_selection? Do we have to make a new ioctl g_n_selections?

I would suggest using the same method that is used by the extended control API for
string controls: if rectangles is too small, then the driver sets the field to the
total number of rectangles and returns -ENOSPC. The application can then reallocate
the memory. I expect that applications that want to do this will actually know how
many rectangles to allocate.

>>>> It's a bit more work to add this to the core code (VIDIOC_SUBDEV_G/S_SELECTION
>>>> should probably be changed at the same time and you have to fix existing drivers
>>>> to check/set the new rectangles field), but it scales much better.
>>>
>>> Also, we would be broking the ABI. Rectangles is not a mandatory
>>> field, and has a value != 0.
>>
>> The spec clearly states that:
>>
>> "The flags and reserved fields of struct v4l2_selection are ignored and they must
>>  be filled with zeros."
>>
>> Any app not doing that is not obeying the API and hence it is an application bug.
>>
>> It's standard practice inside the V4L2 API to handle reserved fields in that way,
>> as it provides a mechanism to extend the API safely in the future.
>>
> 
> That is what I mean, the current programs are writing a 0 on that
> field, but now they are required to write a 1, so the API is broken.
> Maybe we should describe:
> if rectangles is 0, the field r is used (legacy), otherwise the pr,
> even for 1 (multi rectangle).

That's actually what I meant: if rectangles is 0, it will be interpreted as 1.
Sorry if I wasn't clear.

> 
>>>
>>> What we could do is leave the V4L2_CID_SELECTION_BITMASK  out of the
>>> api, but keep the id field on the structure, so the user can define a
>>> private control to do whatever he needs with the id field, wekeep the
>>> ABI compatibility and no big changes are needed.
>>
>> I really don't like that. It artificially limits the number of rectangles to 32
>> and makes the API just cumbersome to use.
> 
> I wasn't seeing the 32 rectangles as a limitation, but if you think
> that it is limiting, then the solution that you provide looks good.

For things like object detection 32 is quite limiting, yes.

> 
>>
>> The changes aren't difficult, just a bit tedious, and the end result does exactly
>> what you want and, I think, is also very useful for things like face detection or
>> object detection in general where a list of rectangles (or points, which is just a
>> 1x1 rectangle) has to be returned in an atomic manner.
>>
>> One thing that I am not certain about is whether v4l2_rect should be used when
>> specifying multiple rectangles. I can imagine that rectangles have an additional
>> type field (e.g. for face detection you might have rectangles for the left eye,
>> right eye and mouth).
> 
> That is where the id field was handy :), you could say rectangle
> id=LEFT_EYE and then have private controls for removing red component
> from rectangles which id is *_EYE.
> 
> My approach was:
> You use the s/g selection api to describe rectangles (input and
> output) and then you use bitmap controls to do things on the
> rectangles: compose,remove red eye, track.....

I'm confused, I thought this was about multiple crop rectangles? Bitmask
controls should definitely not be used to perform actions, that's not how
they should be used. Only a 'button' control can perform an action.

Basically I have no objection if the selection API is extended to support
rectangle lists to allow multi-crop/compose scenarios. But extending it for
effects like red eye removal is something that needs a lot more thought as
I am not certain whether the selection API is suitable for that.

Regards,

	Hans

> 
>>
>> Regards,
>>
>>         Hans
> 
> So
> 
> If the 32 rectangle limitation is a nogo for you, then I would suggest:
> 
> struct v4l2_selection {
>          __u32                   type;
>          __u32                   target;
>          __u32                   flags;
>           union {
>                  struct v4l2_rect        r;
>                  struct v4l2_rect        *pr;
>          };
>          __u32                   rectangles; //if 0, r is used,
> otherwise pr with rectangle components
>          __u32                   id;//0 for compose/crop , N->driver
> dependent (face tracking....)
>         __u32                   reserved[7];
> };
> 
> But:
>  -memory handling has to be done in the core
>  -we have to provide the user a way to know how many rectangles are in use.
> 
> 
> If the 32 rectangle limitiation is acceptable I still like my first proposal.
> But:
> 32 rectangles means 32 ioctls.
> 
> 
> Thanks for your feedback and promptly response :)
> 

Re: [PATCH] RFC: Support for multiple selections

[Ricardo Ribalda Delgado]

Hi Hans

On Wed, Sep 11, 2013 at 3:02 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> Hi Ricardo,
>
> On 09/11/2013 02:13 PM, Ricardo Ribalda Delgado wrote:
>> Hello Hans
>>
>> On Wed, Sep 11, 2013 at 12:49 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>> Hi Ricardo,
>>>
>>> On 09/11/2013 11:34 AM, Ricardo Ribalda Delgado wrote:
>>>> Hi Hans
>>>>
>>>> Thanks for your feedback
>>>>
>>>> On Wed, Sep 11, 2013 at 11:04 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>> Hi Ricardo,
>>>>>
>>>>> On 09/11/2013 10:30 AM, Ricardo Ribalda Delgado wrote:
>>>>>> A new id field is added to the struct selection. On devices that
>>>>>> supports multiple sections this id indicate which of the selection to
>>>>>> modify.
>>>>>>
>>>>>> A new control V4L2_CID_SELECTION_BITMASK selects which of the selections
>>>>>> are used, if the control is set to zero the default rectangles are used.
>>>>>>
>>>>>> This is needed in cases where the user has to change multiple selections
>>>>>> at the same time to get a valid combination.
>>>>>>
>>>>>> On devices where the control V4L2_CID_SELECTION_BITMASK does not exist,
>>>>>> the id field is ignored
>>>>>
>>>>> This feels like a hack to me. A big problem I have with using a control here
>>>>> is that with a control you can't specify for which selection target it is.
>>>>>
>>>>
>>>> I am not sure that I understand what you mean here.
>>>>
>>>> If you set the control to 0x1 you are using selection 0, if you set
>>>> the control to 0x5, you are using selection 0 and 2.
>>>
>>> If you look here:
>>>
>>> http://hverkuil.home.xs4all.nl/spec/media.html#v4l2-selection-targets
>>>
>>> you see that a selection has a 'target': i.e. does the selection define a crop
>>> rectangle, a compose rectange, a default crop rectangle, etc.
>>>
>>> You are extending the API to support multiple rectangles per target and using
>>> a control to select which rectangles are active (if I understand correctly).
>>> But that control does not (and cannot) specify for which target the rectangles
>>> should be activated.
>>
>> I want to have N crop rectangles and N compose rectangles. Every crop
>> rectangle has associated one compose rectangle.
>>
>> This will fit multiple purposes ie:
>>
>> - swap two areas of an image,
>> - multiple readout zones
>> - different decimation per area....
>>
>>
>>>
>>>>
>>>>
>>>>> If you want to set multiple rectangles, why not just pass them directly? E.g.:
>>>>>
>>>>> struct v4l2_selection {
>>>>>         __u32                   type;
>>>>>         __u32                   target;
>>>>>         __u32                   flags;
>>>>>         union {
>>>>>                 struct v4l2_rect        r;
>>>>>                 struct v4l2_rect        *pr;
>>>>>         };
>>>>>         __u32                   rectangles;
>>>>>         __u32                   reserved[8];
>>>>> };
>>>>>
>>>>> If rectangles > 1, then pr is used.
>>>>>
>>>>
>>>> The structure is passed in a ioctl and I dont think that it is a good
>>>> idea that you let the kernel get/set a memory address not encapsulated
>>>> in it. I can see that it could lead to security breaches if there is a
>>>> mistake on the handling.
>>>
>>> It's used in lots of places. It's OK, provided you check the memory carefully.
>>> See e.g. how VIDIOC_G/S_EXT_CTRLS is handled. Usually the memory checks are done
>>> in the v4l2 core and the driver doesn't need to take care of it.
>>>
>>
>> I agree IFF the v4l2 core does the checks. One question raises me: how
>> does the user knows how big is the structure he has to allocate for
>> g_selection? Do we have to make a new ioctl g_n_selections?
>
> I would suggest using the same method that is used by the extended control API for
> string controls: if rectangles is too small, then the driver sets the field to the
> total number of rectangles and returns -ENOSPC. The application can then reallocate
> the memory. I expect that applications that want to do this will actually know how
> many rectangles to allocate.
>

That seems fine for me.


>>>>> It's a bit more work to add this to the core code (VIDIOC_SUBDEV_G/S_SELECTION
>>>>> should probably be changed at the same time and you have to fix existing drivers
>>>>> to check/set the new rectangles field), but it scales much better.
>>>>
>>>> Also, we would be broking the ABI. Rectangles is not a mandatory
>>>> field, and has a value != 0.
>>>
>>> The spec clearly states that:
>>>
>>> "The flags and reserved fields of struct v4l2_selection are ignored and they must
>>>  be filled with zeros."
>>>
>>> Any app not doing that is not obeying the API and hence it is an application bug.
>>>
>>> It's standard practice inside the V4L2 API to handle reserved fields in that way,
>>> as it provides a mechanism to extend the API safely in the future.
>>>
>>
>> That is what I mean, the current programs are writing a 0 on that
>> field, but now they are required to write a 1, so the API is broken.
>> Maybe we should describe:
>> if rectangles is 0, the field r is used (legacy), otherwise the pr,
>> even for 1 (multi rectangle).
>
> That's actually what I meant: if rectangles is 0, it will be interpreted as 1.
> Sorry if I wasn't clear.
>
>>
>>>>
>>>> What we could do is leave the V4L2_CID_SELECTION_BITMASK  out of the
>>>> api, but keep the id field on the structure, so the user can define a
>>>> private control to do whatever he needs with the id field, wekeep the
>>>> ABI compatibility and no big changes are needed.
>>>
>>> I really don't like that. It artificially limits the number of rectangles to 32
>>> and makes the API just cumbersome to use.
>>
>> I wasn't seeing the 32 rectangles as a limitation, but if you think
>> that it is limiting, then the solution that you provide looks good.
>
> For things like object detection 32 is quite limiting, yes.
>
>>
>>>
>>> The changes aren't difficult, just a bit tedious, and the end result does exactly
>>> what you want and, I think, is also very useful for things like face detection or
>>> object detection in general where a list of rectangles (or points, which is just a
>>> 1x1 rectangle) has to be returned in an atomic manner.
>>>
>>> One thing that I am not certain about is whether v4l2_rect should be used when
>>> specifying multiple rectangles. I can imagine that rectangles have an additional
>>> type field (e.g. for face detection you might have rectangles for the left eye,
>>> right eye and mouth).
>>
>> That is where the id field was handy :), you could say rectangle
>> id=LEFT_EYE and then have private controls for removing red component
>> from rectangles which id is *_EYE.
>>
>> My approach was:
>> You use the s/g selection api to describe rectangles (input and
>> output) and then you use bitmap controls to do things on the
>> rectangles: compose,remove red eye, track.....
>
> I'm confused, I thought this was about multiple crop rectangles? Bitmask
> controls should definitely not be used to perform actions, that's not how
> they should be used. Only a 'button' control can perform an action.
>
> Basically I have no objection if the selection API is extended to support
> rectangle lists to allow multi-crop/compose scenarios. But extending it for
> effects like red eye removal is something that needs a lot more thought as
> I am not certain whether the selection API is suitable for that.
>

I only have a real need to extend if for multicrop/compose. But I
liked your idea of using the rectangles also for tracking objects.
That is why I added the id to the structure.

That way the selection api becomes a generic control for areas of the
image. But, we could just remove the id field, and use your original
idea. Leaving that discussion for later.

I will try to prepare a patch so we can talk about something real.
what do you think?

Thanks and best regards!

> Regards,
>
>         Hans
>
>>
>>>
>>> Regards,
>>>
>>>         Hans
>>
>> So
>>
>> If the 32 rectangle limitation is a nogo for you, then I would suggest:
>>
>> struct v4l2_selection {
>>          __u32                   type;
>>          __u32                   target;
>>          __u32                   flags;
>>           union {
>>                  struct v4l2_rect        r;
>>                  struct v4l2_rect        *pr;
>>          };
>>          __u32                   rectangles; //if 0, r is used,
>> otherwise pr with rectangle components
>>          __u32                   id;//0 for compose/crop , N->driver
>> dependent (face tracking....)
>>         __u32                   reserved[7];
>> };
>>
>> But:
>>  -memory handling has to be done in the core
>>  -we have to provide the user a way to know how many rectangles are in use.
>>
>>
>> If the 32 rectangle limitiation is acceptable I still like my first proposal.
>> But:
>> 32 rectangles means 32 ioctls.
>>
>>
>> Thanks for your feedback and promptly response :)
>>



-- 
Ricardo Ribalda

[PATCH] RFCv2: 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.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 54 +++++++++++++++++++++++++++++++-----
 include/uapi/linux/v4l2-subdev.h     | 10 +++++--
 include/uapi/linux/videodev2.h       | 15 ++++++++--
 3 files changed, 68 insertions(+), 11 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 68e6b5e..91d21a4 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\n",
+			prt_names(p->type, v4l2_type_names),
+			p->target, p->flags);
+		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)
@@ -1645,6 +1656,7 @@ static int v4l_g_crop(const struct v4l2_ioctl_ops *ops,
 	struct v4l2_crop *p = arg;
 	struct v4l2_selection s = {
 		.type = p->type,
+		.rectangles = 0,
 	};
 	int ret;
 
@@ -1673,6 +1685,7 @@ static int v4l_s_crop(const struct v4l2_ioctl_ops *ops,
 	struct v4l2_selection s = {
 		.type = p->type,
 		.r = p->c,
+		.rectangles = 0,
 	};
 
 	if (ops->vidioc_s_crop)
@@ -1692,7 +1705,10 @@ 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,
+		.rectangles = 0,
+	};
 	int ret;
 
 	if (ops->vidioc_cropcap)
@@ -1726,6 +1742,30 @@ static int v4l_cropcap(const struct v4l2_ioctl_ops *ops,
 	return 0;
 }
 
+static int v4l_s_selection(const struct v4l2_ioctl_ops *ops,
+				struct file *file, void *fh, void *arg)
+{
+	struct v4l2_selection *s = arg;
+
+	if (s->rectangles &&
+		!access_ok(VERIFY_READ, s->pr, s->rectangles * sizeof(*s->pr)))
+		return -EFAULT;
+
+	return ops->vidioc_s_selection(file, fh, s);
+}
+
+static int v4l_g_selection(const struct v4l2_ioctl_ops *ops,
+				struct file *file, void *fh, void *arg)
+{
+	struct v4l2_selection *s = arg;
+
+	if (s->rectangles &&
+		!access_ok(VERIFY_WRITE, s->pr, s->rectangles * sizeof(*s->pr)))
+		return -EFAULT;
+
+	return ops->vidioc_g_selection(file, fh, s);
+}
+
 static int v4l_log_status(const struct v4l2_ioctl_ops *ops,
 				struct file *file, void *fh, void *arg)
 {
@@ -2018,8 +2058,8 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
 	IOCTL_INFO_FNC(VIDIOC_CROPCAP, v4l_cropcap, v4l_print_cropcap, INFO_FL_CLEAR(v4l2_cropcap, type)),
 	IOCTL_INFO_FNC(VIDIOC_G_CROP, v4l_g_crop, v4l_print_crop, INFO_FL_CLEAR(v4l2_crop, type)),
 	IOCTL_INFO_FNC(VIDIOC_S_CROP, v4l_s_crop, v4l_print_crop, INFO_FL_PRIO),
-	IOCTL_INFO_STD(VIDIOC_G_SELECTION, vidioc_g_selection, v4l_print_selection, 0),
-	IOCTL_INFO_STD(VIDIOC_S_SELECTION, vidioc_s_selection, v4l_print_selection, INFO_FL_PRIO),
+	IOCTL_INFO_FNC(VIDIOC_G_SELECTION, v4l_g_selection, v4l_print_selection, 0),
+	IOCTL_INFO_FNC(VIDIOC_S_SELECTION, v4l_s_selection, v4l_print_selection, INFO_FL_PRIO),
 	IOCTL_INFO_STD(VIDIOC_G_JPEGCOMP, vidioc_g_jpegcomp, v4l_print_jpegcompression, 0),
 	IOCTL_INFO_STD(VIDIOC_S_JPEGCOMP, vidioc_s_jpegcomp, v4l_print_jpegcompression, INFO_FL_PRIO),
 	IOCTL_INFO_FNC(VIDIOC_QUERYSTD, v4l_querystd, v4l_print_std, 0),
diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
index a33c4da..b5ee08b 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 0e80472..691f73b 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -211,6 +211,11 @@ struct v4l2_rect {
 	__s32   height;
 };
 
+struct v4l2_ext_rect {
+	struct v4l2_rect r;
+	__u32   reserved[4];
+};
+
 struct v4l2_fract {
 	__u32   numerator;
 	__u32   denominator;
@@ -807,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.
@@ -817,8 +824,12 @@ 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];
 };
 
 
-- 
1.8.4.rc3