Hi Sylwester, On Fri, Dec 30, 2011 at 11:14:39AM +0100, Sylwester Nawrocki wrote: > 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. I expected the new control to be the third thing as configuration for the awb algorithm, which it turned out not to be. I don't quite understand the purpose of the do_white_balance; the automatic white balance algorithm is operational until it's disabled, and after disabling it the white balance shouldn't change. What is the extra functionality that the do_white_balance control implements? If we agree white_balance_preset works at the same level as white_balance_temerature control, this becomes more simple. I guess no driver should implement both. > > 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. Agreed, if there's a device doing this we will add another control at that time. > 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. Sounds reasonable. > > >>>> 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 :) I'd prefer the long one but I have no strong opinion either way. -- Sakari Ailus e-mail: sakari.ailus@xxxxxx jabber/XMPP/Gmail: sailus@xxxxxxxxxxxxxx -- 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