* [PATCH RFC 0/2] V4L: Add auto focus area control and selection
@ 2012-12-10 13:43 Andrzej Hajda
2012-12-10 13:43 ` [PATCH RFC 1/2] V4L: Add auto focus selection targets Andrzej Hajda
2012-12-10 13:43 ` [PATCH RFC 2/2] V4L: Add V4L2_CID_AUTO_FOCUS_AREA control Andrzej Hajda
0 siblings, 2 replies; 12+ messages in thread
From: Andrzej Hajda @ 2012-12-10 13:43 UTC (permalink / raw)
To: linux-media, Kyungmin Park, Seung-Woo Kim, Sakari Ailus,
Sylwester Nawrocki
This set of patches is created by Sylwester Nawrocki, with small changes by me.
This set of patches extends the camera class with control
V4L2_CID_AUTO_FOCUS_AREA for determining the area of the frame that
camera uses for auto-focus.
The control takes care of three cases:
- V4L2_AUTO_FOCUS_AREA_ALL, normal auto-focus,
whole frame is used for auto-focus,
- V4L2_AUTO_FOCUS_AREA_RECTANGLE, user provides rectangle or spot
as an area of interest,
- V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION, object/face detection engine
of the camera should be used for auto-focus.
In case of the rectangle or the spot its coordinates shall be passed
to the driver using selection API (VIDIOC_SUBDEV_S_SELECTION) with
V4L2_SEL_TGT_AUTO_FOCUS as a target name. In case of spot width and
height of the rectangle shall be set to 0.
We (me and Sylwester) are not sure if this is the best solution.
I would like to propose another solution which seems to me more natural,
but probably it would require extending controls API.
The solution is neither formalized, neither implemented at the moment.
The solution takes an advantage of the fact VIDIOC_(G/S/TRY)_EXT_CTRLS
ioctls can be called with multiple controls per call.
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 control 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:
- 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,
- backward compatibility,
- re-usability - this schema could be used in other controls,
pseudo-controls could be re-used in other controls as well.
I hope this e-mail will trigger some discussion about the proposed solution.
Regards
Andrzej Hajda
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH RFC 1/2] V4L: Add auto focus selection targets
2012-12-10 13:43 [PATCH RFC 0/2] V4L: Add auto focus area control and selection Andrzej Hajda
@ 2012-12-10 13:43 ` Andrzej Hajda
2012-12-11 21:04 ` Sakari Ailus
2012-12-10 13:43 ` [PATCH RFC 2/2] V4L: Add V4L2_CID_AUTO_FOCUS_AREA control Andrzej Hajda
1 sibling, 1 reply; 12+ messages in thread
From: Andrzej Hajda @ 2012-12-10 13:43 UTC (permalink / raw)
To: linux-media, Kyungmin Park, Seung-Woo Kim, Sakari Ailus,
Sylwester Nawrocki
Cc: Andrzej Hajda
From: Sylwester Nawrocki <s.nawrocki@samsung.com>
The camera automatic focus algorithms may require setting up
a spot or rectangle coordinates.
The automatic focus selection targets are introduced in order
to allow applications to query and set such coordinates. Those
selections are intended to be used together with the automatic
focus controls available in the camera control class.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Documentation/DocBook/media/v4l/selection-api.xml | 32 ++++++++++++++++-
.../DocBook/media/v4l/selections-common.xml | 37 ++++++++++++++++++++
.../media/v4l/vidioc-subdev-g-selection.xml | 4 +--
include/uapi/linux/v4l2-common.h | 5 +++
4 files changed, 75 insertions(+), 3 deletions(-)
diff --git a/Documentation/DocBook/media/v4l/selection-api.xml b/Documentation/DocBook/media/v4l/selection-api.xml
index 4c238ce..8caf67b 100644
--- a/Documentation/DocBook/media/v4l/selection-api.xml
+++ b/Documentation/DocBook/media/v4l/selection-api.xml
@@ -1,6 +1,6 @@
<section id="selection-api">
- <title>Experimental API for cropping, composing and scaling</title>
+ <title>Experimental selections API</title>
<note>
<title>Experimental</title>
@@ -9,6 +9,10 @@
interface and may change in the future.</para>
</note>
+ <section>
+
+ <title>Image cropping, composing and scaling</title>
+
<section>
<title>Introduction</title>
@@ -321,5 +325,31 @@ V4L2_BUF_TYPE_VIDEO_OUTPUT </constant> for other devices</para>
</example>
</section>
+ </section>
+
+ <section>
+ <title>Automatic focus regions of interest</title>
+
+<para>The camera automatic focus algorithms may require configuration of
+regions of interest in form of rectangle or spot coordinates. The automatic
+focus selection targets allow applications to query and set such coordinates.
+Those selections are intended to be used together with the
+<constant>V4L2_CID_AUTO_FOCUS_AREA</constant> <link linkend="camera-controls">
+camera class</link> control. The <constant>V4L2_SEL_TGT_AUTO_FOCUS</constant>
+target is used for querying or setting actual spot or rectangle coordinates,
+while <constant>V4L2_SEL_TGT_AUTO_FOCUS_BOUNDS</constant> target determines
+bounds for a single spot or rectangle.
+These selections are only effective when the <constant>V4L2_CID_AUTO_FOCUS_AREA
+</constant>control is set to
+<constant>V4L2_AUTO_FOCUS_AREA_RECTANGLE</constant>. The new coordinates shall
+be accepted and applied to hardware when the focus area control value is
+changed and also during a &VIDIOC-S-SELECTION; ioctl call, only when the focus
+area control is already set to required value.</para>
+
+<para>When the <structfield>width</structfield> and
+<structfield>height</structfield> of the selection rectangle are set to 0 the
+selection determines spot coordinates, rather than a rectangle.</para>
+
+ </section>
</section>
diff --git a/Documentation/DocBook/media/v4l/selections-common.xml b/Documentation/DocBook/media/v4l/selections-common.xml
index 7502f78..9f0c477 100644
--- a/Documentation/DocBook/media/v4l/selections-common.xml
+++ b/Documentation/DocBook/media/v4l/selections-common.xml
@@ -93,6 +93,22 @@
<entry>Yes</entry>
<entry>No</entry>
</row>
+ <row>
+ <entry><constant>V4L2_SEL_TGT_AUTO_FOCUS</constant></entry>
+ <entry>0x1001</entry>
+ <entry>Actual automatic focus rectangle.</entry>
+ <entry>Yes</entry>
+ <entry>Yes</entry>
+ </row>
+ <row>
+ <entry><constant>V4L2_SEL_TGT_AUTO_FOCUS_BOUNDS</constant></entry>
+ <entry>0x1002</entry>
+ <entry>Bounds of the automatic focus region of interest. All valid
+ automatic focus rectangles fit inside the automatic focus bounds
+ rectangle.</entry>
+ <entry>Yes</entry>
+ <entry>Yes</entry>
+ </row>
</tbody>
</tgroup>
</table>
@@ -158,7 +174,28 @@
</tbody>
</tgroup>
</table>
+ </section>
+
+ <section>
+ <title>Automatic focus regions of interest</title>
+
+ <para>The camera automatic focus algorithms may require configuration
+ of a region or multiple regions of interest in form of rectangle or spot
+ coordinates.</para>
+
+ <para>A single rectangle of interest is represented in &v4l2-rect;
+ by the coordinates of the top left corner and the rectangle size. Both
+ the coordinates and sizes are expressed in pixels. When the <structfield>
+ width</structfield> and <structfield>height</structfield> fields of
+ &v4l2-rect; are set to 0 the selection determines spot coordinates,
+ rather than a rectangle.</para>
+ <para>Auto focus rectangles are reset to their default values when the
+ output image format is modified. Drivers should use the output image size
+ as the auto focus rectangle default value, but hardware requirements may
+ prevent this.
+ </para>
+ <para>The auto focus selections on input pads are not defined.</para>
</section>
</section>
diff --git a/Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml b/Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml
index 1ba9e99..95e759f 100644
--- a/Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml
@@ -57,8 +57,8 @@
<para>The selections are used to configure various image
processing functionality performed by the subdevs which affect the
- image size. This currently includes cropping, scaling and
- composition.</para>
+ image size. This currently includes cropping, scaling, composition
+ and automatic focus regions of interest.</para>
<para>The selection API replaces <link
linkend="vidioc-subdev-g-crop">the old subdev crop API</link>. All
diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h
index 4f0667e..0372ccb 100644
--- a/include/uapi/linux/v4l2-common.h
+++ b/include/uapi/linux/v4l2-common.h
@@ -50,6 +50,11 @@
/* Current composing area plus all padding pixels */
#define V4L2_SEL_TGT_COMPOSE_PADDED 0x0103
+/* Auto focus region of interest */
+#define V4L2_SEL_TGT_AUTO_FOCUS 0x0200
+/* Auto focus region bounds */
+#define V4L2_SEL_TGT_AUTO_FOCUS_BOUNDS 0x0201
+
/* Backward compatibility target definitions --- to be removed. */
#define V4L2_SEL_TGT_CROP_ACTIVE V4L2_SEL_TGT_CROP
#define V4L2_SEL_TGT_COMPOSE_ACTIVE V4L2_SEL_TGT_COMPOSE
--
1.7.10.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH RFC 2/2] V4L: Add V4L2_CID_AUTO_FOCUS_AREA control
2012-12-10 13:43 [PATCH RFC 0/2] V4L: Add auto focus area control and selection Andrzej Hajda
2012-12-10 13:43 ` [PATCH RFC 1/2] V4L: Add auto focus selection targets Andrzej Hajda
@ 2012-12-10 13:43 ` Andrzej Hajda
2012-12-11 21:34 ` Sakari Ailus
1 sibling, 1 reply; 12+ messages in thread
From: Andrzej Hajda @ 2012-12-10 13:43 UTC (permalink / raw)
To: linux-media, Kyungmin Park, Seung-Woo Kim, Sakari Ailus,
Sylwester Nawrocki
Cc: Andrzej Hajda
From: Sylwester Nawrocki <s.nawrocki@samsung.com>
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 <s.nawrocki@samsung.com>
Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
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.
<constant>V4L2_CID_3A_LOCK</constant>,
<constant>V4L2_CID_AUTO_FOCUS_START</constant>,
<constant>V4L2_CID_AUTO_FOCUS_STOP</constant>,
- <constant>V4L2_CID_AUTO_FOCUS_STATUS</constant> and
- <constant>V4L2_CID_AUTO_FOCUS_RANGE</constant>.
+ <constant>V4L2_CID_AUTO_FOCUS_STATUS</constant>,
+ <constant>V4L2_CID_AUTO_FOCUS_RANGE</constant> and
+ <constant>V4L2_CID_AUTO_FOCUS_AREA</constant>.
</para>
</listitem>
</orderedlist>
@@ -2586,6 +2587,10 @@ ioctls.</para>
<para>Vendor and device specific media bus pixel formats.
<xref linkend="v4l2-mbus-vendor-spec-fmts" />.</para>
</listitem>
+ <listitem>
+ <para><link linkend="v4l2-auto-focus-area"><constant>
+ V4L2_CID_AUTO_FOCUS_AREA</constant></link> control.</para>
+ </listitem>
</itemizedlist>
</section>
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.</entry>
</row>
<row><entry></entry></row>
+ <row id="v4l2-auto-focus-area">
+ <entry spanname="id">
+ <constant>V4L2_CID_AUTO_FOCUS_AREA</constant> </entry>
+ <entry>enum v4l2_auto_focus_area</entry>
+ </row>
+ <row><entry spanname="descr">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 <constant>V4L2_CID_AUTO_FOCUS_START </constant> control. Or alternatively
+invoke a &VIDIOC-SUBDEV-S-SELECTION; or a &VIDIOC-S-SELECTION; ioctl again.
+In the latter case the new pixel coordinates are applied to hardware only when
+the focus area control was set to
+<constant>V4L2_AUTO_FOCUS_AREA_RECTANGLE</constant>.</entry>
+ </row>
+ <row>
+ <entrytbl spanname="descr" cols="2">
+ <tbody valign="top">
+ <row>
+ <entry><constant>V4L2_AUTO_FOCUS_AREA_ALL</constant> </entry>
+ <entry>Normal auto focus, the focusing area extends over the
+entire frame.</entry>
+ </row>
+ <row>
+ <entry><constant>V4L2_AUTO_FOCUS_AREA_RECTANGLE</constant> </entry>
+ <entry>The auto focus region of interest is determined by the
+<constant>V4L2_SEL_TGT_AUTO_FOCUS</constant> selection rectangle.</entry>
+ </row>
+ <row>
+ <entry><constant>V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION</constant> </entry>
+ <entry>The auto focus region of interest is determined
+by an object (e.g. face) detection engine.</entry>
+ </row>
+ </tbody>
+ </entrytbl>
+ </row>
+ <row><entry spanname="descr">
+ This is an <link linkend="experimental">experimental</link>
+control and may change in the future.</entry>
+ </row>
+ <row><entry></entry></row>
+
<row>
<entry spanname="id"><constant>V4L2_CID_ZOOM_ABSOLUTE</constant> </entry>
<entry>integer</entry>
@@ -3986,7 +4031,7 @@ interface and may change in the future.</para>
<table pgwide="1" frame="none" id="flash-control-id">
<title>Flash Control IDs</title>
-
+
<tgroup cols="4">
<colspec colname="c1" colwidth="1*" />
<colspec colname="c2" colwidth="6*" />
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. -->
<revision>
+ <revnumber>3.9</revnumber>
+ <date>2012-12-10</date>
+ <authorinitials>sn</authorinitials>
+ <revremark>Added V4L2_CID_AUTO_FOCUS_AREA control.</revremark>
+ </revision>
+
+ <revision>
<revnumber>3.6</revnumber>
<date>2012-07-02</date>
<authorinitials>hv</authorinitials>
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,
+};
/* FM Modulator class control IDs */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 1/2] V4L: Add auto focus selection targets
2012-12-10 13:43 ` [PATCH RFC 1/2] V4L: Add auto focus selection targets Andrzej Hajda
@ 2012-12-11 21:04 ` Sakari Ailus
2012-12-12 13:59 ` Andrzej Hajda
0 siblings, 1 reply; 12+ messages in thread
From: Sakari Ailus @ 2012-12-11 21:04 UTC (permalink / raw)
To: Andrzej Hajda
Cc: linux-media, Kyungmin Park, Seung-Woo Kim, Sylwester Nawrocki
Hi Andrzej,
Many thanks for the patch!
On Mon, Dec 10, 2012 at 02:43:38PM +0100, Andrzej Hajda wrote:
> From: Sylwester Nawrocki <s.nawrocki@samsung.com>
>
> The camera automatic focus algorithms may require setting up
> a spot or rectangle coordinates.
>
> The automatic focus selection targets are introduced in order
> to allow applications to query and set such coordinates. Those
> selections are intended to be used together with the automatic
> focus controls available in the camera control class.
>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> Documentation/DocBook/media/v4l/selection-api.xml | 32 ++++++++++++++++-
> .../DocBook/media/v4l/selections-common.xml | 37 ++++++++++++++++++++
> .../media/v4l/vidioc-subdev-g-selection.xml | 4 +--
> include/uapi/linux/v4l2-common.h | 5 +++
> 4 files changed, 75 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/DocBook/media/v4l/selection-api.xml b/Documentation/DocBook/media/v4l/selection-api.xml
> index 4c238ce..8caf67b 100644
> --- a/Documentation/DocBook/media/v4l/selection-api.xml
> +++ b/Documentation/DocBook/media/v4l/selection-api.xml
> @@ -1,6 +1,6 @@
> <section id="selection-api">
>
> - <title>Experimental API for cropping, composing and scaling</title>
> + <title>Experimental selections API</title>
Hmm. I wonder if it'd be enough to call this just "Selection API". There's a
note just below telling it's experimental.
> <note>
> <title>Experimental</title>
> @@ -9,6 +9,10 @@
> interface and may change in the future.</para>
> </note>
>
> + <section>
> +
> + <title>Image cropping, composing and scaling</title>
> +
> <section>
> <title>Introduction</title>
>
> @@ -321,5 +325,31 @@ V4L2_BUF_TYPE_VIDEO_OUTPUT </constant> for other devices</para>
> </example>
>
> </section>
> + </section>
> +
> + <section>
> + <title>Automatic focus regions of interest</title>
> +
> +<para>The camera automatic focus algorithms may require configuration of
> +regions of interest in form of rectangle or spot coordinates. The automatic
> +focus selection targets allow applications to query and set such coordinates.
> +Those selections are intended to be used together with the
> +<constant>V4L2_CID_AUTO_FOCUS_AREA</constant> <link linkend="camera-controls">
> +camera class</link> control. The <constant>V4L2_SEL_TGT_AUTO_FOCUS</constant>
> +target is used for querying or setting actual spot or rectangle coordinates,
> +while <constant>V4L2_SEL_TGT_AUTO_FOCUS_BOUNDS</constant> target determines
> +bounds for a single spot or rectangle.
> +These selections are only effective when the <constant>V4L2_CID_AUTO_FOCUS_AREA
> +</constant>control is set to
> +<constant>V4L2_AUTO_FOCUS_AREA_RECTANGLE</constant>. The new coordinates shall
> +be accepted and applied to hardware when the focus area control value is
> +changed and also during a &VIDIOC-S-SELECTION; ioctl call, only when the focus
> +area control is already set to required value.</para>
> +
> +<para>When the <structfield>width</structfield> and
> +<structfield>height</structfield> of the selection rectangle are set to 0 the
> +selection determines spot coordinates, rather than a rectangle.</para>
> +
> + </section>
>
> </section>
> diff --git a/Documentation/DocBook/media/v4l/selections-common.xml b/Documentation/DocBook/media/v4l/selections-common.xml
> index 7502f78..9f0c477 100644
> --- a/Documentation/DocBook/media/v4l/selections-common.xml
> +++ b/Documentation/DocBook/media/v4l/selections-common.xml
> @@ -93,6 +93,22 @@
> <entry>Yes</entry>
> <entry>No</entry>
> </row>
> + <row>
> + <entry><constant>V4L2_SEL_TGT_AUTO_FOCUS</constant></entry>
> + <entry>0x1001</entry>
> + <entry>Actual automatic focus rectangle.</entry>
> + <entry>Yes</entry>
> + <entry>Yes</entry>
> + </row>
> + <row>
> + <entry><constant>V4L2_SEL_TGT_AUTO_FOCUS_BOUNDS</constant></entry>
> + <entry>0x1002</entry>
> + <entry>Bounds of the automatic focus region of interest. All valid
> + automatic focus rectangles fit inside the automatic focus bounds
> + rectangle.</entry>
> + <entry>Yes</entry>
> + <entry>Yes</entry>
> + </row>
> </tbody>
> </tgroup>
> </table>
> @@ -158,7 +174,28 @@
> </tbody>
> </tgroup>
> </table>
> + </section>
> +
> + <section>
> + <title>Automatic focus regions of interest</title>
> +
> + <para>The camera automatic focus algorithms may require configuration
> + of a region or multiple regions of interest in form of rectangle or spot
> + coordinates.</para>
> +
> + <para>A single rectangle of interest is represented in &v4l2-rect;
> + by the coordinates of the top left corner and the rectangle size. Both
> + the coordinates and sizes are expressed in pixels. When the <structfield>
> + width</structfield> and <structfield>height</structfield> fields of
> + &v4l2-rect; are set to 0 the selection determines spot coordinates,
> + rather than a rectangle.</para>
>
> + <para>Auto focus rectangles are reset to their default values when the
> + output image format is modified. Drivers should use the output image size
> + as the auto focus rectangle default value, but hardware requirements may
> + prevent this.
> + </para>
> + <para>The auto focus selections on input pads are not defined.</para>
> </section>
>
> </section>
> diff --git a/Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml b/Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml
> index 1ba9e99..95e759f 100644
> --- a/Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml
> +++ b/Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml
> @@ -57,8 +57,8 @@
>
> <para>The selections are used to configure various image
> processing functionality performed by the subdevs which affect the
> - image size. This currently includes cropping, scaling and
> - composition.</para>
> + image size. This currently includes cropping, scaling, composition
> + and automatic focus regions of interest.</para>
AF window does not affect image size. :-)
Also, on subdevs one needs to ask the question which other rectangle the AF
window is related to. On video nodes it's obvious that it's the captured
format (or is it?), but on subdevs I could imagine it might be related to
almost any rectangle, depending on hardware.
One option would be to add a new field to tell the parent window.
What about multiple AF windows of interest? That's not unheard of either. I
see a forthcoming need for enumerating targets (and sub-targets such as
window ids) here.
> <para>The selection API replaces <link
> linkend="vidioc-subdev-g-crop">the old subdev crop API</link>. All
> diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h
> index 4f0667e..0372ccb 100644
> --- a/include/uapi/linux/v4l2-common.h
> +++ b/include/uapi/linux/v4l2-common.h
> @@ -50,6 +50,11 @@
> /* Current composing area plus all padding pixels */
> #define V4L2_SEL_TGT_COMPOSE_PADDED 0x0103
>
> +/* Auto focus region of interest */
> +#define V4L2_SEL_TGT_AUTO_FOCUS 0x0200
> +/* Auto focus region bounds */
> +#define V4L2_SEL_TGT_AUTO_FOCUS_BOUNDS 0x0201
I see different numbers here and in the documentation. I'd favour numbers in
the documentation --- these targets are very different from what's defined
up to now.
> +
> /* Backward compatibility target definitions --- to be removed. */
> #define V4L2_SEL_TGT_CROP_ACTIVE V4L2_SEL_TGT_CROP
> #define V4L2_SEL_TGT_COMPOSE_ACTIVE V4L2_SEL_TGT_COMPOSE
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 2/2] V4L: Add V4L2_CID_AUTO_FOCUS_AREA control
2012-12-10 13:43 ` [PATCH RFC 2/2] V4L: Add V4L2_CID_AUTO_FOCUS_AREA control Andrzej Hajda
@ 2012-12-11 21:34 ` Sakari Ailus
2012-12-12 15:14 ` Andrzej Hajda
0 siblings, 1 reply; 12+ messages in thread
From: Sakari Ailus @ 2012-12-11 21:34 UTC (permalink / raw)
To: Andrzej Hajda
Cc: linux-media, Kyungmin Park, Seung-Woo Kim, Sylwester Nawrocki
Hi Andrzej and Sylwester,
Thanks for the patch!
On Mon, Dec 10, 2012 at 02:43:39PM +0100, Andrzej Hajda wrote:
> From: Sylwester Nawrocki <s.nawrocki@samsung.com>
>
> 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 <s.nawrocki@samsung.com>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> 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.
> <constant>V4L2_CID_3A_LOCK</constant>,
> <constant>V4L2_CID_AUTO_FOCUS_START</constant>,
> <constant>V4L2_CID_AUTO_FOCUS_STOP</constant>,
> - <constant>V4L2_CID_AUTO_FOCUS_STATUS</constant> and
> - <constant>V4L2_CID_AUTO_FOCUS_RANGE</constant>.
> + <constant>V4L2_CID_AUTO_FOCUS_STATUS</constant>,
> + <constant>V4L2_CID_AUTO_FOCUS_RANGE</constant> and
> + <constant>V4L2_CID_AUTO_FOCUS_AREA</constant>.
> </para>
> </listitem>
> </orderedlist>
> @@ -2586,6 +2587,10 @@ ioctls.</para>
> <para>Vendor and device specific media bus pixel formats.
> <xref linkend="v4l2-mbus-vendor-spec-fmts" />.</para>
> </listitem>
> + <listitem>
> + <para><link linkend="v4l2-auto-focus-area"><constant>
> + V4L2_CID_AUTO_FOCUS_AREA</constant></link> control.</para>
> + </listitem>
> </itemizedlist>
> </section>
>
> 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.</entry>
> </row>
> <row><entry></entry></row>
>
> + <row id="v4l2-auto-focus-area">
> + <entry spanname="id">
> + <constant>V4L2_CID_AUTO_FOCUS_AREA</constant> </entry>
> + <entry>enum v4l2_auto_focus_area</entry>
> + </row>
> + <row><entry spanname="descr">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 <constant>V4L2_CID_AUTO_FOCUS_START </constant> 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. :-)
> +In the latter case the new pixel coordinates are applied to hardware only when
> +the focus area control was set to
> +<constant>V4L2_AUTO_FOCUS_AREA_RECTANGLE</constant>.</entry>
> + </row>
> + <row>
> + <entrytbl spanname="descr" cols="2">
> + <tbody valign="top">
> + <row>
> + <entry><constant>V4L2_AUTO_FOCUS_AREA_ALL</constant> </entry>
> + <entry>Normal auto focus, the focusing area extends over the
> +entire frame.</entry>
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.).
> + </row>
> + <row>
> + <entry><constant>V4L2_AUTO_FOCUS_AREA_RECTANGLE</constant> </entry>
> + <entry>The auto focus region of interest is determined by the
> +<constant>V4L2_SEL_TGT_AUTO_FOCUS</constant> selection rectangle.</entry>
It's not strictly required in documentation (and that shows) but it'd be
nice to align the paragraphs at equal indentation.
> + </row>
> + <row>
> + <entry><constant>V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION</constant> </entry>
> + <entry>The auto focus region of interest is determined
> +by an object (e.g. face) detection engine.</entry>
> + </row>
> + </tbody>
> + </entrytbl>
> + </row>
> + <row><entry spanname="descr">
> + This is an <link linkend="experimental">experimental</link>
> +control and may change in the future.</entry>
> + </row>
> + <row><entry></entry></row>
> +
> <row>
> <entry spanname="id"><constant>V4L2_CID_ZOOM_ABSOLUTE</constant> </entry>
> <entry>integer</entry>
> @@ -3986,7 +4031,7 @@ interface and may change in the future.</para>
>
> <table pgwide="1" frame="none" id="flash-control-id">
> <title>Flash Control IDs</title>
> -
> +
> <tgroup cols="4">
> <colspec colname="c1" colwidth="1*" />
> <colspec colname="c2" colwidth="6*" />
> 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. -->
>
> <revision>
> + <revnumber>3.9</revnumber>
> + <date>2012-12-10</date>
> + <authorinitials>sn</authorinitials>
> + <revremark>Added V4L2_CID_AUTO_FOCUS_AREA control.</revremark>
> + </revision>
> +
> + <revision>
> <revnumber>3.6</revnumber>
> <date>2012-07-02</date>
> <authorinitials>hv</authorinitials>
> 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.
> +};
>
> /* FM Modulator class control IDs */
>
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 1/2] V4L: Add auto focus selection targets
2012-12-11 21:04 ` Sakari Ailus
@ 2012-12-12 13:59 ` Andrzej Hajda
2012-12-16 12:30 ` Sakari Ailus
0 siblings, 1 reply; 12+ messages in thread
From: Andrzej Hajda @ 2012-12-12 13:59 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, Kyungmin Park, Seung-Woo Kim, Sylwester Nawrocki
Hi Sakari,
Thank You for the review.
On 11.12.2012 22:04, Sakari Ailus wrote:
> Hi Andrzej,
>
> Many thanks for the patch!
>
> On Mon, Dec 10, 2012 at 02:43:38PM +0100, Andrzej Hajda wrote:
>> From: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>
>> The camera automatic focus algorithms may require setting up
>> a spot or rectangle coordinates.
>>
>> The automatic focus selection targets are introduced in order
>> to allow applications to query and set such coordinates. Those
>> selections are intended to be used together with the automatic
>> focus controls available in the camera control class.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>> Documentation/DocBook/media/v4l/selection-api.xml | 32 ++++++++++++++++-
>> .../DocBook/media/v4l/selections-common.xml | 37 ++++++++++++++++++++
>> .../media/v4l/vidioc-subdev-g-selection.xml | 4 +--
>> include/uapi/linux/v4l2-common.h | 5 +++
>> 4 files changed, 75 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/DocBook/media/v4l/selection-api.xml b/Documentation/DocBook/media/v4l/selection-api.xml
>> index 4c238ce..8caf67b 100644
>> --- a/Documentation/DocBook/media/v4l/selection-api.xml
>> +++ b/Documentation/DocBook/media/v4l/selection-api.xml
>> @@ -1,6 +1,6 @@
>> <section id="selection-api">
>>
>> - <title>Experimental API for cropping, composing and scaling</title>
>> + <title>Experimental selections API</title>
> Hmm. I wonder if it'd be enough to call this just "Selection API". There's a
> note just below telling it's experimental.
>
>> <note>
>> <title>Experimental</title>
>> @@ -9,6 +9,10 @@
>> interface and may change in the future.</para>
>> </note>
>>
>> + <section>
>> +
>> + <title>Image cropping, composing and scaling</title>
>> +
>> <section>
>> <title>Introduction</title>
>>
>> @@ -321,5 +325,31 @@ V4L2_BUF_TYPE_VIDEO_OUTPUT </constant> for other devices</para>
>> </example>
>>
>> </section>
>> + </section>
>> +
>> + <section>
>> + <title>Automatic focus regions of interest</title>
>> +
>> +<para>The camera automatic focus algorithms may require configuration of
>> +regions of interest in form of rectangle or spot coordinates. The automatic
>> +focus selection targets allow applications to query and set such coordinates.
>> +Those selections are intended to be used together with the
>> +<constant>V4L2_CID_AUTO_FOCUS_AREA</constant> <link linkend="camera-controls">
>> +camera class</link> control. The <constant>V4L2_SEL_TGT_AUTO_FOCUS</constant>
>> +target is used for querying or setting actual spot or rectangle coordinates,
>> +while <constant>V4L2_SEL_TGT_AUTO_FOCUS_BOUNDS</constant> target determines
>> +bounds for a single spot or rectangle.
>> +These selections are only effective when the <constant>V4L2_CID_AUTO_FOCUS_AREA
>> +</constant>control is set to
>> +<constant>V4L2_AUTO_FOCUS_AREA_RECTANGLE</constant>. The new coordinates shall
>> +be accepted and applied to hardware when the focus area control value is
>> +changed and also during a &VIDIOC-S-SELECTION; ioctl call, only when the focus
>> +area control is already set to required value.</para>
>> +
>> +<para>When the <structfield>width</structfield> and
>> +<structfield>height</structfield> of the selection rectangle are set to 0 the
>> +selection determines spot coordinates, rather than a rectangle.</para>
>> +
>> + </section>
>>
>> </section>
>> diff --git a/Documentation/DocBook/media/v4l/selections-common.xml b/Documentation/DocBook/media/v4l/selections-common.xml
>> index 7502f78..9f0c477 100644
>> --- a/Documentation/DocBook/media/v4l/selections-common.xml
>> +++ b/Documentation/DocBook/media/v4l/selections-common.xml
>> @@ -93,6 +93,22 @@
>> <entry>Yes</entry>
>> <entry>No</entry>
>> </row>
>> + <row>
>> + <entry><constant>V4L2_SEL_TGT_AUTO_FOCUS</constant></entry>
>> + <entry>0x1001</entry>
>> + <entry>Actual automatic focus rectangle.</entry>
>> + <entry>Yes</entry>
>> + <entry>Yes</entry>
>> + </row>
>> + <row>
>> + <entry><constant>V4L2_SEL_TGT_AUTO_FOCUS_BOUNDS</constant></entry>
>> + <entry>0x1002</entry>
>> + <entry>Bounds of the automatic focus region of interest. All valid
>> + automatic focus rectangles fit inside the automatic focus bounds
>> + rectangle.</entry>
>> + <entry>Yes</entry>
>> + <entry>Yes</entry>
>> + </row>
>> </tbody>
>> </tgroup>
>> </table>
>> @@ -158,7 +174,28 @@
>> </tbody>
>> </tgroup>
>> </table>
>> + </section>
>> +
>> + <section>
>> + <title>Automatic focus regions of interest</title>
>> +
>> + <para>The camera automatic focus algorithms may require configuration
>> + of a region or multiple regions of interest in form of rectangle or spot
>> + coordinates.</para>
>> +
>> + <para>A single rectangle of interest is represented in &v4l2-rect;
>> + by the coordinates of the top left corner and the rectangle size. Both
>> + the coordinates and sizes are expressed in pixels. When the <structfield>
>> + width</structfield> and <structfield>height</structfield> fields of
>> + &v4l2-rect; are set to 0 the selection determines spot coordinates,
>> + rather than a rectangle.</para>
>>
>> + <para>Auto focus rectangles are reset to their default values when the
>> + output image format is modified. Drivers should use the output image size
>> + as the auto focus rectangle default value, but hardware requirements may
>> + prevent this.
>> + </para>
>> + <para>The auto focus selections on input pads are not defined.</para>
>> </section>
>>
>> </section>
>> diff --git a/Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml b/Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml
>> index 1ba9e99..95e759f 100644
>> --- a/Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml
>> +++ b/Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml
>> @@ -57,8 +57,8 @@
>>
>> <para>The selections are used to configure various image
>> processing functionality performed by the subdevs which affect the
>> - image size. This currently includes cropping, scaling and
>> - composition.</para>
>> + image size. This currently includes cropping, scaling, composition
>> + and automatic focus regions of interest.</para>
> AF window does not affect image size. :-)
>
> Also, on subdevs one needs to ask the question which other rectangle the AF
> window is related to. On video nodes it's obvious that it's the captured
> format (or is it?), but on subdevs I could imagine it might be related to
> almost any rectangle, depending on hardware.
>
> One option would be to add a new field to tell the parent window.
When user sets AF spot/rectangle his decision is based on what he receives,
ie the output image. So I would prefer to stick the AF window to the
output format of the source pad.
I am not sure in case of capture device, but compose window seems to me
the best choice.
I see no real gain in using 'parent window' field, instead of stick AF
window to coordinates proposed above.
If hardware requires different coordinates driver have all info
necessary to perform translation.
>
> What about multiple AF windows of interest? That's not unheard of either. I
> see a forthcoming need for enumerating targets (and sub-targets such as
> window ids) here.
>
>> <para>The selection API replaces <link
>> linkend="vidioc-subdev-g-crop">the old subdev crop API</link>. All
>> diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h
>> index 4f0667e..0372ccb 100644
>> --- a/include/uapi/linux/v4l2-common.h
>> +++ b/include/uapi/linux/v4l2-common.h
>> @@ -50,6 +50,11 @@
>> /* Current composing area plus all padding pixels */
>> #define V4L2_SEL_TGT_COMPOSE_PADDED 0x0103
>>
>> +/* Auto focus region of interest */
>> +#define V4L2_SEL_TGT_AUTO_FOCUS 0x0200
>> +/* Auto focus region bounds */
>> +#define V4L2_SEL_TGT_AUTO_FOCUS_BOUNDS 0x0201
> I see different numbers here and in the documentation. I'd favour numbers in
> the documentation --- these targets are very different from what's defined
> up to now.
OK
>
>> +
>> /* Backward compatibility target definitions --- to be removed. */
>> #define V4L2_SEL_TGT_CROP_ACTIVE V4L2_SEL_TGT_CROP
>> #define V4L2_SEL_TGT_COMPOSE_ACTIVE V4L2_SEL_TGT_COMPOSE
>>
Regards
Andrzej
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 2/2] V4L: Add V4L2_CID_AUTO_FOCUS_AREA control
2012-12-11 21:34 ` Sakari Ailus
@ 2012-12-12 15:14 ` Andrzej Hajda
2012-12-16 15:00 ` Sakari Ailus
0 siblings, 1 reply; 12+ messages in thread
From: Andrzej Hajda @ 2012-12-12 15:14 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, Kyungmin Park, Seung-Woo Kim, Sylwester Nawrocki
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 <s.nawrocki@samsung.com>
>>
>> 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 <s.nawrocki@samsung.com>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>> 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.
>> <constant>V4L2_CID_3A_LOCK</constant>,
>> <constant>V4L2_CID_AUTO_FOCUS_START</constant>,
>> <constant>V4L2_CID_AUTO_FOCUS_STOP</constant>,
>> - <constant>V4L2_CID_AUTO_FOCUS_STATUS</constant> and
>> - <constant>V4L2_CID_AUTO_FOCUS_RANGE</constant>.
>> + <constant>V4L2_CID_AUTO_FOCUS_STATUS</constant>,
>> + <constant>V4L2_CID_AUTO_FOCUS_RANGE</constant> and
>> + <constant>V4L2_CID_AUTO_FOCUS_AREA</constant>.
>> </para>
>> </listitem>
>> </orderedlist>
>> @@ -2586,6 +2587,10 @@ ioctls.</para>
>> <para>Vendor and device specific media bus pixel formats.
>> <xref linkend="v4l2-mbus-vendor-spec-fmts" />.</para>
>> </listitem>
>> + <listitem>
>> + <para><link linkend="v4l2-auto-focus-area"><constant>
>> + V4L2_CID_AUTO_FOCUS_AREA</constant></link> control.</para>
>> + </listitem>
>> </itemizedlist>
>> </section>
>>
>> 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.</entry>
>> </row>
>> <row><entry></entry></row>
>>
>> + <row id="v4l2-auto-focus-area">
>> + <entry spanname="id">
>> + <constant>V4L2_CID_AUTO_FOCUS_AREA</constant> </entry>
>> + <entry>enum v4l2_auto_focus_area</entry>
>> + </row>
>> + <row><entry spanname="descr">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 <constant>V4L2_CID_AUTO_FOCUS_START </constant> 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
>> +<constant>V4L2_AUTO_FOCUS_AREA_RECTANGLE</constant>.</entry>
>> + </row>
>> + <row>
>> + <entrytbl spanname="descr" cols="2">
>> + <tbody valign="top">
>> + <row>
>> + <entry><constant>V4L2_AUTO_FOCUS_AREA_ALL</constant> </entry>
>> + <entry>Normal auto focus, the focusing area extends over the
>> +entire frame.</entry>
> 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 :)
>
>> + </row>
>> + <row>
>> + <entry><constant>V4L2_AUTO_FOCUS_AREA_RECTANGLE</constant> </entry>
>> + <entry>The auto focus region of interest is determined by the
>> +<constant>V4L2_SEL_TGT_AUTO_FOCUS</constant> selection rectangle.</entry>
> It's not strictly required in documentation (and that shows) but it'd be
> nice to align the paragraphs at equal indentation.
OK
>
>> + </row>
>> + <row>
>> + <entry><constant>V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION</constant> </entry>
>> + <entry>The auto focus region of interest is determined
>> +by an object (e.g. face) detection engine.</entry>
>> + </row>
>> + </tbody>
>> + </entrytbl>
>> + </row>
>> + <row><entry spanname="descr">
>> + This is an <link linkend="experimental">experimental</link>
>> +control and may change in the future.</entry>
>> + </row>
>> + <row><entry></entry></row>
>> +
>> <row>
>> <entry spanname="id"><constant>V4L2_CID_ZOOM_ABSOLUTE</constant> </entry>
>> <entry>integer</entry>
>> @@ -3986,7 +4031,7 @@ interface and may change in the future.</para>
>>
>> <table pgwide="1" frame="none" id="flash-control-id">
>> <title>Flash Control IDs</title>
>> -
>> +
>> <tgroup cols="4">
>> <colspec colname="c1" colwidth="1*" />
>> <colspec colname="c2" colwidth="6*" />
>> 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. -->
>>
>> <revision>
>> + <revnumber>3.9</revnumber>
>> + <date>2012-12-10</date>
>> + <authorinitials>sn</authorinitials>
>> + <revremark>Added V4L2_CID_AUTO_FOCUS_AREA control.</revremark>
>> + </revision>
>> +
>> + <revision>
>> <revnumber>3.6</revnumber>
>> <date>2012-07-02</date>
>> <authorinitials>hv</authorinitials>
>> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 1/2] V4L: Add auto focus selection targets
2012-12-12 13:59 ` Andrzej Hajda
@ 2012-12-16 12:30 ` Sakari Ailus
0 siblings, 0 replies; 12+ messages in thread
From: Sakari Ailus @ 2012-12-16 12:30 UTC (permalink / raw)
To: Andrzej Hajda
Cc: linux-media, Kyungmin Park, Seung-Woo Kim, Sylwester Nawrocki
Hi Andzej,
On Wed, Dec 12, 2012 at 02:59:32PM +0100, Andrzej Hajda wrote:
> On 11.12.2012 22:04, Sakari Ailus wrote:
> >Hi Andrzej,
> >
> >Many thanks for the patch!
> >
> >On Mon, Dec 10, 2012 at 02:43:38PM +0100, Andrzej Hajda wrote:
> >>From: Sylwester Nawrocki <s.nawrocki@samsung.com>
> >>
> >>The camera automatic focus algorithms may require setting up
> >>a spot or rectangle coordinates.
> >>
> >>The automatic focus selection targets are introduced in order
> >>to allow applications to query and set such coordinates. Those
> >>selections are intended to be used together with the automatic
> >>focus controls available in the camera control class.
> >>
> >>Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> >>Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> >>Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >>---
> >> Documentation/DocBook/media/v4l/selection-api.xml | 32 ++++++++++++++++-
> >> .../DocBook/media/v4l/selections-common.xml | 37 ++++++++++++++++++++
> >> .../media/v4l/vidioc-subdev-g-selection.xml | 4 +--
> >> include/uapi/linux/v4l2-common.h | 5 +++
> >> 4 files changed, 75 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/Documentation/DocBook/media/v4l/selection-api.xml b/Documentation/DocBook/media/v4l/selection-api.xml
> >>index 4c238ce..8caf67b 100644
> >>--- a/Documentation/DocBook/media/v4l/selection-api.xml
> >>+++ b/Documentation/DocBook/media/v4l/selection-api.xml
> >>@@ -1,6 +1,6 @@
> >> <section id="selection-api">
> >>- <title>Experimental API for cropping, composing and scaling</title>
> >>+ <title>Experimental selections API</title>
> >Hmm. I wonder if it'd be enough to call this just "Selection API". There's a
> >note just below telling it's experimental.
> >
> >> <note>
> >> <title>Experimental</title>
> >>@@ -9,6 +9,10 @@
> >> interface and may change in the future.</para>
> >> </note>
> >>+ <section>
> >>+
> >>+ <title>Image cropping, composing and scaling</title>
> >>+
> >> <section>
> >> <title>Introduction</title>
> >>@@ -321,5 +325,31 @@ V4L2_BUF_TYPE_VIDEO_OUTPUT </constant> for other devices</para>
> >> </example>
> >> </section>
> >>+ </section>
> >>+
> >>+ <section>
> >>+ <title>Automatic focus regions of interest</title>
> >>+
> >>+<para>The camera automatic focus algorithms may require configuration of
> >>+regions of interest in form of rectangle or spot coordinates. The automatic
> >>+focus selection targets allow applications to query and set such coordinates.
> >>+Those selections are intended to be used together with the
> >>+<constant>V4L2_CID_AUTO_FOCUS_AREA</constant> <link linkend="camera-controls">
> >>+camera class</link> control. The <constant>V4L2_SEL_TGT_AUTO_FOCUS</constant>
> >>+target is used for querying or setting actual spot or rectangle coordinates,
> >>+while <constant>V4L2_SEL_TGT_AUTO_FOCUS_BOUNDS</constant> target determines
> >>+bounds for a single spot or rectangle.
> >>+These selections are only effective when the <constant>V4L2_CID_AUTO_FOCUS_AREA
> >>+</constant>control is set to
> >>+<constant>V4L2_AUTO_FOCUS_AREA_RECTANGLE</constant>. The new coordinates shall
> >>+be accepted and applied to hardware when the focus area control value is
> >>+changed and also during a &VIDIOC-S-SELECTION; ioctl call, only when the focus
> >>+area control is already set to required value.</para>
> >>+
> >>+<para>When the <structfield>width</structfield> and
> >>+<structfield>height</structfield> of the selection rectangle are set to 0 the
> >>+selection determines spot coordinates, rather than a rectangle.</para>
> >>+
> >>+ </section>
> >> </section>
> >>diff --git a/Documentation/DocBook/media/v4l/selections-common.xml b/Documentation/DocBook/media/v4l/selections-common.xml
> >>index 7502f78..9f0c477 100644
> >>--- a/Documentation/DocBook/media/v4l/selections-common.xml
> >>+++ b/Documentation/DocBook/media/v4l/selections-common.xml
> >>@@ -93,6 +93,22 @@
> >> <entry>Yes</entry>
> >> <entry>No</entry>
> >> </row>
> >>+ <row>
> >>+ <entry><constant>V4L2_SEL_TGT_AUTO_FOCUS</constant></entry>
> >>+ <entry>0x1001</entry>
> >>+ <entry>Actual automatic focus rectangle.</entry>
> >>+ <entry>Yes</entry>
> >>+ <entry>Yes</entry>
> >>+ </row>
> >>+ <row>
> >>+ <entry><constant>V4L2_SEL_TGT_AUTO_FOCUS_BOUNDS</constant></entry>
> >>+ <entry>0x1002</entry>
> >>+ <entry>Bounds of the automatic focus region of interest. All valid
> >>+ automatic focus rectangles fit inside the automatic focus bounds
> >>+ rectangle.</entry>
> >>+ <entry>Yes</entry>
> >>+ <entry>Yes</entry>
> >>+ </row>
> >> </tbody>
> >> </tgroup>
> >> </table>
> >>@@ -158,7 +174,28 @@
> >> </tbody>
> >> </tgroup>
> >> </table>
> >>+ </section>
> >>+
> >>+ <section>
> >>+ <title>Automatic focus regions of interest</title>
> >>+
> >>+ <para>The camera automatic focus algorithms may require configuration
> >>+ of a region or multiple regions of interest in form of rectangle or spot
> >>+ coordinates.</para>
> >>+
> >>+ <para>A single rectangle of interest is represented in &v4l2-rect;
> >>+ by the coordinates of the top left corner and the rectangle size. Both
> >>+ the coordinates and sizes are expressed in pixels. When the <structfield>
> >>+ width</structfield> and <structfield>height</structfield> fields of
> >>+ &v4l2-rect; are set to 0 the selection determines spot coordinates,
> >>+ rather than a rectangle.</para>
> >>+ <para>Auto focus rectangles are reset to their default values when the
> >>+ output image format is modified. Drivers should use the output image size
> >>+ as the auto focus rectangle default value, but hardware requirements may
> >>+ prevent this.
> >>+ </para>
> >>+ <para>The auto focus selections on input pads are not defined.</para>
> >> </section>
> >> </section>
> >>diff --git a/Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml b/Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml
> >>index 1ba9e99..95e759f 100644
> >>--- a/Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml
> >>+++ b/Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml
> >>@@ -57,8 +57,8 @@
> >> <para>The selections are used to configure various image
> >> processing functionality performed by the subdevs which affect the
> >>- image size. This currently includes cropping, scaling and
> >>- composition.</para>
> >>+ image size. This currently includes cropping, scaling, composition
> >>+ and automatic focus regions of interest.</para>
> >AF window does not affect image size. :-)
> >
> >Also, on subdevs one needs to ask the question which other rectangle the AF
> >window is related to. On video nodes it's obvious that it's the captured
> >format (or is it?), but on subdevs I could imagine it might be related to
> >almost any rectangle, depending on hardware.
> >
> >One option would be to add a new field to tell the parent window.
> When user sets AF spot/rectangle his decision is based on what he receives,
> ie the output image. So I would prefer to stick the AF window to the
> output format of the source pad.
The source format should describe the format that's the output of the
subdev. In this case that'd be statistics, so I don't think using source
format as the parent for the statistics window would be meaningful.
I wouldn't define the parent rectangle (or format) yet. The selection API
may well be useful in configuring statistics generation, and we may well use
it for that. However we have no implementation on that yet (not even an
implementation that would use a video node to provide statistics to the user
space), so we shouldn't define something that we don't strictly need and
that also may make life difficult in future.
So I propose to leave this undefined for the time being.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 2/2] V4L: Add V4L2_CID_AUTO_FOCUS_AREA control
2012-12-12 15:14 ` Andrzej Hajda
@ 2012-12-16 15:00 ` Sakari Ailus
2012-12-17 0:11 ` Sylwester Nawrocki
0 siblings, 1 reply; 12+ messages in thread
From: Sakari Ailus @ 2012-12-16 15:00 UTC (permalink / raw)
To: Andrzej Hajda
Cc: linux-media, Kyungmin Park, Seung-Woo Kim, Sylwester Nawrocki
Hi Andrzej,
On Wed, Dec 12, 2012 at 04:14:22PM +0100, Andrzej Hajda wrote:
> 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 <s.nawrocki@samsung.com>
> >>
> >>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 <s.nawrocki@samsung.com>
> >>Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> >>Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >>---
> >> 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.
> >> <constant>V4L2_CID_3A_LOCK</constant>,
> >> <constant>V4L2_CID_AUTO_FOCUS_START</constant>,
> >> <constant>V4L2_CID_AUTO_FOCUS_STOP</constant>,
> >>- <constant>V4L2_CID_AUTO_FOCUS_STATUS</constant> and
> >>- <constant>V4L2_CID_AUTO_FOCUS_RANGE</constant>.
> >>+ <constant>V4L2_CID_AUTO_FOCUS_STATUS</constant>,
> >>+ <constant>V4L2_CID_AUTO_FOCUS_RANGE</constant> and
> >>+ <constant>V4L2_CID_AUTO_FOCUS_AREA</constant>.
> >> </para>
> >> </listitem>
> >> </orderedlist>
> >>@@ -2586,6 +2587,10 @@ ioctls.</para>
> >> <para>Vendor and device specific media bus pixel formats.
> >> <xref linkend="v4l2-mbus-vendor-spec-fmts" />.</para>
> >> </listitem>
> >>+ <listitem>
> >>+ <para><link linkend="v4l2-auto-focus-area"><constant>
> >>+ V4L2_CID_AUTO_FOCUS_AREA</constant></link> control.</para>
> >>+ </listitem>
> >> </itemizedlist>
> >> </section>
> >>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.</entry>
> >> </row>
> >> <row><entry></entry></row>
> >>+ <row id="v4l2-auto-focus-area">
> >>+ <entry spanname="id">
> >>+ <constant>V4L2_CID_AUTO_FOCUS_AREA</constant> </entry>
> >>+ <entry>enum v4l2_auto_focus_area</entry>
> >>+ </row>
> >>+ <row><entry spanname="descr">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 <constant>V4L2_CID_AUTO_FOCUS_START </constant> 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.
Continuous AF needs to be an exception to that. It's controlled by
V4L2_CID_FOCUS_AUTO, which interestingly doesn't even mention continuous AF.
> Should CAF be restarted (ie stopped and started again), to use face
> detection?
I wonder if V4L2_CID_AUTO_FOCUS_START should be defined to restart CAF when
V4L2_CID_FOCUS_AUTO is enabled. I don't think we currently have a way to do
that; the current definition says that using V4L2_CID_AUTO_FOCUS_START is
undefined then. What do you think?
> >>+In the latter case the new pixel coordinates are applied to hardware only when
> >>+the focus area control was set to
> >>+<constant>V4L2_AUTO_FOCUS_AREA_RECTANGLE</constant>.</entry>
> >>+ </row>
> >>+ <row>
> >>+ <entrytbl spanname="descr" cols="2">
> >>+ <tbody valign="top">
> >>+ <row>
> >>+ <entry><constant>V4L2_AUTO_FOCUS_AREA_ALL</constant> </entry>
> >>+ <entry>Normal auto focus, the focusing area extends over the
> >>+entire frame.</entry>
> >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.
If the hardware only supports spots, then wouldn't V4L2_AUTO_FOCUS_AREA_ALL
give false information to the user, suggesting the focus area is actually
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 :)
If the algorithm is different in that case, then it should be made a new
control, not implicitly throught a seemingly unrelated control.
We currently don't have one, and this kind of things could be hardware
specific, so this could be a private control IMO.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 2/2] V4L: Add V4L2_CID_AUTO_FOCUS_AREA control
2012-12-16 15:00 ` Sakari Ailus
@ 2012-12-17 0:11 ` Sylwester Nawrocki
2012-12-17 17:24 ` Andrzej Hajda
2013-01-06 22:04 ` Sakari Ailus
0 siblings, 2 replies; 12+ messages in thread
From: Sylwester Nawrocki @ 2012-12-17 0:11 UTC (permalink / raw)
To: Sakari Ailus
Cc: Andrzej Hajda, linux-media, Kyungmin Park, Seung-Woo Kim,
Sylwester Nawrocki
Hi Sakari,
On 12/16/2012 04:00 PM, Sakari Ailus wrote:
>>>> 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.</entry>
>>>> </row>
>>>> <row><entry></entry></row>
>>>> + <row id="v4l2-auto-focus-area">
>>>> + <entry spanname="id">
>>>> + <constant>V4L2_CID_AUTO_FOCUS_AREA</constant> </entry>
>>>> + <entry>enum v4l2_auto_focus_area</entry>
>>>> + </row>
>>>> + <row><entry spanname="descr">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<constant>V4L2_CID_AUTO_FOCUS_START</constant> 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.
>
> Continuous AF needs to be an exception to that. It's controlled by
> V4L2_CID_FOCUS_AUTO, which interestingly doesn't even mention continuous AF.
I think it does, maybe not exactly in these words, but "continuous
automatic focus
adjustments" doesn't sound like a difference thing to me.
>> Should CAF be restarted (ie stopped and started again), to use face
>> detection?
>
> I wonder if V4L2_CID_AUTO_FOCUS_START should be defined to restart CAF when
> V4L2_CID_FOCUS_AUTO is enabled. I don't think we currently have a way to do
> that; the current definition says that using V4L2_CID_AUTO_FOCUS_START is
> undefined then. What do you think?
Yes, it might be worth to reconsider this. However, I would like to see real
use cases first where V4L2_CID_AUTO_FOCUS_START control is needed in
continuous
AF mode.
All in all, we have V4L2_AUTO_FOCUS_STATUS_FAILED AF status control
value and
I can't see anything preventing it to be applicable to CAF. So it might make
sense to define in the API what needs to be done to bring CAF out of
this state.
>>>> +In the latter case the new pixel coordinates are applied to hardware only when
>>>> +the focus area control was set to
>>>> +<constant>V4L2_AUTO_FOCUS_AREA_RECTANGLE</constant>.</entry>
>>>> + </row>
>>>> + <row>
>>>> + <entrytbl spanname="descr" cols="2">
>>>> + <tbody valign="top">
>>>> + <row>
>>>> + <entry><constant>V4L2_AUTO_FOCUS_AREA_ALL</constant> </entry>
>>>> + <entry>Normal auto focus, the focusing area extends over the
>>>> +entire frame.</entry>
>>> 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.
>
> If the hardware only supports spots, then wouldn't V4L2_AUTO_FOCUS_AREA_ALL
> give false information to the user, suggesting the focus area is actually
> the whole image?
I think Andrzej meant to say that there can be hardware that supports:
a. AF where region of interest is whole frame,
b. AF where region of interest is some rectangle of size that may be not
known exactly, and position (center) of that rectangle only is defined
through AF selections.
So you would be really switching AF algorithms by manipulating AF selection
rectangle only.
That said I really think V4L2_AUTO_FOCUS_AREA_ALL is a bad name here.
I originally started with single AF mode control and then after discussions
we came up with V4L2_AUTO_FOCUS_RANGE and V4L2_AUTO_FOCUS_AREA controls.
My motivation behind V4L2_AUTO_FOCUS_AREA_ALL was to provide a menu
item that would allow to select "normal" AF, with supposedly whole frame
being the AF region of interest. "Normal" AF might mean really any area
of the frame, so I propose to just replace V4L2_AUTO_FOCUS_AREA_ALL with
V4L2_AUTO_FOCUS_AREA_AUTO. This entry would naturally mean that AF area
is automatically selected by an ISP and it might not be known exactly to
user. Like in case of those superb AF algorithms that many companies
value to keep secret...
>> 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 :)
>
> If the algorithm is different in that case, then it should be made a new
> control, not implicitly throught a seemingly unrelated control.
>
> We currently don't have one, and this kind of things could be hardware
> specific, so this could be a private control IMO.
We have already something like V4L2_CID_AUTO_FOCUS_MODE private control,
common to multiple Samsung camera sensors. And each device has mostly
different set of options in such a control. Not sure if it wouldn't make
more sense to have standard menu control ID with driver specific entries
for all AF modes. It likely makes sense to have common patterns expressed
in standard controls though. It seems current set of AF controls, together
with V4L2_AUTO_FOCUS_AREA covers pretty much of the functionality,
without resorting to the private controls interface, that is so awkward
to use when you have to deal with multiple different devices...
--
Thanks,
Sylwester
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 2/2] V4L: Add V4L2_CID_AUTO_FOCUS_AREA control
2012-12-17 0:11 ` Sylwester Nawrocki
@ 2012-12-17 17:24 ` Andrzej Hajda
2013-01-06 22:04 ` Sakari Ailus
1 sibling, 0 replies; 12+ messages in thread
From: Andrzej Hajda @ 2012-12-17 17:24 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Sakari Ailus, linux-media, Kyungmin Park, Seung-Woo Kim,
Sylwester Nawrocki
Hi Sylwester and Sakari,
On 17.12.2012 01:11, Sylwester Nawrocki wrote:
> Hi Sakari,
>
> On 12/16/2012 04:00 PM, Sakari Ailus wrote:
>>>>> 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.</entry>
>>>>> </row>
>>>>> <row><entry></entry></row>
>>>>> + <row id="v4l2-auto-focus-area">
>>>>> + <entry spanname="id">
>>>>> + <constant>V4L2_CID_AUTO_FOCUS_AREA</constant> </entry>
>>>>> + <entry>enum v4l2_auto_focus_area</entry>
>>>>> + </row>
>>>>> + <row><entry spanname="descr">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<constant>V4L2_CID_AUTO_FOCUS_START</constant> 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.
>>
>> Continuous AF needs to be an exception to that. It's controlled by
>> V4L2_CID_FOCUS_AUTO, which interestingly doesn't even mention
>> continuous AF.
>
> I think it does, maybe not exactly in these words, but "continuous
> automatic focus
> adjustments" doesn't sound like a difference thing to me.
>
>>> Should CAF be restarted (ie stopped and started again), to use face
>>> detection?
>>
>> I wonder if V4L2_CID_AUTO_FOCUS_START should be defined to restart
>> CAF when
>> V4L2_CID_FOCUS_AUTO is enabled. I don't think we currently have a way
>> to do
>> that; the current definition says that using
>> V4L2_CID_AUTO_FOCUS_START is
>> undefined then. What do you think?
>
> Yes, it might be worth to reconsider this. However, I would like to
> see real
> use cases first where V4L2_CID_AUTO_FOCUS_START control is needed in
> continuous
> AF mode.
> All in all, we have V4L2_AUTO_FOCUS_STATUS_FAILED AF status control
> value and
> I can't see anything preventing it to be applicable to CAF. So it
> might make
> sense to define in the API what needs to be done to bring CAF out of
> this state.
And what about using again V4L2_CID_FOCUS_AUTO=true?
Regarding the proposition that setting V4L2_CID_AUTO_FOCUS_AREA should
not trigger AF/CAF.
I am not sure if it will be good in case of CAF. After setting this
control but before restarting CAF driver will not
reflect hardware state. It does not seem to be dangerous but I do not
see why we should allow for such situation anyway.
In case of AF it seems to me OK. FOCUS_AREA is a setting which will be
used only when AF action is started, ie. only in V4L2_CID_AUTO_FOCUS_START.
Documentation should clearly state that only V4L2_CID_AUTO_FOCUS_START
starts AF.
>>>>> +In the latter case the new pixel coordinates are applied to
>>>>> hardware only when
>>>>> +the focus area control was set to
>>>>> +<constant>V4L2_AUTO_FOCUS_AREA_RECTANGLE</constant>.</entry>
>>>>> + </row>
>>>>> + <row>
>>>>> + <entrytbl spanname="descr" cols="2">
>>>>> + <tbody valign="top">
>>>>> + <row>
>>>>> + <entry><constant>V4L2_AUTO_FOCUS_AREA_ALL</constant> </entry>
>>>>> + <entry>Normal auto focus, the focusing area extends over the
>>>>> +entire frame.</entry>
>>>> 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.
>>
>> If the hardware only supports spots, then wouldn't
>> V4L2_AUTO_FOCUS_AREA_ALL
>> give false information to the user, suggesting the focus area is
>> actually
>> the whole image?
>
> I think Andrzej meant to say that there can be hardware that supports:
>
> a. AF where region of interest is whole frame,
> b. AF where region of interest is some rectangle of size that may be not
> known exactly, and position (center) of that rectangle only is defined
> through AF selections.
>
> So you would be really switching AF algorithms by manipulating AF
> selection
> rectangle only.
>
> That said I really think V4L2_AUTO_FOCUS_AREA_ALL is a bad name here.
> I originally started with single AF mode control and then after
> discussions
> we came up with V4L2_AUTO_FOCUS_RANGE and V4L2_AUTO_FOCUS_AREA controls.
>
> My motivation behind V4L2_AUTO_FOCUS_AREA_ALL was to provide a menu
> item that would allow to select "normal" AF, with supposedly whole frame
> being the AF region of interest. "Normal" AF might mean really any area
> of the frame, so I propose to just replace V4L2_AUTO_FOCUS_AREA_ALL with
> V4L2_AUTO_FOCUS_AREA_AUTO. This entry would naturally mean that AF area
> is automatically selected by an ISP and it might not be known exactly to
> user. Like in case of those superb AF algorithms that many companies
> value to keep secret...
>
>>> 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 :)
>>
>> If the algorithm is different in that case, then it should be made a new
>> control, not implicitly throught a seemingly unrelated control.
>>
>> We currently don't have one, and this kind of things could be hardware
>> specific, so this could be a private control IMO.
>
> We have already something like V4L2_CID_AUTO_FOCUS_MODE private control,
> common to multiple Samsung camera sensors. And each device has mostly
> different set of options in such a control. Not sure if it wouldn't make
> more sense to have standard menu control ID with driver specific entries
> for all AF modes. It likely makes sense to have common patterns expressed
> in standard controls though. It seems current set of AF controls,
> together
> with V4L2_AUTO_FOCUS_AREA covers pretty much of the functionality,
> without resorting to the private controls interface, that is so awkward
> to use when you have to deal with multiple different devices...
>
> --
>
> Thanks,
> Sylwester
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 2/2] V4L: Add V4L2_CID_AUTO_FOCUS_AREA control
2012-12-17 0:11 ` Sylwester Nawrocki
2012-12-17 17:24 ` Andrzej Hajda
@ 2013-01-06 22:04 ` Sakari Ailus
1 sibling, 0 replies; 12+ messages in thread
From: Sakari Ailus @ 2013-01-06 22:04 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Andrzej Hajda, linux-media, Kyungmin Park, Seung-Woo Kim,
Sylwester Nawrocki
Hi Sylwester,
My apologies for the delayed answer.
Sylwester Nawrocki wrote:
> On 12/16/2012 04:00 PM, Sakari Ailus wrote:
>>>>> 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.</entry>
>>>>> </row>
>>>>> <row><entry></entry></row>
>>>>> + <row id="v4l2-auto-focus-area">
>>>>> + <entry spanname="id">
>>>>> + <constant>V4L2_CID_AUTO_FOCUS_AREA</constant> </entry>
>>>>> + <entry>enum v4l2_auto_focus_area</entry>
>>>>> + </row>
>>>>> + <row><entry spanname="descr">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<constant>V4L2_CID_AUTO_FOCUS_START</constant> 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.
>>
>> Continuous AF needs to be an exception to that. It's controlled by
>> V4L2_CID_FOCUS_AUTO, which interestingly doesn't even mention
>> continuous AF.
>
> I think it does, maybe not exactly in these words, but "continuous
> automatic focus
> adjustments" doesn't sound like a difference thing to me.
Oh. I must have missed that. It's ok then.
>>> Should CAF be restarted (ie stopped and started again), to use face
>>> detection?
>>
>> I wonder if V4L2_CID_AUTO_FOCUS_START should be defined to restart CAF
>> when
>> V4L2_CID_FOCUS_AUTO is enabled. I don't think we currently have a way
>> to do
>> that; the current definition says that using V4L2_CID_AUTO_FOCUS_START is
>> undefined then. What do you think?
>
> Yes, it might be worth to reconsider this. However, I would like to see
> real
> use cases first where V4L2_CID_AUTO_FOCUS_START control is needed in
> continuous
> AF mode.
If the CAF focus window is changed, I think it may make sense to tell
the CAF algorithm this. If the window moves around based on e.g. output
from face detection algorithm it's unlikely there's a need to perform a
full search every time this happens.
> All in all, we have V4L2_AUTO_FOCUS_STATUS_FAILED AF status control
> value and
> I can't see anything preventing it to be applicable to CAF. So it might
> make
> sense to define in the API what needs to be done to bring CAF out of
> this state.
How would CAF fail? Would this mean that a pre-defined amount of time
has been spent on searching focus and none has been found? Shouldn't it
be application's decision to tell how long is too long, rather than
driver's?
>>>>> +In the latter case the new pixel coordinates are applied to
>>>>> hardware only when
>>>>> +the focus area control was set to
>>>>> +<constant>V4L2_AUTO_FOCUS_AREA_RECTANGLE</constant>.</entry>
>>>>> + </row>
>>>>> + <row>
>>>>> + <entrytbl spanname="descr" cols="2">
>>>>> + <tbody valign="top">
>>>>> + <row>
>>>>> +
>>>>> <entry><constant>V4L2_AUTO_FOCUS_AREA_ALL</constant> </entry>
>>>>> + <entry>Normal auto focus, the focusing area extends over the
>>>>> +entire frame.</entry>
>>>> 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.
>>
>> If the hardware only supports spots, then wouldn't
>> V4L2_AUTO_FOCUS_AREA_ALL
>> give false information to the user, suggesting the focus area is actually
>> the whole image?
>
> I think Andrzej meant to say that there can be hardware that supports:
>
> a. AF where region of interest is whole frame,
> b. AF where region of interest is some rectangle of size that may be not
> known exactly, and position (center) of that rectangle only is defined
> through AF selections.
>
> So you would be really switching AF algorithms by manipulating AF selection
> rectangle only.
I would keep the two separate: selecting the algorithm and the area of
interest. When the host software runs the algorithm itself the selection
is just about the area and nothing else. I think it'd be more flexible
not to make other decisions based on it in the driver.
Well, an individual driver could still do this if really, really needed
but I don't think it should end up to the V4L2 spec.
> That said I really think V4L2_AUTO_FOCUS_AREA_ALL is a bad name here.
> I originally started with single AF mode control and then after discussions
> we came up with V4L2_AUTO_FOCUS_RANGE and V4L2_AUTO_FOCUS_AREA controls.
>
> My motivation behind V4L2_AUTO_FOCUS_AREA_ALL was to provide a menu
> item that would allow to select "normal" AF, with supposedly whole frame
> being the AF region of interest. "Normal" AF might mean really any area
> of the frame, so I propose to just replace V4L2_AUTO_FOCUS_AREA_ALL with
> V4L2_AUTO_FOCUS_AREA_AUTO. This entry would naturally mean that AF area
> is automatically selected by an ISP and it might not be known exactly to
> user. Like in case of those superb AF algorithms that many companies
> value to keep secret...
There might be cases where the user doesn't wish to choose the area, and
the user shouldn't be forced to do so.
In this case the driver either knows the rectangle or it doesn't. For
the user this shouldn't matter.
If the area isn't known, the driver shouldn't provide access to the
whole selection rectangle, or the control. If the area is known to the
driver, there likely should be a sane default, at least for the cases
where the AF algorithm isn't in the host user space. The sane default
most probably would be something else than the full image area. What
makes sense likely also depends on the hardware (resolution etc.) and
the algorithm used, too.
>>> 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 :)
>>
>> If the algorithm is different in that case, then it should be made a new
>> control, not implicitly throught a seemingly unrelated control.
>>
>> We currently don't have one, and this kind of things could be hardware
>> specific, so this could be a private control IMO.
>
> We have already something like V4L2_CID_AUTO_FOCUS_MODE private control,
> common to multiple Samsung camera sensors. And each device has mostly
> different set of options in such a control. Not sure if it wouldn't make
> more sense to have standard menu control ID with driver specific entries
> for all AF modes. It likely makes sense to have common patterns expressed
> in standard controls though. It seems current set of AF controls, together
> with V4L2_AUTO_FOCUS_AREA covers pretty much of the functionality,
> without resorting to the private controls interface, that is so awkward
> to use when you have to deal with multiple different devices...
How many drivers are there and how many (and which) modes do they
provide? This sounds still a little bit specific to Samsung camera
modules, but if it's present in many then it could make sense to make it
a standard control. I can't say for sure without knowing more, though.
--
Cheers,
Sakari Ailus
sakari.ailus@iki.fi
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2013-01-06 22:01 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-12-10 13:43 [PATCH RFC 0/2] V4L: Add auto focus area control and selection Andrzej Hajda
2012-12-10 13:43 ` [PATCH RFC 1/2] V4L: Add auto focus selection targets Andrzej Hajda
2012-12-11 21:04 ` Sakari Ailus
2012-12-12 13:59 ` Andrzej Hajda
2012-12-16 12:30 ` Sakari Ailus
2012-12-10 13:43 ` [PATCH RFC 2/2] V4L: Add V4L2_CID_AUTO_FOCUS_AREA control Andrzej Hajda
2012-12-11 21:34 ` Sakari Ailus
2012-12-12 15:14 ` Andrzej Hajda
2012-12-16 15:00 ` Sakari Ailus
2012-12-17 0:11 ` Sylwester Nawrocki
2012-12-17 17:24 ` Andrzej Hajda
2013-01-06 22:04 ` Sakari Ailus
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).