Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[Sylwester Nawrocki]

On 01/11/2012 11:36 PM, Sakari Ailus wrote:
>>> This is what the spec says:
>>>
>>> "This is an action control. When set (the value is ignored), the device 
>>> will do
>>> a white balance and then hold the current setting. Contrast this with the
>>> boolean V4L2_CID_AUTO_WHITE_BALANCE, which, when activated, keeps adjusting 
>>> the
>>> white balance."
>>>
>>> I wonder if that should be then changed --- or is it just me who got a 
>>> different
>>> idea from the above description?
>>
>> Only you ? :-) Same as Laurent, I understood this control can be used to do 
>> white
>> balance after pointing camera to a white object. Not sure if the description
>> needs to be changed.
> 
> Definitely it needs to be changed. Who will submit the patch? :-)

If it really bugs you, feel free to send a patch :) Or I'll do it, but only 
after
I handle all other pending controls from my to-do list.

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

Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[Sakari Ailus]

On Wed, Jan 04, 2012 at 11:06:13PM +0100, Sylwester Nawrocki wrote:
> Hi Sakari,
> 
> On 01/04/2012 10:24 PM, Sakari Ailus wrote:
> > 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?
> 
>  Maybe DO_WHITE_BALANCE was inspired by some hardware's behaviour, I don't
>  know. I have nothing against this control. It allows you to perform
>  one-shot white balance in a given moment in time. Simple and clear.
> >>>
> >>> Well, yes, if you have an automatic white balance algorithm which supports
> >>> "one-shot" mode. Typically it's rather a feedback loop. I guess this means
> >>> "just run one iteration".
> >>>
> >>> Something like this should possibly be used to get the white balance
> >>> correct by pointing the camera to an object of known colour (white
> >>> typically, I think). But this isn't it, at least based on the description
> >>> in the spec.
> >>
> >> Then either the spec is incorrect, or I'm mistaken. My understanding of the
> >> DO_WHITE_BALANCE control is exactly what you described.
> > 
> > This is what the spec says:
> > 
> > "This is an action control. When set (the value is ignored), the device 
> > will do
> > a white balance and then hold the current setting. Contrast this with the
> > boolean V4L2_CID_AUTO_WHITE_BALANCE, which, when activated, keeps adjusting 
> > the
> > white balance."
> > 
> > I wonder if that should be then changed --- or is it just me who got a 
> > different
> > idea from the above description?
> 
> Only you ? :-) Same as Laurent, I understood this control can be used to do 
> white
> balance after pointing camera to a white object. Not sure if the description
> needs to be changed.

Definitely it needs to be changed. Who will submit the patch? :-)

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi jabber/XMPP/Gmail: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[Sylwester Nawrocki]

Hi Sakari,

On 01/04/2012 10:24 PM, Sakari Ailus wrote:
> 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?

 Maybe DO_WHITE_BALANCE was inspired by some hardware's behaviour, I don't
 know. I have nothing against this control. It allows you to perform
 one-shot white balance in a given moment in time. Simple and clear.
>>>
>>> Well, yes, if you have an automatic white balance algorithm which supports
>>> "one-shot" mode. Typically it's rather a feedback loop. I guess this means
>>> "just run one iteration".
>>>
>>> Something like this should possibly be used to get the white balance
>>> correct by pointing the camera to an object of known colour (white
>>> typically, I think). But this isn't it, at least based on the description
>>> in the spec.
>>
>> Then either the spec is incorrect, or I'm mistaken. My understanding of the
>> DO_WHITE_BALANCE control is exactly what you described.
> 
> This is what the spec says:
> 
> "This is an action control. When set (the value is ignored), the device will 
> do
> a white balance and then hold the current setting. Contrast this with the
> boolean V4L2_CID_AUTO_WHITE_BALANCE, which, when activated, keeps adjusting 
> the
> white balance."
> 
> I wonder if that should be then changed --- or is it just me who got a 
> different
> idea from the above description?

Only you ? :-) Same as Laurent, I understood this control can be used to do 
white
balance after pointing camera to a white object. Not sure if the description
needs to be changed.

> My understanding is that the operation for getting the white balance 
> information
> from a white object is by far simpler than getting the white balance correct
> without that.
> 
> These seem to be only two references to this control in drivers and both 
> drivers
> are grossly misusing it. On one of them the description is "white balance
> background: blue" and on the other it's "night mode".
> 
> That makes me wonder in what kind of circumstances this control was originally
> introduced. Whatever it was, it seems to have taken place before 16th April in
> 2005. :-)
> 
> I think we could change the description to something more suitable or just
> remove this one...

Why remove it ? It's a useful control. And the abuses at the drivers is 
different
story.

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

Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[Sakari Ailus]

Hi Laurent,

Laurent Pinchart wrote:

Hi Sakari,

On Wednesday 04 January 2012 21:39:34 Sakari Ailus wrote:

On Sun, Jan 01, 2012 at 04:38:21PM +0100, Sylwester Nawrocki wrote:

On 12/30/2011 09:41 PM, Sakari Ailus wrote:

On Fri, Dec 30, 2011 at 11:14:39AM +0100, Sylwester Nawrocki wrote:

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?


