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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
