V4L: Add camera white balance preset control

-------- Original Message --------
Subject: Re: [PATCH 04/15] V4L: Add camera white balance preset control
Date: Fri, 20 Apr 2012 17:22:08 +0200
From: Hans de Goede<[email protected]>
To: Sylwester Nawrocki<[email protected]>

Hi,

Quoted 44 lines · Sylwester Nawrocki

Quoted 32 lines · Hans de Goede

Quoted 12 lines · Sylwester Nawrocki

Add V4L2_CID_WHITE_BALANCE_PRESET control for camera white balance presets. The following items are defined:

- V4L2_WHITE_BALANCE_NONE,
- V4L2_WHITE_BALANCE_INCANDESCENT,
- V4L2_WHITE_BALANCE_FLUORESCENT,
- V4L2_WHITE_BALANCE_HORIZON,
- V4L2_WHITE_BALANCE_DAYLIGHT,
- V4L2_WHITE_BALANCE_FLASH,
- V4L2_WHITE_BALANCE_CLOUDY,
- V4L2_WHITE_BALANCE_SHADE,

Quoted 4 lines

This is a manual white balance control, in addition to V4L2_CID_RED_BALANCE, V4L2_CID_BLUE_BALANCE and V4L2_CID_WHITE_BALANCE_TEMPERATURE. It's useful for camera devices running more complex firmware and exposing white balance preset selection in their user register interface.

Hmm, how is this supposed to work together with the v4l2-ctrls framework? The framework has a concept of a master control, which has a manual value, and the slave controls will only get unlocked (V4L2_CTRL_FLAG_INACTIVE will be cleared) when that master control is set at its manual value, now lets say that we've V4L2_CID_AUTO_WHITE_BALANCE as master with V4L2_CID_WHITE_BALANCE_PRESET and V4L2_CID_RED_BALANCE and V4L2_CID_BLUE_BALANCE slaves. Then when the master control changes from auto to manual all 3 will have their inactive flag cleared, but if the preset value != V4L2_WHITE_BALANCE_NONE
then the red- and blue-balance should have kept their inactive flag. And since this
clearing of the inactive flag is done after v4l2-ctrls.c has called into the driver there is no way for the driver to fix this.

Quoted 4 lines

One could work-around this by not specifying the whitebalance control cluster as being an auto cluster, and doing all the auto cluster stuff from the driver, but that significantly complicates the driver, and we are trying to get away of every driver doing this kinda stuff for itself...

I assumed in common cases there will be just pairs of auto/manual controls (or control groups) used, e.g. V4L2_CID_AUTO_WHITE_BALANCE and V4L2_CID_RED_BALANCE/V4L2_CID_BLUE_BALANCE, or V4L2_CID_AUTO_WHITE_BALANCE and V4L2_CID_WHITE_BALANCE_TEMPERATURE or V4L2_CID_AUTO_WHITE_BALANCE and V4L2_CID_WHITE_BALANCE_PRESET, etc.

Quoted 2 lines

This is really what the control framework support now, i.e. one master cluster control with single value that swithes whole cluster into manual mode, AFAICS.

Right, so my problem is that at least the pwc driver does not work that way.

Quoted 14 lines

I assumed more complicated cases will have to be coded in drivers for now. Or can be handled my the control framework, if those patterns appear to be often used and the control framework is enhanced to better support them.

I tried to come up with a solution that could be also used by the pwc driver and to expose all functionality available there.

Quoted 6 lines

I still believe that the solution I came up with for pwc is better, the auto whitebalance control really is a tri state when you add presets whitebalance is controlled through one of this 3 options:
1) auto
2) preset
3) manual

Quoted 7 lines

2) and 3) will often be exclusive, so we cannot just unlock (clear the V4L2_CTRL_FLAG_INACTIVE flag) on them, when the WB menu is switched to value other than "auto". This would happen when the WB menu and 2), 3) would form an auto cluster.

Moreover, we also have V4L2_CID_WHITE_BALANCE_TEMPERATURE as a manual WB control. I assumed the WB presets are just form of it, with names (labels).

Another way of looking at the preset is as a special form of auto WB, it really is auto WB with a hint from the user about the lighting conditions, and the auto wb algorithm just happens to choose fixed values for temperature or red/green balance
when it gets that user input, this is how the pwc driver currently handles it and this works well, currently the pwc driver has a menu with:

manual
incandescent
fluorescent
daylight
auto

This matches well with the control framework, because the red/blue balance (or for other cameras the temperature) get unlocked when manual, and locked in all other cases, which is exactly what we want. Also it avoids the user confusion where the user chooses manual for awb, and then still cannot control wb, because he must also move the preset to none, that is just bad UI design IMHO.

To be clear where I set tri state before, I really meant more then 2 states, so:

manual
preset 1
preset 2
preset 3
...
preset #
auto

