Re: [PATCH RFC 2/2] V4L: Add V4L2_CID_AUTO_FOCUS_AREA control

2013-01-06 Thread Sakari Ailus
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.
>   
>   
> +
> +
> +V4L2_CID_AUTO_FOCUS_AREA 
> +enum v4l2_auto_focus_area
> +
> +Determines the area of the frame
> that
> +the camera uses for automatic focus. The corresponding coordinates
> of the
> +focusing spot or rectangle can be specified and queried using the
> selection API.
> +To change the auto focus region of interest applications first
> select required
> +mode of this control and then set the rectangle or spot
> coordinates by means
> +of the&VIDIOC-SUBDEV-S-SELECTION; or&VIDIOC-S-SELECTION; ioctl. In
> order to
> +trigger again a one shot auto focus with same coordinates
> applications should
> +use theV4L2_CID_AUTO_FOCUS_START  control. Or
> alternatively
 Extra space above.^

> +invoke a&VIDIOC-SUBDEV-S-SELECTION; or a&VIDIOC-S-SELECTION; ioctl
> again.
 How about requiring explicit V4L2_CID_AUTO_FOCUS_START? If you need to
 specify several AF selection windows, then on which one do you start
 the
 algorithm? A bitmask control explicitly telling which ones are
 active would
 also be needed --- but that's for the future. I think now we just
 need to
 ascertain we don't make that difficult. :-)
>>> Do you mean only V4L2_CID_AUTO_FOCUS_START should start AF?
>>> What about continuous auto-focus (CAF)? In case of the sensor I am
>>> working on, face detection can work in both AF and CAF.
>>
>> 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
> +V4L2_AUTO_FOCUS_AREA_RECTANGLE.
> +
> +
> +
> +
> +
> +   
> V4L2_AUTO_FOCUS_AREA_ALL 
> +Normal auto focus, the focusing area extends over the
> +entire frame.
 Does this need to be explicitly specified? Shouldn't the user just
 choose
 the largest possible AF window instead? I'd even expect that the AF
 window
 might span the whole frame by default (up to driver, hardware etc.).
>>> Yes it could be removed. There are two reasons I have left it:
>>> 1. If hardware support only AF on spots, V4L2_AUTO_FOCUS_AREA_ALL
>>> seems to be more
>>> natural than focusing on the whole image.
>>
>> 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 manipulatin

Re: [PATCH RFC 2/2] V4L: Add V4L2_CID_AUTO_FOCUS_AREA control

2012-12-17 Thread Andrzej Hajda

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.

  
  
+
+
+ V4L2_CID_AUTO_FOCUS_AREA 
+ enum v4l2_auto_focus_area
+
+Determines the area of the frame 
that
+the camera uses for automatic focus. The corresponding 
coordinates of the
+focusing spot or rectangle can be specified and queried using the 
selection API.
+To change the auto focus region of interest applications first 
select required
+mode of this control and then set the rectangle or spot 
coordinates by means
+of the&VIDIOC-SUBDEV-S-SELECTION; or&VIDIOC-S-SELECTION; ioctl. 
In order to
+trigger again a one shot auto focus with same coordinates 
applications should
+use theV4L2_CID_AUTO_FOCUS_START control. Or 
alternatively

Extra space above.^

+invoke a&VIDIOC-SUBDEV-S-SELECTION; or a&VIDIOC-S-SELECTION; 
ioctl again.

How about requiring explicit V4L2_CID_AUTO_FOCUS_START? If you need to
specify several AF selection windows, then on which one do you 
start the
algorithm? A bitmask control explicitly telling which ones are 
active would
also be needed --- but that's for the future. I think now we just 
need to

ascertain we don't make that difficult. :-)

Do you mean only V4L2_CID_AUTO_FOCUS_START should start AF?
What about continuous auto-focus (CAF)? In case of the sensor I am
working on, face detection can work in both AF and CAF.


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
+V4L2_AUTO_FOCUS_AREA_RECTANGLE.
+
+
+
+
+
+ V4L2_AUTO_FOCUS_AREA_ALL 
+Normal auto focus, the focusing area extends over the
+entire frame.
Does this need to be explicitly specified? Shouldn't the user just 
choose
the largest possible AF window instead? I'd even expect that the AF 
window

might span the whole frame by default (up to driver, hardware etc.).

Yes it could be removed. There are two reasons I have left it:
1. If hardware support only AF on spots, V4L2_AUTO_FOCUS_AREA_ALL
seems to be more
natural than focusing on the whole image.


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 t

