[RFC PATCH 0/4] Add some new camera controls

[RFC PATCH 0/4] Add some new camera controls

[HeungJun, Kim]

Hi everyone,

This RFC patch series include new 4 controls ID for digital camera.
I about to suggest these controls by the necessity enabling the M-5MOLS
sensor's function, and I hope to discuss this in here.

Any opinions and thoughts are very welcome!

It's good to connect Sylwester's suggestion for discussing.
- http://www.mail-archive.com/linux-media@vger.kernel.org/msg39907.html

But it's no problem even if it is considered as seperated subject.

1. White Balance Peset
======================

Some camera hardware provides its own preset of white balance,
but fortunately the names of these presets are similar with the others.
So, I thought it can be provided as a generic digital camera API.
I suggest the following as items:

enum v4l2_preset_white_balance {
	V4L2_WHITE_BALANCE_INCANDESCENT = 0,
	V4L2_WHITE_BALANCE_FLUORESCENT = 1,
	V4L2_WHITE_BALANCE_DAYLIGHT = 2,
	V4L2_WHITE_BALANCE_CLOUDY = 3,
	V4L2_WHITE_BALANCE_SHADE = 4,
};

2. Scenemode
============

I had suggested it before. :
http://www.mail-archive.com/linux-media@vger.kernel.org/msg29917.html

And I want to continue this subject on this threads.

The scenemode is also needed in the mobile digital .
The reason I about to suggest this function as CID,
is also the items are used widely & generally.

enum v4l2_scenemode {
	V4L2_SCENEMODE_NONE = 0,
	V4L2_SCENEMODE_NORMAL = 1,
	V4L2_SCENEMODE_PORTRAIT = 2,
	V4L2_SCENEMODE_LANDSCAPE = 3,
	V4L2_SCENEMODE_SPORTS = 4,
	V4L2_SCENEMODE_PARTY_INDOOR = 5,
	V4L2_SCENEMODE_BEACH_SNOW = 6,
	V4L2_SCENEMODE_SUNSET = 7,
	V4L2_SCENEMODE_DAWN_DUSK = 8,
	V4L2_SCENEMODE_FALL = 9,
	V4L2_SCENEMODE_NIGHT = 10,
	V4L2_SCENEMODE_AGAINST_LIGHT = 11,
	V4L2_SCENEMODE_FIRE = 12,
	V4L2_SCENEMODE_TEXT = 13,
	V4L2_SCENEMODE_CANDLE = 14,
};

3. WDR(Wide Dynamic Range)
==========================

This function can be unfamiliar, but it is as known as HDR(High Dynamic Range)
to iPhone users. Although the name is different, but both are the same function.

It makes the image look more clear by adjusting the intensity area of
illumination of the image. This function can be used only turn on/off
like button control, then the actual WDR algorithm are activated in the hardware.

4. Antishake
============

This function compensate and stabilize the shakeness of the stream and image.
So, if this function turned on, the image created without many shakeness.
It means both, the case when compensating the stream's shakeness,
and when stabilizing the image itself.

5. References
=============

- This is the example of the various digital camera's upper controls.
You can find that the term of each control is very similiar.

@ White Balance Preset
http://imaging.nikon.com/history/basics/17/index.htm
http://www.dailyphotographytips.net/camera-controls-and-settings/how-to-set-custom-white-balance/
http://www.digitalcamera-hq.com/articles/how-to-white-balance-your-camera
http://www.digital-photography-school.com/introduction-to-white-balance

@ Scenemode
http://www.digital-photography-school.com/digital-camera-modes
http://www.picturecorrect.com/tips/digital-camera-scene-modes/

@ WDR and HDR
http://en.wikipedia.org/wiki/High_dynamic_range_imaging
http://en.wikipedia.org/wiki/Wide_dynamic_range

@ Ahtishake
http://www.digital-slr-guide.com/digital-slr-anti-shake.html

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

[HeungJun, Kim]

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,

Signed-off-by: HeungJun, Kim <riverful.kim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 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.</entry>
 	  </row>
 	  <row><entry></entry></row>
 
+	  <row id="v4l2-preset-white-balance">
+	    <entry spanname="id"><constant>V4L2_CID_PRESET_WHITE_BALANCE</constant>&nbsp;</entry>
+	    <entry>enum&nbsp;v4l2_preset_white_balance</entry>
+	  </row><row><entry spanname="descr">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
+	  preset always can be counterposed with the unique white balance value.
+	  This is a read/write control.</entry>
+	  </row>
+	  <row>
+	    <entrytbl spanname="descr" cols="2">
+	      <tbody valign="top">
+		<row>
+		  <entry><constant>V4L2_WHITE_BALANCE_INCANDESCENT</constant>&nbsp;</entry>
+		  <entry>White Balance Incandescent/Tungster preset.</entry>
+		</row>
+		<row>
+		  <entry><constant>V4L2_WHITE_BALANCE_FLUORESCENT</constant>&nbsp;</entry>
+		  <entry>White Balance Fluorescent preset.</entry>
+		</row>
+		<row>
+		  <entry><constant>V4L2_WHITE_BALANCE_DAYLIGHT</constant>&nbsp;</entry>
+		  <entry>White Balance Daylight preset.</entry>
+		</row>
+		<row>
+		  <entry><constant>V4L2_WHITE_BALANCE_CLOUDY</constant>&nbsp;</entry>
+		  <entry>White Balance Cloudy preset.</entry>
+		</row>
+		<row>
+		  <entry><constant>V4L2_WHITE_BALANCE_SHADE</constant>&nbsp;</entry>
+		  <entry>White Balance Share preset.</entry>
+		</row>
+	      </tbody>
+	    </entrytbl>
+	  </row>
+	  <row><entry></entry></row>
+
 	  <row>
 	    <entry spanname="id"><constant>V4L2_CID_PRIVACY</constant>&nbsp;</entry>
 	    <entry>boolean</entry>
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",
 		"50 useconds",
@@ -390,6 +398,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 		return camera_exposure_auto;
 	case V4L2_CID_COLORFX:
 		return colorfx;
+	case V4L2_CID_PRESET_WHITE_BALANCE:
+		return preset_white_balance;
 	case V4L2_CID_TUNE_PREEMPHASIS:
 		return tune_preemphasis;
 	case V4L2_CID_FLASH_LED_MODE:
@@ -567,6 +577,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_PRIVACY:			return "Privacy";
 	case V4L2_CID_IRIS_ABSOLUTE:		return "Iris, Absolute";
 	case V4L2_CID_IRIS_RELATIVE:		return "Iris, Relative";
+	case V4L2_CID_PRESET_WHITE_BALANCE:	return "White Balance, Preset";
 
 	/* FM Radio Modulator control */
 	/* Keep the order of the 'case's the same as in videodev2.h! */
@@ -680,6 +691,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_MPEG_STREAM_VBI_FMT:
 	case V4L2_CID_EXPOSURE_AUTO:
 	case V4L2_CID_COLORFX:
+	case V4L2_CID_PRESET_WHITE_BALANCE:
 	case V4L2_CID_TUNE_PREEMPHASIS:
 	case V4L2_CID_FLASH_LED_MODE:
 	case V4L2_CID_FLASH_STROBE_SOURCE:
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 3d62631..a842de0 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1618,6 +1618,15 @@ enum  v4l2_exposure_auto_type {
 #define V4L2_CID_IRIS_ABSOLUTE			(V4L2_CID_CAMERA_CLASS_BASE+17)
 #define V4L2_CID_IRIS_RELATIVE			(V4L2_CID_CAMERA_CLASS_BASE+18)
 
+#define V4L2_CID_PRESET_WHITE_BALANCE		(V4L2_CID_CAMERA_CLASS_BASE+19)
+enum v4l2_preset_white_balance {
+	V4L2_WHITE_BALANCE_INCANDESCENT = 0,
+	V4L2_WHITE_BALANCE_FLUORESCENT = 1,
+	V4L2_WHITE_BALANCE_DAYLIGHT = 2,
+	V4L2_WHITE_BALANCE_CLOUDY = 3,
+	V4L2_WHITE_BALANCE_SHADE = 4,
+};
+
 /* FM Modulator class control IDs */
 #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
 #define V4L2_CID_FM_TX_CLASS			(V4L2_CTRL_CLASS_FM_TX | 1)
-- 
1.7.4.1

[RFC PATCH 2/4] v4l: Add V4L2_CID_SCENEMODE menu control

[HeungJun, Kim]

It adds the new CID for setting Scenemode. This CID is provided as
menu type using the following items:
enum v4l2_scenemode {
	V4L2_SCENEMODE_NONE = 0,
	V4L2_SCENEMODE_NORMAL = 1,
	V4L2_SCENEMODE_PORTRAIT = 2,
	V4L2_SCENEMODE_LANDSCAPE = 3,
	V4L2_SCENEMODE_SPORTS = 4,
	V4L2_SCENEMODE_PARTY_INDOOR = 5,
	V4L2_SCENEMODE_BEACH_SNOW = 6,
	V4L2_SCENEMODE_SUNSET = 7,
	V4L2_SCENEMODE_DAWN_DUSK = 8,
	V4L2_SCENEMODE_FALL = 9,
	V4L2_SCENEMODE_NIGHT = 10,
	V4L2_SCENEMODE_AGAINST_LIGHT = 11,
	V4L2_SCENEMODE_FIRE = 12,
	V4L2_SCENEMODE_TEXT = 13,
	V4L2_SCENEMODE_CANDLE = 14,
};

Signed-off-by: HeungJun, Kim <riverful.kim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 Documentation/DocBook/media/v4l/controls.xml |   88 ++++++++++++++++++++++++++
 drivers/media/video/v4l2-ctrls.c             |   21 ++++++
 include/linux/videodev2.h                    |   19 ++++++
 3 files changed, 128 insertions(+), 0 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
index 350c138..afe1845 100644
--- a/Documentation/DocBook/media/v4l/controls.xml
+++ b/Documentation/DocBook/media/v4l/controls.xml
@@ -2879,6 +2879,94 @@ it one step further. This is a write-only control.</entry>
 	  </row>
 	  <row><entry></entry></row>
 
+	  <row id="v4l2-scenemode">
+	    <entry spanname="id"><constant>V4L2_CID_SCENEMODE</constant>&nbsp;</entry>
+	    <entry>enum&nbsp;v4l2_scenemode</entry>
+	  </row><row><entry spanname="descr">This control sets
+	  the camera's scenemode, and it is provided by the type of
+	  the enum values. The "None" mode means the status
+	  when scenemode algorithm is not activated, like after booting time.
+	  On the other hand, the "Normal" mode means the scenemode algorithm
+	  is activated on the normal mode.</entry>
+	  </row>
+	  <row>
+	    <entrytbl spanname="descr" cols="2">
+	      <tbody valign="top">
+		<row>
+		  <entry><constant>V4L2_SCENEMODE_NONE</constant>&nbsp;</entry>
+		  <entry>Scenemode None.</entry>
+		</row>
+		<row>
+		  <entry><constant>V4L2_SCENEMODE_NORMAL</constant>&nbsp;</entry>
+		  <entry>Scenemode Normal.</entry>
+		</row>
+		<row>
+		  <entry><constant>V4L2_SCENEMODE_PORTRAIT</constant>&nbsp;</entry>
+		  <entry>Scenemode Portrait.</entry>
+		</row>
+		<row>
+		  <entry><constant>V4L2_SCENEMODE_LANDSCAPE</constant>&nbsp;</entry>
+		  <entry>Scenemode Landscape.</entry>
+		</row>
+		<row>
+		  <entry><constant>V4L2_SCENEMODE_SPORTS</constant>&nbsp;</entry>
+		  <entry>Scenemode Sports.</entry>
+		</row>
+		<row>
+		  <entry><constant>V4L2_SCENEMODE_PARTY_INDOOR</constant>&nbsp;</entry>
+		  <entry>Scenemode Party Indoor.</entry>
+		</row>
+		<row>
+		  <entry><constant>V4L2_SCENEMODE_BEACH_SNOW</constant>&nbsp;</entry>
+		  <entry>Scenemode Beach Snow.</entry>
+		</row>
+		<row>
+		  <entry><constant>V4L2_SCENEMODE_SUNSET</constant>&nbsp;</entry>
+		  <entry>Scenemode Beach Snow.</entry>
+		</row>
+		<row>
+		  <entry><constant>V4L2_SCENEMODE_DAWN_DUSK</constant>&nbsp;</entry>
+		  <entry>Scenemode Dawn Dusk.</entry>
+		</row>
+		<row>
+		  <entry><constant>V4L2_SCENEMODE_FALL</constant>&nbsp;</entry>
+		  <entry>Scenemode Fall.</entry>
+		</row>
+		<row>
+		  <entry><constant>V4L2_SCENEMODE_NIGHT</constant>&nbsp;</entry>
+		  <entry>Scenemode Night.</entry>
+		</row>
+		<row>
+		  <entry><constant>V4L2_SCENEMODE_AGAINST_LIGHT</constant>&nbsp;</entry>
+		  <entry>Scenemode Against Light.</entry>
+		</row>
+		<row>
+		  <entry><constant>V4L2_SCENEMODE_FIRE</constant>&nbsp;</entry>
+		  <entry>Scenemode Fire.</entry>
+		</row>
+		<row>
+		  <entry><constant>V4L2_SCENEMODE_TEXT</constant>&nbsp;</entry>
+		  <entry>Scenemode Text.</entry>
+		</row>
+		<row>
+		  <entry><constant>V4L2_SCENEMODE_CANDLE</constant>&nbsp;</entry>
+		  <entry>Scenemode Candle.</entry>
+		</row>
+	      </tbody>
+	    </entrytbl>
+	  </row>
+	  <row><entry></entry></row>
+
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_PRIVACY</constant>&nbsp;</entry>
+	    <entry>boolean</entry>
+	  </row><row><entry spanname="descr">Prevent video from being acquired
+by the camera. When this control is set to <constant>TRUE</constant> (1), no
+image can be captured by the camera. Common means to enforce privacy are
+mechanical obturation of the sensor and firmware image processing, but the
+device is not restricted to these methods. Devices that implement the privacy
+control must support read access and may support write access.</entry>
+	  </row>
 	  <row>
 	    <entry spanname="id"><constant>V4L2_CID_PRIVACY</constant>&nbsp;</entry>
 	    <entry>boolean</entry>
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index f51b576..fef58c2 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -242,6 +242,23 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 		"Shade",
 		NULL,
 	};
