[RFC/PATCH,1/5] v4l: Convert V4L2_CID_FOCUS_AUTO control to a menu control

  Change the V4L2_CID_FOCUS_AUTO control type from boolean to a menu
type. In case of boolean control we had values 0 and 1 corresponding
to manual and automatic focus respectively.

The V4L2_CID_FOCUS_AUTO menu control has currently following items:
  0 - V4L2_FOCUS_MANUAL,
  1 - V4L2_FOCUS_AUTO,
  2 - V4L2_FOCUS_AUTO_MACRO,
  3 - V4L2_FOCUS_AUTO_CONTINUOUS.

To trigger single auto focus action in V4L2_FOCUS_AUTO mode the
V4L2_DO_AUTO_FOCUS control can be used, which is also added in this
patch.

Signed-off-by: Heungjun Kim <riverful.kim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Sylwester Nawrocki <snjw23@gmail.com>
---
 Documentation/DocBook/media/v4l/controls.xml |   52 +++++++++++++++++++++++--
 drivers/media/video/v4l2-ctrls.c             |   13 ++++++-
 include/linux/videodev2.h                    |    8 ++++
 3 files changed, 67 insertions(+), 6 deletions(-)
  

Hi Sylwester,

On Sunday 04 December 2011 16:16:12 Sylwester Nawrocki wrote:
> Change the V4L2_CID_FOCUS_AUTO control type from boolean to a menu
> type. In case of boolean control we had values 0 and 1 corresponding
> to manual and automatic focus respectively.
> 
> The V4L2_CID_FOCUS_AUTO menu control has currently following items:
>   0 - V4L2_FOCUS_MANUAL,
>   1 - V4L2_FOCUS_AUTO,
>   2 - V4L2_FOCUS_AUTO_MACRO,
>   3 - V4L2_FOCUS_AUTO_CONTINUOUS.

I think the mapping is wrong, please see my answer to 2/5.

> To trigger single auto focus action in V4L2_FOCUS_AUTO mode the
> V4L2_DO_AUTO_FOCUS control can be used, which is also added in this
> patch.
> 
> Signed-off-by: Heungjun Kim <riverful.kim@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> Signed-off-by: Sylwester Nawrocki <snjw23@gmail.com>
> ---
>  Documentation/DocBook/media/v4l/controls.xml |   52
> +++++++++++++++++++++++-- drivers/media/video/v4l2-ctrls.c             |  
> 13 ++++++-
>  include/linux/videodev2.h                    |    8 ++++
>  3 files changed, 67 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/controls.xml
> b/Documentation/DocBook/media/v4l/controls.xml index 3bc5ee8..5ccb0b0
> 100644
> --- a/Documentation/DocBook/media/v4l/controls.xml
> +++ b/Documentation/DocBook/media/v4l/controls.xml
> @@ -2782,12 +2782,54 @@ negative values towards infinity. This is a
> write-only control.</entry> </row>
>  	  <row><entry></entry></row>
> 
> -	  <row>
> +	  <row id="v4l2-focus-auto-type">
>  	    <entry
> spanname="id"><constant>V4L2_CID_FOCUS_AUTO</constant>&nbsp;</entry> -	   
> <entry>boolean</entry>
> -	  </row><row><entry spanname="descr">Enables automatic focus
> -adjustments. The effect of manual focus adjustments while this feature
> -is enabled is undefined, drivers should ignore such requests.</entry>
> +	    <entry>enum&nbsp;v4l2_focus_auto_type</entry>
> +	  </row><row><entry spanname="descr">Determines the camera
> +focus mode. The effect of manual focus adjustments while <constant>
> +V4L2_CID_FOCUS_AUTO </constant> is not set to <constant>
> +V4L2_FOCUS_MANUAL</constant> is undefined, drivers should ignore such
> +requests.</entry>
> +	  </row>
> +	  <row>
> +	    <entrytbl spanname="descr" cols="2">
> +	      <tbody valign="top">
> +		<row>
> +		  <entry><constant>V4L2_FOCUS_MANUAL</constant>&nbsp;</entry>
> +		  <entry>Manual focus.</entry>
> +		</row>
> +		<row>
> +		  <entry><constant>V4L2_FOCUS_AUTO</constant>&nbsp;</entry>
> +		  <entry>Single shot auto focus. When switched to
> +this mode the camera focuses on a subject just once. <constant>
> +V4L2_CID_DO_AUTO_FOCUS</constant> control can be used to manually
> +invoke auto focusing.</entry>
> +		</row>

Do we need this single-shot auto-focus menu entry ? We could just use 
V4L2_CID_DO_AUTO_FOCUS in V4L2_FOCUS_MANUAL mode to do this. This is what 
happens with one-shot white balance.

> +		<row>
> +		  <entry><constant>V4L2_FOCUS_AUTO_MACRO</constant>&nbsp;</entry>
> +		  <entry>Macro (close-up) auto focus. Usually camera
> +auto focus algorithms do not attempt to focus on a subject that is
> +closer than a given distance. This mode can be used to tell the camera
> +to use minimum distance for focus that it is capable of.</entry>
> +		</row>
> +		<row>
> +		  <entry><constant>V4L2_FOCUS_AUTO_CONTINUOUS</constant>&nbsp;</entry>
> +		  <entry>Continuous auto focus. When switched to this
> +mode the camera continually adjusts focus.</entry>
> +		</row>
> +	      </tbody>
> +	    </entrytbl>
> +	  </row>
> +	  <row><entry></entry></row>
> +
> +	  <row>
> +	    <entry
> spanname="id"><constant>V4L2_CID_DO_AUTO_FOCUS</constant>&nbsp;</entry> +	
>    <entry>button</entry>
> +	  </row><row><entry spanname="descr">When this control is set

Wouldn't "written" be better than "set" here ? I understand "When this control 
is set" as "while the control has a non-zero value" (but it might just be me).

