Hi Sakari,
On 12/30/2011 12:34 AM, Sakari Ailus wrote:
> On Wed, Dec 28, 2011 at 02:51:38PM +0100, Laurent Pinchart wrote:
>> On Wednesday 28 December 2011 14:35:00 Sylwester Nawrocki wrote:
>>> On 12/28/2011 07:23 AM, HeungJun, Kim wrote:
>>>> It adds the new CID for setting White Balance Preset. This CID is
>>>> provided as menu type using the following items:
>>>> 0 - V4L2_WHITE_BALANCE_INCANDESCENT,
>>>> 1 - V4L2_WHITE_BALANCE_FLUORESCENT,
>>>> 2 - V4L2_WHITE_BALANCE_DAYLIGHT,
>>>> 3 - V4L2_WHITE_BALANCE_CLOUDY,
>>>> 4 - V4L2_WHITE_BALANCE_SHADE,
>>>
>>> I have been also investigating those white balance presets recently and
>>> noticed they're also needed for the pwc driver. Looking at
>>> drivers/media/video/pwc/pwc-v4l2.c there is something like:
>>>
>>> const char * const pwc_auto_whitebal_qmenu[] = {
>>> "Indoor (Incandescant Lighting) Mode",
>>> "Outdoor (Sunlight) Mode",
>>> "Indoor (Fluorescent Lighting) Mode",
>>> "Manual Mode",
>>> "Auto Mode",
>>> NULL
>>> };
>>>
>>> static const struct v4l2_ctrl_config pwc_auto_white_balance_cfg = {
>>> .ops = &pwc_ctrl_ops,
>>> .id = V4L2_CID_AUTO_WHITE_BALANCE,
>>> .type = V4L2_CTRL_TYPE_MENU,
>>> .max = awb_auto,
>>> .qmenu = pwc_auto_whitebal_qmenu,
>>> };
>>>
>>> ...
>>>
>>> cfg = pwc_auto_white_balance_cfg;
>>> cfg.name = v4l2_ctrl_get_name(cfg.id);
>>> cfg.def = def;
>>> pdev->auto_white_balance = v4l2_ctrl_new_custom(hdl, &cfg, NULL);
>>>
>>> So this driver re-defines V4L2_CID_AUTO_WHITE_BALANCE as a menu control
>>> with custom entries. That's interesting... However it works in practice
>>> and applications have access to what's provided by hardware.
>>> Perhaps V4L2_CID_AUTO_WHITE_BALANCE_TEMPERATURE would be a better fit for
>>> that :)
>>>
>>> Nevertheless, redefining standard controls in particular drivers sounds
>>> a little dubious. I wonder if this is a generally agreed approach ?
>>
>> No agreed with me at least :-)
>>
>>> Then, how does your V4L2_CID_PRESET_WHITE_BALANCE control interact with
>>> V4L2_CID_AUTO_WHITE_BALANCE control ? Does V4L2_CID_AUTO_WHITE_BALANCE need
>>> to be set to false for V4L2_CID_PRESET_WHITE_BALANCE to be effective ?
>>
>> Is the preset a fixed white balance setting, or is it an auto white balance
>> with the algorithm tuned for a particular configuration ? In the first case,
>> does it correspond to a fixed white balance temperature value ?
>
> While I'm waiting for a final answer to this, I guess it's the second. There
> are three things involved here:
>
> - V4L2_CID_WHITE_BALANCE_TEMPERATURE: relatively low level control telling
> the colour temperature of the light source. Setting a value for this
> essentially means using manual white balance.
>
> - V4L2_CID_AUTO_WHITE_BALANCE: automatic white balance enabled or disabled.
Was the third thing the V4L2_CID_DO_WHITE_BALANCE control that you wanted to
say ? It's also quite essential functionality, to be able to fix white balance
after pointing camera to a white object. And I would expect
V4L2_CID_WHITE_BALANCE_PRESET control's documentation to state how an
interaction with V4L2_CID_DO_WHITE_BALANCE looks like.
> The new control proposed by HeungJun is input for the automatic white
> balance algorithm unless I'm mistaken. Whether or not the value is static,
> however, might be considered of secondary importance: it is a name instead
> of a number and clearly intended to be used as a high level control. I'd
> still expect it to be a hint for the algorithm.
>
> The value of the new control would have an effect as long as automatic white
> balance is enabled.
The idea to treat the preset as a hint to the algorithm is interesting, however
as it turns out this are just static values (R/B balance) in manual WB mode.
I expect some parameters for adjusting auto WB algorithm (WB (R/G/B) gain bias
or something similar) to be present in sensor's ISP as well. If I remember well
I've seen something like this in one of sensor's documentations.
>>>> diff --git a/Documentation/DocBook/media/v4l/controls.xml
>>>> b/Documentation/DocBook/media/v4l/controls.xml index c0422c6..350c138
>>>> 100644
>>>> --- a/Documentation/DocBook/media/v4l/controls.xml
>>>> +++ b/Documentation/DocBook/media/v4l/controls.xml
>>>> @@ -2841,6 +2841,44 @@ it one step further. This is a write-only
>>>> control.</entry>
>>>>
>>>> </row>
>>>> <row><entry></entry></row>
>>>>
>>>> + <row id="v4l2-preset-white-balance">
>>>> + <entry
>>>> spanname="id"><constant>V4L2_CID_PRESET_WHITE_BALANCE</constant> </
>>>> entry>
>>>
>>> Wouldn't V4L2_CID_WHITE_BALANCE_PRESET be better ?
>>
>> That's what I was about to say.
>
> And the menu items would contain the same prefix with CID_ removed. They're
> going to be long, but I don't see that as an issue for menu items.
Should we call it V4L2_CID_WB_PRESET then ?
Anyway V4L2_WHITE_BALANCE_PRESET_INCADESCENT for example is not that long,
we have control names that almost reach 80 characters :)
--
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