+	static const char * const scenemode[] = {
+		"None",
+		"Normal",
+		"Landscape",
+		"Sports",
+		"Party Indoor",
+		"Beach Snow",
+		"Sunset",
+		"Dawn Dusk",
+		"Fall",
+		"Night",
+		"Against Light",
+		"Fire",
+		"Text",
+		"Candle",
+		NULL,
+	};
 	static const char * const tune_preemphasis[] = {
 		"No Preemphasis",
 		"50 useconds",
@@ -400,6 +417,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 		return colorfx;
 	case V4L2_CID_PRESET_WHITE_BALANCE:
 		return preset_white_balance;
+	case V4L2_CID_SCENEMODE:
+		return scenemode;
 	case V4L2_CID_TUNE_PREEMPHASIS:
 		return tune_preemphasis;
 	case V4L2_CID_FLASH_LED_MODE:
@@ -578,6 +597,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_IRIS_ABSOLUTE:		return "Iris, Absolute";
 	case V4L2_CID_IRIS_RELATIVE:		return "Iris, Relative";
 	case V4L2_CID_PRESET_WHITE_BALANCE:	return "White Balance, Preset";
+	case V4L2_CID_SCENEMODE:		return "Scenemode";
 
 	/* FM Radio Modulator control */
 	/* Keep the order of the 'case's the same as in videodev2.h! */
@@ -692,6 +712,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_EXPOSURE_AUTO:
 	case V4L2_CID_COLORFX:
 	case V4L2_CID_PRESET_WHITE_BALANCE:
+	case V4L2_CID_SCENEMODE:
 	case V4L2_CID_TUNE_PREEMPHASIS:
 	case V4L2_CID_FLASH_LED_MODE:
 	case V4L2_CID_FLASH_STROBE_SOURCE:
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index a842de0..bc14feb 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1627,6 +1627,25 @@ enum v4l2_preset_white_balance {
 	V4L2_WHITE_BALANCE_SHADE = 4,
 };
 
+#define V4L2_CID_SCENEMODE			(V4L2_CID_CAMERA_CLASS_BASE+20)
+enum v4l2_scenemode {
+	V4L2_SCENEMODE_NONE = 0,
+	V4L2_SCENEMODE_NORMAL = 1,
+	V4L2_SCENEMODE_PORTRAIT = 2,
+	V4L2_SCENEMODE_LANDSCAPE = 3,
+	V4L2_SCENEMODE_SPORTS = 4,
+	V4L2_SCENEMODE_PARTY_INDOOR = 5,
+	V4L2_SCENEMODE_BEACH_SNOW = 6,
+	V4L2_SCENEMODE_SUNSET = 7,
+	V4L2_SCENEMODE_DAWN_DUSK = 8,
+	V4L2_SCENEMODE_FALL = 9,
+	V4L2_SCENEMODE_NIGHT = 10,
+	V4L2_SCENEMODE_AGAINST_LIGHT = 11,
+	V4L2_SCENEMODE_FIRE = 12,
+	V4L2_SCENEMODE_TEXT = 13,
+	V4L2_SCENEMODE_CANDLE = 14,
+};
+
 /* FM Modulator class control IDs */
 #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
 #define V4L2_CID_FM_TX_CLASS			(V4L2_CTRL_CLASS_FM_TX | 1)
-- 
1.7.4.1

[RFC PATCH 3/4] v4l: Add V4L2_CID_WDR button control

[HeungJun, Kim]

It adds the new CID for setting White Balance Preset. This CID is provided as
button type. This can commands only if the camera turn on/off this function.

Signed-off-by: HeungJun, Kim <riverful.kim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 Documentation/DocBook/media/v4l/controls.xml |   12 ++++++++++++
 drivers/media/video/v4l2-ctrls.c             |    2 ++
 include/linux/videodev2.h                    |    2 ++
 3 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
index afe1845..bed6c66 100644
--- a/Documentation/DocBook/media/v4l/controls.xml
+++ b/Documentation/DocBook/media/v4l/controls.xml
@@ -2958,6 +2958,18 @@ it one step further. This is a write-only control.</entry>
 	  <row><entry></entry></row>
 
 	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_WDR</constant></entry>
+	    <entry>button</entry>
+	  </row>
+	  <row>
+	    <entry spanname="descr">Wide Dynamic Range. It makes
+	    the image be more clear by adjusting the image's intensity
+	    of the illumination. This function can be provided according to
+	    the capability of the hardware(sensor or AP's multimedia block).
+	    </entry>
+	  </row>
+
+	  <row>
 	    <entry spanname="id"><constant>V4L2_CID_PRIVACY</constant>&nbsp;</entry>
 	    <entry>boolean</entry>
 	  </row><row><entry spanname="descr">Prevent video from being acquired
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index fef58c2..66110bc 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -598,6 +598,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_IRIS_RELATIVE:		return "Iris, Relative";
 	case V4L2_CID_PRESET_WHITE_BALANCE:	return "White Balance, Preset";
 	case V4L2_CID_SCENEMODE:		return "Scenemode";
+	case V4L2_CID_WDR:			return "Wide Dynamic Range";
 
 	/* FM Radio Modulator control */
 	/* Keep the order of the 'case's the same as in videodev2.h! */
@@ -687,6 +688,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 		break;
 	case V4L2_CID_PAN_RESET:
 	case V4L2_CID_TILT_RESET:
+	case V4L2_CID_WDR:
 	case V4L2_CID_FLASH_STROBE:
 	case V4L2_CID_FLASH_STROBE_STOP:
 		*type = V4L2_CTRL_TYPE_BUTTON;
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index bc14feb..f85ad6c 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1646,6 +1646,8 @@ enum v4l2_scenemode {
 	V4L2_SCENEMODE_CANDLE = 14,
 };
 
+#define V4L2_CID_WDR				(V4L2_CID_CAMERA_CLASS_BASE+21)
+
 /* FM Modulator class control IDs */
 #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
 #define V4L2_CID_FM_TX_CLASS			(V4L2_CTRL_CLASS_FM_TX | 1)
-- 
1.7.4.1

[RFC PATCH 4/4] v4l: Add V4L2_CID_ANTISHAKE button control

[HeungJun, Kim]

It adds the new CID for setting Anti-shake. This CID is provided as
button type. This can commands only if the camera turn on/off this function.

Signed-off-by: HeungJun, Kim <riverful.kim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 Documentation/DocBook/media/v4l/controls.xml |   11 +++++++++++
 drivers/media/video/v4l2-ctrls.c             |    2 ++
 include/linux/videodev2.h                    |    1 +
 3 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
index bed6c66..73ff05c 100644
--- a/Documentation/DocBook/media/v4l/controls.xml
+++ b/Documentation/DocBook/media/v4l/controls.xml
@@ -2970,6 +2970,17 @@ it one step further. This is a write-only control.</entry>
 	  </row>
 
 	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_ANTISHAKE</constant></entry>
+	    <entry>button</entry>
+	  </row>
+	  <row>
+	    <entry spanname="descr">Anti-Shake. It makes
+	    the image be more stabilized from the image's shakeness.
+	    This function can be provided according to the capability
+	    of the hardware(sensor or AP's multimedia block).</entry>
+	  </row>
+
+	  <row>
 	    <entry spanname="id"><constant>V4L2_CID_PRIVACY</constant>&nbsp;</entry>
 	    <entry>boolean</entry>
 	  </row><row><entry spanname="descr">Prevent video from being acquired
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 66110bc..05ff6bb 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -599,6 +599,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_PRESET_WHITE_BALANCE:	return "White Balance, Preset";
 	case V4L2_CID_SCENEMODE:		return "Scenemode";
 	case V4L2_CID_WDR:			return "Wide Dynamic Range";
+	case V4L2_CID_ANTISHAKE:		return "Antishake";
 
 	/* FM Radio Modulator control */
 	/* Keep the order of the 'case's the same as in videodev2.h! */
@@ -689,6 +690,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_PAN_RESET:
 	case V4L2_CID_TILT_RESET:
 	case V4L2_CID_WDR:
+	case V4L2_CID_ANTISHAKE:
 	case V4L2_CID_FLASH_STROBE:
 	case V4L2_CID_FLASH_STROBE_STOP:
 		*type = V4L2_CTRL_TYPE_BUTTON;
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index f85ad6c..ddd775f 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1647,6 +1647,7 @@ enum v4l2_scenemode {
 };
 
 #define V4L2_CID_WDR				(V4L2_CID_CAMERA_CLASS_BASE+21)
+#define V4L2_CID_ANTISHAKE			(V4L2_CID_CAMERA_CLASS_BASE+22)
 
 /* FM Modulator class control IDs */
 #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
-- 
1.7.4.1

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 <riverful.kim@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  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.</entry>
>  	  </row>
>  	  <row><entry></entry></row>
>  
> +	  <row id="v4l2-preset-white-balance">
> +	    <entry spanname="id"><constant>V4L2_CID_PRESET_WHITE_BALANCE</constant>&nbsp;</entry>

Wouldn't V4L2_CID_WHITE_BALANCE_PRESET be better ?

> +	    <entry>enum&nbsp;v4l2_preset_white_balance</entry>
> +	  </row><row><entry spanname="descr">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.</entry>

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

> +	  </row>
> +	  <row>
> +	    <entrytbl spanname="descr" cols="2">
> +	      <tbody valign="top">
> +		<row>
> +		  <entry><constant>V4L2_WHITE_BALANCE_INCANDESCENT</constant>&nbsp;</entry>
> +		  <entry>White Balance Incandescent/Tungster preset.</entry>

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.

> +		</row>
> +		<row>
> +		  <entry><constant>V4L2_WHITE_BALANCE_FLUORESCENT</constant>&nbsp;</entry>
> +		  <entry>White Balance Fluorescent preset.</entry>
> +		</row>
> +		<row>
> +		  <entry><constant>V4L2_WHITE_BALANCE_DAYLIGHT</constant>&nbsp;</entry>
> +		  <entry>White Balance Daylight preset.</entry>
> +		</row>
> +		<row>
> +		  <entry><constant>V4L2_WHITE_BALANCE_CLOUDY</constant>&nbsp;</entry>
> +		  <entry>White Balance Cloudy preset.</entry>
> +		</row>
> +		<row>
> +		  <entry><constant>V4L2_WHITE_BALANCE_SHADE</constant>&nbsp;</entry>
> +		  <entry>White Balance Share preset.</entry>

s/Share/Shade

> +		</row>
> +	      </tbody>
> +	    </entrytbl>
> +	  </row>
> +	  <row><entry></entry></row>
> +
>  	  <row>
>  	    <entry spanname="id"><constant>V4L2_CID_PRIVACY</constant>&nbsp;</entry>
>  	    <entry>boolean</entry>
> 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",
>  		"50 useconds",
> @@ -390,6 +398,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		return camera_exposure_auto;
>  	case V4L2_CID_COLORFX:
>  		return colorfx;
> +	case V4L2_CID_PRESET_WHITE_BALANCE:
> +		return preset_white_balance;
>  	case V4L2_CID_TUNE_PREEMPHASIS:
>  		return tune_preemphasis;
>  	case V4L2_CID_FLASH_LED_MODE:
> @@ -567,6 +577,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_PRIVACY:			return "Privacy";
>  	case V4L2_CID_IRIS_ABSOLUTE:		return "Iris, Absolute";
>  	case V4L2_CID_IRIS_RELATIVE:		return "Iris, Relative";
> +	case V4L2_CID_PRESET_WHITE_BALANCE:	return "White Balance, Preset";
>  
>  	/* FM Radio Modulator control */
>  	/* Keep the order of the 'case's the same as in videodev2.h! */
> @@ -680,6 +691,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_MPEG_STREAM_VBI_FMT:
>  	case V4L2_CID_EXPOSURE_AUTO:
>  	case V4L2_CID_COLORFX:
> +	case V4L2_CID_PRESET_WHITE_BALANCE:
>  	case V4L2_CID_TUNE_PREEMPHASIS:
>  	case V4L2_CID_FLASH_LED_MODE:
>  	case V4L2_CID_FLASH_STROBE_SOURCE:
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index 3d62631..a842de0 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -1618,6 +1618,15 @@ enum  v4l2_exposure_auto_type {
>  #define V4L2_CID_IRIS_ABSOLUTE			(V4L2_CID_CAMERA_CLASS_BASE+17)
>  #define V4L2_CID_IRIS_RELATIVE			(V4L2_CID_CAMERA_CLASS_BASE+18)
>  
> +#define V4L2_CID_PRESET_WHITE_BALANCE		(V4L2_CID_CAMERA_CLASS_BASE+19)
> +enum v4l2_preset_white_balance {
> +	V4L2_WHITE_BALANCE_INCANDESCENT = 0,
> +	V4L2_WHITE_BALANCE_FLUORESCENT = 1,
> +	V4L2_WHITE_BALANCE_DAYLIGHT = 2,
> +	V4L2_WHITE_BALANCE_CLOUDY = 3,
> +	V4L2_WHITE_BALANCE_SHADE = 4,
> +};
> +
>  /* FM Modulator class control IDs */
>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
>  #define V4L2_CID_FM_TX_CLASS			(V4L2_CTRL_CLASS_FM_TX | 1)

--

Thanks,
Sylwester

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.</entry>
> > 
> >  	  </row>
> >  	  <row><entry></entry></row>
> > 
> > +	  <row id="v4l2-preset-white-balance">
> > +	    <entry
> > spanname="id"><constant>V4L2_CID_PRESET_WHITE_BALANCE</constant>&nbsp;</
> > entry>
> 
> Wouldn't V4L2_CID_WHITE_BALANCE_PRESET be better ?

That's what I was about to say.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 2/4] v4l: Add V4L2_CID_SCENEMODE menu control

[Laurent Pinchart]

Hi,

On Wednesday 28 December 2011 07:23:46 HeungJun, Kim wrote:
> It adds the new CID for setting Scenemode. This CID is provided as
> menu type using the following items:
> enum v4l2_scenemode {
> 	V4L2_SCENEMODE_NONE = 0,
> 	V4L2_SCENEMODE_NORMAL = 1,
> 	V4L2_SCENEMODE_PORTRAIT = 2,
> 	V4L2_SCENEMODE_LANDSCAPE = 3,
> 	V4L2_SCENEMODE_SPORTS = 4,
> 	V4L2_SCENEMODE_PARTY_INDOOR = 5,
> 	V4L2_SCENEMODE_BEACH_SNOW = 6,
> 	V4L2_SCENEMODE_SUNSET = 7,
> 	V4L2_SCENEMODE_DAWN_DUSK = 8,
> 	V4L2_SCENEMODE_FALL = 9,
> 	V4L2_SCENEMODE_NIGHT = 10,
> 	V4L2_SCENEMODE_AGAINST_LIGHT = 11,
> 	V4L2_SCENEMODE_FIRE = 12,
> 	V4L2_SCENEMODE_TEXT = 13,
> 	V4L2_SCENEMODE_CANDLE = 14,
> };
> 
> Signed-off-by: HeungJun, Kim <riverful.kim@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  Documentation/DocBook/media/v4l/controls.xml |   88
> ++++++++++++++++++++++++++ drivers/media/video/v4l2-ctrls.c             | 
>  21 ++++++
>  include/linux/videodev2.h                    |   19 ++++++
>  3 files changed, 128 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/controls.xml
> b/Documentation/DocBook/media/v4l/controls.xml index 350c138..afe1845
> 100644
> --- a/Documentation/DocBook/media/v4l/controls.xml
> +++ b/Documentation/DocBook/media/v4l/controls.xml
> @@ -2879,6 +2879,94 @@ it one step further. This is a write-only
> control.</entry> </row>
>  	  <row><entry></entry></row>
> 
> +	  <row id="v4l2-scenemode">
> +	    <entry
> spanname="id"><constant>V4L2_CID_SCENEMODE</constant>&nbsp;</entry> +	   
> <entry>enum&nbsp;v4l2_scenemode</entry>
> +	  </row><row><entry spanname="descr">This control sets
> +	  the camera's scenemode, and it is provided by the type of
> +	  the enum values. The "None" mode means the status
> +	  when scenemode algorithm is not activated, like after booting time.
> +	  On the other hand, the "Normal" mode means the scenemode algorithm
> +	  is activated on the normal mode.</entry>

What low-level parameters do the scene mode control ? How does it interact 
with the related controls ?

> +	  </row>
> +	  <row>
> +	    <entrytbl spanname="descr" cols="2">
> +	      <tbody valign="top">
> +		<row>
> +		  <entry><constant>V4L2_SCENEMODE_NONE</constant>&nbsp;</entry>
> +		  <entry>Scenemode None.</entry>
> +		</row>
> +		<row>
> +		  <entry><constant>V4L2_SCENEMODE_NORMAL</constant>&nbsp;</entry>
> +		  <entry>Scenemode Normal.</entry>
> +		</row>
> +		<row>
> +		  <entry><constant>V4L2_SCENEMODE_PORTRAIT</constant>&nbsp;</entry>
> +		  <entry>Scenemode Portrait.</entry>

Could you please describe the scene modes in more details ?

> +		</row>
> +		<row>
> +		  <entry><constant>V4L2_SCENEMODE_LANDSCAPE</constant>&nbsp;</entry>
> +		  <entry>Scenemode Landscape.</entry>
> +		</row>
> +		<row>
> +		  <entry><constant>V4L2_SCENEMODE_SPORTS</constant>&nbsp;</entry>
> +		  <entry>Scenemode Sports.</entry>
> +		</row>
> +		<row>
> +		  <entry><constant>V4L2_SCENEMODE_PARTY_INDOOR</constant>&nbsp;</entry>
> +		  <entry>Scenemode Party Indoor.</entry>
> +		</row>
> +		<row>
> +		  <entry><constant>V4L2_SCENEMODE_BEACH_SNOW</constant>&nbsp;</entry>
> +		  <entry>Scenemode Beach Snow.</entry>
> +		</row>
> +		<row>
> +		  <entry><constant>V4L2_SCENEMODE_SUNSET</constant>&nbsp;</entry>
> +		  <entry>Scenemode Beach Snow.</entry>
> +		</row>
> +		<row>
> +		  <entry><constant>V4L2_SCENEMODE_DAWN_DUSK</constant>&nbsp;</entry>
> +		  <entry>Scenemode Dawn Dusk.</entry>
> +		</row>
> +		<row>
> +		  <entry><constant>V4L2_SCENEMODE_FALL</constant>&nbsp;</entry>
> +		  <entry>Scenemode Fall.</entry>
> +		</row>
> +		<row>
> +		  <entry><constant>V4L2_SCENEMODE_NIGHT</constant>&nbsp;</entry>
> +		  <entry>Scenemode Night.</entry>
> +		</row>
> +		<row>
> +		  <entry><constant>V4L2_SCENEMODE_AGAINST_LIGHT</constant>&nbsp;</entry>
> +		  <entry>Scenemode Against Light.</entry>
> +		</row>
> +		<row>
> +		  <entry><constant>V4L2_SCENEMODE_FIRE</constant>&nbsp;</entry>
> +		  <entry>Scenemode Fire.</entry>
> +		</row>
> +		<row>
> +		  <entry><constant>V4L2_SCENEMODE_TEXT</constant>&nbsp;</entry>
> +		  <entry>Scenemode Text.</entry>
> +		</row>
> +		<row>
> +		  <entry><constant>V4L2_SCENEMODE_CANDLE</constant>&nbsp;</entry>
> +		  <entry>Scenemode Candle.</entry>
> +		</row>
> +	      </tbody>
> +	    </entrytbl>
> +	  </row>
> +	  <row><entry></entry></row>
> +
> +	  <row>
> +	    <entry
> spanname="id"><constant>V4L2_CID_PRIVACY</constant>&nbsp;</entry> +	   
> <entry>boolean</entry>
> +	  </row><row><entry spanname="descr">Prevent video from being acquired
> +by the camera. When this control is set to <constant>TRUE</constant> (1),
> no +image can be captured by the camera. Common means to enforce privacy
> are +mechanical obturation of the sensor and firmware image processing,
> but the +device is not restricted to these methods. Devices that implement
> the privacy +control must support read access and may support write
> access.</entry> +	  </row>
>  	  <row>
>  	    <entry
> spanname="id"><constant>V4L2_CID_PRIVACY</constant>&nbsp;</entry>
> <entry>boolean</entry>

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 3/4] v4l: Add V4L2_CID_WDR button control

[Laurent Pinchart]

hi,

On Wednesday 28 December 2011 07:23:47 HeungJun, Kim wrote:
> It adds the new CID for setting White Balance Preset. This CID is provided

I suppose you mean wide dynamic range here.

> as button type. This can commands only if the camera turn on/off this
> function.

Shouldn't it be a boolean ? A button can only be activated, for one-shot auto-
focus for instance.

> Signed-off-by: HeungJun, Kim <riverful.kim@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  Documentation/DocBook/media/v4l/controls.xml |   12 ++++++++++++
>  drivers/media/video/v4l2-ctrls.c             |    2 ++
>  include/linux/videodev2.h                    |    2 ++
>  3 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/controls.xml
> b/Documentation/DocBook/media/v4l/controls.xml index afe1845..bed6c66
> 100644
> --- a/Documentation/DocBook/media/v4l/controls.xml
> +++ b/Documentation/DocBook/media/v4l/controls.xml
> @@ -2958,6 +2958,18 @@ it one step further. This is a write-only
> control.</entry> <row><entry></entry></row>
> 
>  	  <row>
> +	    <entry spanname="id"><constant>V4L2_CID_WDR</constant></entry>
> +	    <entry>button</entry>
> +	  </row>
> +	  <row>
> +	    <entry spanname="descr">Wide Dynamic Range. It makes
> +	    the image be more clear by adjusting the image's intensity
> +	    of the illumination. This function can be provided according to
> +	    the capability of the hardware(sensor or AP's multimedia block).
> +	    </entry>
> +	  </row>
> +
> +	  <row>
>  	    <entry
> spanname="id"><constant>V4L2_CID_PRIVACY</constant>&nbsp;</entry>
> <entry>boolean</entry>
>  	  </row><row><entry spanname="descr">Prevent video from being acquired
> diff --git a/drivers/media/video/v4l2-ctrls.c
> b/drivers/media/video/v4l2-ctrls.c index fef58c2..66110bc 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -598,6 +598,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_IRIS_RELATIVE:		return "Iris, Relative";
>  	case V4L2_CID_PRESET_WHITE_BALANCE:	return "White Balance, Preset";
>  	case V4L2_CID_SCENEMODE:		return "Scenemode";
> +	case V4L2_CID_WDR:			return "Wide Dynamic Range";
> 
>  	/* FM Radio Modulator control */
>  	/* Keep the order of the 'case's the same as in videodev2.h! */
> @@ -687,6 +688,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
> v4l2_ctrl_type *type, break;
>  	case V4L2_CID_PAN_RESET:
>  	case V4L2_CID_TILT_RESET:
> +	case V4L2_CID_WDR:
>  	case V4L2_CID_FLASH_STROBE:
>  	case V4L2_CID_FLASH_STROBE_STOP:
>  		*type = V4L2_CTRL_TYPE_BUTTON;
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index bc14feb..f85ad6c 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -1646,6 +1646,8 @@ enum v4l2_scenemode {
>  	V4L2_SCENEMODE_CANDLE = 14,
>  };
> 
> +#define V4L2_CID_WDR				(V4L2_CID_CAMERA_CLASS_BASE+21)
> +
>  /* FM Modulator class control IDs */
>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
>  #define V4L2_CID_FM_TX_CLASS			(V4L2_CTRL_CLASS_FM_TX | 1)

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 4/4] v4l: Add V4L2_CID_ANTISHAKE button control

[Laurent Pinchart]

Hi,

On Wednesday 28 December 2011 07:23:48 HeungJun, Kim wrote:
> It adds the new CID for setting Anti-shake. This CID is provided as
> button type. This can commands only if the camera turn on/off this
> function.

Shouldn't it be a boolean control ?

> Signed-off-by: HeungJun, Kim <riverful.kim@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  Documentation/DocBook/media/v4l/controls.xml |   11 +++++++++++
>  drivers/media/video/v4l2-ctrls.c             |    2 ++
>  include/linux/videodev2.h                    |    1 +
>  3 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/controls.xml
> b/Documentation/DocBook/media/v4l/controls.xml index bed6c66..73ff05c
> 100644
> --- a/Documentation/DocBook/media/v4l/controls.xml
> +++ b/Documentation/DocBook/media/v4l/controls.xml
> @@ -2970,6 +2970,17 @@ it one step further. This is a write-only
> control.</entry> </row>
> 
>  	  <row>
> +	    <entry spanname="id"><constant>V4L2_CID_ANTISHAKE</constant></entry>
> +	    <entry>button</entry>
> +	  </row>
> +	  <row>
> +	    <entry spanname="descr">Anti-Shake. It makes
> +	    the image be more stabilized from the image's shakeness.
> +	    This function can be provided according to the capability
> +	    of the hardware(sensor or AP's multimedia block).</entry>

Isn't this usually called image (or video) stabilization ? What stabilization 
techniques does this control cover ?

> +	  </row>
> +
> +	  <row>
>  	    <entry
> spanname="id"><constant>V4L2_CID_PRIVACY</constant>&nbsp;</entry>
> <entry>boolean</entry>
>  	  </row><row><entry spanname="descr">Prevent video from being acquired
> diff --git a/drivers/media/video/v4l2-ctrls.c
> b/drivers/media/video/v4l2-ctrls.c index 66110bc..05ff6bb 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -599,6 +599,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_PRESET_WHITE_BALANCE:	return "White Balance, Preset";
>  	case V4L2_CID_SCENEMODE:		return "Scenemode";
>  	case V4L2_CID_WDR:			return "Wide Dynamic Range";
> +	case V4L2_CID_ANTISHAKE:		return "Antishake";
> 
>  	/* FM Radio Modulator control */
>  	/* Keep the order of the 'case's the same as in videodev2.h! */
> @@ -689,6 +690,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
> v4l2_ctrl_type *type, case V4L2_CID_PAN_RESET:
>  	case V4L2_CID_TILT_RESET:
>  	case V4L2_CID_WDR:
> +	case V4L2_CID_ANTISHAKE:
>  	case V4L2_CID_FLASH_STROBE:
>  	case V4L2_CID_FLASH_STROBE_STOP:
>  		*type = V4L2_CTRL_TYPE_BUTTON;
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index f85ad6c..ddd775f 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -1647,6 +1647,7 @@ enum v4l2_scenemode {
>  };
> 
>  #define V4L2_CID_WDR				(V4L2_CID_CAMERA_CLASS_BASE+21)
> +#define V4L2_CID_ANTISHAKE			(V4L2_CID_CAMERA_CLASS_BASE+22)
> 
>  /* FM Modulator class control IDs */
>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 0/4] Add some new camera controls

[Laurent Pinchart]

Hi,

On Wednesday 28 December 2011 07:23:44 HeungJun, Kim wrote:
> Hi everyone,
> 
> This RFC patch series include new 4 controls ID for digital camera.
> I about to suggest these controls by the necessity enabling the M-5MOLS
> sensor's function, and I hope to discuss this in here.

Thanks for the patches.

The new controls introduced by these patches are very high level. Should they 
be put in their own class ? I also think we should specify how those high-
level controls interact with low-level controls, otherwise applications will 
likely get very confused.

> Any opinions and thoughts are very welcome!
> 
> It's good to connect Sylwester's suggestion for discussing.
> - http://www.mail-archive.com/linux-media@vger.kernel.org/msg39907.html
> 
> But it's no problem even if it is considered as seperated subject.
> 
> 1. White Balance Peset
> ======================
> 
> Some camera hardware provides its own preset of white balance,
> but fortunately the names of these presets are similar with the others.
> So, I thought it can be provided as a generic digital camera API.
> I suggest the following as items:
> 
> enum v4l2_preset_white_balance {
> 	V4L2_WHITE_BALANCE_INCANDESCENT = 0,
> 	V4L2_WHITE_BALANCE_FLUORESCENT = 1,
> 	V4L2_WHITE_BALANCE_DAYLIGHT = 2,
> 	V4L2_WHITE_BALANCE_CLOUDY = 3,
> 	V4L2_WHITE_BALANCE_SHADE = 4,
> };
> 
> 2. Scenemode
> ============
> 
> I had suggested it before. :
> http://www.mail-archive.com/linux-media@vger.kernel.org/msg29917.html
> 
> And I want to continue this subject on this threads.
> 
> The scenemode is also needed in the mobile digital .
> The reason I about to suggest this function as CID,
> is also the items are used widely & generally.
> 
> enum v4l2_scenemode {
> 	V4L2_SCENEMODE_NONE = 0,
> 	V4L2_SCENEMODE_NORMAL = 1,
> 	V4L2_SCENEMODE_PORTRAIT = 2,
> 	V4L2_SCENEMODE_LANDSCAPE = 3,
> 	V4L2_SCENEMODE_SPORTS = 4,
> 	V4L2_SCENEMODE_PARTY_INDOOR = 5,
> 	V4L2_SCENEMODE_BEACH_SNOW = 6,
> 	V4L2_SCENEMODE_SUNSET = 7,
> 	V4L2_SCENEMODE_DAWN_DUSK = 8,
> 	V4L2_SCENEMODE_FALL = 9,
> 	V4L2_SCENEMODE_NIGHT = 10,
> 	V4L2_SCENEMODE_AGAINST_LIGHT = 11,
> 	V4L2_SCENEMODE_FIRE = 12,
> 	V4L2_SCENEMODE_TEXT = 13,
> 	V4L2_SCENEMODE_CANDLE = 14,
> };
> 
> 3. WDR(Wide Dynamic Range)
> ==========================
> 
> This function can be unfamiliar, but it is as known as HDR(High Dynamic
> Range) to iPhone users. Although the name is different, but both are the
> same function.
> 
> It makes the image look more clear by adjusting the intensity area of
> illumination of the image. This function can be used only turn on/off
> like button control, then the actual WDR algorithm are activated in the
> hardware.
> 
> 4. Antishake
> ============
> 
> This function compensate and stabilize the shakeness of the stream and
> image. So, if this function turned on, the image created without many
> shakeness. It means both, the case when compensating the stream's
> shakeness,
> and when stabilizing the image itself.
> 
> 5. References
> =============
> 
> - This is the example of the various digital camera's upper controls.
> You can find that the term of each control is very similiar.
> 
> @ White Balance Preset
> http://imaging.nikon.com/history/basics/17/index.htm
> http://www.dailyphotographytips.net/camera-controls-and-settings/how-to-set
> -custom-white-balance/
> http://www.digitalcamera-hq.com/articles/how-to-white-balance-your-camera
> http://www.digital-photography-school.com/introduction-to-white-balance
> 
> @ Scenemode
> http://www.digital-photography-school.com/digital-camera-modes
> http://www.picturecorrect.com/tips/digital-camera-scene-modes/
> 
> @ WDR and HDR
> http://en.wikipedia.org/wiki/High_dynamic_range_imaging
> http://en.wikipedia.org/wiki/Wide_dynamic_range
> 
> @ Ahtishake
> http://www.digital-slr-guide.com/digital-slr-anti-shake.html

-- 
Regards,

Laurent Pinchart

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-owner@vger.kernel.org [mailto:linux-media-
> owner@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; mchehab@redhat.com; hverkuil@xs4all.nl;
> sakari.ailus@iki.fi; laurent.pinchart@ideasonboard.com;
> kyungmin.park@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 <riverful.kim@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >  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.</entry>
> >  	  </row>
> >  	  <row><entry></entry></row>
> >
> > +	  <row id="v4l2-preset-white-balance">
> > +	    <entry
> spanname="id"><constant>V4L2_CID_PRESET_WHITE_BALANCE</constant>&nbsp;</entry>
> 
> Wouldn't V4L2_CID_WHITE_BALANCE_PRESET be better ?
> 
> > +	    <entry>enum&nbsp;v4l2_preset_white_balance</entry>
> > +	  </row><row><entry spanname="descr">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.</entry>
> 
> We probably only need such a remark if a control is read-only or write-only.
> 
> > +	  </row>
> > +	  <row>
> > +	    <entrytbl spanname="descr" cols="2">
> > +	      <tbody valign="top">
> > +		<row>
> > +
> <entry><constant>V4L2_WHITE_BALANCE_INCANDESCENT</constant>&nbsp;</entry>
> > +		  <entry>White Balance Incandescent/Tungster preset.</entry>
> 
> 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.
> 
> > +		</row>
> > +		<row>
> > +
> <entry><constant>V4L2_WHITE_BALANCE_FLUORESCENT</constant>&nbsp;</entry>
> > +		  <entry>White Balance Fluorescent preset.</entry>
> > +		</row>
> > +		<row>
> > +
> <entry><constant>V4L2_WHITE_BALANCE_DAYLIGHT</constant>&nbsp;</entry>
> > +		  <entry>White Balance Daylight preset.</entry>
> > +		</row>
> > +		<row>
> > +
> <entry><constant>V4L2_WHITE_BALANCE_CLOUDY</constant>&nbsp;</entry>
> > +		  <entry>White Balance Cloudy preset.</entry>
> > +		</row>
> > +		<row>
> > +
> <entry><constant>V4L2_WHITE_BALANCE_SHADE</constant>&nbsp;</entry>
> > +		  <entry>White Balance Share preset.</entry>
> 
> s/Share/Shade
> 
> > +		</row>
> > +	      </tbody>
> > +	    </entrytbl>
> > +	  </row>
> > +	  <row><entry></entry></row>
> > +
> >  	  <row>
> >  	    <entry
> spanname="id"><constant>V4L2_CID_PRIVACY</constant>&nbsp;</entry>
> >  	    <entry>boolean</entry>
> > 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",
> >  		"50 useconds",
> > @@ -390,6 +398,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> >  		return camera_exposure_auto;
> >  	case V4L2_CID_COLORFX:
> >  		return colorfx;
> > +	case V4L2_CID_PRESET_WHITE_BALANCE:
> > +		return preset_white_balance;
> >  	case V4L2_CID_TUNE_PREEMPHASIS:
> >  		return tune_preemphasis;
> >  	case V4L2_CID_FLASH_LED_MODE:
> > @@ -567,6 +577,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> >  	case V4L2_CID_PRIVACY:			return "Privacy";
> >  	case V4L2_CID_IRIS_ABSOLUTE:		return "Iris, Absolute";
> >  	case V4L2_CID_IRIS_RELATIVE:		return "Iris, Relative";
> > +	case V4L2_CID_PRESET_WHITE_BALANCE:	return "White Balance, Preset";
> >
> >  	/* FM Radio Modulator control */
> >  	/* Keep the order of the 'case's the same as in videodev2.h! */
> > @@ -680,6 +691,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
> v4l2_ctrl_type *type,
> >  	case V4L2_CID_MPEG_STREAM_VBI_FMT:
> >  	case V4L2_CID_EXPOSURE_AUTO:
> >  	case V4L2_CID_COLORFX:
> > +	case V4L2_CID_PRESET_WHITE_BALANCE:
> >  	case V4L2_CID_TUNE_PREEMPHASIS:
> >  	case V4L2_CID_FLASH_LED_MODE:
> >  	case V4L2_CID_FLASH_STROBE_SOURCE:
> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > index 3d62631..a842de0 100644
> > --- a/include/linux/videodev2.h
> > +++ b/include/linux/videodev2.h
> > @@ -1618,6 +1618,15 @@ enum  v4l2_exposure_auto_type {
> >  #define V4L2_CID_IRIS_ABSOLUTE			(V4L2_CID_CAMERA_CLASS_BASE+17)
> >  #define V4L2_CID_IRIS_RELATIVE			(V4L2_CID_CAMERA_CLASS_BASE+18)
> >
> > +#define V4L2_CID_PRESET_WHITE_BALANCE		(V4L2_CID_CAMERA_CLASS_BASE+19)
> > +enum v4l2_preset_white_balance {
> > +	V4L2_WHITE_BALANCE_INCANDESCENT = 0,
> > +	V4L2_WHITE_BALANCE_FLUORESCENT = 1,
> > +	V4L2_WHITE_BALANCE_DAYLIGHT = 2,
> > +	V4L2_WHITE_BALANCE_CLOUDY = 3,
> > +	V4L2_WHITE_BALANCE_SHADE = 4,
> > +};
> > +
> >  /* FM Modulator class control IDs */
> >  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
> >  #define V4L2_CID_FM_TX_CLASS			(V4L2_CTRL_CLASS_FM_TX | 1)
> 
> --
> 
> Thanks,
> Sylwester
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@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-owner@vger.kernel.org [mailto:linux-media-
> owner@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; mchehab@redhat.com;
> hverkuil@xs4all.nl; sakari.ailus@iki.fi; kyungmin.park@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
> > > +++ 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>&nbsp;</
> > > 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".

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

RE: [RFC PATCH 2/4] v4l: Add V4L2_CID_SCENEMODE menu control

[HeungJun, Kim]

Hi Laurent,

> -----Original Message-----
> From: linux-media-owner@vger.kernel.org [mailto:linux-media-
> owner@vger.kernel.org] On Behalf Of Laurent Pinchart
> Sent: Wednesday, December 28, 2011 10:56 PM
> To: HeungJun, Kim
> Cc: linux-media@vger.kernel.org; mchehab@redhat.com; hverkuil@xs4all.nl;
> sakari.ailus@iki.fi; s.nawrocki@samsung.com; kyungmin.park@samsung.com
> Subject: Re: [RFC PATCH 2/4] v4l: Add V4L2_CID_SCENEMODE menu control
> 
> Hi,
> 
> On Wednesday 28 December 2011 07:23:46 HeungJun, Kim wrote:
> > It adds the new CID for setting Scenemode. This CID is provided as
> > menu type using the following items:
> > enum v4l2_scenemode {
> > 	V4L2_SCENEMODE_NONE = 0,
> > 	V4L2_SCENEMODE_NORMAL = 1,
> > 	V4L2_SCENEMODE_PORTRAIT = 2,
> > 	V4L2_SCENEMODE_LANDSCAPE = 3,
> > 	V4L2_SCENEMODE_SPORTS = 4,
> > 	V4L2_SCENEMODE_PARTY_INDOOR = 5,
> > 	V4L2_SCENEMODE_BEACH_SNOW = 6,
> > 	V4L2_SCENEMODE_SUNSET = 7,
> > 	V4L2_SCENEMODE_DAWN_DUSK = 8,
> > 	V4L2_SCENEMODE_FALL = 9,
> > 	V4L2_SCENEMODE_NIGHT = 10,
> > 	V4L2_SCENEMODE_AGAINST_LIGHT = 11,
> > 	V4L2_SCENEMODE_FIRE = 12,
> > 	V4L2_SCENEMODE_TEXT = 13,
> > 	V4L2_SCENEMODE_CANDLE = 14,
> > };
> >
> > Signed-off-by: HeungJun, Kim <riverful.kim@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >  Documentation/DocBook/media/v4l/controls.xml |   88
> > ++++++++++++++++++++++++++ drivers/media/video/v4l2-ctrls.c             |
> >  21 ++++++
> >  include/linux/videodev2.h                    |   19 ++++++
> >  3 files changed, 128 insertions(+), 0 deletions(-)
> >
> > diff --git a/Documentation/DocBook/media/v4l/controls.xml
> > b/Documentation/DocBook/media/v4l/controls.xml index 350c138..afe1845
> > 100644
> > --- a/Documentation/DocBook/media/v4l/controls.xml
> > +++ b/Documentation/DocBook/media/v4l/controls.xml
> > @@ -2879,6 +2879,94 @@ it one step further. This is a write-only
> > control.</entry> </row>
> >  	  <row><entry></entry></row>
> >
> > +	  <row id="v4l2-scenemode">
> > +	    <entry
> > spanname="id"><constant>V4L2_CID_SCENEMODE</constant>&nbsp;</entry> +
> > <entry>enum&nbsp;v4l2_scenemode</entry>
> > +	  </row><row><entry spanname="descr">This control sets
> > +	  the camera's scenemode, and it is provided by the type of
> > +	  the enum values. The "None" mode means the status
> > +	  when scenemode algorithm is not activated, like after booting time.
> > +	  On the other hand, the "Normal" mode means the scenemode algorithm
> > +	  is activated on the normal mode.</entry>
> 
> What low-level parameters do the scene mode control ? How does it interact
> with the related controls ?
For using this control, in M-5MOLS sensor case, several register is configured,
like Exposure(locking/Indexed preset/mode), Focus(locking), WhiteBalance, etc.
And I think the process interacting and syncing the register's value should be
up to the each drivers, and the m5mols driver will be.

Anyways, could you explain the difference with low- and high- in more details?
:)
I still did not understand well.

> 
> > +	  </row>
> > +	  <row>
> > +	    <entrytbl spanname="descr" cols="2">
> > +	      <tbody valign="top">
> > +		<row>
> > +		  <entry><constant>V4L2_SCENEMODE_NONE</constant>&nbsp;</entry>
> > +		  <entry>Scenemode None.</entry>
> > +		</row>
> > +		<row>
> > +
<entry><constant>V4L2_SCENEMODE_NORMAL</constant>&nbsp;</entry>
> > +		  <entry>Scenemode Normal.</entry>
> > +		</row>
> > +		<row>
> > +
> <entry><constant>V4L2_SCENEMODE_PORTRAIT</constant>&nbsp;</entry>
> > +		  <entry>Scenemode Portrait.</entry>
> 
> Could you please describe the scene modes in more details ?
Yes, the photographer should adjust the exposure(luminance), focus, iso, etc,
for getting better image according to each specific circumstances.
But, it's uncomfortable to set all controls at every time.
The scene mode can make this at one time. It is just not only setting once.
According to fixed scene circumstance, the internal algorithm is also
needed for it. 

This function is usually provided as just preset, but, it's not just preset,
because the specific algorithm is existed for "scene mode",
except the collection of the camera control preset.

The enumeration I suggest, can cover almost scene mode. Of course, the term
is fixed. The M-5MOLS can use all the scene mode.
Please, see drivers/media/video/m5mols/m5mols_contro.c.

> 
> > +		</row>
> > +		<row>
> > +
> <entry><constant>V4L2_SCENEMODE_LANDSCAPE</constant>&nbsp;</entry>
> > +		  <entry>Scenemode Landscape.</entry>
> > +		</row>
> > +		<row>
> > +
<entry><constant>V4L2_SCENEMODE_SPORTS</constant>&nbsp;</entry>
> > +		  <entry>Scenemode Sports.</entry>
> > +		</row>
> > +		<row>
> > +
> <entry><constant>V4L2_SCENEMODE_PARTY_INDOOR</constant>&nbsp;</entry>
> > +		  <entry>Scenemode Party Indoor.</entry>
> > +		</row>
> > +		<row>
> > +
> <entry><constant>V4L2_SCENEMODE_BEACH_SNOW</constant>&nbsp;</entry>
> > +		  <entry>Scenemode Beach Snow.</entry>
> > +		</row>
> > +		<row>
> > +
<entry><constant>V4L2_SCENEMODE_SUNSET</constant>&nbsp;</entry>
> > +		  <entry>Scenemode Beach Snow.</entry>
> > +		</row>
> > +		<row>
> > +
> <entry><constant>V4L2_SCENEMODE_DAWN_DUSK</constant>&nbsp;</entry>
> > +		  <entry>Scenemode Dawn Dusk.</entry>
> > +		</row>
> > +		<row>
> > +		  <entry><constant>V4L2_SCENEMODE_FALL</constant>&nbsp;</entry>
> > +		  <entry>Scenemode Fall.</entry>
> > +		</row>
> > +		<row>
> > +		  <entry><constant>V4L2_SCENEMODE_NIGHT</constant>&nbsp;</entry>
> > +		  <entry>Scenemode Night.</entry>
> > +		</row>
> > +		<row>
> > +
> <entry><constant>V4L2_SCENEMODE_AGAINST_LIGHT</constant>&nbsp;</entry>
> > +		  <entry>Scenemode Against Light.</entry>
> > +		</row>
> > +		<row>
> > +		  <entry><constant>V4L2_SCENEMODE_FIRE</constant>&nbsp;</entry>
> > +		  <entry>Scenemode Fire.</entry>
> > +		</row>
> > +		<row>
> > +		  <entry><constant>V4L2_SCENEMODE_TEXT</constant>&nbsp;</entry>
> > +		  <entry>Scenemode Text.</entry>
> > +		</row>
> > +		<row>
> > +
<entry><constant>V4L2_SCENEMODE_CANDLE</constant>&nbsp;</entry>
> > +		  <entry>Scenemode Candle.</entry>
> > +		</row>
> > +	      </tbody>
> > +	    </entrytbl>
> > +	  </row>
> > +	  <row><entry></entry></row>
> > +
> > +	  <row>
> > +	    <entry
> > spanname="id"><constant>V4L2_CID_PRIVACY</constant>&nbsp;</entry> +
> > <entry>boolean</entry>
> > +	  </row><row><entry spanname="descr">Prevent video from being acquired
> > +by the camera. When this control is set to <constant>TRUE</constant> (1),
> > no +image can be captured by the camera. Common means to enforce privacy
> > are +mechanical obturation of the sensor and firmware image processing,
> > but the +device is not restricted to these methods. Devices that implement
> > the privacy +control must support read access and may support write
> > access.</entry> +	  </row>
> >  	  <row>
> >  	    <entry
> > spanname="id"><constant>V4L2_CID_PRIVACY</constant>&nbsp;</entry>
> > <entry>boolean</entry>
> 
> --
> Regards,
> 
> Laurent Pinchart
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [RFC PATCH 3/4] v4l: Add V4L2_CID_WDR button control

[HeungJun, Kim]

Hi,

> -----Original Message-----
> From: linux-media-owner@vger.kernel.org [mailto:linux-media-
> owner@vger.kernel.org] On Behalf Of Laurent Pinchart
> Sent: Wednesday, December 28, 2011 10:57 PM
> To: HeungJun, Kim
> Cc: linux-media@vger.kernel.org; mchehab@redhat.com; hverkuil@xs4all.nl;
> sakari.ailus@iki.fi; s.nawrocki@samsung.com; kyungmin.park@samsung.com
> Subject: Re: [RFC PATCH 3/4] v4l: Add V4L2_CID_WDR button control
> 
> hi,
> 
> On Wednesday 28 December 2011 07:23:47 HeungJun, Kim wrote:
> > It adds the new CID for setting White Balance Preset. This CID is provided
> 
> I suppose you mean wide dynamic range here.
Right, it's my miss.

> 
> > as button type. This can commands only if the camera turn on/off this
> > function.
> 
> Shouldn't it be a boolean ? A button can only be activated, for one-shot auto-
> focus for instance.
Any type can be possible, and fine to me. But, it depends on the whole hardware
architecture. The WDR is proceeded and used only in the ISP or another engine
processing image. And, the cases I've seen ever, are just one - The ISP exists
in the sensor.

In M-5MOLS use-case, the ISP is in the M-5MOLS sensor. To the position of
developer,
it's just ok to turn on/off for using this. But, in the other architecture
it might be need more.

But, I anticipate if the other architecture use this function, probably
any other setting seems be not needed any more. The photographer just says,
"turn on the WDR!", not says "adjust parm 1, 2, 3, and turn on WDR!". :)

So, IMHO, I think the any other setting is not needed more for now.

> 
> > Signed-off-by: HeungJun, Kim <riverful.kim@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >  Documentation/DocBook/media/v4l/controls.xml |   12 ++++++++++++
> >  drivers/media/video/v4l2-ctrls.c             |    2 ++
> >  include/linux/videodev2.h                    |    2 ++
> >  3 files changed, 16 insertions(+), 0 deletions(-)
> >
> > diff --git a/Documentation/DocBook/media/v4l/controls.xml
> > b/Documentation/DocBook/media/v4l/controls.xml index afe1845..bed6c66
> > 100644
> > --- a/Documentation/DocBook/media/v4l/controls.xml
> > +++ b/Documentation/DocBook/media/v4l/controls.xml
> > @@ -2958,6 +2958,18 @@ it one step further. This is a write-only
> > control.</entry> <row><entry></entry></row>
> >
> >  	  <row>
> > +	    <entry spanname="id"><constant>V4L2_CID_WDR</constant></entry>
> > +	    <entry>button</entry>
> > +	  </row>
> > +	  <row>
> > +	    <entry spanname="descr">Wide Dynamic Range. It makes
> > +	    the image be more clear by adjusting the image's intensity
> > +	    of the illumination. This function can be provided according to
> > +	    the capability of the hardware(sensor or AP's multimedia block).
> > +	    </entry>
> > +	  </row>
> > +
> > +	  <row>
> >  	    <entry
> > spanname="id"><constant>V4L2_CID_PRIVACY</constant>&nbsp;</entry>
> > <entry>boolean</entry>
> >  	  </row><row><entry spanname="descr">Prevent video from being acquired
> > diff --git a/drivers/media/video/v4l2-ctrls.c
> > b/drivers/media/video/v4l2-ctrls.c index fef58c2..66110bc 100644
> > --- a/drivers/media/video/v4l2-ctrls.c
> > +++ b/drivers/media/video/v4l2-ctrls.c
> > @@ -598,6 +598,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> >  	case V4L2_CID_IRIS_RELATIVE:		return "Iris, Relative";
> >  	case V4L2_CID_PRESET_WHITE_BALANCE:	return "White Balance, Preset";
> >  	case V4L2_CID_SCENEMODE:		return "Scenemode";
> > +	case V4L2_CID_WDR:			return "Wide Dynamic Range";
> >
> >  	/* FM Radio Modulator control */
> >  	/* Keep the order of the 'case's the same as in videodev2.h! */
> > @@ -687,6 +688,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
> > v4l2_ctrl_type *type, break;
> >  	case V4L2_CID_PAN_RESET:
> >  	case V4L2_CID_TILT_RESET:
> > +	case V4L2_CID_WDR:
> >  	case V4L2_CID_FLASH_STROBE:
> >  	case V4L2_CID_FLASH_STROBE_STOP:
> >  		*type = V4L2_CTRL_TYPE_BUTTON;
> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > index bc14feb..f85ad6c 100644
> > --- a/include/linux/videodev2.h
> > +++ b/include/linux/videodev2.h
> > @@ -1646,6 +1646,8 @@ enum v4l2_scenemode {
> >  	V4L2_SCENEMODE_CANDLE = 14,
> >  };
> >
> > +#define V4L2_CID_WDR
(V4L2_CID_CAMERA_CLASS_BASE+21)
> > +
> >  /* FM Modulator class control IDs */
> >  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
> >  #define V4L2_CID_FM_TX_CLASS			(V4L2_CTRL_CLASS_FM_TX |
1)
> 
> --
> Regards,
> 
> Laurent Pinchart
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [RFC PATCH 4/4] v4l: Add V4L2_CID_ANTISHAKE button control

[HeungJun, Kim]

Hi, 

> -----Original Message-----
> From: linux-media-owner@vger.kernel.org [mailto:linux-media-
> owner@vger.kernel.org] On Behalf Of Laurent Pinchart
> Sent: Wednesday, December 28, 2011 10:58 PM
> To: HeungJun, Kim
> Cc: linux-media@vger.kernel.org; mchehab@redhat.com; hverkuil@xs4all.nl;
> sakari.ailus@iki.fi; s.nawrocki@samsung.com; kyungmin.park@samsung.com
> Subject: Re: [RFC PATCH 4/4] v4l: Add V4L2_CID_ANTISHAKE button control
> 
> Hi,
> 
> On Wednesday 28 December 2011 07:23:48 HeungJun, Kim wrote:
> > It adds the new CID for setting Anti-shake. This CID is provided as
> > button type. This can commands only if the camera turn on/off this
> > function.
> 
> Shouldn't it be a boolean control ?
It's also similar case with WDR. Yes, for now, I've never seen this function
needs more modes or settings. On status of turning on, the image can be
compensated shaking and blurring.

> 
> > Signed-off-by: HeungJun, Kim <riverful.kim@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >  Documentation/DocBook/media/v4l/controls.xml |   11 +++++++++++
> >  drivers/media/video/v4l2-ctrls.c             |    2 ++
> >  include/linux/videodev2.h                    |    1 +
> >  3 files changed, 14 insertions(+), 0 deletions(-)
> >
> > diff --git a/Documentation/DocBook/media/v4l/controls.xml
> > b/Documentation/DocBook/media/v4l/controls.xml index bed6c66..73ff05c
> > 100644
> > --- a/Documentation/DocBook/media/v4l/controls.xml
> > +++ b/Documentation/DocBook/media/v4l/controls.xml
> > @@ -2970,6 +2970,17 @@ it one step further. This is a write-only
> > control.</entry> </row>
> >
> >  	  <row>
> > +	    <entry spanname="id"><constant>V4L2_CID_ANTISHAKE</constant></entry>
> > +	    <entry>button</entry>
> > +	  </row>
> > +	  <row>
> > +	    <entry spanname="descr">Anti-Shake. It makes
> > +	    the image be more stabilized from the image's shakeness.
> > +	    This function can be provided according to the capability
> > +	    of the hardware(sensor or AP's multimedia block).</entry>
> 
> Isn't this usually called image (or video) stabilization ? What stabilization
> techniques does this control cover ?
> 
> > +	  </row>
> > +
> > +	  <row>
> >  	    <entry
> > spanname="id"><constant>V4L2_CID_PRIVACY</constant>&nbsp;</entry>
> > <entry>boolean</entry>
> >  	  </row><row><entry spanname="descr">Prevent video from being acquired
> > diff --git a/drivers/media/video/v4l2-ctrls.c
> > b/drivers/media/video/v4l2-ctrls.c index 66110bc..05ff6bb 100644
> > --- a/drivers/media/video/v4l2-ctrls.c
> > +++ b/drivers/media/video/v4l2-ctrls.c
> > @@ -599,6 +599,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> >  	case V4L2_CID_PRESET_WHITE_BALANCE:	return "White Balance, Preset";
> >  	case V4L2_CID_SCENEMODE:		return "Scenemode";
> >  	case V4L2_CID_WDR:			return "Wide Dynamic Range";
> > +	case V4L2_CID_ANTISHAKE:		return "Antishake";
> >
> >  	/* FM Radio Modulator control */
> >  	/* Keep the order of the 'case's the same as in videodev2.h! */
> > @@ -689,6 +690,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
> > v4l2_ctrl_type *type, case V4L2_CID_PAN_RESET:
> >  	case V4L2_CID_TILT_RESET:
> >  	case V4L2_CID_WDR:
> > +	case V4L2_CID_ANTISHAKE:
> >  	case V4L2_CID_FLASH_STROBE:
> >  	case V4L2_CID_FLASH_STROBE_STOP:
> >  		*type = V4L2_CTRL_TYPE_BUTTON;
> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > index f85ad6c..ddd775f 100644
> > --- a/include/linux/videodev2.h
> > +++ b/include/linux/videodev2.h
> > @@ -1647,6 +1647,7 @@ enum v4l2_scenemode {
> >  };
> >
> >  #define V4L2_CID_WDR
(V4L2_CID_CAMERA_CLASS_BASE+21)
> > +#define V4L2_CID_ANTISHAKE			(V4L2_CID_CAMERA_CLASS_BASE+22)
> >
> >  /* FM Modulator class control IDs */
> >  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
> 
> --
> Regards,
> 
> Laurent Pinchart
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [RFC PATCH 0/4] Add some new camera controls

[HeungJun, Kim]

Hi Larent,

Thanks for the comments very well, and I replied the other each mails.

> -----Original Message-----
> From: linux-media-owner@vger.kernel.org [mailto:linux-media-
> owner@vger.kernel.org] On Behalf Of Laurent Pinchart
> Sent: Wednesday, December 28, 2011 11:01 PM
> To: HeungJun, Kim
> Cc: linux-media@vger.kernel.org; mchehab@redhat.com; hverkuil@xs4all.nl;
> sakari.ailus@iki.fi; s.nawrocki@samsung.com; kyungmin.park@samsung.com
> Subject: Re: [RFC PATCH 0/4] Add some new camera controls
> 
> Hi,
> 
> On Wednesday 28 December 2011 07:23:44 HeungJun, Kim wrote:
> > Hi everyone,
> >
> > This RFC patch series include new 4 controls ID for digital camera.
> > I about to suggest these controls by the necessity enabling the M-5MOLS
> > sensor's function, and I hope to discuss this in here.
> 
> Thanks for the patches.
> 
> The new controls introduced by these patches are very high level. Should they
> be put in their own class ? I also think we should specify how those high-
> level controls interact with low-level controls, otherwise applications will
> likely get very confused.
I did not consider yet, but I think it's first to define about what low-/high-
is. I think this is not high- level controls. And, honestly, I don't understand
it's really important to categorize low-/high-, or not.

IMHO, The importance is the just complexity of interacting with each modules.
If this means the level of low-/high-, I can understand this.
But I'm wrong, please explain this. :)

There is some different story, I just got the N900 some days ago :).
The purpose is just understanding Nokia and TI OMAP camera architecture well.
Probably, it helps for me to talk more easily, and I'll be able to speak more
well
with omap workers - you and Sakari.

Happy new year!

> 
> > Any opinions and thoughts are very welcome!
> >
> > It's good to connect Sylwester's suggestion for discussing.
> > - http://www.mail-archive.com/linux-media@vger.kernel.org/msg39907.html
> >
> > But it's no problem even if it is considered as seperated subject.
> >
> > 1. White Balance Peset
> > ======================
> >
> > Some camera hardware provides its own preset of white balance,
> > but fortunately the names of these presets are similar with the others.
> > So, I thought it can be provided as a generic digital camera API.
> > I suggest the following as items:
> >
> > enum v4l2_preset_white_balance {
> > 	V4L2_WHITE_BALANCE_INCANDESCENT = 0,
> > 	V4L2_WHITE_BALANCE_FLUORESCENT = 1,
> > 	V4L2_WHITE_BALANCE_DAYLIGHT = 2,
> > 	V4L2_WHITE_BALANCE_CLOUDY = 3,
> > 	V4L2_WHITE_BALANCE_SHADE = 4,
> > };
> >
> > 2. Scenemode
> > ============
> >
> > I had suggested it before. :
> > http://www.mail-archive.com/linux-media@vger.kernel.org/msg29917.html
> >
> > And I want to continue this subject on this threads.
> >
> > The scenemode is also needed in the mobile digital .
> > The reason I about to suggest this function as CID,
> > is also the items are used widely & generally.
> >
> > enum v4l2_scenemode {
> > 	V4L2_SCENEMODE_NONE = 0,
> > 	V4L2_SCENEMODE_NORMAL = 1,
> > 	V4L2_SCENEMODE_PORTRAIT = 2,
> > 	V4L2_SCENEMODE_LANDSCAPE = 3,
> > 	V4L2_SCENEMODE_SPORTS = 4,
> > 	V4L2_SCENEMODE_PARTY_INDOOR = 5,
> > 	V4L2_SCENEMODE_BEACH_SNOW = 6,
> > 	V4L2_SCENEMODE_SUNSET = 7,
> > 	V4L2_SCENEMODE_DAWN_DUSK = 8,
> > 	V4L2_SCENEMODE_FALL = 9,
> > 	V4L2_SCENEMODE_NIGHT = 10,
> > 	V4L2_SCENEMODE_AGAINST_LIGHT = 11,
> > 	V4L2_SCENEMODE_FIRE = 12,
> > 	V4L2_SCENEMODE_TEXT = 13,
> > 	V4L2_SCENEMODE_CANDLE = 14,
> > };
> >
> > 3. WDR(Wide Dynamic Range)
> > ==========================
> >
> > This function can be unfamiliar, but it is as known as HDR(High Dynamic
> > Range) to iPhone users. Although the name is different, but both are the
> > same function.
> >
> > It makes the image look more clear by adjusting the intensity area of
> > illumination of the image. This function can be used only turn on/off
> > like button control, then the actual WDR algorithm are activated in the
> > hardware.
> >
> > 4. Antishake
> > ============
> >
> > This function compensate and stabilize the shakeness of the stream and
> > image. So, if this function turned on, the image created without many
> > shakeness. It means both, the case when compensating the stream's
> > shakeness,
> > and when stabilizing the image itself.
> >
> > 5. References
> > =============
> >
> > - This is the example of the various digital camera's upper controls.
> > You can find that the term of each control is very similiar.
> >
> > @ White Balance Preset
> > http://imaging.nikon.com/history/basics/17/index.htm
> > http://www.dailyphotographytips.net/camera-controls-and-settings/how-to-set
> > -custom-white-balance/
> > http://www.digitalcamera-hq.com/articles/how-to-white-balance-your-camera
> > http://www.digital-photography-school.com/introduction-to-white-balance
> >
> > @ Scenemode
> > http://www.digital-photography-school.com/digital-camera-modes
> > http://www.picturecorrect.com/tips/digital-camera-scene-modes/
> >
> > @ WDR and HDR
> > http://en.wikipedia.org/wiki/High_dynamic_range_imaging
> > http://en.wikipedia.org/wiki/Wide_dynamic_range
> >
> > @ Ahtishake
> > http://www.digital-slr-guide.com/digital-slr-anti-shake.html
> 
> --
> Regards,
> 
> Laurent Pinchart
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@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, 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.</entry>
> > > 
> > >  	  </row>
> > >  	  <row><entry></entry></row>
> > > 
> > > +	  <row id="v4l2-preset-white-balance">
> > > +	    <entry
> > > spanname="id"><constant>V4L2_CID_PRESET_WHITE_BALANCE</constant>&nbsp;</
> > > 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.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

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.</entry>
> > > > 
> > > >  	  </row>
> > > >  	  <row><entry></entry></row>
> > > > 
> > > > +	  <row id="v4l2-preset-white-balance">
> > > > +	    <entry
> > > > spanname="id"><constant>V4L2_CID_PRESET_WHITE_BALANCE</constant>&nbsp
> > > > ;</ 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.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 2/4] v4l: Add V4L2_CID_SCENEMODE menu control

[Laurent Pinchart]

Hi,

On Thursday 29 December 2011 06:40:57 HeungJun, Kim wrote:
> On Wednesday, December 28, 2011 10:56 PM Laurent Pinchart wrote:
> > On Wednesday 28 December 2011 07:23:46 HeungJun, Kim wrote:

[snip]

> > > diff --git a/Documentation/DocBook/media/v4l/controls.xml
> > > b/Documentation/DocBook/media/v4l/controls.xml index 350c138..afe1845
> > > 100644
> > > --- a/Documentation/DocBook/media/v4l/controls.xml
> > > +++ b/Documentation/DocBook/media/v4l/controls.xml
> > > @@ -2879,6 +2879,94 @@ it one step further. This is a write-only
> > > control.</entry> </row>
> > > 
> > >  	  <row><entry></entry></row>
> > > 
> > > +	  <row id="v4l2-scenemode">
> > > +	    <entry
> > > spanname="id"><constant>V4L2_CID_SCENEMODE</constant>&nbsp;</entry> +
> > > <entry>enum&nbsp;v4l2_scenemode</entry>
> > > +	  </row><row><entry spanname="descr">This control sets
> > > +	  the camera's scenemode, and it is provided by the type of
> > > +	  the enum values. The "None" mode means the status
> > > +	  when scenemode algorithm is not activated, like after booting
> > > time.
> > > +	  On the other hand, the "Normal" mode means the scenemode algorithm
> > > +	  is activated on the normal mode.</entry>
> > 
> > What low-level parameters do the scene mode control ? How does it
> > interact with the related controls ?
> 
> For using this control, in M-5MOLS sensor case, several register is
> configured, like Exposure(locking/Indexed preset/mode), Focus(locking),
> WhiteBalance, etc. And I think the process interacting and syncing the
> register's value should be up to the each drivers, and the m5mols driver
> will be.

Does it mean that the scene mode is handled by the driver, which then 
configures exposure, focus, white balance, ... ?

> Anyways, could you explain the difference with low- and high- in more
> details?
> 
> :)
> 
> I still did not understand well.

The concepts are a bit ill-defined currently. We have low-level sensor 
controls such as exposure time, gains, ... The controls you propose offer a 
higher level interface, likely using software-based algorithms running on the 
sensor instead of just applying hardware parameters.

> > > +	  </row>
> > > +	  <row>
> > > +	    <entrytbl spanname="descr" cols="2">
> > > +	      <tbody valign="top">
> > > +		<row>
> > > +		  <entry><constant>V4L2_SCENEMODE_NONE</constant>&nbsp;</entry>
> > > +		  <entry>Scenemode None.</entry>
> > > +		</row>
> > > +		<row>
> > > +
> 
> <entry><constant>V4L2_SCENEMODE_NORMAL</constant>&nbsp;</entry>
> 
> > > +		  <entry>Scenemode Normal.</entry>
> > > +		</row>
> > > +		<row>
> > > +
> > 
> > <entry><constant>V4L2_SCENEMODE_PORTRAIT</constant>&nbsp;</entry>
> > 
> > > +		  <entry>Scenemode Portrait.</entry>
> > 
> > Could you please describe the scene modes in more details ?
> 
> Yes, the photographer should adjust the exposure(luminance), focus, iso,
> etc, for getting better image according to each specific circumstances.
> But, it's uncomfortable to set all controls at every time.
> The scene mode can make this at one time. It is just not only setting once.
> According to fixed scene circumstance, the internal algorithm is also
> needed for it.
> 
> This function is usually provided as just preset, but, it's not just
> preset, because the specific algorithm is existed for "scene mode",
> except the collection of the camera control preset.
> 
> The enumeration I suggest, can cover almost scene mode. Of course, the term
> is fixed. The M-5MOLS can use all the scene mode.
> Please, see drivers/media/video/m5mols/m5mols_contro.c.

What I meant when asking you to explain the scene modes in more details was to 
improve the documentation by adding a detailed description of each scene mode.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 3/4] v4l: Add V4L2_CID_WDR button control

[Laurent Pinchart]

Hi,

On Thursday 29 December 2011 06:52:55 HeungJun, Kim wrote:
> On Wednesday, December 28, 2011 10:57 PM Laurent Pinchart wrote:
> > On Wednesday 28 December 2011 07:23:47 HeungJun, Kim wrote:
> > > It adds the new CID for setting White Balance Preset. This CID is
> > > provided
> > 
> > I suppose you mean wide dynamic range here.
> 
> Right, it's my miss.
> 
> > > as button type. This can commands only if the camera turn on/off this
> > > function.
> > 
> > Shouldn't it be a boolean ? A button can only be activated, for one-shot
> > auto- focus for instance.
> 
> Any type can be possible, and fine to me. But, it depends on the whole
> hardware architecture. The WDR is proceeded and used only in the ISP or
> another engine processing image. And, the cases I've seen ever, are just
> one - The ISP exists in the sensor.
> 
> In M-5MOLS use-case, the ISP is in the M-5MOLS sensor. To the position of
> developer,
> it's just ok to turn on/off for using this. But, in the other architecture
> it might be need more.

You can't turn a button control on or off. A button control can only be 
activated, it has no state. On/off controls are boolean controls.

> But, I anticipate if the other architecture use this function, probably
> any other setting seems be not needed any more. The photographer just says,
> "turn on the WDR!", not says "adjust parm 1, 2, 3, and turn on WDR!". :)
> 
> So, IMHO, I think the any other setting is not needed more for now.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 0/4] Add some new camera controls