Re: [PATCH RFC 2/2] V4L: Add V4L2_CID_AUTO_FOCUS_AREA control

2012-12-16 Thread 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.


+   
+   
+   V4L2_CID_AUTO_FOCUS_AREA 
+   enum v4l2_auto_focus_area
+   
+   Determines the area of the frame that
+the camera uses for automatic focus. The corresponding coordinates of the
+focusing spot or rectangle can be specified and queried using the selection 
API.
+To change the auto focus region of interest applications first select required
+mode of this control and then set the rectangle or spot coordinates by means
+of the&VIDIOC-SUBDEV-S-SELECTION; or&VIDIOC-S-SELECTION; ioctl. In order to
+trigger again a one shot auto focus with same coordinates applications should
+use theV4L2_CID_AUTO_FOCUS_START  control. Or 
alternatively

Extra space above.^


+invoke a&VIDIOC-SUBDEV-S-SELECTION; or a&VIDIOC-S-SELECTION; ioctl again.

How about requiring explicit V4L2_CID_AUTO_FOCUS_START? If you need to
specify several AF selection windows, then on which one do you start the
algorithm? A bitmask control explicitly telling which ones are active would
also be needed --- but that's for the future. I think now we just need to
ascertain we don't make that difficult. :-)

Do you mean only V4L2_CID_AUTO_FOCUS_START should start AF?
What about continuous auto-focus (CAF)? In case of the sensor I am
working on, face detection can work in both AF and CAF.


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
+V4L2_AUTO_FOCUS_AREA_RECTANGLE.
+   
+   
+   
+   
+   
+   
V4L2_AUTO_FOCUS_AREA_ALL 
+   Normal auto focus, the focusing area extends over the
+entire frame.

Does this need to be explicitly specified? Shouldn't the user just choose
the largest possible AF window instead? I'd even expect that the AF window
might span the whole frame by default (up to driver, hardware etc.).

Yes it could be removed. There are two reasons I have left it:
1. If hardware support only AF on spots, V4L2_AUTO_FOCUS_AREA_ALL
seems to be more
natural than focusing on the whole image.


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
pre

Re: [PATCH RFC 2/2] V4L: Add V4L2_CID_AUTO_FOCUS_AREA control