Maybe DO_WHITE_BALANCE was inspired by some hardware's behaviour, I don't
know. I have nothing against this control. It allows you to perform
one-shot white balance in a given moment in time. Simple and clear.


Well, yes, if you have an automatic white balance algorithm which supports
"one-shot" mode. Typically it's rather a feedback loop. I guess this means
"just run one iteration".

Something like this should possibly be used to get the white balance
correct by pointing the camera to an object of known colour (white
typically, I think). But this isn't it, at least based on the description
in the spec.


Then either the spec is incorrect, or I'm mistaken. My understanding of the
DO_WHITE_BALANCE control is exactly what you described.


This is what the spec says:

"This is an action control. When set (the value is ignored), the device 
will do a white balance and then hold the current setting. Contrast this 
with the boolean V4L2_CID_AUTO_WHITE_BALANCE, which, when activated, 
keeps adjusting the white balance."


I wonder if that should be then changed --- or is it just me who got a 
different idea from the above description?


My understanding is that the operation for getting the white balance 
information from a white object is by far simpler than getting th

Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[Laurent Pinchart]

Hi Sakari,

On Wednesday 04 January 2012 21:39:34 Sakari Ailus wrote:
> On Sun, Jan 01, 2012 at 04:38:21PM +0100, Sylwester Nawrocki wrote:
> > On 12/30/2011 09:41 PM, Sakari Ailus wrote:
> > > On Fri, Dec 30, 2011 at 11:14:39AM +0100, Sylwester Nawrocki wrote:
> > >> 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?
> > 
> > Maybe DO_WHITE_BALANCE was inspired by some hardware's behaviour, I don't
> > know. I have nothing against this control. It allows you to perform
> > one-shot white balance in a given moment in time. Simple and clear.
> 
> Well, yes, if you have an automatic white balance algorithm which supports
> "one-shot" mode. Typically it's rather a feedback loop. I guess this means
> "just run one iteration".
> 
> Something like this should possibly be used to get the white balance
> correct by pointing the

Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[Sakari Ailus]

Hi Sylwester,

On Sun, Jan 01, 2012 at 04:38:21PM +0100, Sylwester Nawrocki wrote:
> On 12/30/2011 09:41 PM, Sakari Ailus wrote:
> > On Fri, Dec 30, 2011 at 11:14:39AM +0100, Sylwester Nawrocki wrote:
> >> 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?
> 
> Maybe DO_WHITE_BALANCE was inspired by some hardware's behaviour, I don't
> know. I have nothing against this control. It allows you to perform one-shot
> white balance in a given moment in time. Simple and clear.

Well, yes, if you have an automatic white balance algorithm which supports
"one-shot" mode. Typically it's rather a feedback loop. I guess this means
"just run one iteration".

Something like this should possibly be used to get the white balance correct
by pointing the camera to an object of known colour (white typically, I
think). But this isn't it, at least based on the description in the spec.

> > If we agree white_balance_preset works at the same level as
> > white_balance_temerature control, this becomes more simple. I guess no
> >

Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[Sylwester Nawrocki]

Hi HeungJun,

On 01/02/2012 05:38 AM, Kim, Heungjun wrote:
> On 2011년 12월 30일 19:30, Sylwester Nawrocki wrote:
>> On 12/29/2011 06:08 AM, HeungJun, Kim wrote:
>>> I guess the WBP menu controls of pwc driver is probably defined in the other
>>> headers, for users being well known the PWC hardware. So it should be 
>>> managed
>>> separately to videodev2.h. Is it right? Even if the way might be slightly
>>> different, it can't avoid to be "managed separately".
>>>
>>> It means the users being not well known the specific hardware like PWC,
>>> have difficulty to use that driver well.
>>> And, at least, It doesn't looks generic API for me.
>>> In this case, the unfamiliar user with such unique hardware, can use
>>> whatever he wants to use finally, after finding&  looking around the 
>>> headers.
>> Applications can query drivers for supported controls and populate user 
>> control
>> panels dynamically, based on information from VIDIOC_QUERYCTRL and
>> VIDIOC_QUERYMENU
>> ioctls. Not needing to rely on menu items definition in videodev2.h.
>> I had a feeling you weren't considering such case. :)
> You're right in that meaning. And it might be a good point.
> But, I think these 2 ioctl can not handle about this issue.
> 
> Before using VIDIOC_QUERYCTRL and VIDIOC_QUERYMENU, the user should know which
> CID name
> is used in the videodev2.h, and anyway it can not be avoidable the user find 
> out
> this name in it. :)

Yes, in many cases it is required to know the CID in advance, however it is not
mandatory for all applications.

> At least I've seen nobody makes the application just to open(), queryctrl(),
> querymenu(), and close(),
> only for scanning the specific control is existed or not.
> Until now, I have known these 2 ioctl is generally used for formating the UI
> componets like button, menu, and etc, on the screen.

Yup.

> So, it's safe to say that the user who knows that specific control is also 
> know
> the CID name,
> the user knows such specific controls don't need even VIDIOC_QUERYCTRL and
> VIDIOC_QUERYMENU.

I respectfully disagree. Properly written applications must use 
VIDIOC_QUERYCTRL/
VIDIOC_QUERYMENU ioctls, as many v4l2 controls have now driver-specific value
range.

Please see this application for instance [1], it doesn't hard code any control
IDs in it, it only uses V4L2_CID_BASE, V4L2_CID_PRIVATE_BASE and 
V4L2_CID_LASTP1.

Yet, it can handle any control, as long as it supports the control's type.

> And IMHO, this is not related about pulling out the hidden(?) controls 
> generally
> used in the camera,
> on the videodev2.h. I think it's only generic defined in videodev2.h.
> 
> I really had wondered why the controls I thought very general for camera is in
> hidden(?) the specific driver,
> not in the videodev2.h. It was just start to consider this issues.

I think you misunderstood me, I didn't mean to force anyone to use private
controls for common features. :)

> Regards,
> Heungjun Kim
> 
>> Perhaps it's uncommon in embedded systems though.

[1] http://sourceforge.net/projects/v4l2ucp/files/v4l2ucp/2.0/

-- 

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

Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[Sylwester Nawrocki]

Hi,

On 12/28/2011 02:35 PM, 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:

How about adding

V4L2_WHITE_BALANCE_PRESET_NONE or
V4L2_WHITE_BALANCE_PRESET_UNDEFINED

to this menu ? It might cover "Manual Mode" entry in pwc_auto_whitebal_qmenu.
Also it might be useful not only as a read-only item for applications,
when there are multiple means of setting up white balance supported by a
driver, i.e. blue/red balance, component gains, etc.

>> 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
> };

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

Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[Kim, Heungjun]

On 2011년 12월 30일 19:30, Sylwester Nawrocki wrote:

Hi HeungJun,

On 12/29/2011 06:08 AM, HeungJun, Kim wrote:

-Original Message-
From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
ow...@vger.kernel.org] On Behalf Of Laurent Pinchart
Sent: Wednesday, December 28, 2011 10:52 PM
To: Sylwester Nawrocki
Cc: HeungJun, Kim; linux-media@vger.kernel.org; mche...@redhat.com;
hverk...@xs4all.nl; sakari.ai...@iki.fi; kyungmin.p...@samsung.com; Hans de
Goede
Subject: Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu
control

Hi,

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 :-)

I guess the WBP menu controls of pwc driver is probably defined in the other
headers, for users being well known the PWC hardware. So it should be managed
separately to videodev2.h. Is it right? Even if the way might be slightly
different, it can't avoid to be "managed separately".

It means the users being not well known the specific hardware like PWC,
have difficulty to use that driver well.
And, at least, It doesn't looks generic API for me.
In this case, the unfamiliar user with such unique hardware, can use
whatever he wants to use finally, after finding&  looking around the headers.

Applications can query drivers for supported controls and populate user control
panels dynamically, based on information from VIDIOC_QUERYCTRL and 
VIDIOC_QUERYMENU
ioctls. Not needing to rely on menu items definition in videodev2.h.
I had a feeling you weren't considering such case. :)

You're right in that meaning. And it might be a good point.
But, I think these 2 ioctl can not handle about this issue.

Before using VIDIOC_QUERYCTRL and VIDIOC_QUERYMENU, the user should know 
which CID name
is used in the videodev2.h, and anyway it can not be avoidable the user 
find out this name in it. :)
At least I've seen nobody makes the application just to open(), 
queryctrl(), querymenu(), and close(),

only for scanning the specific control is existed or not.
Until now, I have known these 2 ioctl is generally used for formating 
the UI componets like button,

menu, and etc, on the screen.
So, it's safe to say that the user who knows that specific control is 
also know the CID name,
the user knows such specific controls don't need even VIDIOC_QUERYCTRL 
and VIDIOC_QUERYMENU.


And IMHO, this is not related about pulling out the hidden(?) controls 
generally used in the camera,

on the videodev2.h. I think it's only generic defined in videodev2.h.

I really had wondered why the controls I thought very general for camera 
is in hidden(?) the specific driver,

not in the videodev2.h. It was just start to consider this issues.

Regards,
Heungjun Kim


Perhaps it's uncommon in embedded systems though.

--

Regards,
Sylwester



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

Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[Sylwester Nawrocki]

Hi,

On 12/30/2011 09:41 PM, Sakari Ailus wrote:
> On Fri, Dec 30, 2011 at 11:14:39AM +0100, Sylwester Nawrocki wrote:
>> 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?

Maybe DO_WHITE_BALANCE was inspired by some hardware's behaviour, I don't
know. I have nothing against this control. It allows you to perform one-shot
white balance in a given moment in time. Simple and clear.

> 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.

Yes, AFAIU those presets are just WB temperature, with names instead
of numbers. Thus it doesn't make much sense to expose both at the driver.

But in manual white balance mode camera could be switched to new WB value,
with component gain/balance controls, DO_WHITE_BALANCE or whatever, rendering
the preset setting invalid. Should we then have an invalid/unknown item in
the presets menu ? This would be only allowed to set by driver, i.e. read-only
for applications. If device provide multiple means for setting white balance
it is quite likely that at some point wb might not match any preset.

Having auto, manual and presets in one menu control wouldn't require that,
but we rather can't just change the V4L2_CID_WHITE_BALANCE control type now.

>>> The new 

Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

['Sakari Ailus']

Hi Hans,

On Fri, Dec 30, 2011 at 07:56:39PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 12/30/2011 07:42 PM, 'Sakari Ailus' wrote:
> >Hi Hans,
> >
> >On Fri, Dec 30, 2011 at 09:41:57AM +0100, Hans de Goede wrote:
> >...
> >>Right, so the above is exactly why I ended up making the pwc whitebalance
> >>control the way it is, the user can essentially choice between a number
> >>of options:
> >>1) auto whitebal
> >>2) a number of preset whitebal values (seems your proposal has some more 
> >>then the pwc
> >>driver, which is fine)
> >>3) manual whitebal, at which point the user may set whitebal through one of:
> >>a) a color temperature control
> >>b) red and blue balance controls
> >>c) red, green and blue gains
> >
> >d) red, green, blue and... another green. This is how some raw bayer sensors
> >can be controlled.
> >
> 
> Yes, but have you ever tried setting the 2 green gains to different values?
> (Hint the result is not pretty) I strongly believe the 2 separate green gains 
> are
> only there as it is easier to do things that way (it keeps the sensor array 
> symmetric) and
> there is no value in controlling them separately.

That's debatable. I agree that most of the time they're best kept same, but
I don't think that'd still universally true. We could add controls for the
five and depending on the capabilities of the hardware the driver would
expose either three or four.

In any case, I dont't think these controls are really something for the user
interface for the regular user, even with three components.

> >>Notice that we also need to add some standardized controls for the 3c case, 
> >>but that
> >>is a different discussion.
> >
> >I was planning to add the gain to low-level controls once the other  sensor
> >controls have been properly discussed, and hopefully approved, first.
> >
> >>Seeing how this discussion has evolved I believe that what I did in the pwc 
> >>driver
> >>is actually right from the user pov, the user gets one simple menu control 
> >>which
> >>allows the user to choice between auto / preset 1 - x / manual and since as
> >>described above choosing one of the options excludes the other options from 
> >>being
> >>active I believe having this all in one control is the right thing to do.
> >
> >I still think automatic white balance should be separate from the rest, as
> >it currently is (V4L2_CID_AUTO_WHITE_BALANCE). If such presets aren't
> >available, the way the user would enable automatic white balance changes,
> >which is bad.
> 
> Well we can just put in the spec that the control can be either a boolean or
> a menu control depending on if it has presets. I really believe that we should
> not make this 2 separate controls, it does not match from a user pov.

For a test program like yavta that's fine, but for an application that
could be worse. Applications should be able to rely that the type of
controls is constant. The control frame work also assumes this.

Do you see adverse effects in providing two controls?

> There are 2 things which the user wants to control here:
> 
> 1) How the value of the white balance controls gets controlled, which is
> one of: a) auto b) select values from preset 1 - # c) manaul
> 
> 2) The white balance controls themselves (temperature, or color balances ...),
> but only if 1) is set to manual
> 
> There is no reason to separate 1) in 2 separate controls, that will only serve
> to confuse the user.
> 
> As for the theoretical automatic wb which takes some hints like the presets,
> AFAIK no such device exists and it is unlikely one will show up in the near
> future. IOW it is purely theoretical and thus not interesting.

I beg to disagree. Remember that not all cameras that use V4L2 are webcams.
The Nokia N9 does use similar configuration items as hints for the white
balance algorithm. The algorithm is implemented in Linux user space, and to
provide full functionality over V4L2 interface that kind of a control would
eventually be required.

That said, there are other issues in the way of supporting the N9 camera
properly for the V4L2 applications.

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi jabber/XMPP/Gmail: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[Sakari Ailus]

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/med

Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[Hans de Goede]

Hi,

On 12/30/2011 07:42 PM, 'Sakari Ailus' wrote:

Hi Hans,

On Fri, Dec 30, 2011 at 09:41:57AM +0100, Hans de Goede wrote:
...

Right, so the above is exactly why I ended up making the pwc whitebalance
control the way it is, the user can essentially choice between a number
of options:
1) auto whitebal
2) a number of preset whitebal values (seems your proposal has some more then 
the pwc
driver, which is fine)
3) manual whitebal, at which point the user may set whitebal through one of:
a) a color temperature control
b) red and blue balance controls
c) red, green and blue gains


d) red, green, blue and... another green. This is how some raw bayer sensors
can be controlled.



Yes, but have you ever tried setting the 2 green gains to different values?
(Hint the result is not pretty) I strongly believe the 2 separate green gains 
are
only there as it is easier to do things that way (it keeps the sensor array 
symmetric) and
there is no value in controlling them separately.


Notice that we also need to add some standardized controls for the 3c case, but 
that
is a different discussion.


I was planning to add the gain to low-level controls once the other  sensor
controls have been properly discussed, and hopefully approved, first.


Seeing how this discussion has evolved I believe that what I did in the pwc 
driver
is actually right from the user pov, the user gets one simple menu control which
allows the user to choice between auto / preset 1 - x / manual and since as
described above choosing one of the options excludes the other options from 
being
active I believe having this all in one control is the right thing to do.


I still think automatic white balance should be separate from the rest, as
it currently is (V4L2_CID_AUTO_WHITE_BALANCE). If such presets aren't
available, the way the user would enable automatic white balance changes,
which is bad.


Well we can just put in the spec that the control can be either a boolean or
a menu control depending on if it has presets. I really believe that we should
not make this 2 separate controls, it does not match from a user pov.

There are 2 things which the user wants to control here:

1) How the value of the white balance controls gets controlled, which is
one of: a) auto b) select values from preset 1 - # c) manaul

2) The white balance controls themselves (temperature, or color balances ...),
but only if 1) is set to manual

There is no reason to separate 1) in 2 separate controls, that will only serve
to confuse the user.

As for the theoretical automatic wb which takes some hints like the presets,
AFAIK no such device exists and it is unlikely one will show up in the near
future. IOW it is purely theoretical and thus not interesting.

Regards,

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

Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

['Sakari Ailus']

Hi Hans,

On Fri, Dec 30, 2011 at 09:41:57AM +0100, Hans de Goede wrote:
...
> Right, so the above is exactly why I ended up making the pwc whitebalance
> control the way it is, the user can essentially choice between a number
> of options:
> 1) auto whitebal
> 2) a number of preset whitebal values (seems your proposal has some more then 
> the pwc
>driver, which is fine)
> 3) manual whitebal, at which point the user may set whitebal through one of:
>a) a color temperature control
>b) red and blue balance controls
>c) red, green and blue gains

d) red, green, blue and... another green. This is how some raw bayer sensors
can be controlled.

> Notice that we also need to add some standardized controls for the 3c case, 
> but that
> is a different discussion.

I was planning to add the gain to low-level controls once the other  sensor
controls have been properly discussed, and hopefully approved, first.

> Seeing how this discussion has evolved I believe that what I did in the pwc 
> driver
> is actually right from the user pov, the user gets one simple menu control 
> which
> allows the user to choice between auto / preset 1 - x / manual and since as
> described above choosing one of the options excludes the other options from 
> being
> active I believe having this all in one control is the right thing to do.

I still think automatic white balance should be separate from the rest, as
it currently is (V4L2_CID_AUTO_WHITE_BALANCE). If such presets aren't
available, the way the user would enable automatic white balance changes,
which is bad.

Also the automatic white balance algorithms may have related controls, which
would bring back the same situatio of some controls not being active at all
times --- which I don't see as a problem at all.

Regards,

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi jabber/XMPP/Gmail: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

['Sakari Ailus']

Hi HeungJun,

On Fri, Dec 30, 2011 at 03:35:59PM +0900, HeungJun, Kim wrote:
> Hi Sakari,
> 
> Thanks for the comments!
> 
> Your comments help me to order my thoughts and re-send RFC.
> 
> Anyway, I hope to be happy new year :)
> 
> > -Original Message-
> > From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> > ow...@vger.kernel.org] On Behalf Of Sakari Ailus
> > Sent: Friday, December 30, 2011 8:34 AM
> > To: Laurent Pinchart
> > Cc: Sylwester Nawrocki; HeungJun, Kim; linux-media@vger.kernel.org;
> > mche...@redhat.com; hverk...@xs4all.nl; kyungmin.p...@samsung.com; Hans de
> > Goede
> > Subject: Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu
> > control
> > 
> > Hi Laurent, Sylwester and HeungJun,
> > 
> > On Wed, Dec 28, 2011 at 02:51:38PM +0100, Laurent Pinchart wrote:
> > > Hi,
> > >
> > > 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.
> > 
> > The new control proposed by HeungJun is input for the automatic white
> > balance

Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[Sylwester Nawrocki]

Hi HeungJun,

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,

While at it, how about adding V4L2_WHITE_BALANCE_LED_LIGHT as well ?

--

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

Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[Sylwester Nawrocki]

Hi HeungJun,

On 12/29/2011 06:08 AM, HeungJun, Kim wrote:
>> -Original Message-
>> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
>> ow...@vger.kernel.org] On Behalf Of Laurent Pinchart
>> Sent: Wednesday, December 28, 2011 10:52 PM
>> To: Sylwester Nawrocki
>> Cc: HeungJun, Kim; linux-media@vger.kernel.org; mche...@redhat.com;
>> hverk...@xs4all.nl; sakari.ai...@iki.fi; kyungmin.p...@samsung.com; Hans de
>> Goede
>> Subject: Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu
>> control
>>
>> Hi,
>>
>> 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 :-)
>
> I guess the WBP menu controls of pwc driver is probably defined in the other
> headers, for users being well known the PWC hardware. So it should be managed
> separately to videodev2.h. Is it right? Even if the way might be slightly
> different, it can't avoid to be "managed separately".
> 
> It means the users being not well known the specific hardware like PWC,
> have difficulty to use that driver well.
> And, at least, It doesn't looks generic API for me.
> In this case, the unfamiliar user with such unique hardware, can use
> whatever he wants to use finally, after finding & looking around the headers.

Applications can query drivers for supported controls and populate user control
panels dynamically, based on information from VIDIOC_QUERYCTRL and 
VIDIOC_QUERYMENU
ioctls. Not needing to rely on menu items definition in videodev2.h.
I had a feeling you weren't considering such case. :)

Perhaps it's uncommon in embedded systems though.

--

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

Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[Sylwester Nawrocki]

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.

  
  

 +
 +  >>> spanname="id">V4L2_CID_PRESET_WHITE_BALANCE >>> 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 "unsub

Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[Hans de Goede]

Hi,

On 12/30/2011 07:35 AM, HeungJun, Kim wrote:

Hi Sakari,

Thanks for the comments!

Your comments help me to order my thoughts and re-send RFC.






The value of the new control would have an effect as long as automatic white
balance is enabled.

No, it's a kind of Manual White Balance, not Auto. It's the same level of
V4L2_CID_WHITE_BALANCE_TEMPERATURE. So, only when V4L2_CID_AUTO_WHITE_BALANCE is

disabled, this control is enabled.

The relationship between each white balance controls by my understanding is
here.

Auto White Balance
   - V4L2_CID_AUTO_WHITE_BALANCE(Boolean)
 : enable/disable Auto white balance.
 : Enable means current mode is Auto, and disable means current mode is
Manual

Manual White Balance
   - V4L2_CID_WHITE_BALANCE_TEMPERATURE(integer)
 : Setting the temperature of Manual
 : Only when the V4L2_CID_AUTO_WHITE_BALANCE is disabled, and current mode
Manual.

- V4L2_CID_WHITE_BALANCE_PRESET(menu) - I suggested
 : Setting the specific temperature value(but, the value is not fetched by
user) of Manual
 : Only when the V4L2_CID_AUTO_WHITE_BALANCE is disabled, and current mode
Manual.

The "input" is right. And, this "input" just triggers the ISP(sensor) set the
specific
manual white balance value embedded in the ISP.
I think this control does not affect the Auto White Balance.


Right, so the above is exactly why I ended up making the pwc whitebalance
control the way it is, the user can essentially choice between a number
of options:
1) auto whitebal
2) a number of preset whitebal values (seems your proposal has some more then 
the pwc
   driver, which is fine)
3) manual whitebal, at which point the user may set whitebal through one of:
   a) a color temperature control
   b) red and blue balance controls
   c) red, green and blue gains

Notice that we also need to add some standardized controls for the 3c case, but 
that
is a different discussion.

Seeing how this discussion has evolved I believe that what I did in the pwc 
driver
is actually right from the user pov, the user gets one simple menu control which
allows the user to choice between auto / preset 1 - x / manual and since as
described above choosing one of the options excludes the other options from 
being
active I believe having this all in one control is the right thing to do.

Regards,

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

RE: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[HeungJun, Kim]

Hi Sakari,

Thanks for the comments!

Your comments help me to order my thoughts and re-send RFC.

Anyway, I hope to be happy new year :)

> -Original Message-
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Sakari Ailus
> Sent: Friday, December 30, 2011 8:34 AM
> To: Laurent Pinchart
> Cc: Sylwester Nawrocki; HeungJun, Kim; linux-media@vger.kernel.org;
> mche...@redhat.com; hverk...@xs4all.nl; kyungmin.p...@samsung.com; Hans de
> Goede
> Subject: Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu
> control
> 
> Hi Laurent, Sylwester and HeungJun,
> 
> On Wed, Dec 28, 2011 at 02:51:38PM +0100, Laurent Pinchart wrote:
> > Hi,
> >
> > 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.
> 
> 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.
No, it's a kind of Manual White Balance, not Auto. It's the same level of
V4L2_CID_WHITE_BALANCE_TEMPERATURE. So, only when V4L2_CID_AUTO_WHITE_BALANCE is

disabled, this control is enabled.

The relationship between each white balance controls by my understanding is
here.

Auto White Balance
  - V4L2

Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[Kim, Heungjun]

Hi,

On 2011년 12월 30일 08:58, Laurent Pinchart wrote:

Hi,

On Thursday 29 December 2011 06:08:07 HeungJun, Kim wrote:

On Wednesday, December 28, 2011 10:52 PM 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 :-)

I guess the WBP menu controls of pwc driver is probably defined in the
other headers, for users being well known the PWC hardware. So it should
be managed separately to videodev2.h. Is it right? Even if the way might
be slightly different, it can't avoid to be "managed separately".

It means the users being not well known the specific hardware like PWC,
have difficulty to use that driver well.
And, at least, It doesn't looks generic API for me.
In this case, the unfamiliar user with such unique hardware, can use
whatever he wants to use finally, after finding&  looking around the
headers.

I think Sylwester was just pointing the pwc driver out as implementing a
custom white balance preset control to make sure we're aware of it, not to
advocate such an approach. We need to make sure that the generic white balance
preset control can handle the pwc driver's needs.

Ok, I understand what I miss on your point.


On the other hand, if the definition is defined in the videodev2.h, or at
least the videodev2.h also include separated API's headers(I'm not sure
this way, but my real meaning is needed to be consolidated only one way to
control hardware, like videodev2.h), it looks more be better.

As such meaning, I agreed to Sylwester's word "dubious".

But, here's the thing.
If the any control suggested does not look general function on camera,
and it is used "only at the specific hardware", we should avoid this.

As you know, the White Balance Preset is very general camera's feature,
and the term's name is also very normal to the digital camera users.
Almost digital camera have this function.

Probably, Laurent might be concern about such "generality". Am I right?

I agree that a generic white balance preset control is needed. I think the
documentation should be improved though, to clearly define how the control
works.
So, your point is the documentation which is described how these 
controls interact previous controls,
e.g, like WHITE_BALANCE_PRESET as I suggested and previous remained 
DO/TEMPRATURE, etc.



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 ?

[snip]


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.




+   
+   V4L2_CID_PRESET_WHITE_BALANCE 
;

Wouldn't V4L2_CID_WHITE_BALANCE_PRESET be better ?

That's what I was about to say.

I saw the controls related with White Balance and most cid's name
is V4L2_CID__WHITE_BALANCE. So, I just used that n

Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[Laurent Pinchart]

Hi,

On Thursday 29 December 2011 06:08:07 HeungJun, Kim wrote:
> On Wednesday, December 28, 2011 10:52 PM 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 :-)
> 
> I guess the WBP menu controls of pwc driver is probably defined in the
> other headers, for users being well known the PWC hardware. So it should
> be managed separately to videodev2.h. Is it right? Even if the way might
> be slightly different, it can't avoid to be "managed separately".
> 
> It means the users being not well known the specific hardware like PWC,
> have difficulty to use that driver well.
> And, at least, It doesn't looks generic API for me.
> In this case, the unfamiliar user with such unique hardware, can use
> whatever he wants to use finally, after finding & looking around the
> headers.

I think Sylwester was just pointing the pwc driver out as implementing a 
custom white balance preset control to make sure we're aware of it, not to 
advocate such an approach. We need to make sure that the generic white balance 
preset control can handle the pwc driver's needs.

> On the other hand, if the definition is defined in the videodev2.h, or at
> least the videodev2.h also include separated API's headers(I'm not sure
> this way, but my real meaning is needed to be consolidated only one way to
> control hardware, like videodev2.h), it looks more be better.
> 
> As such meaning, I agreed to Sylwester's word "dubious".
> 
> But, here's the thing.
> If the any control suggested does not look general function on camera,
> and it is used "only at the specific hardware", we should avoid this.
> 
> As you know, the White Balance Preset is very general camera's feature,
> and the term's name is also very normal to the digital camera users.
> Almost digital camera have this function.
> 
> Probably, Laurent might be concern about such "generality". Am I right?

I agree that a generic white balance preset control is needed. I think the 
documentation should be improved though, to clearly define how the control 
works.

> > > 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 ?
> > 
> > [snip]
> > 
> > > > 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.
> > > > 
> > > >   
> > > >   
> > > > 
> > > > + 
> > > > +> > > spanname="id">V4L2_CID_PRESET_WHITE_BALANCE 
> > > > ;
> > > 
> > >

Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[Sakari Ailus]

Hi Laurent, Sylwester and HeungJun,

On Wed, Dec 28, 2011 at 02:51:38PM +0100, Laurent Pinchart wrote:
> Hi,
> 
> 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.

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.

> 
> > > 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.
> > > 
> > > 
> > > 
> > > 
> > > +   
> > > +  > > spanname="id">V4L2_CID_PRESET_WHITE_BALANCE  > > 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.

Cheers,

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi jabber/XMPP/Gmail: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[HeungJun, Kim]

Hi Laurent and Sylwester,

> -Original Message-
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Laurent Pinchart
> Sent: Wednesday, December 28, 2011 10:52 PM
> To: Sylwester Nawrocki
> Cc: HeungJun, Kim; linux-media@vger.kernel.org; mche...@redhat.com;
> hverk...@xs4all.nl; sakari.ai...@iki.fi; kyungmin.p...@samsung.com; Hans de
> Goede
> Subject: Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu
> control
> 
> Hi,
> 
> 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 :-)
I guess the WBP menu controls of pwc driver is probably defined in the other
headers, for users being well known the PWC hardware. So it should be managed
separately to videodev2.h. Is it right? Even if the way might be slightly
different, it can't avoid to be "managed separately".

It means the users being not well known the specific hardware like PWC,
have difficulty to use that driver well.
And, at least, It doesn't looks generic API for me.
In this case, the unfamiliar user with such unique hardware, can use
whatever he wants to use finally, after finding & looking around the headers.

On the other hand, if the definition is defined in the videodev2.h, or at least
the videodev2.h also include separated API's headers(I'm not sure this way,
but my real meaning is needed to be consolidated only one way to control 
hardware,
like videodev2.h), it looks more be better. 

As such meaning, I agreed to Sylwester's word "dubious".

But, here's the thing.
If the any control suggested does not look general function on camera,
and it is used "only at the specific hardware", we should avoid this.

As you know, the White Balance Preset is very general camera's feature,
and the term's name is also very normal to the digital camera users.
Almost digital camera have this function.

Probably, Laurent might be concern about such "generality". Am I right?

> 
> > 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 ?
> 
> [snip]
> 
> > > 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
>

RE: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[HeungJun, Kim]

Hi Sylwester,

I'll continue this conversation to the Laurent's reply.
So, please check the others.

> -Original Message-
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Sylwester Nawrocki
> Sent: Wednesday, December 28, 2011 10:35 PM
> To: HeungJun, Kim
> Cc: linux-media@vger.kernel.org; mche...@redhat.com; hverk...@xs4all.nl;
> sakari.ai...@iki.fi; laurent.pinch...@ideasonboard.com;
> kyungmin.p...@samsung.com; Hans de Goede
> Subject: Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu
> control
> 
> Hi,
> 
> 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 ?
> 
> 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 ?
> 
> > Signed-off-by: HeungJun, Kim 
> > Signed-off-by: Kyungmin Park 
> > ---
> >  Documentation/DocBook/media/v4l/controls.xml |   38
> ++
> >  drivers/media/video/v4l2-ctrls.c |   12 
> >  include/linux/videodev2.h|9 ++
> >  3 files changed, 59 insertions(+), 0 deletions(-)
> >
> > 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.
> >   
> >   
> >
> > + 
> > +spanname="id">V4L2_CID_PRESET_WHITE_BALANCE 
> 
> Wouldn't V4L2_CID_WHITE_BALANCE_PRESET be better ?
> 
> > +   enum v4l2_preset_white_balance
> > + This control sets
> > + the camera's white balance by the presets. These preset is provided
> > + by the type of the enum values which are generally provided
> > + by several digital cameras. But, it dosen't mean that the specific
> 
> s/dosent'/doesn't
> 
> > + preset always can be counterposed with the unique white balance value.
> > + This is a read/write control.
> 
> We probably only need such a remark if a control is read-only or write-only.
> 
> > + 
> > + 
> > +   
> > + 
> > +   
> > +
> V4L2_WHITE_BALANCE_INCANDESCENT 
> > + White Balance Incandescent/Tungster preset.
> 
> s/Tungster/Tungsten
> 
> If we are to have these presets, then we need to be a little bit more verbose
> about what they mean. To avoid situation similar to V4L2_CID_COLORFX control
> where nobody knows exactly what's an exact meaning of the menu items.
> 
> > +   
> > +   
> >

Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[Laurent Pinchart]

Hi,

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 ?

[snip]

> > 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.
> > 
> >   
> >   
> > 
> > + 
> > +> spanname="id">V4L2_CID_PRESET_WHITE_BALANCE  > entry>
> 
> Wouldn't V4L2_CID_WHITE_BALANCE_PRESET be better ?

That's what I was about to say.

-- 
Regards,

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

Re: [RFC PATCH 1/4] v4l: Add V4L2_CID_PRESET_WHITE_BALANCE menu control

[Sylwester Nawrocki]

Hi,

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 ?

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 ?

> Signed-off-by: HeungJun, Kim 
> Signed-off-by: Kyungmin Park 
> ---
>  Documentation/DocBook/media/v4l/controls.xml |   38 
> ++
>  drivers/media/video/v4l2-ctrls.c |   12 
>  include/linux/videodev2.h|9 ++
>  3 files changed, 59 insertions(+), 0 deletions(-)
> 
> 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.
> 
> 
>  
> +   
> +  spanname="id">V4L2_CID_PRESET_WHITE_BALANCE 

Wouldn't V4L2_CID_WHITE_BALANCE_PRESET be better ?

> + enum v4l2_preset_white_balance
> +   This control sets
> +   the camera's white balance by the presets. These preset is provided
> +   by the type of the enum values which are generally provided
> +   by several digital cameras. But, it dosen't mean that the specific

s/dosent'/doesn't

> +   preset always can be counterposed with the unique white balance value.
> +   This is a read/write control.

We probably only need such a remark if a control is read-only or write-only.

> +   
> +   
> + 
> +   
> + 
> +   
> V4L2_WHITE_BALANCE_INCANDESCENT 
> +   White Balance Incandescent/Tungster preset.

s/Tungster/Tungsten

If we are to have these presets, then we need to be a little bit more verbose
about what they mean. To avoid situation similar to V4L2_CID_COLORFX control
where nobody knows exactly what's an exact meaning of the menu items.

> + 
> + 
> +   
> V4L2_WHITE_BALANCE_FLUORESCENT 
> +   White Balance Fluorescent preset.
> + 
> + 
> +   
> V4L2_WHITE_BALANCE_DAYLIGHT 
> +   White Balance Daylight preset.
> + 
> + 
> +   
> V4L2_WHITE_BALANCE_CLOUDY 
> +   White Balance Cloudy preset.
> + 
> + 
> +   
> V4L2_WHITE_BALANCE_SHADE 
> +   White Balance Share preset.

s/Share/Shade

> + 
> +   
> + 
> +   
> +   
> +
> 
>spanname="id">V4L2_CID_PRIVACY 
>   boolean
> diff --git a/drivers/media/video/v4l2-ctrls.c 
> b/drivers/media/video/v4l2-ctrls.c
> index 0f415da..f51b576 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -234,6 +234,14 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>   "Vivid",
>   NULL
>   };
> + static const char * const preset_white_balance[] = {
> + "Incandescent",
> + "Fluorescent",
> + "Daylight",
> + "Cloudy",
> + "Shade",
> + NULL,
> + };
>   static const char * const tune_preemphasis[] = {
>   "No Preemphasis",
>