[Laurent Pinchart]

Hi,

On Thursday 29 December 2011 07:15:46 HeungJun, Kim wrote:
> On Wednesday, December 28, 2011 11:01 PM Laurent Pinchart wrote:
> > On Wednesday 28 December 2011 07:23:44 HeungJun, Kim wrote:
> > > Hi everyone,
> > > 
> > > This RFC patch series include new 4 controls ID for digital camera.
> > > I about to suggest these controls by the necessity enabling the M-5MOLS
> > > sensor's function, and I hope to discuss this in here.
> > 
> > Thanks for the patches.
> > 
> > The new controls introduced by these patches are very high level. Should
> > they be put in their own class ? I also think we should specify how
> > those high- level controls interact with low-level controls, otherwise
> > applications will likely get very confused.
> 
> I did not consider yet, but I think it's first to define about what
> low-/high- is. I think this is not high- level controls. And, honestly, I
> don't understand it's really important to categorize low-/high-, or not.
> 
> IMHO, The importance is the just complexity of interacting with each
> modules. If this means the level of low-/high-, I can understand this.
> But I'm wrong, please explain this. :)

Low level controls are usually handled in hardware and pretty much self-
contained. High level controls are usually software algorithms (possibly 
running on the sensor itself) that modify low level controls behind the scene.

