From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mailout4.w1.samsung.com ([210.118.77.14]:55088 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752872Ab2LLPO0 (ORCPT ); Wed, 12 Dec 2012 10:14:26 -0500 Received: from eucpsbgm1.samsung.com (unknown [203.254.199.244]) by mailout4.w1.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0MEX00COLBMZBSB0@mailout4.w1.samsung.com> for linux-media@vger.kernel.org; Wed, 12 Dec 2012 15:17:10 +0000 (GMT) Received: from [106.116.147.88] by eusync1.samsung.com (Oracle Communications Messaging Server 7u4-23.01(7.0.4.23.0) 64bit (built Aug 10 2011)) with ESMTPA id <0MEX00LP6BNZM540@eusync1.samsung.com> for linux-media@vger.kernel.org; Wed, 12 Dec 2012 15:14:24 +0000 (GMT) Message-id: <50C89F4E.6010701@samsung.com> Date: Wed, 12 Dec 2012 16:14:22 +0100 From: Andrzej Hajda MIME-version: 1.0 To: Sakari Ailus Cc: linux-media@vger.kernel.org, Kyungmin Park , Seung-Woo Kim , Sylwester Nawrocki Subject: Re: [PATCH RFC 2/2] V4L: Add V4L2_CID_AUTO_FOCUS_AREA control References: <1355147019-25375-1-git-send-email-a.hajda@samsung.com> <1355147019-25375-3-git-send-email-a.hajda@samsung.com> <20121211213404.GC3747@valkosipuli.retiisi.org.uk> In-reply-to: <20121211213404.GC3747@valkosipuli.retiisi.org.uk> Content-type: text/plain; charset=ISO-8859-1; format=flowed Content-transfer-encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Hi Sakari, Thanks for the review. On 11.12.2012 22:34, Sakari Ailus wrote: > Hi Andrzej and Sylwester, > > Thanks for the patch! > > On Mon, Dec 10, 2012 at 02:43:39PM +0100, Andrzej Hajda wrote: >> From: Sylwester Nawrocki >> >> Add control for automatic focus area selection. >> This control determines the area of the frame that the camera uses >> for automatic focus. >> >> Signed-off-by: Sylwester Nawrocki >> Signed-off-by: Andrzej Hajda >> Signed-off-by: Kyungmin Park >> --- >> Documentation/DocBook/media/v4l/compat.xml | 9 +++-- >> Documentation/DocBook/media/v4l/controls.xml | 47 +++++++++++++++++++++++++- >> Documentation/DocBook/media/v4l/v4l2.xml | 7 ++++ >> drivers/media/v4l2-core/v4l2-ctrls.c | 10 ++++++ >> include/uapi/linux/v4l2-controls.h | 6 ++++ >> 5 files changed, 76 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/DocBook/media/v4l/compat.xml b/Documentation/DocBook/media/v4l/compat.xml >> index 4fdf6b5..e8b53da 100644 >> --- a/Documentation/DocBook/media/v4l/compat.xml >> +++ b/Documentation/DocBook/media/v4l/compat.xml >> @@ -2452,8 +2452,9 @@ that used it. It was originally scheduled for removal in 2.6.35. >> V4L2_CID_3A_LOCK, >> V4L2_CID_AUTO_FOCUS_START, >> V4L2_CID_AUTO_FOCUS_STOP, >> - V4L2_CID_AUTO_FOCUS_STATUS and >> - V4L2_CID_AUTO_FOCUS_RANGE. >> + V4L2_CID_AUTO_FOCUS_STATUS, >> + V4L2_CID_AUTO_FOCUS_RANGE and >> + V4L2_CID_AUTO_FOCUS_AREA. >> >> >> >> @@ -2586,6 +2587,10 @@ ioctls. >> Vendor and device specific media bus pixel formats. >> . >> >> + >> + >> + V4L2_CID_AUTO_FOCUS_AREA control. >> + >> >> >> >> diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml >> index 7fe5be1..9d4af8a 100644 >> --- a/Documentation/DocBook/media/v4l/controls.xml >> +++ b/Documentation/DocBook/media/v4l/controls.xml >> @@ -3347,6 +3347,51 @@ use its minimum possible distance for auto focus. >> >> >> >> + >> + >> + V4L2_CID_AUTO_FOCUS_AREA  >> + enum v4l2_auto_focus_area >> + >> + Determines the area of the frame that >> +the camera uses for automatic focus. The corresponding coordinates of the >> +focusing spot or rectangle can be specified and queried using the selection API. >> +To change the auto focus region of interest applications first select required >> +mode of this control and then set the rectangle or spot coordinates by means >> +of the &VIDIOC-SUBDEV-S-SELECTION; or &VIDIOC-S-SELECTION; ioctl. In order to >> +trigger again a one shot auto focus with same coordinates applications should >> +use the V4L2_CID_AUTO_FOCUS_START control. Or alternatively > Extra space above. ^ > >> +invoke a &VIDIOC-SUBDEV-S-SELECTION; or a &VIDIOC-S-SELECTION; ioctl again. > How about requiring explicit V4L2_CID_AUTO_FOCUS_START? If you need to > specify several AF selection windows, then on which one do you start the > algorithm? A bitmask control explicitly telling which ones are active would > also be needed --- but that's for the future. I think now we just need to > ascertain we don't make that difficult. :-) Do you mean only V4L2_CID_AUTO_FOCUS_START should start AF? What about continuous auto-focus (CAF)? In case of the sensor I am working on, face detection can work in both AF and CAF. Should CAF be restarted (ie stopped and started again), to use face detection? >> +In the latter case the new pixel coordinates are applied to hardware only when >> +the focus area control was set to >> +V4L2_AUTO_FOCUS_AREA_RECTANGLE. >> + >> + >> + >> + >> + >> + V4L2_AUTO_FOCUS_AREA_ALL  >> + Normal auto focus, the focusing area extends over the >> +entire frame. > Does this need to be explicitly specified? Shouldn't the user just choose > the largest possible AF window instead? I'd even expect that the AF window > might span the whole frame by default (up to driver, hardware etc.). Yes it could be removed. There are two reasons I have left it: 1. If hardware support only AF on spots, V4L2_AUTO_FOCUS_AREA_ALL seems to be more natural than focusing on the whole image. 2. (Hypothetical) Instructing HW to area-focusing on the whole are could have different results than just starting default auto-focus, ie there could be different algorithms involved. It is just a prediction based on my current experience :) > >> + >> + >> + V4L2_AUTO_FOCUS_AREA_RECTANGLE  >> + The auto focus region of interest is determined by the >> +V4L2_SEL_TGT_AUTO_FOCUS selection rectangle. > It's not strictly required in documentation (and that shows) but it'd be > nice to align the paragraphs at equal indentation. OK > >> + >> + >> + V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION  >> + The auto focus region of interest is determined >> +by an object (e.g. face) detection engine. >> + >> + >> + >> + >> + >> + This is an experimental >> +control and may change in the future. >> + >> + >> + >> >> V4L2_CID_ZOOM_ABSOLUTE  >> integer >> @@ -3986,7 +4031,7 @@ interface and may change in the future. >> >> >> Flash Control IDs >> - >> + >> >> >> >> diff --git a/Documentation/DocBook/media/v4l/v4l2.xml b/Documentation/DocBook/media/v4l/v4l2.xml >> index 10ccde9..1477540 100644 >> --- a/Documentation/DocBook/media/v4l/v4l2.xml >> +++ b/Documentation/DocBook/media/v4l/v4l2.xml >> @@ -140,6 +140,13 @@ structs, ioctls) must be noted in more detail in the history chapter >> applications. --> >> >> >> + 3.9 >> + 2012-12-10 >> + sn >> + Added V4L2_CID_AUTO_FOCUS_AREA control. >> + >> + >> + >> 3.6 >> 2012-07-02 >> hv >> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c >> index f6ee201..9cdf4b8 100644 >> --- a/drivers/media/v4l2-core/v4l2-ctrls.c >> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c >> @@ -236,6 +236,12 @@ const char * const *v4l2_ctrl_get_menu(u32 id) >> "Spot", >> NULL >> }; >> + static const char * const camera_auto_focus_area[] = { >> + "All", >> + "Rectangle", >> + "Object Detection", >> + NULL >> + }; >> static const char * const camera_auto_focus_range[] = { >> "Auto", >> "Normal", >> @@ -497,6 +503,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id) >> return camera_exposure_auto; >> case V4L2_CID_EXPOSURE_METERING: >> return camera_exposure_metering; >> + case V4L2_CID_AUTO_FOCUS_AREA: >> + return camera_auto_focus_area; >> case V4L2_CID_AUTO_FOCUS_RANGE: >> return camera_auto_focus_range; >> case V4L2_CID_COLORFX: >> @@ -732,6 +740,7 @@ const char *v4l2_ctrl_get_name(u32 id) >> case V4L2_CID_AUTO_FOCUS_STOP: return "Auto Focus, Stop"; >> case V4L2_CID_AUTO_FOCUS_STATUS: return "Auto Focus, Status"; >> case V4L2_CID_AUTO_FOCUS_RANGE: return "Auto Focus, Range"; >> + case V4L2_CID_AUTO_FOCUS_AREA: return "Auto Focus, Area"; >> >> /* FM Radio Modulator control */ >> /* Keep the order of the 'case's the same as in videodev2.h! */ >> @@ -881,6 +890,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, >> case V4L2_CID_MPEG_STREAM_TYPE: >> case V4L2_CID_MPEG_STREAM_VBI_FMT: >> case V4L2_CID_EXPOSURE_AUTO: >> + case V4L2_CID_AUTO_FOCUS_AREA: >> case V4L2_CID_AUTO_FOCUS_RANGE: >> case V4L2_CID_COLORFX: >> case V4L2_CID_AUTO_N_PRESET_WHITE_BALANCE: >> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h >> index f56c945..0eb1c1a 100644 >> --- a/include/uapi/linux/v4l2-controls.h >> +++ b/include/uapi/linux/v4l2-controls.h >> @@ -683,6 +683,12 @@ enum v4l2_auto_focus_range { >> V4L2_AUTO_FOCUS_RANGE_INFINITY = 3, >> }; >> >> +#define V4L2_CID_AUTO_FOCUS_AREA (V4L2_CID_CAMERA_CLASS_BASE+32) >> +enum v4l2_auto_focus_area { >> + V4L2_AUTO_FOCUS_AREA_ALL = 0, >> + V4L2_AUTO_FOCUS_AREA_RECTANGLE = 1, >> + V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION = 2, > How about using #defines? It's easier for the user space when it can just > test #ifdef instead of relying on interesting hacks such as those in VLC > source in modules/access/v4l2/v4l2.h. > > You can easily either provide a substitute or just not compile in the > feature. Kernel also uses this hack/feature, grep started in kernel sources shows some of them: grep -C3 -Prn '^#define (.*) \1$' include/ > >> +}; >> >> /* FM Modulator class control IDs */ >> Regards Andrzej