2012-12-16 Thread Sakari Ailus
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 
> >>
> >>Add control for automatic focus area selection.
> >>This control determines the area of the frame that the camera uses
> >>for automatic focus.
> >>
> >>Signed-off-by: Sylwester Nawrocki 
> >>Signed-off-by: Andrzej Hajda 
> >>Signed-off-by: Kyungmin Park 
> >>---
> >>  Documentation/DocBook/media/v4l/compat.xml   |9 +++--
> >>  Documentation/DocBook/media/v4l/controls.xml |   47 
> >> +-
> >>  Documentation/DocBook/media/v4l/v4l2.xml |7 
> >>  drivers/media/v4l2-core/v4l2-ctrls.c |   10 ++
> >>  include/uapi/linux/v4l2-controls.h   |6 
> >>  5 files changed, 76 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/Documentation/DocBook/media/v4l/compat.xml 
> >>b/Documentation/DocBook/media/v4l/compat.xml
> >>index 4fdf6b5..e8b53da 100644
> >>--- a/Documentation/DocBook/media/v4l/compat.xml
> >>+++ b/Documentation/DocBook/media/v4l/compat.xml
> >>@@ -2452,8 +2452,9 @@ that used it. It was originally scheduled for removal 
> >>in 2.6.35.
> >>  V4L2_CID_3A_LOCK,
> >>  V4L2_CID_AUTO_FOCUS_START,
> >>  V4L2_CID_AUTO_FOCUS_STOP,
> >>- V4L2_CID_AUTO_FOCUS_STATUS and
> >>- V4L2_CID_AUTO_FOCUS_RANGE.
> >>+ V4L2_CID_AUTO_FOCUS_STATUS,
> >>+ V4L2_CID_AUTO_FOCUS_RANGE and
> >>+ V4L2_CID_AUTO_FOCUS_AREA.
> >>  
> >>  
> >>
> >>@@ -2586,6 +2587,10 @@ ioctls.
> >>  Vendor and device specific media bus pixel formats.
> >>.
> >>  
> >>+
> >>+ 
> >>+ V4L2_CID_AUTO_FOCUS_AREA control.
> >>+
> >>
> >>  
> >>diff --git a/Documentation/DocBook/media/v4l/controls.xml 
> >>b/Documentation/DocBook/media/v4l/controls.xml
> >>index 7fe5be1..9d4af8a 100644
> >>--- a/Documentation/DocBook/media/v4l/controls.xml
> >>+++ b/Documentation/DocBook/media/v4l/controls.xml
> >>@@ -3347,6 +3347,51 @@ use its minimum possible distance for auto 
> >>focus.
> >>  
> >>  
> >>+ 
> >>+   
> >>+ V4L2_CID_AUTO_FOCUS_AREA 
> >>+   enum v4l2_auto_focus_area
> >>+ 
> >>+ Determines the area of the frame that
> >>+the camera uses for automatic focus. The corresponding coordinates of the
> >>+focusing spot or rectangle can be specified and queried using the 
> >>selection API.
> >>+To change the auto focus region of interest applications first select 
> >>required
> >>+mode of this control and then set the rectangle or spot coordinates by 
> >>means
> >>+of the &VIDIOC-SUBDEV-S-SELECTION; or &VIDIOC-S-SELECTION; ioctl. In order 
> >>to
> >>+trigger again a one shot auto focus with same coordinates applications 
> >>should
> >>+use the V4L2_CID_AUTO_FOCUS_START  control. Or 
> >>alternatively
> >Extra space above.^
> >
> >>+invoke a &VIDIOC-SUBDEV-S-SELECTION; or a &VIDIOC-S-SELECTION; ioctl again.
> >How about requiring explicit V4L2_CID_AUTO_FOCUS_START? If you need to
> >specify several AF selection windows, then on which one do you start the
> >algorithm? A bitmask control explicitly telling which ones are active would
> >also be needed --- but that's for the future. I think now we just need to
> >ascertain we don't make that difficult. :-)
> Do you mean only V4L2_CID_AUTO_FOCUS_START should start AF?
> What about continuous auto-focus (CAF)? In case of the sensor I am
> working on, face detection can work in both AF and CAF.

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
> >>+V4L2_AUTO_FOCUS_AREA_RECTANGLE.
> >>+ 
> >>+ 
> >>+   
> >>+ 
> >>+   
> >>+ 
> >>V4L2_AUTO_FOCUS_AREA_ALL 
> >>+ Normal auto focus, the focusing area extends over the
> >>+entire frame.
> >Does this need to be explicitly specified? Shouldn't the user just choose
> >the largest possible AF window instead? I'd even expect that the AF window
> >might span the whole frame by default (up to driver, hardware etc.).
> Yes it could be removed. There are two reasons I have left it:
> 1. If hardware support only AF on spots, V4L2_AUTO_FOCUS_AREA_ALL
> seems to be more
> natural than focusing on the whole image.

If the hardware only supports spots, then wouldn'

Re: [PATCH RFC 2/2] V4L: Add V4L2_CID_AUTO_FOCUS_AREA control

2012-12-12 Thread Andrzej Hajda

Hi Sakari,

Thanks for the review.

On 11.12.2012 22:34, Sakari Ailus wrote:

Hi Andrzej and Sylwester,

Thanks for the patch!

On Mon, Dec 10, 2012 at 02:43:39PM +0100, Andrzej Hajda wrote:

From: Sylwester Nawrocki 

Add control for automatic focus area selection.
This control determines the area of the frame that the camera uses
for automatic focus.

Signed-off-by: Sylwester Nawrocki 
Signed-off-by: Andrzej Hajda 
Signed-off-by: Kyungmin Park 
---
  Documentation/DocBook/media/v4l/compat.xml   |9 +++--
  Documentation/DocBook/media/v4l/controls.xml |   47 +-
  Documentation/DocBook/media/v4l/v4l2.xml |7 
  drivers/media/v4l2-core/v4l2-ctrls.c |   10 ++
  include/uapi/linux/v4l2-controls.h   |6 
  5 files changed, 76 insertions(+), 3 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/compat.xml 
b/Documentation/DocBook/media/v4l/compat.xml
index 4fdf6b5..e8b53da 100644
--- a/Documentation/DocBook/media/v4l/compat.xml
+++ b/Documentation/DocBook/media/v4l/compat.xml
@@ -2452,8 +2452,9 @@ that used it. It was originally scheduled for removal in 
2.6.35.
  V4L2_CID_3A_LOCK,
  V4L2_CID_AUTO_FOCUS_START,
  V4L2_CID_AUTO_FOCUS_STOP,
- V4L2_CID_AUTO_FOCUS_STATUS and
- V4L2_CID_AUTO_FOCUS_RANGE.
+ V4L2_CID_AUTO_FOCUS_STATUS,
+ V4L2_CID_AUTO_FOCUS_RANGE and
+ V4L2_CID_AUTO_FOCUS_AREA.
  
  