If a sensor exposes both an exposure time control and a scene mode control 
that modifies exposure time handling, documentation of the scene mode control 
must explain how the two interacts and what applications can and can't do.

> There is some different story, I just got the N900 some days ago :).
> The purpose is just understanding Nokia and TI OMAP camera architecture
> well. Probably, it helps for me to talk more easily, and I'll be able to
> speak more well
> with omap workers - you and Sakari.

-- 
Regards,

Laurent Pinchart

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.</entry>
>>>>>
>>>>>   	</row>
>>>>>   	<row><entry></entry></row>
>>>>>
>>>>> +	<row id="v4l2-preset-white-balance">
>>>>> +	<entry
>>>>> spanname="id"><constant>V4L2_CID_PRESET_WHITE_BALANCE</constant>&nbsp
>>>>> ;</ 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.
>

RE: [RFC PATCH 2/4] v4l: Add V4L2_CID_SCENEMODE menu control

[HeungJun, Kim]

Hi,

> -----Original Message-----
> From: linux-media-owner@vger.kernel.org [mailto:linux-media-
> owner@vger.kernel.org] On Behalf Of Laurent Pinchart
> Sent: Friday, December 30, 2011 9:11 AM
> To: HeungJun, Kim
> Cc: linux-media@vger.kernel.org; mchehab@redhat.com; hverkuil@xs4all.nl;
> sakari.ailus@iki.fi; s.nawrocki@samsung.com; kyungmin.park@samsung.com
> Subject: Re: [RFC PATCH 2/4] v4l: Add V4L2_CID_SCENEMODE menu control
> 
> Hi,
> 
> On Thursday 29 December 2011 06:40:57 HeungJun, Kim wrote:
> > On Wednesday, December 28, 2011 10:56 PM Laurent Pinchart wrote:
> > > On Wednesday 28 December 2011 07:23:46 HeungJun, Kim wrote:
> 
> [snip]
> 
> > > > diff --git a/Documentation/DocBook/media/v4l/controls.xml
> > > > b/Documentation/DocBook/media/v4l/controls.xml index 350c138..afe1845
> > > > 100644
> > > > --- a/Documentation/DocBook/media/v4l/controls.xml
> > > > +++ b/Documentation/DocBook/media/v4l/controls.xml
> > > > @@ -2879,6 +2879,94 @@ it one step further. This is a write-only
> > > > control.</entry> </row>
> > > >
> > > >  	  <row><entry></entry></row>
> > > >
> > > > +	  <row id="v4l2-scenemode">
> > > > +	    <entry
> > > > spanname="id"><constant>V4L2_CID_SCENEMODE</constant>&nbsp;</entry> +
> > > > <entry>enum&nbsp;v4l2_scenemode</entry>
> > > > +	  </row><row><entry spanname="descr">This control sets
> > > > +	  the camera's scenemode, and it is provided by the type of
> > > > +	  the enum values. The "None" mode means the status
> > > > +	  when scenemode algorithm is not activated, like after booting
> > > > time.
> > > > +	  On the other hand, the "Normal" mode means the scenemode
> algorithm
> > > > +	  is activated on the normal mode.</entry>
> > >
> > > What low-level parameters do the scene mode control ? How does it
> > > interact with the related controls ?
> >
> > For using this control, in M-5MOLS sensor case, several register is
> > configured, like Exposure(locking/Indexed preset/mode), Focus(locking),
> > WhiteBalance, etc. And I think the process interacting and syncing the
> > register's value should be up to the each drivers, and the m5mols driver
> > will be.
> 
> Does it mean that the scene mode is handled by the driver, which then
> configures exposure, focus, white balance, ... ?
Yes, the other controls(exposure/focus/wb, etc) is needed to be changed
for setting scene mode. So, In M-5MOLS case, the user set the scene mode,
then the driver should write the registers - scene mode itself register,
and even the other controls registers related with scene mode. 

