Re: [RFCv2 PATCH 01/10] v4l2-controls: add motion detection controls.

[Hans Verkuil <hverkuil@xs4all.nl>]

On 08/21/2013 11:36 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Monday 12 August 2013 12:58:24 Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Add support for two motion detection controls and a 'detect control class'.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/media/v4l2-core/v4l2-ctrls.c | 33 +++++++++++++++++++++++++++------
>> include/uapi/linux/v4l2-controls.h   | 14 ++++++++++++++
>>  2 files changed, 41 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
>> b/drivers/media/v4l2-core/v4l2-ctrls.c index fccd08b..89e7cfb 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -456,6 +456,12 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>  		"RGB full range (0-255)",
>>  		NULL,
>>  	};
>> +	static const char * const detect_motion_mode[] = {
>> +		"Disabled",
>> +		"Global",
>> +		"Regional",
>> +		NULL,
>> +	};
>>
>>
>>  	switch (id) {
>> @@ -545,6 +551,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>  	case V4L2_CID_DV_TX_RGB_RANGE:
>>  	case V4L2_CID_DV_RX_RGB_RANGE:
>>  		return dv_rgb_range;
>> +	case V4L2_CID_DETECT_MOTION_MODE:
>> +		return detect_motion_mode;
>>
>>  	default:
>>  		return NULL;
>> @@ -557,7 +565,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>  {
>>  	switch (id) {
>>  	/* USER controls */
>> -	/* Keep the order of the 'case's the same as in videodev2.h! */
>> +	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> 
> Maybe we could replace all the individual occurences of that comment with a 
> single one at the beginning of the switch ?

It's a pretty long switch, so I think it is good that this comment is repeated
every so often.

> 
>>  	case V4L2_CID_USER_CLASS:		return "User Controls";
>>  	case V4L2_CID_BRIGHTNESS:		return "Brightness";
>>  	case V4L2_CID_CONTRAST:			return "Contrast";
>> @@ -601,7 +609,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>  	case V4L2_CID_COLORFX_CBCR:		return "Color Effects, CbCr";
>>
>>  	/* MPEG controls */
>> -	/* Keep the order of the 'case's the same as in videodev2.h! */
>> +	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
>>  	case V4L2_CID_MPEG_CLASS:		return "MPEG Encoder Controls";
>>  	case V4L2_CID_MPEG_STREAM_TYPE:		return "Stream Type";
>>  	case V4L2_CID_MPEG_STREAM_PID_PMT:	return "Stream PMT Program ID";
>> @@ -701,7 +709,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>  	case V4L2_CID_MPEG_VIDEO_REPEAT_SEQ_HEADER:		return "Repeat Sequence
>> Header";
>>
>>  	/* CAMERA controls */
>> -	/* Keep the order of the 'case's the same as in videodev2.h! */
>> +	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
>>  	case V4L2_CID_CAMERA_CLASS:		return "Camera Controls";
>>  	case V4L2_CID_EXPOSURE_AUTO:		return "Auto Exposure";
>>  	case V4L2_CID_EXPOSURE_ABSOLUTE:	return "Exposure Time, Absolute";
>> @@ -735,8 +743,8 @@ const char *v4l2_ctrl_get_name(u32 id)
>>  	case V4L2_CID_AUTO_FOCUS_STATUS:	return "Auto Focus, Status";
>>  	case V4L2_CID_AUTO_FOCUS_RANGE:		return "Auto Focus, Range";
>>
>> -	/* FM Radio Modulator control */
>> -	/* Keep the order of the 'case's the same as in videodev2.h! */
>> +	/* FM Radio Modulator controls */
>> +	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
>>  	case V4L2_CID_FM_TX_CLASS:		return "FM Radio Modulator Controls";
>>  	case V4L2_CID_RDS_TX_DEVIATION:		return "RDS Signal Deviation";
>>  	case V4L2_CID_RDS_TX_PI:		return "RDS Program ID";
>> @@ -759,6 +767,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>  	case V4L2_CID_TUNE_ANTENNA_CAPACITOR:	return "Tune Antenna Capacitor";
>>
>>  	/* Flash controls */
>> +	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
>>  	case V4L2_CID_FLASH_CLASS:		return "Flash Controls";
>>  	case V4L2_CID_FLASH_LED_MODE:		return "LED Mode";
>>  	case V4L2_CID_FLASH_STROBE_SOURCE:	return "Strobe Source";
>> @@ -774,7 +783,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>  	case V4L2_CID_FLASH_READY:		return "Ready to Strobe";
>>
>>  	/* JPEG encoder controls */
>> -	/* Keep the order of the 'case's the same as in videodev2.h! */
>> +	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
>>  	case V4L2_CID_JPEG_CLASS:		return "JPEG Compression Controls";
>>  	case V4L2_CID_JPEG_CHROMA_SUBSAMPLING:	return "Chroma Subsampling";
>>  	case V4L2_CID_JPEG_RESTART_INTERVAL:	return "Restart Interval";
>> @@ -782,18 +791,21 @@ const char *v4l2_ctrl_get_name(u32 id)
>>  	case V4L2_CID_JPEG_ACTIVE_MARKER:	return "Active Markers";
>>
>>  	/* Image source controls */
>> +	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
>>  	case V4L2_CID_IMAGE_SOURCE_CLASS:	return "Image Source Controls";
>>  	case V4L2_CID_VBLANK:			return "Vertical Blanking";
>>  	case V4L2_CID_HBLANK:			return "Horizontal Blanking";
>>  	case V4L2_CID_ANALOGUE_GAIN:		return "Analogue Gain";
>>
>>  	/* Image processing controls */
>> +	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
>>  	case V4L2_CID_IMAGE_PROC_CLASS:		return "Image Processing Controls";
>>  	case V4L2_CID_LINK_FREQ:		return "Link Frequency";
>>  	case V4L2_CID_PIXEL_RATE:		return "Pixel Rate";
>>  	case V4L2_CID_TEST_PATTERN:		return "Test Pattern";
>>
>>  	/* DV controls */
>> +	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
>>  	case V4L2_CID_DV_CLASS:			return "Digital Video Controls";
>>  	case V4L2_CID_DV_TX_HOTPLUG:		return "Hotplug Present";
>>  	case V4L2_CID_DV_TX_RXSENSE:		return "RxSense Present";
>> @@ -806,6 +818,12 @@ const char *v4l2_ctrl_get_name(u32 id)
>>  	case V4L2_CID_FM_RX_CLASS:		return "FM Radio Receiver Controls";
>>  	case V4L2_CID_TUNE_DEEMPHASIS:		return "De-Emphasis";
>>  	case V4L2_CID_RDS_RECEPTION:		return "RDS Reception";
>> +
>> +	/* FM Radio Receiver controls */
>> +	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
>> +	case V4L2_CID_DETECT_CLASS:		return "Detection Controls";
>> +	case V4L2_CID_DETECT_MOTION_MODE:	return "Motion Detection Mode";
>> +	case V4L2_CID_DETECT_MOTION_THRESHOLD:	return "Motion Detection
>> Threshold"; default:
>>  		return NULL;
>>  	}
>> @@ -914,6 +932,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
>> v4l2_ctrl_type *type, case V4L2_CID_DV_RX_RGB_RANGE:
>>  	case V4L2_CID_TEST_PATTERN:
>>  	case V4L2_CID_TUNE_DEEMPHASIS:
>> +	case V4L2_CID_DETECT_MOTION_MODE:
>>  		*type = V4L2_CTRL_TYPE_MENU;
>>  		break;
>>  	case V4L2_CID_LINK_FREQ:
>> @@ -937,6 +956,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
>> v4l2_ctrl_type *type, case V4L2_CID_IMAGE_PROC_CLASS:
>>  	case V4L2_CID_DV_CLASS:
>>  	case V4L2_CID_FM_RX_CLASS:
>> +	case V4L2_CID_DETECT_CLASS:
>>  		*type = V4L2_CTRL_TYPE_CTRL_CLASS;
>>  		/* You can neither read not write these */
>>  		*flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY;
>> @@ -1009,6 +1029,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
>> v4l2_ctrl_type *type, case V4L2_CID_PILOT_TONE_FREQUENCY:
>>  	case V4L2_CID_TUNE_POWER_LEVEL:
>>  	case V4L2_CID_TUNE_ANTENNA_CAPACITOR:
>> +	case V4L2_CID_DETECT_MOTION_THRESHOLD:
>>  		*flags |= V4L2_CTRL_FLAG_SLIDER;
>>  		break;
>>  	case V4L2_CID_PAN_RELATIVE:
>> diff --git a/include/uapi/linux/v4l2-controls.h
>> b/include/uapi/linux/v4l2-controls.h index e90a88a..d88eebd 100644
>> --- a/include/uapi/linux/v4l2-controls.h
>> +++ b/include/uapi/linux/v4l2-controls.h
>> @@ -60,6 +60,7 @@
>>  #define V4L2_CTRL_CLASS_IMAGE_PROC	0x009f0000	/* Image processing 
> controls
>> */ #define V4L2_CTRL_CLASS_DV		0x00a00000	/* Digital Video controls */
>> #define V4L2_CTRL_CLASS_FM_RX		0x00a10000	/* FM Receiver controls */
>> +#define V4L2_CTRL_CLASS_DETECT		0x00a20000	/* Detection controls */
>>
>>  /* User-class control IDs */
>>
>> @@ -853,4 +854,17 @@ enum v4l2_deemphasis {
>>
>>  #define V4L2_CID_RDS_RECEPTION			(V4L2_CID_FM_RX_CLASS_BASE + 2)
>>
>> +
>> +/*  Detection-class control IDs defined by V4L2 */
>> +#define V4L2_CID_DETECT_CLASS_BASE		(V4L2_CTRL_CLASS_DETECT | 0x900)
>> +#define V4L2_CID_DETECT_CLASS			(V4L2_CTRL_CLASS_DETECT | 1)
>> +
>> +#define	V4L2_CID_DETECT_MOTION_MODE		(V4L2_CID_DETECT_CLASS_BASE + 1)
>> +enum v4l2_detect_motion_mode {
>> +	V4L2_DETECT_MOTION_DISABLED	= 0,
>> +	V4L2_DETECT_MOTION_GLOBAL	= 1,
>> +	V4L2_DETECT_MOTION_REGIONAL	= 2,
>> +};
>> +#define	V4L2_CID_DETECT_MOTION_THRESHOLD	(V4L2_CID_DETECT_CLASS_BASE 
> + 2)
>> +
> 
> How many more controls do you expect in this class ? Maybe we should make it a 
> bit generic, by creating an EVENT class that would contain controls pertaining 
> to event generation ?

There could be quite a few controls here for all sorts of <something> detections
(face, smile, object, etc.). An event class seems awfully vague to me. If I am
looking for motion detection controls I wouldn't expect to find them in an EVENT
class.

Regards,

	Hans