> +the camera will perform one shot auto focus. The effect of using this
> +control when <constant>V4L2_CID_FOCUS_AUTO</constant> is in mode
> +different than <constant>V4L2_FOCUS_AUTO</constant> is undefined,
> +drivers should ignore such requests. </entry>
>  	  </row>
>  	  <row><entry></entry></row>
> 
> diff --git a/drivers/media/video/v4l2-ctrls.c
> b/drivers/media/video/v4l2-ctrls.c index 0f415da..7d8862f 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -221,6 +221,13 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		"Aperture Priority Mode",
>  		NULL
>  	};
> +	static const char * const camera_focus_auto[] = {
> +		"Manual Focus",
> +		"One-shot Auto Focus",
> +		"Macro Auto Focus",
> +		"Continuous Auto Focus",
> +		NULL
> +	};
>  	static const char * const colorfx[] = {
>  		"None",
>  		"Black & White",
> @@ -388,6 +395,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		return camera_power_line_frequency;
>  	case V4L2_CID_EXPOSURE_AUTO:
>  		return camera_exposure_auto;
> +	case V4L2_CID_FOCUS_AUTO:
> +		return camera_focus_auto;
>  	case V4L2_CID_COLORFX:
>  		return colorfx;
>  	case V4L2_CID_TUNE_PREEMPHASIS:
> @@ -567,6 +576,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_PRIVACY:			return "Privacy";
>  	case V4L2_CID_IRIS_ABSOLUTE:		return "Iris, Absolute";
>  	case V4L2_CID_IRIS_RELATIVE:		return "Iris, Relative";
> +	case V4L2_CID_DO_AUTO_FOCUS:		return "Do Auto Focus";
> 
>  	/* FM Radio Modulator control */
>  	/* Keep the order of the 'case's the same as in videodev2.h! */
> @@ -633,7 +643,6 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
> v4l2_ctrl_type *type, case V4L2_CID_MPEG_VIDEO_GOP_CLOSURE:
>  	case V4L2_CID_MPEG_VIDEO_PULLDOWN:
>  	case V4L2_CID_EXPOSURE_AUTO_PRIORITY:
> -	case V4L2_CID_FOCUS_AUTO:
>  	case V4L2_CID_PRIVACY:
>  	case V4L2_CID_AUDIO_LIMITER_ENABLED:
>  	case V4L2_CID_AUDIO_COMPRESSION_ENABLED:
> @@ -658,6 +667,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
> v4l2_ctrl_type *type, case V4L2_CID_TILT_RESET:
>  	case V4L2_CID_FLASH_STROBE:
>  	case V4L2_CID_FLASH_STROBE_STOP:
> +	case V4L2_CID_DO_AUTO_FOCUS:
>  		*type = V4L2_CTRL_TYPE_BUTTON;
>  		*flags |= V4L2_CTRL_FLAG_WRITE_ONLY;
>  		*min = *max = *step = *def = 0;
> @@ -679,6 +689,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_FOCUS_AUTO:
>  	case V4L2_CID_COLORFX:
>  	case V4L2_CID_TUNE_PREEMPHASIS:
>  	case V4L2_CID_FLASH_LED_MODE:
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index 4b752d5..9acb514 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -1608,6 +1608,12 @@ enum  v4l2_exposure_auto_type {
>  #define V4L2_CID_FOCUS_ABSOLUTE			(V4L2_CID_CAMERA_CLASS_BASE+10)
>  #define V4L2_CID_FOCUS_RELATIVE			(V4L2_CID_CAMERA_CLASS_BASE+11)
>  #define V4L2_CID_FOCUS_AUTO			(V4L2_CID_CAMERA_CLASS_BASE+12)
> +enum v4l2_focus_auto_type {
> +	V4L2_FOCUS_MANUAL = 0,
> +	V4L2_FOCUS_AUTO = 1,
> +	V4L2_FOCUS_AUTO_MACRO = 2,
> +	V4L2_FOCUS_AUTO_CONTINUOUS = 3,
> +};
> 
>  #define V4L2_CID_ZOOM_ABSOLUTE			(V4L2_CID_CAMERA_CLASS_BASE+13)
>  #define V4L2_CID_ZOOM_RELATIVE			(V4L2_CID_CAMERA_CLASS_BASE+14)
> @@ -1618,6 +1624,8 @@ enum  v4l2_exposure_auto_type {
>  #define V4L2_CID_IRIS_ABSOLUTE			(V4L2_CID_CAMERA_CLASS_BASE+17)
>  #define V4L2_CID_IRIS_RELATIVE			(V4L2_CID_CAMERA_CLASS_BASE+18)
> 
> +#define V4L2_CID_DO_AUTO_FOCUS			(V4L2_CID_CAMERA_CLASS_BASE+19)
> +
>  /* FM Modulator class control IDs */
>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
>  #define V4L2_CID_FM_TX_CLASS			(V4L2_CTRL_CLASS_FM_TX | 1)
  

Hi Laurent,

On 12/06/2011 01:31 PM, Laurent Pinchart wrote:
> Hi Sylwester,
> 
> On Sunday 04 December 2011 16:16:12 Sylwester Nawrocki wrote:
>> Change the V4L2_CID_FOCUS_AUTO control type from boolean to a menu
>> type. In case of boolean control we had values 0 and 1 corresponding
>> to manual and automatic focus respectively.
>>
>> The V4L2_CID_FOCUS_AUTO menu control has currently following items:
>>   0 - V4L2_FOCUS_MANUAL,
>>   1 - V4L2_FOCUS_AUTO,
>>   2 - V4L2_FOCUS_AUTO_MACRO,
>>   3 - V4L2_FOCUS_AUTO_CONTINUOUS.
> 
> I think the mapping is wrong, please see my answer to 2/5.

Yes, agreed. Please see my answer to you answer to 2/5 :)

> 
>> To trigger single auto focus action in V4L2_FOCUS_AUTO mode the
>> V4L2_DO_AUTO_FOCUS control can be used, which is also added in this
>> patch.
>>
>> Signed-off-by: Heungjun Kim <riverful.kim@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> Signed-off-by: Sylwester Nawrocki <snjw23@gmail.com>
>> ---
>>  Documentation/DocBook/media/v4l/controls.xml |   52
>> +++++++++++++++++++++++-- drivers/media/video/v4l2-ctrls.c             |  
>> 13 ++++++-
>>  include/linux/videodev2.h                    |    8 ++++
>>  3 files changed, 67 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/DocBook/media/v4l/controls.xml
>> b/Documentation/DocBook/media/v4l/controls.xml index 3bc5ee8..5ccb0b0
>> 100644
>> --- a/Documentation/DocBook/media/v4l/controls.xml
>> +++ b/Documentation/DocBook/media/v4l/controls.xml
>> @@ -2782,12 +2782,54 @@ negative values towards infinity. This is a
>> write-only control.</entry> </row>
>>  	  <row><entry></entry></row>
>>
>> -	  <row>
>> +	  <row id="v4l2-focus-auto-type">
>>  	    <entry
>> spanname="id"><constant>V4L2_CID_FOCUS_AUTO</constant>&nbsp;</entry> -	   
>> <entry>boolean</entry>
>> -	  </row><row><entry spanname="descr">Enables automatic focus
>> -adjustments. The effect of manual focus adjustments while this feature
>> -is enabled is undefined, drivers should ignore such requests.</entry>
>> +	    <entry>enum&nbsp;v4l2_focus_auto_type</entry>
>> +	  </row><row><entry spanname="descr">Determines the camera
>> +focus mode. The effect of manual focus adjustments while <constant>
>> +V4L2_CID_FOCUS_AUTO </constant> is not set to <constant>
>> +V4L2_FOCUS_MANUAL</constant> is undefined, drivers should ignore such
>> +requests.</entry>
>> +	  </row>
>> +	  <row>
>> +	    <entrytbl spanname="descr" cols="2">
>> +	      <tbody valign="top">
>> +		<row>
>> +		  <entry><constant>V4L2_FOCUS_MANUAL</constant>&nbsp;</entry>
>> +		  <entry>Manual focus.</entry>
>> +		</row>
>> +		<row>
>> +		  <entry><constant>V4L2_FOCUS_AUTO</constant>&nbsp;</entry>
>> +		  <entry>Single shot auto focus. When switched to
>> +this mode the camera focuses on a subject just once. <constant>
>> +V4L2_CID_DO_AUTO_FOCUS</constant> control can be used to manually
>> +invoke auto focusing.</entry>
>> +		</row>
> 
> Do we need this single-shot auto-focus menu entry ? We could just use 
> V4L2_CID_DO_AUTO_FOCUS in V4L2_FOCUS_MANUAL mode to do this. This is what 
> happens with one-shot white balance.

There might be various flavours of single-shot auto focus, there may be some
more focus modes than those in this patch.
If we have removed them from the menu entry then we would likely have to
come up with more V4L2_CID_DO_AUTO_FOCUS_* controls.

> 
>> +		<row>
>> +		  <entry><constant>V4L2_FOCUS_AUTO_MACRO</constant>&nbsp;</entry>
>> +		  <entry>Macro (close-up) auto focus. Usually camera
>> +auto focus algorithms do not attempt to focus on a subject that is
>> +closer than a given distance. This mode can be used to tell the camera
>> +to use minimum distance for focus that it is capable of.</entry>
>> +		</row>
>> +		<row>
>> +		  <entry><constant>V4L2_FOCUS_AUTO_CONTINUOUS</constant>&nbsp;</entry>
>> +		  <entry>Continuous auto focus. When switched to this
>> +mode the camera continually adjusts focus.</entry>
>> +		</row>
>> +	      </tbody>
>> +	    </entrytbl>
>> +	  </row>
>> +	  <row><entry></entry></row>
>> +
>> +	  <row>
>> +	    <entry
>> spanname="id"><constant>V4L2_CID_DO_AUTO_FOCUS</constant>&nbsp;</entry> +	
>>    <entry>button</entry>
>> +	  </row><row><entry spanname="descr">When this control is set
> 
> Wouldn't "written" be better than "set" here ? I understand "When this control 
> is set" as "while the control has a non-zero value" (but it might just be me).

Sure, I could change to that. However this is a button control, so "written"
doesn't quite match IMHO. And we do set/get controls, rather than read/write.

Originally I had (copied form V4L2_CID_DO_WHITE_BALANCE):

"This is an action control. When it is set (the value is ignored)..."

How about "When this control is applied.." ?

>
>> +the camera will perform one shot auto focus. The effect of using this
>> +control when <constant>V4L2_CID_FOCUS_AUTO</constant> is in mode
>> +different than <constant>V4L2_FOCUS_AUTO</constant> is undefined,
>> +drivers should ignore such requests. </entry>
>>  	  </row>

--

Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Sylwester,

On Sun, Dec 04, 2011 at 04:16:12PM +0100, Sylwester Nawrocki wrote:
> Change the V4L2_CID_FOCUS_AUTO control type from boolean to a menu
> type. In case of boolean control we had values 0 and 1 corresponding
> to manual and automatic focus respectively.
> 
> The V4L2_CID_FOCUS_AUTO menu control has currently following items:
>   0 - V4L2_FOCUS_MANUAL,
>   1 - V4L2_FOCUS_AUTO,
>   2 - V4L2_FOCUS_AUTO_MACRO,
>   3 - V4L2_FOCUS_AUTO_CONTINUOUS.

I would put the macro mode to a separate menu since it's configuration for
how the regular AF works rather than really different mode.

...

> @@ -567,6 +576,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_PRIVACY:			return "Privacy";
>  	case V4L2_CID_IRIS_ABSOLUTE:		return "Iris, Absolute";
>  	case V4L2_CID_IRIS_RELATIVE:		return "Iris, Relative";
> +	case V4L2_CID_DO_AUTO_FOCUS:		return "Do Auto Focus";

I'd perhaps use "begin" or "start". How does the user learn the autofocus
has finished? I think this looks like quite similar problem than telling the
flash strobe status to the user. The solution could also be similar to that.

Cheers,
  

Hi Sakari,

On 12/10/2011 11:33 AM, Sakari Ailus wrote:
> On Sun, Dec 04, 2011 at 04:16:12PM +0100, Sylwester Nawrocki wrote:
>> Change the V4L2_CID_FOCUS_AUTO control type from boolean to a menu
>> type. In case of boolean control we had values 0 and 1 corresponding
>> to manual and automatic focus respectively.
>>
>> The V4L2_CID_FOCUS_AUTO menu control has currently following items:
>>   0 - V4L2_FOCUS_MANUAL,
>>   1 - V4L2_FOCUS_AUTO,
>>   2 - V4L2_FOCUS_AUTO_MACRO,
>>   3 - V4L2_FOCUS_AUTO_CONTINUOUS.
> 
> I would put the macro mode to a separate menu since it's configuration for
> how the regular AF works rather than really different mode.

Yes, makes sense. Most likely there could be also continuous macro auto focus..
I don't have yet an idea what could be a name for that new menu though.

Many Samsung devices have also something like guided auto focus, where the
application can specify location in the frame for focusing on. IIRC this could
be also single-shot or continuous. So it could make sense to group MACRO and
"guided" auto focus in one menu, what do you think ?


>> @@ -567,6 +576,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>  	case V4L2_CID_PRIVACY:			return "Privacy";
>>  	case V4L2_CID_IRIS_ABSOLUTE:		return "Iris, Absolute";
>>  	case V4L2_CID_IRIS_RELATIVE:		return "Iris, Relative";
>> +	case V4L2_CID_DO_AUTO_FOCUS:		return "Do Auto Focus";
> 
> I'd perhaps use "begin" or "start". How does the user learn the autofocus

I agree, it's not something than is finished after G_CTRL call returns.

V4L2_CID_START_AUTO_FOCUS sounds better to me.

> has finished? I think this looks like quite similar problem than telling the
> flash strobe status to the user. The solution could also be similar to that.

I guess you're suggesting an auto focus status control? Together with the control
events it would be nice interface for notifying the applications.
  

On 12/10/2011 11:33 AM, Sakari Ailus wrote:
> On Sun, Dec 04, 2011 at 04:16:12PM +0100, Sylwester Nawrocki wrote:
>> Change the V4L2_CID_FOCUS_AUTO control type from boolean to a menu
>> type. In case of boolean control we had values 0 and 1 corresponding
>> to manual and automatic focus respectively.
>>
>> The V4L2_CID_FOCUS_AUTO menu control has currently following items:
>>   0 - V4L2_FOCUS_MANUAL,
>>   1 - V4L2_FOCUS_AUTO,
>>   2 - V4L2_FOCUS_AUTO_MACRO,
>>   3 - V4L2_FOCUS_AUTO_CONTINUOUS.
> 
> I would put the macro mode to a separate menu since it's configuration for
> how the regular AF works rather than really different mode.
> 
> ...
> 
>> @@ -567,6 +576,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>  	case V4L2_CID_PRIVACY:			return "Privacy";
>>  	case V4L2_CID_IRIS_ABSOLUTE:		return "Iris, Absolute";
>>  	case V4L2_CID_IRIS_RELATIVE:		return "Iris, Relative";
>> +	case V4L2_CID_DO_AUTO_FOCUS:		return "Do Auto Focus";
> 
> I'd perhaps use "begin" or "start". How does the user learn the autofocus
> has finished? I think this looks like quite similar problem than telling the
> flash strobe status to the user. The solution could also be similar to that.

Hi,

considering your previous comments, how about the below focusing control set ?

Controls for starting/stopping auto focusing (for V4L2_CID_FOCUS_AUTO ==
V4L2_FOCUS_MANUAL):

V4L2_CID_START_AUTO_FOCUS  - (button) - start auto focusing,
V4L2_CID_STOP_AUTO_FOCUS   - (button) - stop auto focusing (might be also
                                        useful in V4L2_FOCUS_AUTO_CONTINUOUS
                                        mode)
and auto focus status:

V4L2_CID_AUTO_FOCUS_STATUS - (boolean) - whether focusing is in progress or not
                             (when V4L2_CID_FOCUS_AUTO == V4L2_FOCUS_MANUAL)


V4L2_CID_FOCUS_AUTO would basically retain it's current semantics:

V4L2_CID_FOCUS_AUTO        - (boolean) - select focus type (manual/auto
                                         continuous)
        V4L2_FOCUS_MANUAL              - manual focus
        V4L2_FOCUS_AUTO_CONTINUOUS     - continuous auto focus
                                         (or V4L2_FOCUS_AUTO)

New menu control to choose behaviour of auto focus (either single-shot
or continuous):

V4L2_CID_AF_MODE                        - select auto focus mode
        V4L2_AF_MODE_NORMAL             - "normal" auto focus
        V4L2_AF_MODE_MACRO              - macro (close-up)
        V4L2_AF_MODE_SPOT               - spot location passed with
                                          selection API or other control
        V4L2_AF_MODE_FACE_DETECTION	- focus point indicated by internal
                                          face detector


I might try it out and implement in M-5MOLS driver, to see how it works
in practice.

Does it make sense for you ?

--

Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Sylwester,

Apologies for my late answer.

On Sat, Dec 10, 2011 at 03:42:41PM +0100, Sylwester Nawrocki wrote:
> Hi Sakari,
> 
> On 12/10/2011 11:33 AM, Sakari Ailus wrote:
> > On Sun, Dec 04, 2011 at 04:16:12PM +0100, Sylwester Nawrocki wrote:
> >> Change the V4L2_CID_FOCUS_AUTO control type from boolean to a menu
> >> type. In case of boolean control we had values 0 and 1 corresponding
> >> to manual and automatic focus respectively.
> >>
> >> The V4L2_CID_FOCUS_AUTO menu control has currently following items:
> >>   0 - V4L2_FOCUS_MANUAL,
> >>   1 - V4L2_FOCUS_AUTO,
> >>   2 - V4L2_FOCUS_AUTO_MACRO,
> >>   3 - V4L2_FOCUS_AUTO_CONTINUOUS.
> > 
> > I would put the macro mode to a separate menu since it's configuration for
> > how the regular AF works rather than really different mode.
> 
> Yes, makes sense. Most likely there could be also continuous macro auto focus..
> I don't have yet an idea what could be a name for that new menu though.

V4L2_CID_FOCUS_AUTO_DISTANCE? It could then have choices FULL or MACRO.

> Many Samsung devices have also something like guided auto focus, where the
> application can specify location in the frame for focusing on. IIRC this could
> be also single-shot or continuous. So it could make sense to group MACRO and
> "guided" auto focus in one menu, what do you think ?

I think it could be a separate menu. It's not connected to the distance
parameter. We also need to discuss how the af statistics window
configuration is done. I'm not certain there could even be a standardised
interface which could be used to control it all.

> >> @@ -567,6 +576,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> >>  	case V4L2_CID_PRIVACY:			return "Privacy";
> >>  	case V4L2_CID_IRIS_ABSOLUTE:		return "Iris, Absolute";
> >>  	case V4L2_CID_IRIS_RELATIVE:		return "Iris, Relative";
> >> +	case V4L2_CID_DO_AUTO_FOCUS:		return "Do Auto Focus";
> > 
> > I'd perhaps use "begin" or "start". How does the user learn the autofocus
> 
> I agree, it's not something than is finished after G_CTRL call returns.
> 
> V4L2_CID_START_AUTO_FOCUS sounds better to me.
> 
> > has finished? I think this looks like quite similar problem than telling the
> > flash strobe status to the user. The solution could also be similar to that.
> 
> I guess you're suggesting an auto focus status control? Together with the control
> events it would be nice interface for notifying the applications.

I agree.
  

On 12/31/2011 01:00 PM, Sakari Ailus wrote:
> Hi Sylwester,
> 
> Apologies for my late answer.

No problem, thanks for your comments!

> On Sat, Dec 10, 2011 at 03:42:41PM +0100, Sylwester Nawrocki wrote:
>> Hi Sakari,
>>
>> On 12/10/2011 11:33 AM, Sakari Ailus wrote:
>>> On Sun, Dec 04, 2011 at 04:16:12PM +0100, Sylwester Nawrocki wrote:
>>>> Change the V4L2_CID_FOCUS_AUTO control type from boolean to a menu
>>>> type. In case of boolean control we had values 0 and 1 corresponding
>>>> to manual and automatic focus respectively.
>>>>
>>>> The V4L2_CID_FOCUS_AUTO menu control has currently following items:
>>>>   0 - V4L2_FOCUS_MANUAL,
>>>>   1 - V4L2_FOCUS_AUTO,
>>>>   2 - V4L2_FOCUS_AUTO_MACRO,
>>>>   3 - V4L2_FOCUS_AUTO_CONTINUOUS.
>>>
>>> I would put the macro mode to a separate menu since it's configuration for
>>> how the regular AF works rather than really different mode.
>>
>> Yes, makes sense. Most likely there could be also continuous macro auto focus..
>> I don't have yet an idea what could be a name for that new menu though.
> 
> V4L2_CID_FOCUS_AUTO_DISTANCE? It could then have choices FULL or MACRO.

How about V4L2_CID_FOCUS_AUTO_SCAN_RANGE ? Which would then have choices:
	NORMAL,
	MACRO,
	INFINITY
?

>> Many Samsung devices have also something like guided auto focus, where the
>> application can specify location in the frame for focusing on. IIRC this could
>> be also single-shot or continuous. So it could make sense to group MACRO and
>> "guided" auto focus in one menu, what do you think ?
> 
> I think it could be a separate menu. It's not connected to the distance

OK, let me summarize

* controls for starting/stopping auto focusing (V4L2_CID_FOCUS_AUTO == false)

  V4L2_CID_START_AUTO_FOCUS (button) - start auto focusing,
  V4L2_CID_STOP_AUTO_FOCUS  (button) - stop auto focusing (might be also
                                       useful in V4L2_FOCUS_AUTO == true),
* auto focus status

  V4L2_CID_AUTO_FOCUS_STATUS (menu, read-only) - whether focusing is in
                                                 progress or not,
  possible entries:

  - V4L2_AUTO_FOCUS_STATUS_IDLE,    // auto focusing not enabled or force stopped
  - V4L2_AUTO_FOCUS_STATUS_BUSY,    // focusing in progress
  - V4L2_AUTO_FOCUS_STATUS_SUCCESS, // single-shot auto focusing succeed
                                    // or continuous AF in progress
  - V4L2_AUTO_FOCUS_STATUS_FAIL,    // auto focusing failed


* V4L2_CID_FOCUS_AUTO would retain its current semantics:

  V4L2_CID_FOCUS_AUTO (boolean) - selects auto/manual focus
      false - manual
      true  - auto continuous

* AF algorithm scan range, V4L2_CID_FOCUS_AUTO_SCAN_RANGE with choices:

  - V4L2_AUTO_FOCUS_SCAN_RANGE_NORMAL,
  - V4L2_AUTO_FOCUS_SCAN_RANGE_MACRO,
  - V4L2_AUTO_FOCUS_SCAN_RANGE_INFINITY


New menu control to choose behaviour of auto focus (either single-shot
or continuous):

* select auto focus mode

V4L2_CID_AUTO_FOCUS_MODE
        V4L2_AUTO_FOCUS_MODE_NORMAL     - "normal" auto focus (whole frame?)
        V4L2_AUTO_FOCUS_MODE_SPOT       - spot location passed with other
                                          controls or selection API
        V4L2_AUTO_FOCUS_MODE_RECTANGLE  - rectangle passed with other
                                          controls or selection API


> parameter. We also need to discuss how the af statistics window
> configuration is done. I'm not certain there could even be a standardised

Do we need multiple windows for AF statistics ?

If not, I'm inclined to use four separate controls for window configuration.
(X, Y, WIDTH, HEIGHT). This was Hans' preference in previous discussions [1].

> interface which could be used to control it all.


[1] http://www.mail-archive.com/linux-media@vger.kernel.org/msg25647.html
  

Hi Sylwester,

On Sunday 01 January 2012 17:49:11 Sylwester Nawrocki wrote:
> On 12/31/2011 01:00 PM, Sakari Ailus wrote:
> > On Sat, Dec 10, 2011 at 03:42:41PM +0100, Sylwester Nawrocki wrote:
> >> On 12/10/2011 11:33 AM, Sakari Ailus wrote:
> >>> On Sun, Dec 04, 2011 at 04:16:12PM +0100, Sylwester Nawrocki wrote:
> >>>> Change the V4L2_CID_FOCUS_AUTO control type from boolean to a menu
> >>>> type. In case of boolean control we had values 0 and 1 corresponding
> >>>> to manual and automatic focus respectively.
> >>>> 
> >>>> The V4L2_CID_FOCUS_AUTO menu control has currently following items:
> >>>>   0 - V4L2_FOCUS_MANUAL,
> >>>>   1 - V4L2_FOCUS_AUTO,
> >>>>   2 - V4L2_FOCUS_AUTO_MACRO,
> >>>>   3 - V4L2_FOCUS_AUTO_CONTINUOUS.
> >>> 
> >>> I would put the macro mode to a separate menu since it's configuration
> >>> for how the regular AF works rather than really different mode.
> >> 
> >> Yes, makes sense. Most likely there could be also continuous macro auto
> >> focus.. I don't have yet an idea what could be a name for that new menu
> >> though.
> > 
> > V4L2_CID_FOCUS_AUTO_DISTANCE? It could then have choices FULL or MACRO.
> 
> How about V4L2_CID_FOCUS_AUTO_SCAN_RANGE ? Which would then have choices:
> 	NORMAL,
> 	MACRO,
> 	INFINITY
> ?
> 
> >> Many Samsung devices have also something like guided auto focus, where
> >> the application can specify location in the frame for focusing on. IIRC
> >> this could be also single-shot or continuous. So it could make sense to
> >> group MACRO and "guided" auto focus in one menu, what do you think ?
> > 
> > I think it could be a separate menu. It's not connected to the distance
> 
> OK, let me summarize
> 
> * controls for starting/stopping auto focusing (V4L2_CID_FOCUS_AUTO ==
> false)
> 
>   V4L2_CID_START_AUTO_FOCUS (button) - start auto focusing,
>   V4L2_CID_STOP_AUTO_FOCUS  (button) - stop auto focusing (might be also
>                                        useful in V4L2_FOCUS_AUTO == true),

Maybe V4L2_CID_AUTO_FOCUS_START and V4L2_CID_AUTO_FOCUS_STOP to be consistent 
with the other proposed controls ?

> * auto focus status
> 
>   V4L2_CID_AUTO_FOCUS_STATUS (menu, read-only) - whether focusing is in
>                                                  progress or not,
>   possible entries:
> 
>   - V4L2_AUTO_FOCUS_STATUS_IDLE,    // auto focusing not enabled or force
> stopped - V4L2_AUTO_FOCUS_STATUS_BUSY,    // focusing in progress
>   - V4L2_AUTO_FOCUS_STATUS_SUCCESS, // single-shot auto focusing succeed
>                                     // or continuous AF in progress
>   - V4L2_AUTO_FOCUS_STATUS_FAIL,    // auto focusing failed
> 
> 
> * V4L2_CID_FOCUS_AUTO would retain its current semantics:
> 
>   V4L2_CID_FOCUS_AUTO (boolean) - selects auto/manual focus
>       false - manual
>       true  - auto continuous
> 
> * AF algorithm scan range, V4L2_CID_FOCUS_AUTO_SCAN_RANGE with choices:
> 
>   - V4L2_AUTO_FOCUS_SCAN_RANGE_NORMAL,
>   - V4L2_AUTO_FOCUS_SCAN_RANGE_MACRO,
>   - V4L2_AUTO_FOCUS_SCAN_RANGE_INFINITY
> 
> 
> New menu control to choose behaviour of auto focus (either single-shot
> or continuous):
> 
> * select auto focus mode
> 
> V4L2_CID_AUTO_FOCUS_MODE
>         V4L2_AUTO_FOCUS_MODE_NORMAL     - "normal" auto focus (whole frame?)
>         V4L2_AUTO_FOCUS_MODE_SPOT       - spot location passed with other
>         controls or selection API
>         V4L2_AUTO_FOCUS_MODE_RECTANGLE  - rectangle passed with other
>         controls or selection API

Soudns good to me.

> > parameter. We also need to discuss how the af statistics window
> > configuration is done. I'm not certain there could even be a standardised
> 
> Do we need multiple windows for AF statistics ?
>
> If not, I'm inclined to use four separate controls for window
> configuration. (X, Y, WIDTH, HEIGHT). This was Hans' preference in
> previous discussions [1].

For the OMAP3 ISP we need multiple statistics windows. AEWB can use more than 
32 windows. Having separate controls for that wouldn't be practical.

> > interface which could be used to control it all.
> 
> [1] http://www.mail-archive.com/linux-media@vger.kernel.org/msg25647.html
  

Hi Laurent,

On 01/02/2012 12:16 PM, Laurent Pinchart wrote:
>> * controls for starting/stopping auto focusing (V4L2_CID_FOCUS_AUTO ==
>> false)
>>
>>   V4L2_CID_START_AUTO_FOCUS (button) - start auto focusing,
>>   V4L2_CID_STOP_AUTO_FOCUS  (button) - stop auto focusing (might be also
>>                                        useful in V4L2_FOCUS_AUTO == true),
> 
> Maybe V4L2_CID_AUTO_FOCUS_START and V4L2_CID_AUTO_FOCUS_STOP to be consistent 
> with the other proposed controls ?

Yes, you're right, I'll change them to make consistent with others.
I've noticed that too, but a little bit too late:)

>> * auto focus status
>>
>>   V4L2_CID_AUTO_FOCUS_STATUS (menu, read-only) - whether focusing is in
>>                                                  progress or not,
>>   possible entries:
>>
>>   - V4L2_AUTO_FOCUS_STATUS_IDLE,    // auto focusing not enabled or force
>>                                        stopped 
>>   - V4L2_AUTO_FOCUS_STATUS_BUSY,    // focusing in progress
>>   - V4L2_AUTO_FOCUS_STATUS_SUCCESS, // single-shot auto focusing succeed
>>                                     // or continuous AF in progress
>>   - V4L2_AUTO_FOCUS_STATUS_FAIL,    // auto focusing failed
>>
>>
>> * V4L2_CID_FOCUS_AUTO would retain its current semantics:
>>
>>   V4L2_CID_FOCUS_AUTO (boolean) - selects auto/manual focus
>>       false - manual
>>       true  - auto continuous
>>
>> * AF algorithm scan range, V4L2_CID_FOCUS_AUTO_SCAN_RANGE with choices:
>>
>>   - V4L2_AUTO_FOCUS_SCAN_RANGE_NORMAL,
>>   - V4L2_AUTO_FOCUS_SCAN_RANGE_MACRO,
>>   - V4L2_AUTO_FOCUS_SCAN_RANGE_INFINITY
>>
...
>>
>> * select auto focus mode
>>
>> V4L2_CID_AUTO_FOCUS_MODE
>>         V4L2_AUTO_FOCUS_MODE_NORMAL     - "normal" auto focus (whole frame?)
>>         V4L2_AUTO_FOCUS_MODE_SPOT       - spot location passed with other
>>         controls or selection API
>>         V4L2_AUTO_FOCUS_MODE_RECTANGLE  - rectangle passed with other
>>         controls or selection API
> 
> Soudns good to me.
>
>>> parameter. We also need to discuss how the af statistics window
>>> configuration is done. I'm not certain there could even be a standardised
>>
>> Do we need multiple windows for AF statistics ?
>>
>> If not, I'm inclined to use four separate controls for window
>> configuration. (X, Y, WIDTH, HEIGHT). This was Hans' preference in
>> previous discussions [1].
> 
> For the OMAP3 ISP we need multiple statistics windows. AEWB can use more than 
> 32 windows. Having separate controls for that wouldn't be practical.

OK, so the control API in current form doesn't seem capable of setting up the
statistics windows. There is also little space in struct v4l2_ext_control for
any major extensions.

We might need to define dedicated set of selection targets in the selection
API for handling multiple windows.

Yet, to avoid forcing applications to use the selection API where rectangles
aren't needed - only single spot coordinates, how about defining following
two controls ?

* AF spot coordinates when focus mode is set to V4L2_AUTO_FOCUS_MODE_SPOT

 - V4L2_CID_AUTO_FOCUS_POSITION_X - horizontal position in pixels relative
                                    to the left of frame
 - V4L2_CID_AUTO_FOCUS_POSITION_Y - vertical position in pixels relative
                                    to the top of frame

--

Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Sylwester,

On Monday 02 January 2012 21:55:31 Sylwester Nawrocki wrote:
> On 01/02/2012 12:16 PM, Laurent Pinchart wrote:
> >> * controls for starting/stopping auto focusing (V4L2_CID_FOCUS_AUTO ==
> >> false)
> >> 
> >>   V4L2_CID_START_AUTO_FOCUS (button) - start auto focusing,
> >>   V4L2_CID_STOP_AUTO_FOCUS  (button) - stop auto focusing (might be also
> >>   
> >>                                        useful in V4L2_FOCUS_AUTO ==
> >>                                        true),
> > 
> > Maybe V4L2_CID_AUTO_FOCUS_START and V4L2_CID_AUTO_FOCUS_STOP to be
> > consistent with the other proposed controls ?
> 
> Yes, you're right, I'll change them to make consistent with others.
> I've noticed that too, but a little bit too late:)
> 
> >> * auto focus status
> >> 
> >>   V4L2_CID_AUTO_FOCUS_STATUS (menu, read-only) - whether focusing is in
> >>   
> >>                                                  progress or not,
> >>   
> >>   possible entries:
> >>   
> >>   - V4L2_AUTO_FOCUS_STATUS_IDLE,    // auto focusing not enabled or
> >>   force
> >>   
> >>                                        stopped
> >>   
> >>   - V4L2_AUTO_FOCUS_STATUS_BUSY,    // focusing in progress
> >>   - V4L2_AUTO_FOCUS_STATUS_SUCCESS, // single-shot auto focusing succeed
> >>   
> >>                                     // or continuous AF in progress
> >>   
> >>   - V4L2_AUTO_FOCUS_STATUS_FAIL,    // auto focusing failed
> >> 
> >> * V4L2_CID_FOCUS_AUTO would retain its current semantics:
> >>   V4L2_CID_FOCUS_AUTO (boolean) - selects auto/manual focus
> >>   
> >>       false - manual
> >>       true  - auto continuous
> >> 
> >> * AF algorithm scan range, V4L2_CID_FOCUS_AUTO_SCAN_RANGE with choices:
> >>   - V4L2_AUTO_FOCUS_SCAN_RANGE_NORMAL,
> >>   - V4L2_AUTO_FOCUS_SCAN_RANGE_MACRO,
> >>   - V4L2_AUTO_FOCUS_SCAN_RANGE_INFINITY
> 
> ...
> 
> >> * select auto focus mode
> >> 
> >> V4L2_CID_AUTO_FOCUS_MODE
> >> 
> >>         V4L2_AUTO_FOCUS_MODE_NORMAL     - "normal" auto focus (whole
> >>         frame?) V4L2_AUTO_FOCUS_MODE_SPOT       - spot location passed
> >>         with other controls or selection API
> >>         V4L2_AUTO_FOCUS_MODE_RECTANGLE  - rectangle passed with other
> >>         controls or selection API
> > 
> > Soudns good to me.
> > 
> >>> parameter. We also need to discuss how the af statistics window
> >>> configuration is done. I'm not certain there could even be a
> >>> standardised
> >> 
> >> Do we need multiple windows for AF statistics ?
> >> 
> >> If not, I'm inclined to use four separate controls for window
> >> configuration. (X, Y, WIDTH, HEIGHT). This was Hans' preference in
> >> previous discussions [1].
> > 
> > For the OMAP3 ISP we need multiple statistics windows. AEWB can use more
> > than 32 windows. Having separate controls for that wouldn't be
> > practical.
> 
> OK, so the control API in current form doesn't seem capable of setting up
> the statistics windows. There is also little space in struct
> v4l2_ext_control for any major extensions.
> 
> We might need to define dedicated set of selection targets in the selection
> API for handling multiple windows.
> 
> Yet, to avoid forcing applications to use the selection API where
> rectangles aren't needed - only single spot coordinates, how about
> defining following two controls ?
> 
> * AF spot coordinates when focus mode is set to V4L2_AUTO_FOCUS_MODE_SPOT
> 
>  - V4L2_CID_AUTO_FOCUS_POSITION_X - horizontal position in pixels relative
>                                     to the left of frame
>  - V4L2_CID_AUTO_FOCUS_POSITION_Y - vertical position in pixels relative
>                                     to the top of frame

What about a point control type instead ? :-) X and Y coordinates could be 
stored on 32 bits each.
  

Hi Sylwester,

On Sun, Jan 01, 2012 at 05:49:11PM +0100, Sylwester Nawrocki wrote:
> On 12/31/2011 01:00 PM, Sakari Ailus wrote:
> > Hi Sylwester,
> > 
> > Apologies for my late answer.
> 
> No problem, thanks for your comments!
> 
> > On Sat, Dec 10, 2011 at 03:42:41PM +0100, Sylwester Nawrocki wrote:
> >> Hi Sakari,
> >>
> >> On 12/10/2011 11:33 AM, Sakari Ailus wrote:
> >>> On Sun, Dec 04, 2011 at 04:16:12PM +0100, Sylwester Nawrocki wrote:
> >>>> Change the V4L2_CID_FOCUS_AUTO control type from boolean to a menu
> >>>> type. In case of boolean control we had values 0 and 1 corresponding
> >>>> to manual and automatic focus respectively.
> >>>>
> >>>> The V4L2_CID_FOCUS_AUTO menu control has currently following items:
> >>>>   0 - V4L2_FOCUS_MANUAL,
> >>>>   1 - V4L2_FOCUS_AUTO,
> >>>>   2 - V4L2_FOCUS_AUTO_MACRO,
> >>>>   3 - V4L2_FOCUS_AUTO_CONTINUOUS.
> >>>
> >>> I would put the macro mode to a separate menu since it's configuration for
> >>> how the regular AF works rather than really different mode.
> >>
> >> Yes, makes sense. Most likely there could be also continuous macro auto focus..
> >> I don't have yet an idea what could be a name for that new menu though.
> > 
> > V4L2_CID_FOCUS_AUTO_DISTANCE? It could then have choices FULL or MACRO.
> 
> How about V4L2_CID_FOCUS_AUTO_SCAN_RANGE ? Which would then have choices:
> 	NORMAL,
> 	MACRO,
> 	INFINITY
> ?

What does INFINITY signify? That lens is permanently positioned there?

The name of the control sounds good to me, but I might change the order of
FOCUS and AUTO in it.

> >> Many Samsung devices have also something like guided auto focus, where the
> >> application can specify location in the frame for focusing on. IIRC this could
> >> be also single-shot or continuous. So it could make sense to group MACRO and
> >> "guided" auto focus in one menu, what do you think ?
> > 
> > I think it could be a separate menu. It's not connected to the distance
> 
> OK, let me summarize
> 
> * controls for starting/stopping auto focusing (V4L2_CID_FOCUS_AUTO == false)
> 
>   V4L2_CID_START_AUTO_FOCUS (button) - start auto focusing,
>   V4L2_CID_STOP_AUTO_FOCUS  (button) - stop auto focusing (might be also
>                                        useful in V4L2_FOCUS_AUTO == true),
> * auto focus status
> 
>   V4L2_CID_AUTO_FOCUS_STATUS (menu, read-only) - whether focusing is in
>                                                  progress or not,
>   possible entries:
> 
>   - V4L2_AUTO_FOCUS_STATUS_IDLE,    // auto focusing not enabled or force stopped
>   - V4L2_AUTO_FOCUS_STATUS_BUSY,    // focusing in progress
>   - V4L2_AUTO_FOCUS_STATUS_SUCCESS, // single-shot auto focusing succeed
>                                     // or continuous AF in progress
>   - V4L2_AUTO_FOCUS_STATUS_FAIL,    // auto focusing failed
> 
> 
> * V4L2_CID_FOCUS_AUTO would retain its current semantics:
> 
>   V4L2_CID_FOCUS_AUTO (boolean) - selects auto/manual focus
>       false - manual
>       true  - auto continuous
> 
> * AF algorithm scan range, V4L2_CID_FOCUS_AUTO_SCAN_RANGE with choices:
> 
>   - V4L2_AUTO_FOCUS_SCAN_RANGE_NORMAL,
>   - V4L2_AUTO_FOCUS_SCAN_RANGE_MACRO,
>   - V4L2_AUTO_FOCUS_SCAN_RANGE_INFINITY
> 
> 
> New menu control to choose behaviour of auto focus (either single-shot
> or continuous):
> 
> * select auto focus mode
> 
> V4L2_CID_AUTO_FOCUS_MODE
>         V4L2_AUTO_FOCUS_MODE_NORMAL     - "normal" auto focus (whole frame?)
>         V4L2_AUTO_FOCUS_MODE_SPOT       - spot location passed with other
>                                           controls or selection API
>         V4L2_AUTO_FOCUS_MODE_RECTANGLE  - rectangle passed with other
>                                           controls or selection API
> 
> 
> > parameter. We also need to discuss how the af statistics window
> > configuration is done. I'm not certain there could even be a standardised
> 
> Do we need multiple windows for AF statistics ?
> 
> If not, I'm inclined to use four separate controls for window configuration.
> (X, Y, WIDTH, HEIGHT). This was Hans' preference in previous discussions [1].
> 
> > interface which could be used to control it all.
> 
> 
> [1] http://www.mail-archive.com/linux-media@vger.kernel.org/msg25647.html
> 
> -- 
> Regards,
> Sylwester
  

Hi Sylwester,

On Mon, Jan 02, 2012 at 09:55:31PM +0100, Sylwester Nawrocki wrote:
> Hi Laurent,
> 
> On 01/02/2012 12:16 PM, Laurent Pinchart wrote:
> >> * controls for starting/stopping auto focusing (V4L2_CID_FOCUS_AUTO ==
> >> false)
> >>
> >>   V4L2_CID_START_AUTO_FOCUS (button) - start auto focusing,
> >>   V4L2_CID_STOP_AUTO_FOCUS  (button) - stop auto focusing (might be also
> >>                                        useful in V4L2_FOCUS_AUTO == true),
> > 
> > Maybe V4L2_CID_AUTO_FOCUS_START and V4L2_CID_AUTO_FOCUS_STOP to be consistent 
> > with the other proposed controls ?
> 
> Yes, you're right, I'll change them to make consistent with others.
> I've noticed that too, but a little bit too late:)
> 
> >> * auto focus status
> >>
> >>   V4L2_CID_AUTO_FOCUS_STATUS (menu, read-only) - whether focusing is in
> >>                                                  progress or not,
> >>   possible entries:
> >>
> >>   - V4L2_AUTO_FOCUS_STATUS_IDLE,    // auto focusing not enabled or force
> >>                                        stopped 
> >>   - V4L2_AUTO_FOCUS_STATUS_BUSY,    // focusing in progress
> >>   - V4L2_AUTO_FOCUS_STATUS_SUCCESS, // single-shot auto focusing succeed
> >>                                     // or continuous AF in progress
> >>   - V4L2_AUTO_FOCUS_STATUS_FAIL,    // auto focusing failed
> >>
> >>
> >> * V4L2_CID_FOCUS_AUTO would retain its current semantics:
> >>
> >>   V4L2_CID_FOCUS_AUTO (boolean) - selects auto/manual focus
> >>       false - manual
> >>       true  - auto continuous
> >>
> >> * AF algorithm scan range, V4L2_CID_FOCUS_AUTO_SCAN_RANGE with choices:
> >>
> >>   - V4L2_AUTO_FOCUS_SCAN_RANGE_NORMAL,
> >>   - V4L2_AUTO_FOCUS_SCAN_RANGE_MACRO,
> >>   - V4L2_AUTO_FOCUS_SCAN_RANGE_INFINITY
> >>
> ...
> >>
> >> * select auto focus mode
> >>
> >> V4L2_CID_AUTO_FOCUS_MODE
> >>         V4L2_AUTO_FOCUS_MODE_NORMAL     - "normal" auto focus (whole frame?)
> >>         V4L2_AUTO_FOCUS_MODE_SPOT       - spot location passed with other
> >>         controls or selection API
> >>         V4L2_AUTO_FOCUS_MODE_RECTANGLE  - rectangle passed with other
> >>         controls or selection API
> > 
> > Soudns good to me.
> >
> >>> parameter. We also need to discuss how the af statistics window
> >>> configuration is done. I'm not certain there could even be a standardised
> >>
> >> Do we need multiple windows for AF statistics ?
> >>
> >> If not, I'm inclined to use four separate controls for window
> >> configuration. (X, Y, WIDTH, HEIGHT). This was Hans' preference in
> >> previous discussions [1].
> > 
> > For the OMAP3 ISP we need multiple statistics windows. AEWB can use more than 
> > 32 windows. Having separate controls for that wouldn't be practical.
> 
> OK, so the control API in current form doesn't seem capable of setting up the
> statistics windows. There is also little space in struct v4l2_ext_control for
> any major extensions.
> 
> We might need to define dedicated set of selection targets in the selection
> API for handling multiple windows.
> 
> Yet, to avoid forcing applications to use the selection API where rectangles
> aren't needed - only single spot coordinates, how about defining following
> two controls ?
> 
> * AF spot coordinates when focus mode is set to V4L2_AUTO_FOCUS_MODE_SPOT
> 
>  - V4L2_CID_AUTO_FOCUS_POSITION_X - horizontal position in pixels relative
>                                     to the left of frame
>  - V4L2_CID_AUTO_FOCUS_POSITION_Y - vertical position in pixels relative
>                                     to the top of frame

What's a spot focus mode?

Any AF statistics from a single pixel have no meaning, so there has to be
more. It sounds like a small rectangle to me. :-)

But being able to provide more windows would be rather important. You don't
need to look at too special cameras before you can have seven or so focus
windows.

The selection API could be used for this, as proposed already. We could have
a new V4L2_(SUBDEV_)SELECTION_STAT_AF target to configure windows. We could
add an id field to define the window ID to struct v4l2_(subdev_)selection.
One control would be required to tell how many statistics windows do you
have.

Cheers,
  

Hi Laurent,

On 01/03/2012 02:55 PM, Laurent Pinchart wrote:
>>>>> parameter. We also need to discuss how the af statistics window
>>>>> configuration is done. I'm not certain there could even be a
>>>>> standardised
>>>>
>>>> Do we need multiple windows for AF statistics ?
>>>>
>>>> If not, I'm inclined to use four separate controls for window
>>>> configuration. (X, Y, WIDTH, HEIGHT). This was Hans' preference in
>>>> previous discussions [1].
>>>
>>> For the OMAP3 ISP we need multiple statistics windows. AEWB can use more
>>> than 32 windows. Having separate controls for that wouldn't be
>>> practical.
>>
>> OK, so the control API in current form doesn't seem capable of setting up
>> the statistics windows. There is also little space in struct
>> v4l2_ext_control for any major extensions.
>>
>> We might need to define dedicated set of selection targets in the selection
>> API for handling multiple windows.
>>
>> Yet, to avoid forcing applications to use the selection API where
>> rectangles aren't needed - only single spot coordinates, how about
>> defining following two controls ?
>>
>> * AF spot coordinates when focus mode is set to V4L2_AUTO_FOCUS_MODE_SPOT
>>
>>  - V4L2_CID_AUTO_FOCUS_POSITION_X - horizontal position in pixels relative
>>                                     to the left of frame
>>  - V4L2_CID_AUTO_FOCUS_POSITION_Y - vertical position in pixels relative
>>                                     to the top of frame
> 
> What about a point control type instead ? :-) X and Y coordinates could be 
> stored on 32 bits each.

That's more appealing than two separate controls :-) If Hans agrees to
add a point control type (fingers crossed :)) I could prepare relevant patch
to see how it looks like. I've analysed roughly what would need to be changed,
the effort is quite significant but not so invasive for drivers.

I thought about using new V4L2_CTRL_FLAG* for VIDIOC_QUERYCTRL to indicate
which field of the point data structure is queried.

The only real problem seem to be events, I can't see simple method for adding
two sets of min/max/step/def values to the control event payload. There would
probably have to be two separate control change events for each point structure
field.
  

Hi Sakari,

On 01/04/2012 02:22 PM, Sakari Ailus wrote:
>>>>>> The V4L2_CID_FOCUS_AUTO menu control has currently following items:
>>>>>>   0 - V4L2_FOCUS_MANUAL,
>>>>>>   1 - V4L2_FOCUS_AUTO,
>>>>>>   2 - V4L2_FOCUS_AUTO_MACRO,
>>>>>>   3 - V4L2_FOCUS_AUTO_CONTINUOUS.
>>>>>
>>>>> I would put the macro mode to a separate menu since it's configuration for
>>>>> how the regular AF works rather than really different mode.
>>>>
>>>> Yes, makes sense. Most likely there could be also continuous macro auto focus..
>>>> I don't have yet an idea what could be a name for that new menu though.
>>>
>>> V4L2_CID_FOCUS_AUTO_DISTANCE? It could then have choices FULL or MACRO.
>>
>> How about V4L2_CID_FOCUS_AUTO_SCAN_RANGE ? Which would then have choices:
>> 	NORMAL,
>> 	MACRO,
>> 	INFINITY
>> ?
> 
> What does INFINITY signify? That lens is permanently positioned there?

Yes, I think that's just what it is supposed to mean, but I would have to double
check in the specs.

HeungJun, would you have some more details about the INFINITY setting ?

> The name of the control sounds good to me, but I might change the order of
> FOCUS and AUTO in it.

Ok, I'll fix it.

--

Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Sakari,

On 01/04/2012 03:04 PM, Sakari Ailus wrote:
>> On 01/02/2012 12:16 PM, Laurent Pinchart wrote:
>>>> * controls for starting/stopping auto focusing (V4L2_CID_FOCUS_AUTO ==
>>>> false)
>>>>
>>>>    V4L2_CID_START_AUTO_FOCUS (button) - start auto focusing,
>>>>    V4L2_CID_STOP_AUTO_FOCUS  (button) - stop auto focusing (might be also
>>>>                                         useful in V4L2_FOCUS_AUTO == true),
>>>
>>> Maybe V4L2_CID_AUTO_FOCUS_START and V4L2_CID_AUTO_FOCUS_STOP to be consistent
>>> with the other proposed controls ?
>>
>> Yes, you're right, I'll change them to make consistent with others.
>> I've noticed that too, but a little bit too late:)
>>
>>>> * auto focus status
>>>>
>>>>    V4L2_CID_AUTO_FOCUS_STATUS (menu, read-only) - whether focusing is in
>>>>                                                   progress or not,
>>>>    possible entries:
>>>>
>>>>    - V4L2_AUTO_FOCUS_STATUS_IDLE,    // auto focusing not enabled or force
>>>>                                         stopped
>>>>    - V4L2_AUTO_FOCUS_STATUS_BUSY,    // focusing in progress
>>>>    - V4L2_AUTO_FOCUS_STATUS_SUCCESS, // single-shot auto focusing succeed
>>>>                                      // or continuous AF in progress
>>>>    - V4L2_AUTO_FOCUS_STATUS_FAIL,    // auto focusing failed
>>>>
>>>>
>>>> * V4L2_CID_FOCUS_AUTO would retain its current semantics:
>>>>
>>>>    V4L2_CID_FOCUS_AUTO (boolean) - selects auto/manual focus
>>>>        false - manual
>>>>        true  - auto continuous
>>>>
>>>> * AF algorithm scan range, V4L2_CID_FOCUS_AUTO_SCAN_RANGE with choices:
>>>>
>>>>    - V4L2_AUTO_FOCUS_SCAN_RANGE_NORMAL,
>>>>    - V4L2_AUTO_FOCUS_SCAN_RANGE_MACRO,
>>>>    - V4L2_AUTO_FOCUS_SCAN_RANGE_INFINITY
>>>>
>> ...
>>>>
>>>> * select auto focus mode
>>>>
>>>> V4L2_CID_AUTO_FOCUS_MODE
>>>>          V4L2_AUTO_FOCUS_MODE_NORMAL     - "normal" auto focus (whole frame?)
>>>>          V4L2_AUTO_FOCUS_MODE_SPOT       - spot location passed with other
>>>>          controls or selection API
>>>>          V4L2_AUTO_FOCUS_MODE_RECTANGLE  - rectangle passed with other
>>>>          controls or selection API
>>>
>>> Soudns good to me.
>>>
>>>>> parameter. We also need to discuss how the af statistics window
>>>>> configuration is done. I'm not certain there could even be a standardised
>>>>
>>>> Do we need multiple windows for AF statistics ?
>>>>
>>>> If not, I'm inclined to use four separate controls for window
>>>> configuration. (X, Y, WIDTH, HEIGHT). This was Hans' preference in
>>>> previous discussions [1].
>>>
>>> For the OMAP3 ISP we need multiple statistics windows. AEWB can use more than
>>> 32 windows. Having separate controls for that wouldn't be practical.
>>
>> OK, so the control API in current form doesn't seem capable of setting up the
>> statistics windows. There is also little space in struct v4l2_ext_control for
>> any major extensions.
>>
>> We might need to define dedicated set of selection targets in the selection
>> API for handling multiple windows.
>>
>> Yet, to avoid forcing applications to use the selection API where rectangles
>> aren't needed - only single spot coordinates, how about defining following
>> two controls ?
>>
>> * AF spot coordinates when focus mode is set to V4L2_AUTO_FOCUS_MODE_SPOT
>>
>>   - V4L2_CID_AUTO_FOCUS_POSITION_X - horizontal position in pixels relative
>>                                      to the left of frame
>>   - V4L2_CID_AUTO_FOCUS_POSITION_Y - vertical position in pixels relative
>>                                      to the top of frame
> 
> What's a spot focus mode?
> 
> Any AF statistics from a single pixel have no meaning, so there has to be
> more. It sounds like a small rectangle to me. :-)

Yes, it must be a small rectangle. But there may be no way to configure or 
read this rectangle's parameters, hence this menu option. If some hardware 
doesn't implement such feature it can just skip the spot focus menu item.

And in case camera doesn't provide any interface for AF statistics windows 
configuration I don't see a good reason to use the selection API. 

> But being able to provide more windows would be rather important. You don't
> need to look at too special cameras before you can have seven or so focus
> windows.
> 
> The selection API could be used for this, as proposed already. We could have
> a new V4L2_(SUBDEV_)SELECTION_STAT_AF target to configure windows. We could
> add an id field to define the window ID to struct v4l2_(subdev_)selection.
> One control would be required to tell how many statistics windows do you
> have.

Yeah, sounds good to me, better than defining selection target base and
the target pool size. :) Ack.

--
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html