> 
> > Anyways, could you explain the difference with low- and high- in more
> > details?
> >
> > :)
> >
> > I still did not understand well.
> 
> The concepts are a bit ill-defined currently. We have low-level sensor
> controls such as exposure time, gains, ... The controls you propose offer a
> higher level interface, likely using software-based algorithms running on the
> sensor instead of just applying hardware parameters.
I could understand high-/low- level meaning with your explanation. Thank you. 

> 
> > > > +	  </row>
> > > > +	  <row>
> > > > +	    <entrytbl spanname="descr" cols="2">
> > > > +	      <tbody valign="top">
> > > > +		<row>
> > > > +
> <entry><constant>V4L2_SCENEMODE_NONE</constant>&nbsp;</entry>
> > > > +		  <entry>Scenemode None.</entry>
> > > > +		</row>
> > > > +		<row>
> > > > +
> >
> > <entry><constant>V4L2_SCENEMODE_NORMAL</constant>&nbsp;</entry>
> >
> > > > +		  <entry>Scenemode Normal.</entry>
> > > > +		</row>
> > > > +		<row>
> > > > +
> > >
> > > <entry><constant>V4L2_SCENEMODE_PORTRAIT</constant>&nbsp;</entry>
> > >
> > > > +		  <entry>Scenemode Portrait.</entry>
> > >
> > > Could you please describe the scene modes in more details ?
> >
> > Yes, the photographer should adjust the exposure(luminance), focus, iso,
> > etc, for getting better image according to each specific circumstances.
> > But, it's uncomfortable to set all controls at every time.
> > The scene mode can make this at one time. It is just not only setting once.
> > According to fixed scene circumstance, the internal algorithm is also
> > needed for it.
> >
> > This function is usually provided as just preset, but, it's not just
> > preset, because the specific algorithm is existed for "scene mode",
> > except the collection of the camera control preset.
> >
> > The enumeration I suggest, can cover almost scene mode. Of course, the term
> > is fixed. The M-5MOLS can use all the scene mode.
> > Please, see drivers/media/video/m5mols/m5mols_contro.c.
> 
> What I meant when asking you to explain the scene modes in more details was to
> improve the documentation by adding a detailed description of each scene mode.
Ok, I'll add the relation about the new and previous controls in more details.

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

