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.</entry>
</row>
<row><entry></entry></row>
+ <row id="v4l2-preset-white-balance">
+ <entry
spanname="id"><constant>V4L2_CID_PRESET_WHITE_BALANCE</constant> 
;</ entry>
Wouldn't V4L2_CID_WHITE_BALANCE_PRESET be better ?
That's what I was about to say.
I saw the controls related with White Balance and most cid's name
is V4L2_CID_XXXX_WHITE_BALANCE. So, I just used that name.
But, I don't know why the PRESET's position is important.
Can anyone explain to me?
After I know it, I seems to talk about this subject.
Anyway, I guess Laurent had in mind for another class "preset".
I'm not sure about the name, but I think it might make sense to put those
high-level camera controls in a new class.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