@@ -2586,6 +2587,10 @@ ioctls.
  Vendor and device specific media bus pixel formats.
.
  
+
+ 
+ V4L2_CID_AUTO_FOCUS_AREA control.
+

  
  
diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml

index 7fe5be1..9d4af8a 100644
--- a/Documentation/DocBook/media/v4l/controls.xml
+++ b/Documentation/DocBook/media/v4l/controls.xml
@@ -3347,6 +3347,51 @@ use its minimum possible distance for auto focus.
  
  
  
+	  

+   
+ V4L2_CID_AUTO_FOCUS_AREA 
+   enum v4l2_auto_focus_area
+ 
+ Determines the area of the frame that
+the camera uses for automatic focus. The corresponding coordinates of the
+focusing spot or rectangle can be specified and queried using the selection 
API.
+To change the auto focus region of interest applications first select required
+mode of this control and then set the rectangle or spot coordinates by means
+of the &VIDIOC-SUBDEV-S-SELECTION; or &VIDIOC-S-SELECTION; ioctl. In order to
+trigger again a one shot auto focus with same coordinates applications should
+use the V4L2_CID_AUTO_FOCUS_START  control. Or 
alternatively

Extra space above.^


+invoke a &VIDIOC-SUBDEV-S-SELECTION; or a &VIDIOC-S-SELECTION; ioctl again.

How about requiring explicit V4L2_CID_AUTO_FOCUS_START? If you need to
specify several AF selection windows, then on which one do you start the
algorithm? A bitmask control explicitly telling which ones are active would
also be needed --- but that's for the future. I think now we just need to
ascertain we don't make that difficult. :-)

Do you mean only V4L2_CID_AUTO_FOCUS_START should start AF?
What about continuous auto-focus (CAF)? In case of the sensor I am 
working on, face detection can work in both AF and CAF.
Should CAF be restarted (ie stopped and started again), to use face 
detection?



+In the latter case the new pixel coordinates are applied to hardware only when
+the focus area control was set to
+V4L2_AUTO_FOCUS_AREA_RECTANGLE.
+ 
+ 
+   
+ 
+   
+ 
V4L2_AUTO_FOCUS_AREA_ALL 
+ Normal auto focus, the focusing area extends over the
+entire frame.

Does this need to be explicitly specified? Shouldn't the user just choose
the largest possible AF window instead? I'd even expect that the AF window
might span the whole frame by default (up to driver, hardware etc.).

Yes it could be removed. There are two reasons I have left it:
1. If hardware support only AF on spots, V4L2_AUTO_FOCUS_AREA_ALL seems 
to be more

natural than focusing on the whole image.
2. (Hypothetical) Instructing HW to area-focusing on the whole are could 
have different results than just starting default auto-focus,
ie there could be different algorithms involved. It is just a prediction 
based on my current experience :)






+   
+   
+ 
V4L2_AUTO_FOCUS_AREA_RECTANGLE 
+ The auto focus region of interest is determined by the
+V4L2_SEL_TGT_AUTO_FOCUS selection rectangle.

It's not strictly required in documentation (and that shows) but it'd be
nice to align the paragraphs at equal indentation.

OK



+   
+   
+ 
V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION 
+ The auto focus region of interest is determined
+by an object (e.g. face) detection engine.
+  

Re: [PATCH RFC 2/2] V4L: Add V4L2_CID_AUTO_FOCUS_AREA control

2012-12-11 Thread Sakari Ailus
Hi Andrzej and Sylwester,

Thanks for the patch!

On Mon, Dec 10, 2012 at 02:43:39PM +0100, Andrzej Hajda wrote:
> From: Sylwester Nawrocki 
> 
> Add control for automatic focus area selection.
> This control determines the area of the frame that the camera uses
> for automatic focus.
> 
> Signed-off-by: Sylwester Nawrocki 
> Signed-off-by: Andrzej Hajda 
> Signed-off-by: Kyungmin Park 
> ---
>  Documentation/DocBook/media/v4l/compat.xml   |9 +++--
>  Documentation/DocBook/media/v4l/controls.xml |   47 
> +-
>  Documentation/DocBook/media/v4l/v4l2.xml |7 
>  drivers/media/v4l2-core/v4l2-ctrls.c |   10 ++
>  include/uapi/linux/v4l2-controls.h   |6 
>  5 files changed, 76 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/compat.xml 
> b/Documentation/DocBook/media/v4l/compat.xml
> index 4fdf6b5..e8b53da 100644
> --- a/Documentation/DocBook/media/v4l/compat.xml
> +++ b/Documentation/DocBook/media/v4l/compat.xml
> @@ -2452,8 +2452,9 @@ that used it. It was originally scheduled for removal 
> in 2.6.35.
> V4L2_CID_3A_LOCK,
> V4L2_CID_AUTO_FOCUS_START,
> V4L2_CID_AUTO_FOCUS_STOP,
> -   V4L2_CID_AUTO_FOCUS_STATUS and
> -   V4L2_CID_AUTO_FOCUS_RANGE.
> +   V4L2_CID_AUTO_FOCUS_STATUS,
> +   V4L2_CID_AUTO_FOCUS_RANGE and
> +   V4L2_CID_AUTO_FOCUS_AREA.
> 
>  
>
> @@ -2586,6 +2587,10 @@ ioctls.
> Vendor and device specific media bus pixel formats.
>   .
>  
> +
> +   
> +   V4L2_CID_AUTO_FOCUS_AREA control.
> +
>
>  
>  
> diff --git a/Documentation/DocBook/media/v4l/controls.xml 
> b/Documentation/DocBook/media/v4l/controls.xml
> index 7fe5be1..9d4af8a 100644
> --- a/Documentation/DocBook/media/v4l/controls.xml
> +++ b/Documentation/DocBook/media/v4l/controls.xml
> @@ -3347,6 +3347,51 @@ use its minimum possible distance for auto 
> focus.
> 
> 
>  
> +   
> + 
> +   V4L2_CID_AUTO_FOCUS_AREA 
> + enum v4l2_auto_focus_area
> +   
> +   Determines the area of the frame that
> +the camera uses for automatic focus. The corresponding coordinates of the
> +focusing spot or rectangle can be specified and queried using the selection 
> API.
> +To change the auto focus region of interest applications first select 
> required
> +mode of this control and then set the rectangle or spot coordinates by means
> +of the &VIDIOC-SUBDEV-S-SELECTION; or &VIDIOC-S-SELECTION; ioctl. In order to
> +trigger again a one shot auto focus with same coordinates applications should
> +use the V4L2_CID_AUTO_FOCUS_START  control. Or 
> alternatively

Extra space above.^

> +invoke a &VIDIOC-SUBDEV-S-SELECTION; or a &VIDIOC-S-SELECTION; ioctl again.

How about requiring explicit V4L2_CID_AUTO_FOCUS_START? If you need to
specify several AF selection windows, then on which one do you start the
algorithm? A bitmask control explicitly telling which ones are active would
also be needed --- but that's for the future. I think now we just need to
ascertain we don't make that difficult. :-)

> +In the latter case the new pixel coordinates are applied to hardware only 
> when
> +the focus area control was set to
> +V4L2_AUTO_FOCUS_AREA_RECTANGLE.
> +   
> +   
> + 
> +   
> + 
> +   
> V4L2_AUTO_FOCUS_AREA_ALL 
> +   Normal auto focus, the focusing area extends over the
> +entire frame.

Does this need to be explicitly specified? Shouldn't the user just choose
the largest possible AF window instead? I'd even expect that the AF window
might span the whole frame by default (up to driver, hardware etc.).

> + 
> + 
> +   
> V4L2_AUTO_FOCUS_AREA_RECTANGLE 
> +   The auto focus region of interest is determined by the
> +V4L2_SEL_TGT_AUTO_FOCUS selection rectangle.

It's not strictly required in documentation (and that shows) but it'd be
nice to align the paragraphs at equal indentation.

> + 
> + 
> +   
> V4L2_AUTO_FOCUS_AREA_OBJECT_DETECTION 
> +   The auto focus region of interest is determined
> +by an object (e.g. face) detection engine.
> + 
> +   
> + 
> +   
> +   
> + This is an experimental
> +control and may change in the future.
> +   
> +   
> +
> 
>spanname="id">V4L2_CID_ZOOM_ABSOLUTE 
>   integer
> @@ -3986,7 +4031,7 @@ interface and may change in the future.
>  
>
>Flash Control IDs
> -
> +
>
>   
>   
> diff --git a/Documentation/DocBook/media/v4l/v4l2.xml 
> b/Documentation/DocBook/media/v4l/v4l2.xml
> index 10ccde9..1477540 100644
> --- a/Documentation/DocBook/media/v4l/v4l2.xml
> +++ b/Documentation/DocBook/media/v4l/v4l2.xml
> @@ -140,6 +140,13 @@ structs, ioctls