RE: [RFC PATCH 3/4] v4l: Add V4L2_CID_WDR button control

[HeungJun, Kim]

Hi,

> -----Original Message-----
> From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
> Sent: Friday, December 30, 2011 9:13 AM
> To: HeungJun, Kim
> Cc: linux-media@vger.kernel.org; mchehab@redhat.com; hverkuil@xs4all.nl;
> sakari.ailus@iki.fi; s.nawrocki@samsung.com; kyungmin.park@samsung.com
> Subject: Re: [RFC PATCH 3/4] v4l: Add V4L2_CID_WDR button control
> 
> Hi,
> 
> On Thursday 29 December 2011 06:52:55 HeungJun, Kim wrote:
> > On Wednesday, December 28, 2011 10:57 PM Laurent Pinchart wrote:
> > > On Wednesday 28 December 2011 07:23:47 HeungJun, Kim wrote:
> > > > It adds the new CID for setting White Balance Preset. This CID is
> > > > provided
> > >
> > > I suppose you mean wide dynamic range here.
> >
> > Right, it's my miss.
> >
> > > > as button type. This can commands only if the camera turn on/off this
> > > > function.
> > >
> > > Shouldn't it be a boolean ? A button can only be activated, for one-shot
> > > auto- focus for instance.
> >
> > Any type can be possible, and fine to me. But, it depends on the whole
> > hardware architecture. The WDR is proceeded and used only in the ISP or
> > another engine processing image. And, the cases I've seen ever, are just
> > one - The ISP exists in the sensor.
> >
> > In M-5MOLS use-case, the ISP is in the M-5MOLS sensor. To the position of
> > developer,
> > it's just ok to turn on/off for using this. But, in the other architecture
> > it might be need more.
> 
> You can't turn a button control on or off. A button control can only be
> activated, it has no state. On/off controls are boolean controls.
Ah, ok. I'll modify this to Boolean for next version. You're right.

