| Message ID | 1378888254-5236-1-git-send-email-ricardo.ribalda@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> </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]; > }; > > > -- 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
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 > >> -- 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
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 -- 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
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 :)
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 :) > -- 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
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 :) >>
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> </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]; };
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(-)