* RFC: add parameters to V4L controls @ 2013-01-07 10:46 Andrzej Hajda 2013-01-07 12:10 ` Hans Verkuil 2013-01-12 22:05 ` Sakari Ailus 0 siblings, 2 replies; 13+ messages in thread From: Andrzej Hajda @ 2013-01-07 10:46 UTC (permalink / raw) To: linux-media; +Cc: Sylwester Nawrocki Hi, I have included this proposition already in the post "[PATCH RFC 0/2] V4L: Add auto focus area control and selection" but it left unanswered. I repost it again in a separate e-mail, I hope this way it will be easier to attract attention. Problem description Currently V4L2 controls can have only single value (of type int, int64, string). Some hardware controls require more than single int parameter, for example to set auto-focus (AF) rectangle four coordinates should be passed, to set auto-focus spot two coordinates should be passed. Current solution In case of AF rectangle we can reuse selection API as in "[PATCH RFC 0/2] V4L: Add auto focus area control and selection" post. Pros: - reuse existing API, Cons: - two IOCTL's to perform one action, - non-atomic operation, - fits well only for rectangles and spots (but with unused fields width, height), in case of other parameters we should find a different way. Proposed solution The solution takes an advantage of the fact VIDIOC_(G/S/TRY)_EXT_CTRLS ioctls can be called with multiple controls per call. I will present it using AF area control example. There could be added four pseudo-controls, lets call them for short: LEFT, TOP, WIDTH, HEIGHT. Those controls could be passed together with V4L2_AUTO_FOCUS_AREA_RECTANGLE control in one ioctl as a kind of parameters. For example setting auto-focus spot would require calling VIDIOC_S_EXT_CTRLS with the following controls: - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE - LEFT = ... - RIGHT = ... Setting AF rectangle: - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE - LEFT = ... - TOP = ... - WIDTH = ... - HEIGHT = ... Setting AF object detection (no parameters required): - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION I have presented all three cases to show the advantages of this solution: - atomicity - control and its parameters are passed in one call, - flexibility - we are not limited by a fixed number of parameters, - no-redundancy - we can pass only required parameters (no need to pass null width and height in case of spot selection), - extensibility - it is possible to extend parameters in the future, for example add parameters to V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION, without breaking API, - backward compatibility, - re-usability - this schema could be used in other controls, pseudo-controls could be re-used in other controls as well. - API backward compatibility. Regards Andrzej Hajda ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: RFC: add parameters to V4L controls 2013-01-07 10:46 RFC: add parameters to V4L controls Andrzej Hajda @ 2013-01-07 12:10 ` Hans Verkuil 2013-01-07 20:02 ` Laurent Pinchart 2013-01-31 17:17 ` Sylwester Nawrocki 2013-01-12 22:05 ` Sakari Ailus 1 sibling, 2 replies; 13+ messages in thread From: Hans Verkuil @ 2013-01-07 12:10 UTC (permalink / raw) To: Andrzej Hajda Cc: linux-media, Sylwester Nawrocki, Sakari Ailus, Laurent Pinchart On Mon January 7 2013 11:46:38 Andrzej Hajda wrote: > Hi, > > I have included this proposition already in the post "[PATCH RFC 0/2] > V4L: Add auto focus area control and selection" but it left unanswered. > I repost it again in a separate e-mail, I hope this way it will be > easier to attract attention. > > Problem description > > Currently V4L2 controls can have only single value (of type int, int64, > string). Some hardware controls require more than single int parameter, > for example to set auto-focus (AF) rectangle four coordinates should be > passed, to set auto-focus spot two coordinates should be passed. > > Current solution > > In case of AF rectangle we can reuse selection API as in "[PATCH RFC > 0/2] V4L: Add auto focus area control and selection" post. > Pros: > - reuse existing API, > Cons: > - two IOCTL's to perform one action, > - non-atomic operation, > - fits well only for rectangles and spots (but with unused fields width, > height), in case of other parameters we should find a different way. > > Proposed solution > > The solution takes an advantage of the fact VIDIOC_(G/S/TRY)_EXT_CTRLS > ioctls can be called with multiple controls per call. > > I will present it using AF area control example. > > There could be added four pseudo-controls, lets call them for short: > LEFT, TOP, WIDTH, HEIGHT. > Those controls could be passed together with V4L2_AUTO_FOCUS_AREA_RECTANGLE > control in one ioctl as a kind of parameters. > > For example setting auto-focus spot would require calling VIDIOC_S_EXT_CTRLS > with the following controls: > - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE > - LEFT = ... > - RIGHT = ... > > Setting AF rectangle: > - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE > - LEFT = ... > - TOP = ... > - WIDTH = ... > - HEIGHT = ... > > Setting AF object detection (no parameters required): > - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION If you want to do this, then you have to make LEFT/TOP/WIDTH/HEIGHT real controls. There is no such thing as a pseudo control. So you need five new controls in total: V4L2_CID_AUTO_FOCUS_AREA V4L2_CID_AUTO_FOCUS_LEFT V4L2_CID_AUTO_FOCUS_RIGHT V4L2_CID_AUTO_FOCUS_WIDTH V4L2_CID_AUTO_FOCUS_HEIGHT I have no problem with this from the point of view of the control API, but whether this is the best solution for implementing auto-focus is a different issue and input from sensor specialists is needed as well (added Laurent and Sakari to the CC list). The primary concern I have is that this does not scale to multiple focus rectangles. This might not be relevant to auto focus, though. Regards, Hans > > I have presented all three cases to show the advantages of this solution: > - atomicity - control and its parameters are passed in one call, > - flexibility - we are not limited by a fixed number of parameters, > - no-redundancy - we can pass only required parameters > (no need to pass null width and height in case of spot selection), > - extensibility - it is possible to extend parameters in the future, > for example add parameters to V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION, > without breaking API, > - backward compatibility, > - re-usability - this schema could be used in other controls, > pseudo-controls could be re-used in other controls as well. > - API backward compatibility. > > > Regards > Andrzej Hajda > -- > 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: RFC: add parameters to V4L controls 2013-01-07 12:10 ` Hans Verkuil @ 2013-01-07 20:02 ` Laurent Pinchart 2013-01-08 12:26 ` Andrzej Hajda 2013-01-31 17:17 ` Sylwester Nawrocki 1 sibling, 1 reply; 13+ messages in thread From: Laurent Pinchart @ 2013-01-07 20:02 UTC (permalink / raw) To: Hans Verkuil; +Cc: Andrzej Hajda, linux-media, Sylwester Nawrocki, Sakari Ailus Hi Hans, On Monday 07 January 2013 13:10:54 Hans Verkuil wrote: > On Mon January 7 2013 11:46:38 Andrzej Hajda wrote: > > Hi, > > > > I have included this proposition already in the post "[PATCH RFC 0/2] > > V4L: Add auto focus area control and selection" but it left unanswered. > > I repost it again in a separate e-mail, I hope this way it will be > > easier to attract attention. > > > > Problem description > > > > Currently V4L2 controls can have only single value (of type int, int64, > > string). Some hardware controls require more than single int parameter, > > for example to set auto-focus (AF) rectangle four coordinates should be > > passed, to set auto-focus spot two coordinates should be passed. > > > > Current solution > > > > In case of AF rectangle we can reuse selection API as in "[PATCH RFC > > 0/2] V4L: Add auto focus area control and selection" post. > > Pros: > > - reuse existing API, > > Cons: > > - two IOCTL's to perform one action, > > - non-atomic operation, Is it really an issue in your use cases, or is this only a theoretical problem ? > > - fits well only for rectangles and spots (but with unused fields width, > > height), in case of other parameters we should find a different way. > > > > Proposed solution > > > > The solution takes an advantage of the fact VIDIOC_(G/S/TRY)_EXT_CTRLS > > ioctls can be called with multiple controls per call. > > > > I will present it using AF area control example. > > > > There could be added four pseudo-controls, lets call them for short: > > LEFT, TOP, WIDTH, HEIGHT. > > Those controls could be passed together with > > V4L2_AUTO_FOCUS_AREA_RECTANGLE > > control in one ioctl as a kind of parameters. > > > > For example setting auto-focus spot would require calling > > VIDIOC_S_EXT_CTRLS with the following controls: > > - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE > > - LEFT = ... > > - RIGHT = ... > > > > Setting AF rectangle: > > - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE > > - LEFT = ... > > - TOP = ... > > - WIDTH = ... > > - HEIGHT = ... > > > > Setting AF object detection (no parameters required): > > - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION > > If you want to do this, then you have to make LEFT/TOP/WIDTH/HEIGHT real > controls. There is no such thing as a pseudo control. So you need five > new controls in total: > > V4L2_CID_AUTO_FOCUS_AREA > V4L2_CID_AUTO_FOCUS_LEFT > V4L2_CID_AUTO_FOCUS_RIGHT > V4L2_CID_AUTO_FOCUS_WIDTH > V4L2_CID_AUTO_FOCUS_HEIGHT > > I have no problem with this from the point of view of the control API, but > whether this is the best solution for implementing auto-focus is a different > issue and input from sensor specialists is needed as well (added Laurent > and Sakari to the CC list). > > The primary concern I have is that this does not scale to multiple focus > rectangles. This might not be relevant to auto focus, though. Time to implement a rectangle control type ? Or to make it possible to issue atomic transactions across ioctls ? We've been postponing this discussion for ages. > > I have presented all three cases to show the advantages of this solution: > > - atomicity - control and its parameters are passed in one call, > > - flexibility - we are not limited by a fixed number of parameters, > > - no-redundancy - we can pass only required parameters > > > > (no need to pass null width and height in case of spot selection), > > > > - extensibility - it is possible to extend parameters in the future, > > for example add parameters to V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION, > > without breaking API, > > - backward compatibility, > > - re-usability - this schema could be used in other controls, > > > > pseudo-controls could be re-used in other controls as well. > > > > - API backward compatibility. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: RFC: add parameters to V4L controls 2013-01-07 20:02 ` Laurent Pinchart @ 2013-01-08 12:26 ` Andrzej Hajda 0 siblings, 0 replies; 13+ messages in thread From: Andrzej Hajda @ 2013-01-08 12:26 UTC (permalink / raw) To: Laurent Pinchart Cc: Hans Verkuil, linux-media, Sylwester Nawrocki, Sakari Ailus On 07.01.2013 21:02, Laurent Pinchart wrote: > Hi Hans, > > On Monday 07 January 2013 13:10:54 Hans Verkuil wrote: >> On Mon January 7 2013 11:46:38 Andrzej Hajda wrote: >>> Hi, >>> >>> I have included this proposition already in the post "[PATCH RFC 0/2] >>> V4L: Add auto focus area control and selection" but it left unanswered. >>> I repost it again in a separate e-mail, I hope this way it will be >>> easier to attract attention. >>> >>> Problem description >>> >>> Currently V4L2 controls can have only single value (of type int, int64, >>> string). Some hardware controls require more than single int parameter, >>> for example to set auto-focus (AF) rectangle four coordinates should be >>> passed, to set auto-focus spot two coordinates should be passed. >>> >>> Current solution >>> >>> In case of AF rectangle we can reuse selection API as in "[PATCH RFC >>> 0/2] V4L: Add auto focus area control and selection" post. >>> Pros: >>> - reuse existing API, >>> Cons: >>> - two IOCTL's to perform one action, >>> - non-atomic operation, > Is it really an issue in your use cases, or is this only a theoretical problem > ? Currently it is not a blocker, but it shows additional issues which we should deal with, for example: - which ioctls should trigger the AF action, - default values for AF selection rectangle/spot, which should be (probably) reset after each change of format on device/pad. Documenting it will make the situation clear for app/driver developers, but with atomic ioctl we could just forget about it. Btw. there is already discussion about it in '[PATCH RFC 2/2] V4L: Add V4L2_CID_AUTO_FOCUS_AREA control'. > >>> - fits well only for rectangles and spots (but with unused fields width, >>> height), in case of other parameters we should find a different way. >>> >>> Proposed solution >>> >>> The solution takes an advantage of the fact VIDIOC_(G/S/TRY)_EXT_CTRLS >>> ioctls can be called with multiple controls per call. >>> >>> I will present it using AF area control example. >>> >>> There could be added four pseudo-controls, lets call them for short: >>> LEFT, TOP, WIDTH, HEIGHT. >>> Those controls could be passed together with >>> V4L2_AUTO_FOCUS_AREA_RECTANGLE >>> control in one ioctl as a kind of parameters. >>> >>> For example setting auto-focus spot would require calling >>> VIDIOC_S_EXT_CTRLS with the following controls: >>> - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE >>> - LEFT = ... >>> - RIGHT = ... >>> >>> Setting AF rectangle: >>> - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE >>> - LEFT = ... >>> - TOP = ... >>> - WIDTH = ... >>> - HEIGHT = ... >>> >>> Setting AF object detection (no parameters required): >>> - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION >> If you want to do this, then you have to make LEFT/TOP/WIDTH/HEIGHT real >> controls. There is no such thing as a pseudo control. So you need five >> new controls in total: >> >> V4L2_CID_AUTO_FOCUS_AREA >> V4L2_CID_AUTO_FOCUS_LEFT >> V4L2_CID_AUTO_FOCUS_RIGHT >> V4L2_CID_AUTO_FOCUS_WIDTH >> V4L2_CID_AUTO_FOCUS_HEIGHT >> >> I have no problem with this from the point of view of the control API, but >> whether this is the best solution for implementing auto-focus is a different >> issue and input from sensor specialists is needed as well (added Laurent >> and Sakari to the CC list). >> >> The primary concern I have is that this does not scale to multiple focus >> rectangles. This might not be relevant to auto focus, though. > Time to implement a rectangle control type ? Or to make it possible to issue > atomic transactions across ioctls ? We've been postponing this discussion for > ages. > >>> I have presented all three cases to show the advantages of this solution: >>> - atomicity - control and its parameters are passed in one call, >>> - flexibility - we are not limited by a fixed number of parameters, >>> - no-redundancy - we can pass only required parameters >>> >>> (no need to pass null width and height in case of spot selection), >>> >>> - extensibility - it is possible to extend parameters in the future, >>> for example add parameters to V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION, >>> without breaking API, >>> - backward compatibility, >>> - re-usability - this schema could be used in other controls, >>> >>> pseudo-controls could be re-used in other controls as well. >>> >>> - API backward compatibility. Regards, Andrzej ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: RFC: add parameters to V4L controls 2013-01-07 12:10 ` Hans Verkuil 2013-01-07 20:02 ` Laurent Pinchart @ 2013-01-31 17:17 ` Sylwester Nawrocki 2013-02-01 22:17 ` Laurent Pinchart 1 sibling, 1 reply; 13+ messages in thread From: Sylwester Nawrocki @ 2013-01-31 17:17 UTC (permalink / raw) To: Hans Verkuil; +Cc: Andrzej Hajda, linux-media, Sakari Ailus, Laurent Pinchart Hi, On 01/07/2013 01:10 PM, Hans Verkuil wrote: > On Mon January 7 2013 11:46:38 Andrzej Hajda wrote: [...] >> Currently V4L2 controls can have only single value (of type int, int64, >> string). Some hardware controls require more than single int parameter, >> for example to set auto-focus (AF) rectangle four coordinates should be >> passed, to set auto-focus spot two coordinates should be passed. >> >> Current solution >> >> In case of AF rectangle we can reuse selection API as in "[PATCH RFC >> 0/2] V4L: Add auto focus area control and selection" post. >> Pros: >> - reuse existing API, >> Cons: >> - two IOCTL's to perform one action, >> - non-atomic operation, >> - fits well only for rectangles and spots (but with unused fields width, >> height), in case of other parameters we should find a different way. >> >> Proposed solution >> >> The solution takes an advantage of the fact VIDIOC_(G/S/TRY)_EXT_CTRLS >> ioctls can be called with multiple controls per call. >> >> I will present it using AF area control example. >> >> There could be added four pseudo-controls, lets call them for short: >> LEFT, TOP, WIDTH, HEIGHT. >> Those controls could be passed together with V4L2_AUTO_FOCUS_AREA_RECTANGLE >> control in one ioctl as a kind of parameters. >> >> For example setting auto-focus spot would require calling VIDIOC_S_EXT_CTRLS >> with the following controls: >> - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE >> - LEFT = ... >> - RIGHT = ... >> >> Setting AF rectangle: >> - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE >> - LEFT = ... >> - TOP = ... >> - WIDTH = ... >> - HEIGHT = ... >> >> Setting AF object detection (no parameters required): >> - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION > > If you want to do this, then you have to make LEFT/TOP/WIDTH/HEIGHT real > controls. There is no such thing as a pseudo control. So you need five > new controls in total: > > V4L2_CID_AUTO_FOCUS_AREA > V4L2_CID_AUTO_FOCUS_LEFT > V4L2_CID_AUTO_FOCUS_RIGHT > V4L2_CID_AUTO_FOCUS_WIDTH > V4L2_CID_AUTO_FOCUS_HEIGHT > > I have no problem with this from the point of view of the control API, but > whether this is the best solution for implementing auto-focus is a different > issue and input from sensor specialists is needed as well (added Laurent and > Sakari to the CC list). > > The primary concern I have is that this does not scale to multiple focus > rectangles. This might not be relevant to auto focus, though. I think for more advanced hardware/configurations there is a need to associate more information with the rectangles anyway. So the selections API seems too limited. Probably a new IOCTL would be needed for that, either standard or private. We've discussed it here with Andrzej and using such 4 controls to specify the AF rectangle looks sufficient from our POV. I would just probably rename LEFT/RIGHT to POS_X/POS_Y or something, as these 2 controls could be used in a focus mode where only spot position needs to be specified. -- Thanks, Sylwester ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: RFC: add parameters to V4L controls 2013-01-31 17:17 ` Sylwester Nawrocki @ 2013-02-01 22:17 ` Laurent Pinchart 2013-02-03 18:53 ` Sylwester Nawrocki 0 siblings, 1 reply; 13+ messages in thread From: Laurent Pinchart @ 2013-02-01 22:17 UTC (permalink / raw) To: Sylwester Nawrocki; +Cc: Hans Verkuil, Andrzej Hajda, linux-media, Sakari Ailus Hi Sylwester, On Thursday 31 January 2013 18:17:42 Sylwester Nawrocki wrote: > On 01/07/2013 01:10 PM, Hans Verkuil wrote: > > On Mon January 7 2013 11:46:38 Andrzej Hajda wrote: > [...] > > >> Currently V4L2 controls can have only single value (of type int, int64, > >> string). Some hardware controls require more than single int parameter, > >> for example to set auto-focus (AF) rectangle four coordinates should be > >> passed, to set auto-focus spot two coordinates should be passed. > >> > >> Current solution > >> > >> In case of AF rectangle we can reuse selection API as in "[PATCH RFC > >> 0/2] V4L: Add auto focus area control and selection" post. > >> Pros: > >> - reuse existing API, > >> Cons: > >> - two IOCTL's to perform one action, > >> - non-atomic operation, > >> - fits well only for rectangles and spots (but with unused fields width, > >> height), in case of other parameters we should find a different way. > >> > >> Proposed solution > >> > >> The solution takes an advantage of the fact VIDIOC_(G/S/TRY)_EXT_CTRLS > >> ioctls can be called with multiple controls per call. > >> > >> I will present it using AF area control example. > >> > >> There could be added four pseudo-controls, lets call them for short: > >> LEFT, TOP, WIDTH, HEIGHT. Those controls could be passed together with > >> V4L2_AUTO_FOCUS_AREA_RECTANGLE control in one ioctl as a kind of > >> parameters. > >> > >> For example setting auto-focus spot would require calling > >> VIDIOC_S_EXT_CTRLS with the following controls: > >> - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE > >> - LEFT = ... > >> - RIGHT = ... > >> > >> Setting AF rectangle: > >> - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE > >> - LEFT = ... > >> - TOP = ... > >> - WIDTH = ... > >> - HEIGHT = ... > >> > >> Setting AF object detection (no parameters required): > >> - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION > > > > If you want to do this, then you have to make LEFT/TOP/WIDTH/HEIGHT real > > controls. There is no such thing as a pseudo control. So you need five > > new controls in total: > > > > V4L2_CID_AUTO_FOCUS_AREA > > V4L2_CID_AUTO_FOCUS_LEFT > > V4L2_CID_AUTO_FOCUS_RIGHT > > V4L2_CID_AUTO_FOCUS_WIDTH > > V4L2_CID_AUTO_FOCUS_HEIGHT > > > > I have no problem with this from the point of view of the control API, but > > whether this is the best solution for implementing auto-focus is a > > different issue and input from sensor specialists is needed as well > > (added Laurent and Sakari to the CC list). > > > > The primary concern I have is that this does not scale to multiple focus > > rectangles. This might not be relevant to auto focus, though. > > I think for more advanced hardware/configurations there is a need to > associate more information with the rectangles anyway. So the selections > API seems too limited. Probably a new IOCTL would be needed for that, > either standard or private. > > We've discussed it here with Andrzej and using such 4 controls to specify > the AF rectangle looks sufficient from our POV. > > I would just probably rename LEFT/RIGHT to POS_X/POS_Y or something, > as these 2 controls could be used in a focus mode where only spot > position needs to be specified. If position and size are sufficient, could we use the selection API instead ? An alternative would be to introduce rectangle controls. I'm a bit uncomfortable with using 4 controls here, as this could quickly grow out of control. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: RFC: add parameters to V4L controls 2013-02-01 22:17 ` Laurent Pinchart @ 2013-02-03 18:53 ` Sylwester Nawrocki 2013-02-06 20:26 ` Sakari Ailus 0 siblings, 1 reply; 13+ messages in thread From: Sylwester Nawrocki @ 2013-02-03 18:53 UTC (permalink / raw) To: Laurent Pinchart Cc: Sylwester Nawrocki, Hans Verkuil, Andrzej Hajda, linux-media, Sakari Ailus Hi Laurent, On 02/01/2013 11:17 PM, Laurent Pinchart wrote: [...] >>>> There could be added four pseudo-controls, lets call them for short: >>>> LEFT, TOP, WIDTH, HEIGHT. Those controls could be passed together with >>>> V4L2_AUTO_FOCUS_AREA_RECTANGLE control in one ioctl as a kind of >>>> parameters. >>>> >>>> For example setting auto-focus spot would require calling >>>> VIDIOC_S_EXT_CTRLS with the following controls: >>>> - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE >>>> - LEFT = ... >>>> - RIGHT = ... >>>> >>>> Setting AF rectangle: >>>> - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE >>>> - LEFT = ... >>>> - TOP = ... >>>> - WIDTH = ... >>>> - HEIGHT = ... >>>> >>>> Setting AF object detection (no parameters required): >>>> - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION >>> >>> If you want to do this, then you have to make LEFT/TOP/WIDTH/HEIGHT real >>> controls. There is no such thing as a pseudo control. So you need five >>> new controls in total: >>> >>> V4L2_CID_AUTO_FOCUS_AREA >>> V4L2_CID_AUTO_FOCUS_LEFT >>> V4L2_CID_AUTO_FOCUS_RIGHT >>> V4L2_CID_AUTO_FOCUS_WIDTH >>> V4L2_CID_AUTO_FOCUS_HEIGHT >>> >>> I have no problem with this from the point of view of the control API, but >>> whether this is the best solution for implementing auto-focus is a >>> different issue and input from sensor specialists is needed as well >>> (added Laurent and Sakari to the CC list). >>> >>> The primary concern I have is that this does not scale to multiple focus >>> rectangles. This might not be relevant to auto focus, though. >> >> I think for more advanced hardware/configurations there is a need to >> associate more information with the rectangles anyway. So the selections >> API seems too limited. Probably a new IOCTL would be needed for that, >> either standard or private. >> >> We've discussed it here with Andrzej and using such 4 controls to specify >> the AF rectangle looks sufficient from our POV. >> >> I would just probably rename LEFT/RIGHT to POS_X/POS_Y or something, >> as these 2 controls could be used in a focus mode where only spot >> position needs to be specified. > > If position and size are sufficient, could we use the selection API instead ? > An alternative would be to introduce rectangle controls. I'm a bit > uncomfortable with using 4 controls here, as this could quickly grow out of > control. Yes, the selection API could be used as well. I actually have tested this in the past with the s5c73m3 camera and its spot auto focus mode. I just wanted to be sure there is no better alternatives, as it looked a bit unusual to handle single feature with a mix of the controls and the selection API calls. Although it works, such an interface looks a bit clumsy to me, especially in cases where all we need is to pass just (x,y) coordinates. I have quickly added support for rectangle controls type [1] to see how big changes it would require and what would be missing without significant changes in the controls API. So the main issues there are: the min/max/step/default value cannot be queried (VIDIOC_QUERYCTRL) and it is troublesome to handle them in the kernel, the control value change events wouldn't really work. I learnt VIDIOC_QUERYCTRL is not supported for V4L2_CTRL_TYPE_INTEGER64 control type, then maybe we could have similarly some features not available for V4L2_CTRL_TYPE_RECTANGLE ? Until there are further extensions that address this;) [1] http://git.linuxtv.org/snawrocki/media.git/ov965x-2-rect-type-ctrl -- Regards, Sylwester ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: RFC: add parameters to V4L controls 2013-02-03 18:53 ` Sylwester Nawrocki @ 2013-02-06 20:26 ` Sakari Ailus 2013-02-10 19:44 ` Sylwester Nawrocki 2013-02-12 8:14 ` Hans Verkuil 0 siblings, 2 replies; 13+ messages in thread From: Sakari Ailus @ 2013-02-06 20:26 UTC (permalink / raw) To: Sylwester Nawrocki Cc: Laurent Pinchart, Sylwester Nawrocki, Hans Verkuil, Andrzej Hajda, linux-media Hi Sylwester, On Sun, Feb 03, 2013 at 07:53:50PM +0100, Sylwester Nawrocki wrote: > Hi Laurent, > > On 02/01/2013 11:17 PM, Laurent Pinchart wrote: > [...] > >>>>There could be added four pseudo-controls, lets call them for short: > >>>>LEFT, TOP, WIDTH, HEIGHT. Those controls could be passed together with > >>>>V4L2_AUTO_FOCUS_AREA_RECTANGLE control in one ioctl as a kind of > >>>>parameters. > >>>> > >>>>For example setting auto-focus spot would require calling > >>>>VIDIOC_S_EXT_CTRLS with the following controls: > >>>>- V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE > >>>>- LEFT = ... > >>>>- RIGHT = ... > >>>> > >>>>Setting AF rectangle: > >>>>- V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE > >>>>- LEFT = ... > >>>>- TOP = ... > >>>>- WIDTH = ... > >>>>- HEIGHT = ... > >>>> > >>>>Setting AF object detection (no parameters required): > >>>>- V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION > >>> > >>>If you want to do this, then you have to make LEFT/TOP/WIDTH/HEIGHT real > >>>controls. There is no such thing as a pseudo control. So you need five > >>>new controls in total: > >>> > >>>V4L2_CID_AUTO_FOCUS_AREA > >>>V4L2_CID_AUTO_FOCUS_LEFT > >>>V4L2_CID_AUTO_FOCUS_RIGHT > >>>V4L2_CID_AUTO_FOCUS_WIDTH > >>>V4L2_CID_AUTO_FOCUS_HEIGHT > >>> > >>>I have no problem with this from the point of view of the control API, but > >>>whether this is the best solution for implementing auto-focus is a > >>>different issue and input from sensor specialists is needed as well > >>>(added Laurent and Sakari to the CC list). > >>> > >>>The primary concern I have is that this does not scale to multiple focus > >>>rectangles. This might not be relevant to auto focus, though. > >> > >>I think for more advanced hardware/configurations there is a need to > >>associate more information with the rectangles anyway. So the selections > >>API seems too limited. Probably a new IOCTL would be needed for that, > >>either standard or private. > >> > >>We've discussed it here with Andrzej and using such 4 controls to specify > >>the AF rectangle looks sufficient from our POV. > >> > >>I would just probably rename LEFT/RIGHT to POS_X/POS_Y or something, > >>as these 2 controls could be used in a focus mode where only spot > >>position needs to be specified. > > > >If position and size are sufficient, could we use the selection API instead ? > >An alternative would be to introduce rectangle controls. I'm a bit > >uncomfortable with using 4 controls here, as this could quickly grow out of > >control. > > Yes, the selection API could be used as well. I actually have tested this > in the past with the s5c73m3 camera and its spot auto focus mode. > > I just wanted to be sure there is no better alternatives, as it looked > a bit unusual to handle single feature with a mix of the controls and > the selection API calls. Although it works, such an interface looks a bit > clumsy to me, especially in cases where all we need is to pass just (x,y) > coordinates. Selections are essentially controls but for rectangles. The original use case was to support configuring scaling, cropping etc. on subdevs but they're not really bound to image processing configuration. Controls have been more generic to begin with. > I have quickly added support for rectangle controls type [1] to see how > big changes it would require and what would be missing without significant > changes in the controls API. > > So the main issues there are: the min/max/step/default value cannot > be queried (VIDIOC_QUERYCTRL) and it is troublesome to handle them in > the kernel, the control value change events wouldn't really work. > > I learnt VIDIOC_QUERYCTRL is not supported for V4L2_CTRL_TYPE_INTEGER64 > control type, then maybe we could have similarly some features not > available for V4L2_CTRL_TYPE_RECTANGLE ? Until there are further > extensions that address this;) > > [1] http://git.linuxtv.org/snawrocki/media.git/ov965x-2-rect-type-ctrl Hmm. Had you proposed this two years ago, selections could well look entirely different. We still have them now. There would be use cases for pad specific controls, too; pixel rate for instance should be one. For this reason I don't see selections really much different from controls. The selections are the same on subdevs and video nodes. Unifying them (with some compat code for either of the current interfaces) and providing a new IOCTL to access both was what I thought could be one solution to the problem. Or --- we could add "selection controls" which would be just like selections but with the control interface. What's relevant in struct v4l2_ext_control would be the ID field, while the "value" field in struct v4l2_ext_control would be a pointer to a struct describing the selection control. Half of the reserved field could be used for the pad (they're 16-bit ints). No control ID clashes with the selection IDs, so this could even work with the existing selection targets. Either solution would avoid creating another rectangle type with an ID that would be separate from selections. Thoughts, comments? -- Kind regards, Sakari Ailus e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: RFC: add parameters to V4L controls 2013-02-06 20:26 ` Sakari Ailus @ 2013-02-10 19:44 ` Sylwester Nawrocki 2013-02-12 8:14 ` Hans Verkuil 1 sibling, 0 replies; 13+ messages in thread From: Sylwester Nawrocki @ 2013-02-10 19:44 UTC (permalink / raw) To: Sakari Ailus Cc: Laurent Pinchart, Sylwester Nawrocki, Hans Verkuil, Andrzej Hajda, linux-media Hi Sakari, Thanks for the feedback! On 02/06/2013 09:26 PM, Sakari Ailus wrote: > Selections are essentially controls but for rectangles. The selection API was originally designed as a replacement for the cropping, "insertion" and scaling API (VIDIOC_*_CROP* ioctls), in order to improve it. So now we have Image Cropping, Composing and Scaling API on /dev/video* interfaces and for the subdevs the selections definition appears more generic, with current supported feature set exactly same as for /dev/video*. The selection API wasn't meant as a generic interface to configure whatever rectangles at the beginning. After use cases popped up, we've said - maybe we can reuse it for camera auto focus or exposure metering ROI selection, etc. Hence I disagree the selection API _is_ as generic as the controls API. We might wish it to be, but it currently is not. It now consistently covers image cropping, composing and scaling. I'm not sure it is a good idea to try to design "primitive sub-APIs" and then have something useful by combining a few of them. IMHO it is going to be painful for the user and the kernel space. > The original use case was to support configuring scaling, cropping etc. on > subdevs but they're not really bound to image processing configuration. We had first support for the selection API on video nodes and only after that on subdevs, where the meaning of the selection API was made a bit more generic. > Controls have been more generic to begin with. Agreed. >> I have quickly added support for rectangle controls type [1] to see how >> big changes it would require and what would be missing without significant >> changes in the controls API. >> >> So the main issues there are: the min/max/step/default value cannot >> be queried (VIDIOC_QUERYCTRL) and it is troublesome to handle them in >> the kernel, the control value change events wouldn't really work. >> >> I learnt VIDIOC_QUERYCTRL is not supported for V4L2_CTRL_TYPE_INTEGER64 >> control type, then maybe we could have similarly some features not >> available for V4L2_CTRL_TYPE_RECTANGLE ? Until there are further >> extensions that address this;) >> >> [1] http://git.linuxtv.org/snawrocki/media.git/ov965x-2-rect-type-ctrl > > Hmm. Had you proposed this two years ago, selections could well look > entirely different. > > We still have them now. There would be use cases for pad specific controls, > too; pixel rate for instance should be one. For this reason I don't see > selections really much different from controls. > > The selections are the same on subdevs and video nodes. Unifying them (with > some compat code for either of the current interfaces) and providing a new > IOCTL to access both was what I thought could be one solution to the > problem. Subdevs are not supposed to be accessed by generic applications, are they ? Then what would we need a compat ioctl for ? Should /dev/video drivers just pass-through selection ioctls with selection targets they don't support to their respective subdevs ? > Or --- we could add "selection controls" which would be just like selections > but with the control interface. What's relevant in struct v4l2_ext_control > would be the ID field, while the "value" field in struct v4l2_ext_control > would be a pointer to a struct describing the selection control. Half of the > reserved field could be used for the pad (they're 16-bit ints). No control > ID clashes with the selection IDs, so this could even work with the existing > selection targets. > > Either solution would avoid creating another rectangle type with an ID that > would be separate from selections. > > Thoughts, comments? I would rather have clear separation of what the VIDIOC_*_SELECTION are intended for and what can be done with the controls API. Let's not emulate the existing selection API through the controls API. -- Regards, Sylwester ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: RFC: add parameters to V4L controls 2013-02-06 20:26 ` Sakari Ailus 2013-02-10 19:44 ` Sylwester Nawrocki @ 2013-02-12 8:14 ` Hans Verkuil 1 sibling, 0 replies; 13+ messages in thread From: Hans Verkuil @ 2013-02-12 8:14 UTC (permalink / raw) To: Sakari Ailus Cc: Sylwester Nawrocki, Laurent Pinchart, Sylwester Nawrocki, Andrzej Hajda, linux-media On Wed February 6 2013 21:26:32 Sakari Ailus wrote: > Hi Sylwester, > > On Sun, Feb 03, 2013 at 07:53:50PM +0100, Sylwester Nawrocki wrote: > > Hi Laurent, > > > > On 02/01/2013 11:17 PM, Laurent Pinchart wrote: > > [...] > > >>>>There could be added four pseudo-controls, lets call them for short: > > >>>>LEFT, TOP, WIDTH, HEIGHT. Those controls could be passed together with > > >>>>V4L2_AUTO_FOCUS_AREA_RECTANGLE control in one ioctl as a kind of > > >>>>parameters. > > >>>> > > >>>>For example setting auto-focus spot would require calling > > >>>>VIDIOC_S_EXT_CTRLS with the following controls: > > >>>>- V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE > > >>>>- LEFT = ... > > >>>>- RIGHT = ... > > >>>> > > >>>>Setting AF rectangle: > > >>>>- V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE > > >>>>- LEFT = ... > > >>>>- TOP = ... > > >>>>- WIDTH = ... > > >>>>- HEIGHT = ... > > >>>> > > >>>>Setting AF object detection (no parameters required): > > >>>>- V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION > > >>> > > >>>If you want to do this, then you have to make LEFT/TOP/WIDTH/HEIGHT real > > >>>controls. There is no such thing as a pseudo control. So you need five > > >>>new controls in total: > > >>> > > >>>V4L2_CID_AUTO_FOCUS_AREA > > >>>V4L2_CID_AUTO_FOCUS_LEFT > > >>>V4L2_CID_AUTO_FOCUS_RIGHT > > >>>V4L2_CID_AUTO_FOCUS_WIDTH > > >>>V4L2_CID_AUTO_FOCUS_HEIGHT > > >>> > > >>>I have no problem with this from the point of view of the control API, but > > >>>whether this is the best solution for implementing auto-focus is a > > >>>different issue and input from sensor specialists is needed as well > > >>>(added Laurent and Sakari to the CC list). > > >>> > > >>>The primary concern I have is that this does not scale to multiple focus > > >>>rectangles. This might not be relevant to auto focus, though. > > >> > > >>I think for more advanced hardware/configurations there is a need to > > >>associate more information with the rectangles anyway. So the selections > > >>API seems too limited. Probably a new IOCTL would be needed for that, > > >>either standard or private. > > >> > > >>We've discussed it here with Andrzej and using such 4 controls to specify > > >>the AF rectangle looks sufficient from our POV. > > >> > > >>I would just probably rename LEFT/RIGHT to POS_X/POS_Y or something, > > >>as these 2 controls could be used in a focus mode where only spot > > >>position needs to be specified. > > > > > >If position and size are sufficient, could we use the selection API instead ? > > >An alternative would be to introduce rectangle controls. I'm a bit > > >uncomfortable with using 4 controls here, as this could quickly grow out of > > >control. > > > > Yes, the selection API could be used as well. I actually have tested this > > in the past with the s5c73m3 camera and its spot auto focus mode. > > > > I just wanted to be sure there is no better alternatives, as it looked > > a bit unusual to handle single feature with a mix of the controls and > > the selection API calls. Although it works, such an interface looks a bit > > clumsy to me, especially in cases where all we need is to pass just (x,y) > > coordinates. > > Selections are essentially controls but for rectangles. > > The original use case was to support configuring scaling, cropping etc. on > subdevs but they're not really bound to image processing configuration. > > Controls have been more generic to begin with. > > > I have quickly added support for rectangle controls type [1] to see how > > big changes it would require and what would be missing without significant > > changes in the controls API. > > > > So the main issues there are: the min/max/step/default value cannot > > be queried (VIDIOC_QUERYCTRL) and it is troublesome to handle them in > > the kernel, the control value change events wouldn't really work. > > > > I learnt VIDIOC_QUERYCTRL is not supported for V4L2_CTRL_TYPE_INTEGER64 > > control type, then maybe we could have similarly some features not > > available for V4L2_CTRL_TYPE_RECTANGLE ? Until there are further > > extensions that address this;) > > > > [1] http://git.linuxtv.org/snawrocki/media.git/ov965x-2-rect-type-ctrl > > Hmm. Had you proposed this two years ago, selections could well look > entirely different. > > We still have them now. There would be use cases for pad specific controls, > too; pixel rate for instance should be one. For this reason I don't see > selections really much different from controls. > > The selections are the same on subdevs and video nodes. Unifying them (with > some compat code for either of the current interfaces) and providing a new > IOCTL to access both was what I thought could be one solution to the > problem. > > Or --- we could add "selection controls" which would be just like selections > but with the control interface. What's relevant in struct v4l2_ext_control > would be the ID field, while the "value" field in struct v4l2_ext_control > would be a pointer to a struct describing the selection control. Half of the > reserved field could be used for the pad (they're 16-bit ints). No control > ID clashes with the selection IDs, so this could even work with the existing > selection targets. As I have stated before: I am opposed to adding compound controls to the control API. Each control should map to one value and one value only. Always remember that the control API is meant not just to pass values but to be shown in a generic GUI (control panel like). If you start adding compound values, then that becomes very hard for GUI apps to set up, not to mention that you will open the door to a zoo of different control types. So any solution that adds compound types to the control API will be NACKed by me. I just thought I'd state this clearly to prevent people going down the wrong trail. Regards, Hans > Either solution would avoid creating another rectangle type with an ID that > would be separate from selections. > > Thoughts, comments? > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: RFC: add parameters to V4L controls 2013-01-07 10:46 RFC: add parameters to V4L controls Andrzej Hajda 2013-01-07 12:10 ` Hans Verkuil @ 2013-01-12 22:05 ` Sakari Ailus 2013-01-15 14:34 ` Andrzej Hajda 1 sibling, 1 reply; 13+ messages in thread From: Sakari Ailus @ 2013-01-12 22:05 UTC (permalink / raw) To: Andrzej Hajda Cc: linux-media, Sylwester Nawrocki, Laurent Pinchart, Hans Verkuil Hi Andrzej, Andrzej Hajda wrote: > Hi, > > I have included this proposition already in the post "[PATCH RFC 0/2] > V4L: Add auto focus area control and selection" but it left unanswered. > I repost it again in a separate e-mail, I hope this way it will be > easier to attract attention. > > Problem description > > Currently V4L2 controls can have only single value (of type int, int64, > string). Some hardware controls require more than single int parameter, > for example to set auto-focus (AF) rectangle four coordinates should be > passed, to set auto-focus spot two coordinates should be passed. > > Current solution > > In case of AF rectangle we can reuse selection API as in "[PATCH RFC > 0/2] V4L: Add auto focus area control and selection" post. > Pros: > - reuse existing API, > Cons: > - two IOCTL's to perform one action, I think changing AF mode and AF window of interest are still two operations: you may well change just either one, and be happy with it. You might want to disable AF during the configuration from the application. Would this work for you? > - non-atomic operation, True, but this is the way V4L2 works. There are many cases where implementing multiple more or less unrelated operations atomically would be beneficial, but so far there always have been workarounds to perform those actions non-atomicly. Format configuration, for example. Atomic operations are hard to get right and typically the required effort refutes the gain of doing so in drivers, and everything that may be done atomically always must be implemented beforehand in drivers. Your use case would be from the more simple end, though. > - fits well only for rectangles and spots (but with unused fields width, > height), in case of other parameters we should find a different way. > > Proposed solution > > The solution takes an advantage of the fact VIDIOC_(G/S/TRY)_EXT_CTRLS > ioctls can be called with multiple controls per call. > > I will present it using AF area control example. > > There could be added four pseudo-controls, lets call them for short: > LEFT, TOP, WIDTH, HEIGHT. > Those controls could be passed together with V4L2_AUTO_FOCUS_AREA_RECTANGLE > control in one ioctl as a kind of parameters. > > For example setting auto-focus spot would require calling > VIDIOC_S_EXT_CTRLS > with the following controls: > - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE > - LEFT = ... > - RIGHT = ... > > Setting AF rectangle: > - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE > - LEFT = ... > - TOP = ... > - WIDTH = ... > - HEIGHT = ... > > Setting AF object detection (no parameters required): > - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION > > I have presented all three cases to show the advantages of this solution: > - atomicity - control and its parameters are passed in one call, > - flexibility - we are not limited by a fixed number of parameters, > - no-redundancy - we can pass only required parameters > (no need to pass null width and height in case of spot selection), > - extensibility - it is possible to extend parameters in the future, > for example add parameters to V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION, > without breaking API, > - backward compatibility, > - re-usability - this schema could be used in other controls, > pseudo-controls could be re-used in other controls as well. > - API backward compatibility. What I'm not terribly fond of in the above proposal is that it uses several controls to describe rectangles which are an obvious domain of the selection API: selections are roughly like controls but rather use a rectangle type instead of a single integer value (or a string). Also, I can't see any other reason to use controls for this than making the operation atomic. -- Kind regards, Sakari Ailus sakari.ailus@iki.fi ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: RFC: add parameters to V4L controls 2013-01-12 22:05 ` Sakari Ailus @ 2013-01-15 14:34 ` Andrzej Hajda 2013-01-21 11:17 ` Laurent Pinchart 0 siblings, 1 reply; 13+ messages in thread From: Andrzej Hajda @ 2013-01-15 14:34 UTC (permalink / raw) To: Sakari Ailus Cc: linux-media, Sylwester Nawrocki, Laurent Pinchart, Hans Verkuil On 12.01.2013 23:05, Sakari Ailus wrote: > Hi Andrzej, > > Andrzej Hajda wrote: >> Hi, >> >> I have included this proposition already in the post "[PATCH RFC 0/2] >> V4L: Add auto focus area control and selection" but it left unanswered. >> I repost it again in a separate e-mail, I hope this way it will be >> easier to attract attention. >> >> Problem description >> >> Currently V4L2 controls can have only single value (of type int, int64, >> string). Some hardware controls require more than single int parameter, >> for example to set auto-focus (AF) rectangle four coordinates should be >> passed, to set auto-focus spot two coordinates should be passed. >> >> Current solution >> >> In case of AF rectangle we can reuse selection API as in "[PATCH RFC >> 0/2] V4L: Add auto focus area control and selection" post. >> Pros: >> - reuse existing API, >> Cons: >> - two IOCTL's to perform one action, > I think changing AF mode and AF window of interest are still two > operations: you may well change just either one, and be happy with it. I see no point in changing AF window of interest without applying area AF. So we will have here always two operations in that case. > You might want to disable AF during the configuration from the > application. Would this work for you? > >> - non-atomic operation, > True, but this is the way V4L2 works. > > There are many cases where implementing multiple more or less unrelated > operations atomically would be beneficial, but so far there always have > been workarounds to perform those actions non-atomicly. Format > configuration, for example. > > Atomic operations are hard to get right and typically the required > effort refutes the gain of doing so in drivers, and everything that may > be done atomically always must be implemented beforehand in drivers. > > Your use case would be from the more simple end, though. > >> - fits well only for rectangles and spots (but with unused fields width, >> height), in case of other parameters we should find a different way. >> >> Proposed solution >> >> The solution takes an advantage of the fact VIDIOC_(G/S/TRY)_EXT_CTRLS >> ioctls can be called with multiple controls per call. >> >> I will present it using AF area control example. >> >> There could be added four pseudo-controls, lets call them for short: >> LEFT, TOP, WIDTH, HEIGHT. >> Those controls could be passed together with V4L2_AUTO_FOCUS_AREA_RECTANGLE >> control in one ioctl as a kind of parameters. >> >> For example setting auto-focus spot would require calling >> VIDIOC_S_EXT_CTRLS >> with the following controls: >> - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE >> - LEFT = ... >> - RIGHT = ... >> >> Setting AF rectangle: >> - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE >> - LEFT = ... >> - TOP = ... >> - WIDTH = ... >> - HEIGHT = ... >> >> Setting AF object detection (no parameters required): >> - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION >> >> I have presented all three cases to show the advantages of this solution: >> - atomicity - control and its parameters are passed in one call, >> - flexibility - we are not limited by a fixed number of parameters, >> - no-redundancy - we can pass only required parameters >> (no need to pass null width and height in case of spot selection), >> - extensibility - it is possible to extend parameters in the future, >> for example add parameters to V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION, >> without breaking API, >> - backward compatibility, >> - re-usability - this schema could be used in other controls, >> pseudo-controls could be re-used in other controls as well. >> - API backward compatibility. > What I'm not terribly fond of in the above proposal is that it uses > several controls to describe rectangles which are an obvious domain of > the selection API: selections are roughly like controls but rather use a > rectangle type instead of a single integer value (or a string). The problem wit AF is that it can require different set of parameters depending on the hardware: - spots (ex. Galaxy S3), - rectangles (ex. Samsung NX210), - multiple rectangles with weight or priority (ex. Samsung DV300F), - ... (To be honest I am sure only for GalaxyS3, other examples is just my prediction based on the analysis of user manuals of those cameras) Implementation of all those cases would be quite straightforward (for me) using proposed pseudo-controls, without it every time it would require serious brainstorming and API extending. And this is the main reason of my proposition. > > Also, I can't see any other reason to use controls for this than making > the operation atomic. > Regards Andrzej Hajda ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: RFC: add parameters to V4L controls 2013-01-15 14:34 ` Andrzej Hajda @ 2013-01-21 11:17 ` Laurent Pinchart 0 siblings, 0 replies; 13+ messages in thread From: Laurent Pinchart @ 2013-01-21 11:17 UTC (permalink / raw) To: Andrzej Hajda; +Cc: Sakari Ailus, linux-media, Sylwester Nawrocki, Hans Verkuil Hi Andrzej, On Tuesday 15 January 2013 15:34:47 Andrzej Hajda wrote: > On 12.01.2013 23:05, Sakari Ailus wrote: > > Andrzej Hajda wrote: > >> Hi, > >> > >> I have included this proposition already in the post "[PATCH RFC 0/2] > >> V4L: Add auto focus area control and selection" but it left unanswered. > >> I repost it again in a separate e-mail, I hope this way it will be > >> easier to attract attention. > >> > >> Problem description > >> > >> Currently V4L2 controls can have only single value (of type int, int64, > >> string). Some hardware controls require more than single int parameter, > >> for example to set auto-focus (AF) rectangle four coordinates should be > >> passed, to set auto-focus spot two coordinates should be passed. > >> > >> Current solution > >> > >> In case of AF rectangle we can reuse selection API as in "[PATCH RFC > >> 0/2] V4L: Add auto focus area control and selection" post. > >> Pros: > >> - reuse existing API, > >> Cons: > >> - two IOCTL's to perform one action, > > > > I think changing AF mode and AF window of interest are still two > > operations: you may well change just either one, and be happy with it. > > I see no point in changing AF window of interest without applying area AF. > So we will have here always two operations in that case. > > > You might want to disable AF during the configuration from the > > application. Would this work for you? > > > >> - non-atomic operation, > > > > True, but this is the way V4L2 works. > > > > There are many cases where implementing multiple more or less unrelated > > operations atomically would be beneficial, but so far there always have > > been workarounds to perform those actions non-atomicly. Format > > configuration, for example. > > > > Atomic operations are hard to get right and typically the required > > effort refutes the gain of doing so in drivers, and everything that may > > be done atomically always must be implemented beforehand in drivers. > > > > Your use case would be from the more simple end, though. > > > >> - fits well only for rectangles and spots (but with unused fields width, > >> height), in case of other parameters we should find a different way. > >> > >> Proposed solution > >> > >> The solution takes an advantage of the fact VIDIOC_(G/S/TRY)_EXT_CTRLS > >> ioctls can be called with multiple controls per call. > >> > >> I will present it using AF area control example. > >> > >> There could be added four pseudo-controls, lets call them for short: > >> LEFT, TOP, WIDTH, HEIGHT. > >> Those controls could be passed together with > >> V4L2_AUTO_FOCUS_AREA_RECTANGLE > >> control in one ioctl as a kind of parameters. > >> > >> For example setting auto-focus spot would require calling > >> VIDIOC_S_EXT_CTRLS > >> with the following controls: > >> - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE > >> - LEFT = ... > >> - RIGHT = ... > >> > >> Setting AF rectangle: > >> - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_RECTANGLE > >> - LEFT = ... > >> - TOP = ... > >> - WIDTH = ... > >> - HEIGHT = ... > >> > >> Setting AF object detection (no parameters required): > >> - V4L2_CID_AUTO_FOCUS_AREA = V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION > >> > >> I have presented all three cases to show the advantages of this solution: > >> - atomicity - control and its parameters are passed in one call, > >> - flexibility - we are not limited by a fixed number of parameters, > >> - no-redundancy - we can pass only required parameters > >> > >> (no need to pass null width and height in case of spot selection), > >> > >> - extensibility - it is possible to extend parameters in the future, > >> for example add parameters to V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION, > >> without breaking API, > >> - backward compatibility, > >> - re-usability - this schema could be used in other controls, > >> > >> pseudo-controls could be re-used in other controls as well. > >> > >> - API backward compatibility. > > > > What I'm not terribly fond of in the above proposal is that it uses > > several controls to describe rectangles which are an obvious domain of > > the selection API: selections are roughly like controls but rather use a > > rectangle type instead of a single integer value (or a string). > > The problem wit AF is that it can require different set of parameters > depending on the hardware: > - spots (ex. Galaxy S3), > - rectangles (ex. Samsung NX210), > - multiple rectangles with weight or priority (ex. Samsung DV300F), If it gets that complex, I wonder whether a custom ioctl wouldn't be better. I'm not sure we could really standardize multiple rectangles with weights in a useful way. > - ... > (To be honest I am sure only for GalaxyS3, other examples is just my > prediction based on the analysis of user manuals of those cameras) > > Implementation of all those cases would be quite straightforward (for > me) using proposed pseudo-controls, without it every time it would > require serious brainstorming and API extending. And this is the main > reason of my proposition. > > > Also, I can't see any other reason to use controls for this than making > > the operation atomic. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2013-02-12 8:14 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-01-07 10:46 RFC: add parameters to V4L controls Andrzej Hajda 2013-01-07 12:10 ` Hans Verkuil 2013-01-07 20:02 ` Laurent Pinchart 2013-01-08 12:26 ` Andrzej Hajda 2013-01-31 17:17 ` Sylwester Nawrocki 2013-02-01 22:17 ` Laurent Pinchart 2013-02-03 18:53 ` Sylwester Nawrocki 2013-02-06 20:26 ` Sakari Ailus 2013-02-10 19:44 ` Sylwester Nawrocki 2013-02-12 8:14 ` Hans Verkuil 2013-01-12 22:05 ` Sakari Ailus 2013-01-15 14:34 ` Andrzej Hajda 2013-01-21 11:17 ` Laurent Pinchart
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).