> 
> > But, I anticipate if the other architecture use this function, probably
> > any other setting seems be not needed any more. The photographer just says,
> > "turn on the WDR!", not says "adjust parm 1, 2, 3, and turn on WDR!". :)
> >
> > So, IMHO, I think the any other setting is not needed more for now.
> 
> --
> Regards,
> 
> Laurent Pinchart

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-owner@vger.kernel.org [mailto:linux-media-
> owner@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;
> mchehab@redhat.com; hverkuil@xs4all.nl; kyungmin.park@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_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.

Regards,
Heungjun Kim

> 
> >
> > > > 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>&nbsp;</
> > > > 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.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [RFC PATCH 0/4] Add some new camera controls

[HeungJun, Kim]

Hi,

> -----Original Message-----
> From: linux-media-owner@vger.kernel.org [mailto:linux-media-
> owner@vger.kernel.org] On Behalf Of Laurent Pinchart
> Sent: Friday, December 30, 2011 9:16 AM
> To: HeungJun, Kim
> Cc: linux-media@vger.kernel.org; mchehab@redhat.com; hverkuil@xs4all.nl;
> sakari.ailus@iki.fi; s.nawrocki@samsung.com; kyungmin.park@samsung.com
> Subject: Re: [RFC PATCH 0/4] Add some new camera controls
> 
> Hi,
> 
> On Thursday 29 December 2011 07:15:46 HeungJun, Kim wrote:
> > On Wednesday, December 28, 2011 11:01 PM Laurent Pinchart wrote:
> > > On Wednesday 28 December 2011 07:23:44 HeungJun, Kim wrote:
> > > > Hi everyone,
> > > >
> > > > This RFC patch series include new 4 controls ID for digital camera.
> > > > I about to suggest these controls by the necessity enabling the M-5MOLS
> > > > sensor's function, and I hope to discuss this in here.
> > >
> > > Thanks for the patches.
> > >
> > > The new controls introduced by these patches are very high level. Should
> > > they be put in their own class ? I also think we should specify how
> > > those high- level controls interact with low-level controls, otherwise
> > > applications will likely get very confused.
> >
> > I did not consider yet, but I think it's first to define about what
> > low-/high- is. I think this is not high- level controls. And, honestly, I
> > don't understand it's really important to categorize low-/high-, or not.
> >
> > IMHO, The importance is the just complexity of interacting with each
> > modules. If this means the level of low-/high-, I can understand this.
> > But I'm wrong, please explain this. :)
> 
> Low level controls are usually handled in hardware and pretty much self-
> contained. High level controls are usually software algorithms (possibly
> running on the sensor itself) that modify low level controls behind the scene.
> 
> If a sensor exposes both an exposure time control and a scene mode control
> that modifies exposure time handling, documentation of the scene mode control
> must explain how the two interacts and what applications can and can't do.
Ok, thanks for the explanation.
I should include the ISP's contents in the documentation, and this might be
the key I explain easily.

I'll prepare the next RFC add the more details in the document.
Probably, it seems to be lack of explanation about these controls I suggested.

Regards,
Heungjun Kim

> 
> > There is some different story, I just got the N900 some days ago :).
> > The purpose is just understanding Nokia and TI OMAP camera architecture
> > well. Probably, it helps for me to talk more easily, and I'll be able to
> > speak more well
> > with omap workers - you and Sakari.
> 
> --
> Regards,
> 
> Laurent Pinchart
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@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

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

<snip>

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

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.</entry>
>>>>
>>>>  	  </row>
>>>>  	  <row><entry></entry></row>
>>>>
>>>> +	  <row id="v4l2-preset-white-balance">
>>>> +	    <entry
>>>> spanname="id"><constant>V4L2_CID_PRESET_WHITE_BALANCE</constant>&nbsp;</
>>>> 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

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-owner@vger.kernel.org [mailto:linux-media-
>> owner@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; mchehab@redhat.com;
>> hverkuil@xs4all.nl; sakari.ailus@iki.fi; kyungmin.park@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

Re: [RFC PATCH 0/4] Add some new camera controls

[Sylwester Nawrocki]

Hi Laurent,

On 12/28/2011 03:01 PM, Laurent Pinchart wrote:
> On Wednesday 28 December 2011 07:23:44 HeungJun, Kim wrote:
>> This RFC patch series include new 4 controls ID for digital camera.
>> I about to suggest these controls by the necessity enabling the M-5MOLS
>> sensor's function, and I hope to discuss this in here.
> 
> Thanks for the patches.
> 
> The new controls introduced by these patches are very high level. Should they 
> be put in their own class ? I also think we should specify how those high-
> level controls interact with low-level controls, otherwise applications will 
> likely get very confused.

I agree we may need a separate control class for those high-level controls.
They are mostly applicable to software ISP algorithms, run either on digital
signal processor embedded in the sensor or on a processor being part of an SoC.

Thus we would three levels of controls for camera,
 1) image source class (lowest possible level), dealing mostly with hardware
    registers;
 2) "normal" camera controls (V4L2_CID_CAMERA_CLASS) [2];
 3) high level camera controls (for camera software algorithms)

plus some camera controls are in the user controls class. I'm not sure why there
are camera controls in the user control class, perhaps there was no camera
class yet at the time V4L2_CID_EXPOSURE or V4L2_CID_BACKLIGHT_COMPENSATION were
added. I might be missing something else.

I'm afraid a little it might be hard to distinguish if some control should
belong to 2) or 3), as sensors' logic complexity and advancement varies.

Although I can see an advantage of logically separating controls which have
influence on one or more other (lower level) controls. And separate control
class would be helpful in that.

The candidates to such control class might be:

* V4L2_CID_METERING_MODE,
* V4L2_CID_EXPOSURE_BIAS,
* V4L2_CID_ISO,
* V4L2_CID_WHITE_BALANCE_PRESET,
* V4L2_CID_SCENEMODE,
* V4L2_CID_WDR,
* V4L2_CID_ANTISHAKE,

[1] http://patchwork.linuxtv.org/patch/8923/
[2] http://linuxtv.org/downloads/v4l-dvb-apis/extended-controls.html#camera-controls

--

Regards,
Sylwester

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

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-owner@vger.kernel.org [mailto:linux-media-
> > owner@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;
> > mchehab@redhat.com; hverkuil@xs4all.nl; kyungmin.park@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.

I guess M-5MOLS doesn't provide a colour temperature or any other means to
specify white balance manually besides the presets?

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

Yes, if the white balance is specified as colour temperature. As Hans noted,
there are other way to specify it. If auto wb is enabled, this control also
could be busy to tell the colour temperature estimate. That depends on the
device, though.

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

if that really is just a fixed white balance preset with a name, this
definitely makes sense.

Kind regards,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

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.ailus@iki.fi     jabber/XMPP/Gmail: sailus@retiisi.org.uk

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

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/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>&nbsp;</
> >>>> entry>
> >>>
> >>> Wouldn't V4L2_CID_WHITE_BALANCE_PRESET be better ?
> >>
> >> That's what I was about to say.
> > 
> > And the menu items would contain the same prefix with CID_ removed. They're
> > going to be long, but I don't see that as an issue for menu items.
> 
> Should we call it V4L2_CID_WB_PRESET then ?
> 
> Anyway V4L2_WHITE_BALANCE_PRESET_INCADESCENT for example is not that long,
> we have control names that almost reach 80 characters :)

I'd prefer the long one but I have no strong opinion either way.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

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.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

Re: [RFC PATCH 3/4] v4l: Add V4L2_CID_WDR button control