menu

integer menu

This is really what the user can choose to dividing this into"

auto
manual

preset none
preset 1
preset 2
preset 3
...
preset #

Will just confuse the user, the user selects manual and then he still cannot control the wb (imagine confused look on users face) ...

Quoted 3 lines

I am a bit concerned that with the menu control we might exclude at API level a possibility of altering some of the controls simultaneously, e.g. preset and red/blue balance.

But that makes no sense, with a preset we are choosing fixed red/blue balance settings, so red/blue balance (or the temperature) should be locked when a preset is active.

Quoted 17 lines

Quoted 3 lines

I know you are worried about having a V4L2_CID_AUTO_WHITE_BALANCE control which is not a boolean breaking apps, well the pwc driver is a quite popular driver and I've had 0 bug reports about my turning it into a menu...

I wish I could simply change the control type to a menu, but breaking ABI doesn't sound like an option. I think the patch doing something like the at the core level would have been rejected right away. So the new menu control sounds better to me, except that it doesn't make our situation much easier, however it might be what we wanted in long term.

Quoted 6 lines

Alternatively we could add a new V4L2_CID_WHITE_BALANCE_MENU control and use that for drivers which have presets rather then the old V4L2_CID_AUTO_WHITE_BALANCE
to ensure that userspace won't trip over it being a menu all of a sudden, but I believe that it is really important to properly reflect the tri-state nature of the awb once presets come into play, rather then trying to bolt something

Quoted 3 lines

Is it really tri-state ? I think it just an auto (perhaps multiple auto at some point?) and multiple manual states. Should we really consider the presets as something other than manual control ?

As said above it is a more then 2 states situation, as for the preset being something other then manual, yes, because with the preset active the manual controls are locked to the preset selected value(s).

Quoted 6 lines

Quoted 3 lines

on top of / to the side of our current bi-state control. And I hope that my above example of what sort of troubles using the bolt-on solution will give, clearly demonstrates that the bolt-on solution is a bad idea.

If we decided to add a new V4L2_CID_WHITE_BALANCE_MENU control, then we should probably deprecate V4L2_CID_AUTO_WHITE_BALANCE, as having both would be confusing.

Agreed.

Quoted 2 lines

Then the V4L2_CID_AUTO_WHITE_BALANCE would have been removed and maybe after some time added as an alias for V4L2_CID_WHITE_BALANCE_MENU.

I would not do that just document it as deprecated and keep it around so that apps hardcoding control ids keep compiling.

Quoted 2 lines

I'm also not very happy with appending a _MENU suffix, indicating the control's type.

True, but I'm sure if we can agree on the menu approach we can then come up with a better name maybe:

V4L2_CID_AUTO_N_PRESET_WHITE_BALANCE

Hi,

So I'm going to rework patch 04/15, to remove V4L2_WHITE_BALANCE_PRESET_NONE and add AUTO, MANUAL entries. Only the CID name is still an open issue.

Some examples to start with:

#define V4L2_CID_WHITE_BALANCE_AUTO (V4L2_CID_CAMERA_CLASS_BASE+XX) enum v4l2_white_balance_auto {
V4L2_WHITE_BALANCE_AUTO, V4L2_WHITE_BALANCE_MANUAL, V4L2_WHITE_BALANCE_INCANDESCENT, V4L2_WHITE_BALANCE_FLUORESCENT, V4L2_WHITE_BALANCE_FLUORESCENT_H, V4L2_WHITE_BALANCE_HORIZON, V4L2_WHITE_BALANCE_DAYLIGHT, V4L2_WHITE_BALANCE_FLASH, V4L2_WHITE_BALANCE_CLOUDY, V4L2_WHITE_BALANCE_SHADE,
};

or

#define V4L2_CID_AUTO_N_PRESET_WHITE_BALANCE (V4L2_CID_CAMERA_CLASS_BASE+XX) enum v4l2_auto_n_preset_white_balance {
V4L2_WHITE_BALANCE_AUTO,
V4L2_WHITE_BALANCE_MANUAL, V4L2_WHITE_BALANCE_INCANDESCENT, V4L2_WHITE_BALANCE_FLUORESCENT, V4L2_WHITE_BALANCE_HORIZON,
V4L2_WHITE_BALANCE_DAYLIGHT,
V4L2_WHITE_BALANCE_FLASH,
V4L2_WHITE_BALANCE_CLOUDY,
V4L2_WHITE_BALANCE_SHADE,
};

enum v4l2_auto_n_preset_white_balance {
	V4L2_AUTO_N_PRESET_WHITE_BALANCE_AUTO,
	V4L2_AUTO_N_PRESET_WHITE_BALANCE_MANUAL,
	V4L2_AUTO_N_PRESET_WHITE_BALANCE_INCANDESCENT,
	...

would be unfortunately a bit cumbersome.