Re: [RFC/PATCH 2/5] uvc: Adapt the driver to new type of V4L2_CID_FOCUS_AUTO control

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

Hi Laurent,

On 12/06/2011 01:26 PM, Laurent Pinchart wrote:
> On Sunday 04 December 2011 16:16:13 Sylwester Nawrocki wrote:
>> From: Heungjun Kim <riverful.kim@samsung.com>
>>
>> The V4L2_CID_FOCUS_AUTO control has been converted from boolean type,
>> where control's value 0 and 1 were corresponding to manual and automatic
>> focus respectively, to menu type with following menu items:
>>   0 - V4L2_FOCUS_MANUAL,
>>   1 - V4L2_FOCUS_AUTO,
>>   2 - V4L2_FOCUS_AUTO_MACRO,
>>   3 - V4L2_FOCUS_AUTO_CONTINUOUS.
>>
>> According to this change the uvc control mappings are modified to retain
>> original sematics, where 0 corresponds to manual and 1 to auto focus.
> 
> UVC auto-focus works in continuous mode, not single-shot mode. As the existing

Hmm, I suspected that.

> V4L2_CID_FOCUS_AUTO uses 0 to mean manual focus and 1 to mean continuous auto-
> focus, shouldn't this patch set define the following values instead ?
> 
>    0 - V4L2_FOCUS_MANUAL
>    1 - V4L2_FOCUS_AUTO
>    2 - V4L2_FOCUS_AUTO_MACRO
>    3 - V4L2_FOCUS_AUTO_SINGLE_SHOT

It's not that bad, at least we would not have changed the existing ABI.

It depends how other focus modes are defined, e.g. V4L2_FOCUS_AUTO_MACRO.
There is also an auto focus driven by face detection module output.

The question would be whether we want to append _SINGLE_SHOT or _CONTINUOUS
to the names. And if most of the focus modes are single-shot we will
probably need a common "do auto-focus" control for them.

What do you think about such assignment:

   0 - V4L2_FOCUS_MANUAL,
   1 - V4L2_FOCUS_AUTO_CONTINUOUS,
   2 - V4L2_FOCUS_AUTO,
   3 - V4L2_FOCUS_AUTO_MACRO,

?

I'm not 100% sure right now, but the macro and "face detection"
are rather "single-shot". Single-shot focus might be more common. Perhaps
someone else can put more light on that :-)

Using a "single-shot notation" we might have something like:

   0 - V4L2_FOCUS_MANUAL,
   1 - V4L2_FOCUS_AUTO,
   2 - V4L2_FOCUS_AUTO_SINGLE_SHOT,
   3 - V4L2_FOCUS_AUTO_MACRO_SINGLE_SHOT,
   3 - V4L2_FOCUS_AUTO_FACE_DETECTION_SINGLE_SHOT,

I'm not sure if this convention is the best one.

Alternatively we could define a "do-focus" control for each mode,
but then these controls have to be properly coordinated, for exclusive
operation.

> 
> V4L2_FOCUS_AUTO_SINGLE_SHOT could also be named V4L2_FOCUS_SINGLE_SHOT.
> 
>> Signed-off-by: Heungjun Kim <riverful.kim@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>
>> ---
>> The V4L2_CID_FOCUS_AUTO control in V4L2_FOCUS_AUTO mode does only
>> a one-shot auto focus, when switched from V4L2_FOCUS_MANUAL.
>> It might be worth to implement also the V4L2_CID_DO_AUTO_FOCUS button
>> control in uvc, however I didn't take time yet to better understand
>> the driver and add this. I also don't have any uvc hardware to test
>> this patch so it's just compile tested.
>> ---
>>  drivers/media/video/uvc/uvc_ctrl.c |    9 ++++++++-
>>  1 files changed, 8 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/media/video/uvc/uvc_ctrl.c
>> b/drivers/media/video/uvc/uvc_ctrl.c index 254d326..6860ca1 100644
>> --- a/drivers/media/video/uvc/uvc_ctrl.c
>> +++ b/drivers/media/video/uvc/uvc_ctrl.c
>> @@ -365,6 +365,11 @@ static struct uvc_menu_info exposure_auto_controls[] =
>> { { 8, "Aperture Priority Mode" },
>>  };
>>
>> +static struct uvc_menu_info focus_auto_controls[] = {
>> +	{ 0, "Manual Mode" },
>> +	{ 1, "Auto Mode" },

Do you think it could be better to change this to:

+	{ 0, "Manual Focus" },
+	{ 1, "Continuous Auto Focus" },

?

--

Regards,
Sylwester