[Sakari Ailus]

Hi HeungJun,

On Wed, Dec 28, 2011 at 03:23:47PM +0900, HeungJun, Kim wrote:
> It adds the new CID for setting White Balance Preset. This CID is provided as
> button type. This can commands only if the camera turn on/off this function.
> 
> Signed-off-by: HeungJun, Kim <riverful.kim@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  Documentation/DocBook/media/v4l/controls.xml |   12 ++++++++++++
>  drivers/media/video/v4l2-ctrls.c             |    2 ++
>  include/linux/videodev2.h                    |    2 ++
>  3 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
> index afe1845..bed6c66 100644
> --- a/Documentation/DocBook/media/v4l/controls.xml
> +++ b/Documentation/DocBook/media/v4l/controls.xml
> @@ -2958,6 +2958,18 @@ it one step further. This is a write-only control.</entry>
>  	  <row><entry></entry></row>
>  
>  	  <row>
> +	    <entry spanname="id"><constant>V4L2_CID_WDR</constant></entry>

Just a simple comment: how about V4L2_CID_WIDE_DYNAMIC_RANGE instead? I
dont't think it'd be too long.

> +	    <entry>button</entry>
> +	  </row>
> +	  <row>
> +	    <entry spanname="descr">Wide Dynamic Range. It makes
> +	    the image be more clear by adjusting the image's intensity
> +	    of the illumination. This function can be provided according to
> +	    the capability of the hardware(sensor or AP's multimedia block).
> +	    </entry>
> +	  </row>
> +
> +	  <row>
>  	    <entry spanname="id"><constant>V4L2_CID_PRIVACY</constant>&nbsp;</entry>
>  	    <entry>boolean</entry>
>  	  </row><row><entry spanname="descr">Prevent video from being acquired
> diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
> index fef58c2..66110bc 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -598,6 +598,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_IRIS_RELATIVE:		return "Iris, Relative";
>  	case V4L2_CID_PRESET_WHITE_BALANCE:	return "White Balance, Preset";
>  	case V4L2_CID_SCENEMODE:		return "Scenemode";
> +	case V4L2_CID_WDR:			return "Wide Dynamic Range";
>  
>  	/* FM Radio Modulator control */
>  	/* Keep the order of the 'case's the same as in videodev2.h! */
> @@ -687,6 +688,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  		break;
>  	case V4L2_CID_PAN_RESET:
>  	case V4L2_CID_TILT_RESET:
> +	case V4L2_CID_WDR:
>  	case V4L2_CID_FLASH_STROBE:
>  	case V4L2_CID_FLASH_STROBE_STOP:
>  		*type = V4L2_CTRL_TYPE_BUTTON;
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index bc14feb..f85ad6c 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -1646,6 +1646,8 @@ enum v4l2_scenemode {
>  	V4L2_SCENEMODE_CANDLE = 14,
>  };
>  
> +#define V4L2_CID_WDR				(V4L2_CID_CAMERA_CLASS_BASE+21)
> +
>  /* FM Modulator class control IDs */
>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
>  #define V4L2_CID_FM_TX_CLASS			(V4L2_CTRL_CLASS_FM_TX | 1)
> -- 
> 1.7.4.1
> 

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

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

Ack.

>> I expect some parameters for adjusting auto WB algorithm (WB (R/G/B) gain bias
>> or something similar) to be present in sensor's ISP as well. If I remember well
>> I've seen something like this in one of sensor's documentations.
> 
> Sounds reasonable.
> 
>>>>>> diff --git a/Documentation/DocBook/media/v4l/controls.xml
>>>>>> b/Documentation/DocBook/media/v4l/controls.xml index c0422c6..350c138
>>>>>> 100644
>>>>>> --- a/Documentation/DocBook/media/v4l/controls.xml
>>>>>> +++ b/Documentation/DocBook/media/v4l/controls.xml
>>>>>> @@ -2841,6 +2841,44 @@ it one step further. This is a write-only
>>>>>> control.</entry>
>>>>>>
>>>>>>  	  </row>
>>>>>>  	  <row><entry></entry></row>
>>>>>>
>>>>>> +	  <row id="v4l2-preset-white-balance">
>>>>>> +	    <entry
>>>>>> spanname="id"><constant>V4L2_CID_PRESET_WHITE_BALANCE</constant>&nbsp;</
>>>>>> entry>
>>>>>
>>>>> Wouldn't V4L2_CID_WHITE_BALANCE_PRESET be better ?
>>>>
>>>> That's what I was about to say.
>>>
>>> And the menu items would contain the same prefix with CID_ removed. They're
>>> going to be long, but I don't see that as an issue for menu items.
>>
>> Should we call it V4L2_CID_WB_PRESET then ?
>>
>> Anyway V4L2_WHITE_BALANCE_PRESET_INCADESCENT for example is not that long,
>> we have control names that almost reach 80 characters :)
> 
> I'd prefer the long one but I have no strong opinion either way.

Ok, let's keep the long one then.


Happy New Year! :)

S.

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-owner@vger.kernel.org [mailto:linux-media-
>>> owner@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; mchehab@redhat.com;
>>> hverkuil@xs4all.nl; sakari.ailus@iki.fi; kyungmin.park@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
>

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

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

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

That's very true. I think an "undefined" menu item would be an option, at
least I can't think of a better one right now.

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

In that case, "manual" would be just another name for "unknown" in case
where automatic white balance has been turned off.

Also, as Hans noted, colour temperature is just one way to specify white
balance. I guess that to achieve a perfect result we should acquire the
whole spectrum for each pixel, and make an estimation on the spectrum of the
light source. That doesn't sound feasible. :-)

But there are other options than just colour temperature. That still might
be the only really practical one for the end user.

Cheers,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

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

> > > 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.
> 
> That's very true. I think an "undefined" menu item would be an option, at
> least I can't think of a better one right now.
> 
> > 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.
> 
> In that case, "manual" would be just another name for "unknown" in case
> where automatic white balance has been turned off.
> 
> Also, as Hans noted, colour temperature is just one way to specify white
> balance. I guess that to achieve a perfect result we should acquire the
> whole spectrum for each pixel, and make an estimation on the spectrum of
> the light source. That doesn't sound feasible. :-)
> 
> But there are other options than just colour temperature. That still might
> be the only really practical one for the end user.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 0/4] Add some new camera controls

[Sakari Ailus]

Hi Sylwester,

On Fri, Dec 30, 2011 at 12:18:40PM +0100, Sylwester Nawrocki wrote:
> On 12/28/2011 03:01 PM, Laurent Pinchart wrote:
> > On Wednesday 28 December 2011 07:23:44 HeungJun, Kim wrote:
> >> This RFC patch series include new 4 controls ID for digital camera.
> >> I about to suggest these controls by the necessity enabling the M-5MOLS
> >> sensor's function, and I hope to discuss this in here.
> > 
> > Thanks for the patches.
> > 
> > The new controls introduced by these patches are very high level. Should they 
> > be put in their own class ? I also think we should specify how those high-
> > level controls interact with low-level controls, otherwise applications will 
> > likely get very confused.
> 
> I agree we may need a separate control class for those high-level controls.
> They are mostly applicable to software ISP algorithms, run either on digital
> signal processor embedded in the sensor or on a processor being part of an SoC.
> 
> Thus we would three levels of controls for camera,
>  1) image source class (lowest possible level), dealing mostly with hardware
>     registers;

I intended the image source class for controls which only deal with the a/d
conversion itself. Other controls would be elsewhere.

There hasn't been a final decision on this yet, but an alternative which has
been also discussed is just to call this a "low level" control class.

>  2) "normal" camera controls (V4L2_CID_CAMERA_CLASS) [2];
>  3) high level camera controls (for camera software algorithms)

Almost all the automatic anything algorithms are impelemented in software.
But when software is involved, the possibilities are mostly limitless; it's
a matter of imagination what kind of interesting white balance algorithms 

> plus some camera controls are in the user controls class. I'm not sure why there
> are camera controls in the user control class, perhaps there was no camera
> class yet at the time V4L2_CID_EXPOSURE or V4L2_CID_BACKLIGHT_COMPENSATION were
> added. I might be missing something else.

The camera control class is relatively new, so that's likely what happened.

Speaking of the classes --- I could resend my patch which allows changing
controls in different classes in the same s_ext_ctrls call --- I looked at
it some time ago and no driver had any limitations on this; it's just the
documentation and the control framework which do.

> I'm afraid a little it might be hard to distinguish if some control should
> belong to 2) or 3), as sensors' logic complexity and advancement varies.

I can see two main use cases:

1. V4L2 / V4L2 subdev / MC as the low level API for camera control and

2. Regular V4L2 applications.

For most controls it's clear which of the two classes they belong to.

> Although I can see an advantage of logically separating controls which have
> influence on one or more other (lower level) controls. And separate control
> class would be helpful in that.
> 
> The candidates to such control class might be:
> 
> * V4L2_CID_METERING_MODE,
> * V4L2_CID_EXPOSURE_BIAS,
> * V4L2_CID_ISO,
> * V4L2_CID_WHITE_BALANCE_PRESET,
> * V4L2_CID_SCENEMODE,
> * V4L2_CID_WDR,
> * V4L2_CID_ANTISHAKE,

The list looks good to me.

Cheers,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

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

Regards,

-- 
Sakari Ailus
sakari.ailus@iki.fi

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

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.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

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

Re: [RFC PATCH 0/4] Add some new camera controls

[Sylwester Nawrocki]

Hi everybody,

On 01/04/2012 10:07 PM, Sakari Ailus wrote:
> On Fri, Dec 30, 2011 at 12:18:40PM +0100, Sylwester Nawrocki wrote:
>> Thus we would three levels of controls for camera,
>>   1) image source class (lowest possible level), dealing mostly with hardware
>>      registers;
> 
> I intended the image source class for controls which only deal with the a/d
> conversion itself. Other controls would be elsewhere.
> 
> There hasn't been a final decision on this yet, but an alternative which has
> been also discussed is just to call this a "low level" control class.
> 
>>   2) "normal" camera controls (V4L2_CID_CAMERA_CLASS) [2];
>>   3) high level camera controls (for camera software algorithms)
...
> 
>> I'm afraid a little it might be hard to distinguish if some control should
>> belong to 2) or 3), as sensors' logic complexity and advancement varies.
> 
> I can see two main use cases:
> 
> 1. V4L2 / V4L2 subdev / MC as the low level API for camera control and
> 
> 2. Regular V4L2 applications.
> 
> For most controls it's clear which of the two classes they belong to.

Have you any ideas on what the class' name could be ? I thought about 
V4L2_CTRL_CLASS_HIGH_LEVEL_CAMERA or V4L2_CTRL_CLASS_CAMERA_USER although 
I'm not too happy with any of them and it seems hard to make up some 
reasonable name, when we already have V4L2_CTRL_CLASS_CAMERA.

>> Although I can see an advantage of logically separating controls which have
>> influence on one or more other (lower level) controls. And separate control
>> class would be helpful in that.
>>
>> The candidates to such control class might be:
>>
>> * V4L2_CID_METERING_MODE,
>> * V4L2_CID_EXPOSURE_BIAS,
>> * V4L2_CID_ISO,
>> * V4L2_CID_WHITE_BALANCE_PRESET,
>> * V4L2_CID_SCENEMODE,
>> * V4L2_CID_WDR,
>> * V4L2_CID_ANTISHAKE,
> 
> The list looks good to me.

--

Thanks,
Sylwester

Re: [RFC PATCH 0/4] Add some new camera controls

[Sakari Ailus]

Hi Sylwester,

On Sat, Jan 28, 2012 at 06:01:59PM +0100, Sylwester Nawrocki wrote:
> On 01/04/2012 10:07 PM, Sakari Ailus wrote:
> > On Fri, Dec 30, 2011 at 12:18:40PM +0100, Sylwester Nawrocki wrote:
> >> Thus we would three levels of controls for camera,
> >>   1) image source class (lowest possible level), dealing mostly with hardware
> >>      registers;
> > 
> > I intended the image source class for controls which only deal with the a/d
> > conversion itself. Other controls would be elsewhere.
> > 
> > There hasn't been a final decision on this yet, but an alternative which has
> > been also discussed is just to call this a "low level" control class.
> > 
> >>   2) "normal" camera controls (V4L2_CID_CAMERA_CLASS) [2];
> >>   3) high level camera controls (for camera software algorithms)
> ...
> > 
> >> I'm afraid a little it might be hard to distinguish if some control should
> >> belong to 2) or 3), as sensors' logic complexity and advancement varies.
> > 
> > I can see two main use cases:
> > 
> > 1. V4L2 / V4L2 subdev / MC as the low level API for camera control and
> > 
> > 2. Regular V4L2 applications.
> > 
> > For most controls it's clear which of the two classes they belong to.
> 
> Have you any ideas on what the class' name could be ? I thought about 
> V4L2_CTRL_CLASS_HIGH_LEVEL_CAMERA or V4L2_CTRL_CLASS_CAMERA_USER although 
> I'm not too happy with any of them and it seems hard to make up some 
> reasonable name, when we already have V4L2_CTRL_CLASS_CAMERA.

I might continue to use the current V4L2_CTRL_CLASS_CAMERA to that --- as
far I understand most are quite high level controls. We could create a new
class for the low level controls instead.

Should the new class be for camera controls only or for any low level
controls? I'd perhaps vote for a camera, or even sensor low level class. (I
actually call it image source class in the patchset.)

Another question is how should we call the class for low level controls
which may or may not be implemented in sensor. Digital gain, for example.

> >> Although I can see an advantage of logically separating controls which have
> >> influence on one or more other (lower level) controls. And separate control
> >> class would be helpful in that.
> >>
> >> The candidates to such control class might be:
> >>
> >> * V4L2_CID_METERING_MODE,
> >> * V4L2_CID_EXPOSURE_BIAS,
> >> * V4L2_CID_ISO,
> >> * V4L2_CID_WHITE_BALANCE_PRESET,
> >> * V4L2_CID_SCENEMODE,
> >> * V4L2_CID_WDR,
> >> * V4L2_CID_ANTISHAKE,
> > 
> > The list looks good to me.

Cheers,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk