gspca V4L2_CID_EXPOSURE_AUTO and VIDIOC_G/S/TRY_EXT_CTRLS

gspca V4L2_CID_EXPOSURE_AUTO and VIDIOC_G/S/TRY_EXT_CTRLS

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 1645 bytes --]

Hi,

I noticed that AEC (Automatic Exposure Control, or
V4L2_CID_EXPOSURE_AUTO) does not work in the ov534 gspca driver, either
from guvcview or qv4l2.

From what I can see (but I do not have a deep knowledge of v4l2) this
happens because:
  - V4L2_CID_EXPOSURE_AUTO is of class V4L2_CTRL_CLASS_CAMERA which is
    greater than V4L2_CTRL_CLASS_USER
  - some programs use VIDIOC_G_EXT_CTRLS with this class of controls;
    for instance v4l2-ctrl does more or less this:

	if (V4L2_CTRL_ID2CLASS(qctrl.id) != V4L2_CTRL_CLASS_USER)
		test_ioctl(fd, VIDIOC_G_EXT_CTRLS, &ctrls)
	else
		test_ioctl(fd, VIDIOC_G_CTRL, &ctrl)

  - gspca does not handle the _EXT_CTRLS ioclts

So in ov534, but I think in m5602 too, V4L2_CID_EXPOSURE_AUTO does not
work from guvcview, qv4l2, or v4l2-ctrl, for instance the latter fails
with the message:

	error 25 getting ext_ctrl Auto Exposure

I tried adding an hackish implementation of vidioc_g_ext_ctrls and
vidioc_s_ext_ctrls to gspca, and with these V4L2_CID_EXPOSURE_AUTO seems
to work, but I need to learn more about this kind of controls before
I can propose a decent implementation for mainline inclusion myself, so
if anyone wants to anticipate me I'd be glad to test :)

Unrelated, but maybe worth mentioning is that V4L2_CID_EXPOSURE_AUTO is
of type MENU, while some drivers are treating it as a boolean, I think
I can fix this one if needed.

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[RFC PATCH 0/3] gspca: Implement VIDIOC_G_EXT_CTRLS and VIDIOC_S_EXT_CTRLS

[Antonio Ospite]

Hi,

this is a first attempt at implementing VIDIOC_G_EXT_CTRLS and
VIDIOC_S_EXT_CTRLS, these ioclt are needed in order to make controls of
type different than V4L2_CTRL_CLASS_USER work in gspca.

An example of such a control is V4L2_CID_EXPOSURE_AUTO which a couple of gspca
subdrivers define but which can't be controlled from v4l2 userspace apps right
now.

Let me know what do you think and if you have any suggestion. If needed I will
add VIDIOC_TRY_EXT_CTRLS too.

Thanks,
   Antonio

Antonio Ospite (3):
  [media] gspca - main: rename get_ctrl to get_ctrl_index
  [media] gspca - main: factor out the logic to set and get controls
  [media] gspca - main: implement vidioc_g_ext_ctrls and
    vidioc_s_ext_ctrls

 drivers/media/video/gspca/gspca.c |  184 ++++++++++++++++++++++++-------------
 1 file changed, 119 insertions(+), 65 deletions(-)

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[RFC PATCH 1/3] [media] gspca - main: rename get_ctrl to get_ctrl_index

[Antonio Ospite]

This reflects better what the function does and is also in preparation
of a refactoring of setting and getting controls.

Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
---
 drivers/media/video/gspca/gspca.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
index ca5a2b1..bc9d037 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -1415,7 +1415,7 @@ out:
 	return ret;
 }
 
-static int get_ctrl(struct gspca_dev *gspca_dev,
+static int get_ctrl_index(struct gspca_dev *gspca_dev,
 				   int id)
 {
 	const struct ctrl *ctrls;
@@ -1458,7 +1458,7 @@ static int vidioc_queryctrl(struct file *file, void *priv,
 			idx = i;
 		}
 	} else {
-		idx = get_ctrl(gspca_dev, id);
+		idx = get_ctrl_index(gspca_dev, id);
 	}
 	if (idx < 0)
 		return -EINVAL;
@@ -1483,7 +1483,7 @@ static int vidioc_s_ctrl(struct file *file, void *priv,
 	struct gspca_ctrl *gspca_ctrl;
 	int idx, ret;
 
-	idx = get_ctrl(gspca_dev, ctrl->id);
+	idx = get_ctrl_index(gspca_dev, ctrl->id);
 	if (idx < 0)
 		return -EINVAL;
 	if (gspca_dev->ctrl_inac & (1 << idx))
@@ -1531,7 +1531,7 @@ static int vidioc_g_ctrl(struct file *file, void *priv,
 	const struct ctrl *ctrls;
 	int idx, ret;
 
-	idx = get_ctrl(gspca_dev, ctrl->id);
+	idx = get_ctrl_index(gspca_dev, ctrl->id);
 	if (idx < 0)
 		return -EINVAL;
 	ctrls = &gspca_dev->sd_desc->ctrls[idx];
-- 
1.7.10

[RFC PATCH 2/3] [media] gspca - main: factor out the logic to set and get controls

[Antonio Ospite]

Factor out the logic to set and get controls from vidioc_s_ctrl()
and vidioc_g_ctrl() so that the code can be reused in the coming
implementation of vidioc_s_ext_ctrls() and vidioc_g_ext_ctrls().

Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
---
 drivers/media/video/gspca/gspca.c |  148 ++++++++++++++++++++-----------------
 1 file changed, 80 insertions(+), 68 deletions(-)

diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
index bc9d037..ba1bda9 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -1432,6 +1432,84 @@ static int get_ctrl_index(struct gspca_dev *gspca_dev,
 	return -1;
 }
 
+static int gspca_set_ctrl(struct gspca_dev *gspca_dev,
+			  __u32 id, __s32 value)
+{
+	const struct ctrl *ctrls;
+	struct gspca_ctrl *gspca_ctrl;
+	int idx, ret;
+
+	idx = get_ctrl_index(gspca_dev, id);
+	if (idx < 0)
+		return -EINVAL;
+	if (gspca_dev->ctrl_inac & (1 << idx))
+		return -EINVAL;
+	ctrls = &gspca_dev->sd_desc->ctrls[idx];
+	if (gspca_dev->cam.ctrls != NULL) {
+		gspca_ctrl = &gspca_dev->cam.ctrls[idx];
+		if (value < gspca_ctrl->min
+		    || value > gspca_ctrl->max)
+			return -ERANGE;
+	} else {
+		gspca_ctrl = NULL;
+		if (value < ctrls->qctrl.minimum
+		    || value > ctrls->qctrl.maximum)
+			return -ERANGE;
+	}
+	PDEBUG(D_CONF, "set ctrl [%08x] = %d", id, value);
+	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
+		return -ERESTARTSYS;
+	if (!gspca_dev->present) {
+		ret = -ENODEV;
+		goto out;
+	}
+	gspca_dev->usb_err = 0;
+	if (ctrls->set != NULL) {
+		ret = ctrls->set(gspca_dev, value);
+		goto out;
+	}
+	if (gspca_ctrl != NULL) {
+		gspca_ctrl->val = value;
+		if (ctrls->set_control != NULL
+		 && gspca_dev->streaming)
+			ctrls->set_control(gspca_dev);
+	}
+	ret = gspca_dev->usb_err;
+out:
+	mutex_unlock(&gspca_dev->usb_lock);
+	return ret;
+}
+
+static int gspca_get_ctrl(struct gspca_dev *gspca_dev,
+			  __u32 id, __s32 *value)
+{
+	const struct ctrl *ctrls;
+	int idx, ret;
+
+	idx = get_ctrl_index(gspca_dev, id);
+	if (idx < 0)
+		return -EINVAL;
+	ctrls = &gspca_dev->sd_desc->ctrls[idx];
+
+	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
+		return -ERESTARTSYS;
+	if (!gspca_dev->present) {
+		ret = -ENODEV;
+		goto out;
+	}
+	gspca_dev->usb_err = 0;
+	if (ctrls->get != NULL) {
+		ret = ctrls->get(gspca_dev, value);
+		goto out;
+	}
+	if (gspca_dev->cam.ctrls != NULL)
+		*value = gspca_dev->cam.ctrls[idx].val;
+	ret = 0;
+out:
+	mutex_unlock(&gspca_dev->usb_lock);
+	return ret;
+}
+
 static int vidioc_queryctrl(struct file *file, void *priv,
 			   struct v4l2_queryctrl *q_ctrl)
 {
@@ -1479,80 +1557,14 @@ static int vidioc_s_ctrl(struct file *file, void *priv,
 			 struct v4l2_control *ctrl)
 {
 	struct gspca_dev *gspca_dev = priv;
-	const struct ctrl *ctrls;
-	struct gspca_ctrl *gspca_ctrl;
-	int idx, ret;
-
-	idx = get_ctrl_index(gspca_dev, ctrl->id);
-	if (idx < 0)
-		return -EINVAL;
-	if (gspca_dev->ctrl_inac & (1 << idx))
-		return -EINVAL;
-	ctrls = &gspca_dev->sd_desc->ctrls[idx];
-	if (gspca_dev->cam.ctrls != NULL) {
-		gspca_ctrl = &gspca_dev->cam.ctrls[idx];
-		if (ctrl->value < gspca_ctrl->min
-		    || ctrl->value > gspca_ctrl->max)
-			return -ERANGE;
-	} else {
-		gspca_ctrl = NULL;
-		if (ctrl->value < ctrls->qctrl.minimum
-		    || ctrl->value > ctrls->qctrl.maximum)
-			return -ERANGE;
-	}
-	PDEBUG(D_CONF, "set ctrl [%08x] = %d", ctrl->id, ctrl->value);
-	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
-		return -ERESTARTSYS;
-	if (!gspca_dev->present) {
-		ret = -ENODEV;
-		goto out;
-	}
-	gspca_dev->usb_err = 0;
-	if (ctrls->set != NULL) {
-		ret = ctrls->set(gspca_dev, ctrl->value);
-		goto out;
-	}
-	if (gspca_ctrl != NULL) {
-		gspca_ctrl->val = ctrl->value;
-		if (ctrls->set_control != NULL
-		 && gspca_dev->streaming)
-			ctrls->set_control(gspca_dev);
-	}
-	ret = gspca_dev->usb_err;
-out:
-	mutex_unlock(&gspca_dev->usb_lock);
-	return ret;
+	return gspca_set_ctrl(gspca_dev, ctrl->id, ctrl->value);
 }
 
 static int vidioc_g_ctrl(struct file *file, void *priv,
 			 struct v4l2_control *ctrl)
 {
 	struct gspca_dev *gspca_dev = priv;
-	const struct ctrl *ctrls;
-	int idx, ret;
-
-	idx = get_ctrl_index(gspca_dev, ctrl->id);
-	if (idx < 0)
-		return -EINVAL;
-	ctrls = &gspca_dev->sd_desc->ctrls[idx];
-
-	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
-		return -ERESTARTSYS;
-	if (!gspca_dev->present) {
-		ret = -ENODEV;
-		goto out;
-	}
-	gspca_dev->usb_err = 0;
-	if (ctrls->get != NULL) {
-		ret = ctrls->get(gspca_dev, &ctrl->value);
-		goto out;
-	}
-	if (gspca_dev->cam.ctrls != NULL)
-		ctrl->value = gspca_dev->cam.ctrls[idx].val;
-	ret = 0;
-out:
-	mutex_unlock(&gspca_dev->usb_lock);
-	return ret;
+	return gspca_get_ctrl(gspca_dev, ctrl->id, &ctrl->value);
 }
 
 static int vidioc_querymenu(struct file *file, void *priv,
-- 
1.7.10

[RFC PATCH 3/3] [media] gspca - main: implement vidioc_g_ext_ctrls and vidioc_s_ext_ctrls

[Antonio Ospite]

This makes it possible for applications to handle controls with a class
different than V4L2_CTRL_CLASS_USER for gspca subdevices, like for
instance V4L2_CID_EXPOSURE_AUTO which some subdrivers use and which
can't be controlled right now.

See
http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/47010
for an example of a problem fixed by this change.

NOTE: gspca currently won't handle control types like
V4L2_CTRL_TYPE_INTEGER64 or V4L2_CTRL_TYPE_STRING, so just the
__s32 field 'value' of 'struct v4l2_ext_control' is handled for now.

Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
---
 drivers/media/video/gspca/gspca.c |   42 +++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
index ba1bda9..7906093 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -1567,6 +1567,46 @@ static int vidioc_g_ctrl(struct file *file, void *priv,
 	return gspca_get_ctrl(gspca_dev, ctrl->id, &ctrl->value);
 }
 
+static int vidioc_s_ext_ctrls(struct file *file, void *priv,
+			 struct v4l2_ext_controls *ext_ctrls)
+{
+	struct gspca_dev *gspca_dev = priv;
+	int ret = 0;
+	int i;
+
+	for (i = 0; i < ext_ctrls->count; i++) {
+		struct v4l2_ext_control *ctrl;
+
+		ctrl = ext_ctrls->controls + i;
+		ret = gspca_set_ctrl(gspca_dev, ctrl->id, ctrl->value);
+		if (ret < 0) {
+			ext_ctrls->error_idx = i;
+			return ret;
+		}
+	}
+	return ret;
+}
+
+static int vidioc_g_ext_ctrls(struct file *file, void *priv,
+			 struct v4l2_ext_controls *ext_ctrls)
+{
+	struct gspca_dev *gspca_dev = priv;
+	int i;
+	int ret = 0;
+
+	for (i = 0; i < ext_ctrls->count; i++) {
+		struct v4l2_ext_control *ctrl;
+
+		ctrl = ext_ctrls->controls + i;
+		ret = gspca_get_ctrl(gspca_dev, ctrl->id, &ctrl->value);
+		if (ret < 0) {
+			ext_ctrls->error_idx = i;
+			return ret;
+		}
+	}
+	return ret;
+}
+
 static int vidioc_querymenu(struct file *file, void *priv,
 			    struct v4l2_querymenu *qmenu)
 {
@@ -2260,6 +2300,8 @@ static const struct v4l2_ioctl_ops dev_ioctl_ops = {
 	.vidioc_queryctrl	= vidioc_queryctrl,
 	.vidioc_g_ctrl		= vidioc_g_ctrl,
 	.vidioc_s_ctrl		= vidioc_s_ctrl,
+	.vidioc_g_ext_ctrls	= vidioc_g_ext_ctrls,
+	.vidioc_s_ext_ctrls	= vidioc_s_ext_ctrls,
 	.vidioc_querymenu	= vidioc_querymenu,
 	.vidioc_enum_input	= vidioc_enum_input,
 	.vidioc_g_input		= vidioc_g_input,
-- 
1.7.10

Re: gspca V4L2_CID_EXPOSURE_AUTO and VIDIOC_G/S/TRY_EXT_CTRLS

[Jean-Francois Moine]

On Wed, 18 Apr 2012 15:37:20 +0200
Antonio Ospite <ospite@studenti.unina.it> wrote:

> I noticed that AEC (Automatic Exposure Control, or
> V4L2_CID_EXPOSURE_AUTO) does not work in the ov534 gspca driver, either
> from guvcview or qv4l2.
	[snip]
> So in ov534, but I think in m5602 too, V4L2_CID_EXPOSURE_AUTO does not
> work from guvcview, qv4l2, or v4l2-ctrl, for instance the latter fails
> with the message:
> 
> 	error 25 getting ext_ctrl Auto Exposure
> 
> I tried adding an hackish implementation of vidioc_g_ext_ctrls and
> vidioc_s_ext_ctrls to gspca, and with these V4L2_CID_EXPOSURE_AUTO seems
> to work, but I need to learn more about this kind of controls before
> I can propose a decent implementation for mainline inclusion myself, so
> if anyone wants to anticipate me I'd be glad to test :)
> 
> Unrelated, but maybe worth mentioning is that V4L2_CID_EXPOSURE_AUTO is
> of type MENU, while some drivers are treating it as a boolean, I think
> I can fix this one if needed.

Hi Antonio,

Yes, V4L2_CID_EXPOSURE_AUTO is of class V4L2_CTRL_CLASS_CAMERA, and, as
the associated menu shows, it is not suitable for webcams.

In the webcam world, the autoexposure is often the same as the
autogain: in the knee algorithm
(http://81.209.78.62:8080/docs/LowLightOptimization.html - also look at
gspca/sonixb.c), both exposure and gain are concerned. The cases where
a user wants only autoexposure (fixed gain) or autogain (fixed
exposure) are rare. If you want people to be able to do that, you
should add a new webcam control, V4L2_CID_AUTOEXPOSURE, and also add it
to each driver which implements the knee algorithm, and handle the three
cases, autogain only, autoexposure only and knee.

Then, looking about your implementation of vidioc_s_ext_ctrls, I found
it was a bit simple: setting many controls is atomic, i.e., if any
error occurs at some point, the previous controls should be reset to
their original values. Same about vidioc_g_ext_ctrls: the mutex must be
taken only once for the values do not change. You also do not check if
the controls are in a same control class. Anyway, are these ioctl's
needed?

Regards.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [RFC PATCH 3/3] [media] gspca - main: implement vidioc_g_ext_ctrls and vidioc_s_ext_ctrls

[Hans Verkuil]

Hi Antonio,

My apologies for the late review, I missed this the first time around.

Since I am the 'control guy' :-) I thought I should give my view on this.
And that is the real long-term approach to implementing extended controls
in gspca is to add support in gspca for the control framework (see
Documentation/video4linux/v4l2-controls.txt). That will probably simplify
control handling in gspca anyway. The main problem is how to do it in such
a manner that we can convert gspca drivers to the control framework one by
one.

I am inclined to NACK code that adds a driver-specific extended control
implementation instead of going to the control framework because I know
from painful personal experience that it is very hard to get it right.

I might have some time (no guarantees yet) to help with this. It would
certainly be interesting to add support for the control framework in the
gspca core. Hmm, perhaps that's a job for the weekend...

Regards,

	Hans

On Friday, April 20, 2012 17:19:11 Antonio Ospite wrote:
> This makes it possible for applications to handle controls with a class
> different than V4L2_CTRL_CLASS_USER for gspca subdevices, like for
> instance V4L2_CID_EXPOSURE_AUTO which some subdrivers use and which
> can't be controlled right now.
> 
> See
> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/47010
> for an example of a problem fixed by this change.
> 
> NOTE: gspca currently won't handle control types like
> V4L2_CTRL_TYPE_INTEGER64 or V4L2_CTRL_TYPE_STRING, so just the
> __s32 field 'value' of 'struct v4l2_ext_control' is handled for now.
> 
> Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
> ---
>  drivers/media/video/gspca/gspca.c |   42 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
> index ba1bda9..7906093 100644
> --- a/drivers/media/video/gspca/gspca.c
> +++ b/drivers/media/video/gspca/gspca.c
> @@ -1567,6 +1567,46 @@ static int vidioc_g_ctrl(struct file *file, void *priv,
>  	return gspca_get_ctrl(gspca_dev, ctrl->id, &ctrl->value);
>  }
>  
> +static int vidioc_s_ext_ctrls(struct file *file, void *priv,
> +			 struct v4l2_ext_controls *ext_ctrls)
> +{
> +	struct gspca_dev *gspca_dev = priv;
> +	int ret = 0;
> +	int i;
> +
> +	for (i = 0; i < ext_ctrls->count; i++) {
> +		struct v4l2_ext_control *ctrl;
> +
> +		ctrl = ext_ctrls->controls + i;
> +		ret = gspca_set_ctrl(gspca_dev, ctrl->id, ctrl->value);
> +		if (ret < 0) {
> +			ext_ctrls->error_idx = i;
> +			return ret;
> +		}
> +	}
> +	return ret;
> +}
> +
> +static int vidioc_g_ext_ctrls(struct file *file, void *priv,
> +			 struct v4l2_ext_controls *ext_ctrls)
> +{
> +	struct gspca_dev *gspca_dev = priv;
> +	int i;
> +	int ret = 0;
> +
> +	for (i = 0; i < ext_ctrls->count; i++) {
> +		struct v4l2_ext_control *ctrl;
> +
> +		ctrl = ext_ctrls->controls + i;
> +		ret = gspca_get_ctrl(gspca_dev, ctrl->id, &ctrl->value);
> +		if (ret < 0) {
> +			ext_ctrls->error_idx = i;
> +			return ret;
> +		}
> +	}
> +	return ret;
> +}
> +
>  static int vidioc_querymenu(struct file *file, void *priv,
>  			    struct v4l2_querymenu *qmenu)
>  {
> @@ -2260,6 +2300,8 @@ static const struct v4l2_ioctl_ops dev_ioctl_ops = {
>  	.vidioc_queryctrl	= vidioc_queryctrl,
>  	.vidioc_g_ctrl		= vidioc_g_ctrl,
>  	.vidioc_s_ctrl		= vidioc_s_ctrl,
> +	.vidioc_g_ext_ctrls	= vidioc_g_ext_ctrls,
> +	.vidioc_s_ext_ctrls	= vidioc_s_ext_ctrls,
>  	.vidioc_querymenu	= vidioc_querymenu,
>  	.vidioc_enum_input	= vidioc_enum_input,
>  	.vidioc_g_input		= vidioc_g_input,
> 

Re: [RFC PATCH 3/3] [media] gspca - main: implement vidioc_g_ext_ctrls and vidioc_s_ext_ctrls

[Jean-Francois Moine]

On Fri, 27 Apr 2012 10:20:23 +0200
Hans Verkuil <hverkuil@xs4all.nl> wrote:

> I might have some time (no guarantees yet) to help with this. It would
> certainly be interesting to add support for the control framework in the
> gspca core. Hmm, perhaps that's a job for the weekend...

Hi Hans,

I hope you will not do it! The way gspca treats the controls is static,
quick and very small. The controls in the subdrivers ask only for the
declaration of the controls and functions to send the values to the
webcams. Actually, not all subdrivers have been converted to the new
gspca control handling, but, when done, it will save more memory.

Best regards.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [RFC PATCH 3/3] [media] gspca - main: implement vidioc_g_ext_ctrls and vidioc_s_ext_ctrls

[Hans Verkuil]

On Friday, April 27, 2012 11:24:43 Jean-Francois Moine wrote:
> On Fri, 27 Apr 2012 10:20:23 +0200
> Hans Verkuil <hverkuil@xs4all.nl> wrote:
> 
> > I might have some time (no guarantees yet) to help with this. It would
> > certainly be interesting to add support for the control framework in the
> > gspca core. Hmm, perhaps that's a job for the weekend...
> 
> Hi Hans,
> 
> I hope you will not do it! The way gspca treats the controls is static,
> quick and very small. The controls in the subdrivers ask only for the
> declaration of the controls and functions to send the values to the
> webcams. Actually, not all subdrivers have been converted to the new
> gspca control handling, but, when done, it will save more memory.

And that is exactly what the control framework also does for you. Drivers
only have to declare the controls and have a function to set their value.
Everything else is handled by the control framework. And you get support
for the extended control API for free and also support for control events,
plus any future changes/enhancements to how controls are done will be
automatically added. Not to mention that it ensures consistent and correct
behavior of the control API towards applications.

Note that the control code is already part of videodev.ko, so you have that
code in memory anyway. So why not use it?

Regards,

	Hans

Re: [RFC PATCH 3/3] [media] gspca - main: implement vidioc_g_ext_ctrls and vidioc_s_ext_ctrls

[Hans de Goede]

Hi,

On 04/27/2012 11:24 AM, Jean-Francois Moine wrote:
> On Fri, 27 Apr 2012 10:20:23 +0200
> Hans Verkuil<hverkuil@xs4all.nl>  wrote:
>
>> I might have some time (no guarantees yet) to help with this. It would
>> certainly be interesting to add support for the control framework in the
>> gspca core. Hmm, perhaps that's a job for the weekend...
>
> Hi Hans,
>
> I hope you will not do it! The way gspca treats the controls is static,
> quick and very small. The controls in the subdrivers ask only for the
> declaration of the controls and functions to send the values to the
> webcams. Actually, not all subdrivers have been converted to the new
> gspca control handling, but, when done, it will save more memory.

Actually I've moving gspca over to the control framework on my to-do
list. This will allows us to remove hacks like we have in sonixb.c for
coarse exposure / fine exposure controls. More in general it will allow
(in an easy way) to have per sensor control min/max/step values.

But the most important reason for me to want to use the control framework
in gspca is to get support for control events. Which allow a control-panel
app to dynamically update its display when settings are changed to some
other way (ie another control app, or software autogain).

I do plan to do this in way so that we can do this one subdriver at a
time. I've no idea when I'll get around to doing the first driver
though :)

Regards,

Hans

Re: gspca V4L2_CID_EXPOSURE_AUTO and VIDIOC_G/S/TRY_EXT_CTRLS

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 4060 bytes --]

On Fri, 27 Apr 2012 09:53:09 +0200
Jean-Francois Moine <moinejf@free.fr> wrote:

> On Wed, 18 Apr 2012 15:37:20 +0200
> Antonio Ospite <ospite@studenti.unina.it> wrote:
> 
> > I noticed that AEC (Automatic Exposure Control, or
> > V4L2_CID_EXPOSURE_AUTO) does not work in the ov534 gspca driver, either
> > from guvcview or qv4l2.
> 	[snip]
> > So in ov534, but I think in m5602 too, V4L2_CID_EXPOSURE_AUTO does not
> > work from guvcview, qv4l2, or v4l2-ctrl, for instance the latter fails
> > with the message:
> > 
> > 	error 25 getting ext_ctrl Auto Exposure
> > 
> > I tried adding an hackish implementation of vidioc_g_ext_ctrls and
> > vidioc_s_ext_ctrls to gspca, and with these V4L2_CID_EXPOSURE_AUTO seems
> > to work, but I need to learn more about this kind of controls before
> > I can propose a decent implementation for mainline inclusion myself, so
> > if anyone wants to anticipate me I'd be glad to test :)
> > 
> > Unrelated, but maybe worth mentioning is that V4L2_CID_EXPOSURE_AUTO is
> > of type MENU, while some drivers are treating it as a boolean, I think
> > I can fix this one if needed.
> 
> Hi Antonio,
> 
> Yes, V4L2_CID_EXPOSURE_AUTO is of class V4L2_CTRL_CLASS_CAMERA, and, as
> the associated menu shows, it is not suitable for webcams.
>

Where is that menu you refer to? Maybe camera_exposure_auto in
drivers/media/video/v4l2-ctrls.c which mentions also "Shutter Priority
Mode" and "Aperture Priority Mode"?

Naively one would expect that a web _camera_ could use some controls of
type V4L2_CTRL_CLASS_CAMERA.

> In the webcam world, the autoexposure is often the same as the
> autogain: in the knee algorithm
> (http://81.209.78.62:8080/docs/LowLightOptimization.html - also look at
> gspca/sonixb.c), both exposure and gain are concerned.

From the document you point at I still understand that from a user point
of view autoexposure and autogain are _independent_ parameters (Table
1), it's just that for such algorithm to work well they should be _both_
enabled.

> The cases where
> a user wants only autoexposure (fixed gain) or autogain (fixed
> exposure) are rare.
>
> If you want people to be able to do that, you
> should add a new webcam control, V4L2_CID_AUTOEXPOSURE, and also add it
> to each driver which implements the knee algorithm, and handle the three
> cases, autogain only, autoexposure only and knee.

The real problem here is that _manual_ exposure does not work in ov534
because the user cannot turn off what we are currently calling auto
exposure.

> Then, looking about your implementation of vidioc_s_ext_ctrls, I found
> it was a bit simple: setting many controls is atomic, i.e., if any
> error occurs at some point, the previous controls should be reset to
> their original values. Same about vidioc_g_ext_ctrls: the mutex must be
> taken only once for the values do not change. You also do not check if
> the controls are in a same control class.

I see, it was my first shot and I just wanted to start the discussion
with a "works here" implementation. I think that using some v4l2
infrastructure like the control-framework like Hans proposes could be
better in the long run. IIUC this could also prevent drivers having to
handle menu entries themselves like we are doing now in sd_querymenu(),
right?

If you two reach an agreement and he gets to do it I'll surely port
over drivers using V4L2_CID_EXPOSURE_AUTO.

> Anyway, are these ioctl's needed?
> 

Whether they are really needed or not, that depends on the definition of
webcam, the definition of "camera" in V4L2, and the relationship
between the two.

If a webcam IS-A v4l2 camera, then I'd expect it to be able to use
V4L2_CTRL_CLASS_CAMERA controls, and then EXT controls should be made
accessible by gspca somehow.

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: gspca V4L2_CID_EXPOSURE_AUTO and VIDIOC_G/S/TRY_EXT_CTRLS

[Hans Verkuil]

On Friday, April 27, 2012 14:04:16 Antonio Ospite wrote:
> On Fri, 27 Apr 2012 09:53:09 +0200
> Jean-Francois Moine <moinejf@free.fr> wrote:
> 
> > On Wed, 18 Apr 2012 15:37:20 +0200
> > Antonio Ospite <ospite@studenti.unina.it> wrote:
> > 
> > > I noticed that AEC (Automatic Exposure Control, or
> > > V4L2_CID_EXPOSURE_AUTO) does not work in the ov534 gspca driver, either
> > > from guvcview or qv4l2.
> > 	[snip]
> > > So in ov534, but I think in m5602 too, V4L2_CID_EXPOSURE_AUTO does not
> > > work from guvcview, qv4l2, or v4l2-ctrl, for instance the latter fails
> > > with the message:
> > > 
> > > 	error 25 getting ext_ctrl Auto Exposure
> > > 
> > > I tried adding an hackish implementation of vidioc_g_ext_ctrls and
> > > vidioc_s_ext_ctrls to gspca, and with these V4L2_CID_EXPOSURE_AUTO seems
> > > to work, but I need to learn more about this kind of controls before
> > > I can propose a decent implementation for mainline inclusion myself, so
> > > if anyone wants to anticipate me I'd be glad to test :)
> > > 
> > > Unrelated, but maybe worth mentioning is that V4L2_CID_EXPOSURE_AUTO is
> > > of type MENU, while some drivers are treating it as a boolean, I think
> > > I can fix this one if needed.
> > 
> > Hi Antonio,
> > 
> > Yes, V4L2_CID_EXPOSURE_AUTO is of class V4L2_CTRL_CLASS_CAMERA, and, as
> > the associated menu shows, it is not suitable for webcams.
> >
> 
> Where is that menu you refer to? Maybe camera_exposure_auto in
> drivers/media/video/v4l2-ctrls.c which mentions also "Shutter Priority
> Mode" and "Aperture Priority Mode"?
> 
> Naively one would expect that a web _camera_ could use some controls of
> type V4L2_CTRL_CLASS_CAMERA.
> 
> > In the webcam world, the autoexposure is often the same as the
> > autogain: in the knee algorithm
> > (http://81.209.78.62:8080/docs/LowLightOptimization.html - also look at
> > gspca/sonixb.c), both exposure and gain are concerned.
> 
> From the document you point at I still understand that from a user point
> of view autoexposure and autogain are _independent_ parameters (Table
> 1), it's just that for such algorithm to work well they should be _both_
> enabled.
> 
> > The cases where
> > a user wants only autoexposure (fixed gain) or autogain (fixed
> > exposure) are rare.
> >
> > If you want people to be able to do that, you
> > should add a new webcam control, V4L2_CID_AUTOEXPOSURE, and also add it
> > to each driver which implements the knee algorithm, and handle the three
> > cases, autogain only, autoexposure only and knee.
> 
> The real problem here is that _manual_ exposure does not work in ov534
> because the user cannot turn off what we are currently calling auto
> exposure.
> 
> > Then, looking about your implementation of vidioc_s_ext_ctrls, I found
> > it was a bit simple: setting many controls is atomic, i.e., if any
> > error occurs at some point, the previous controls should be reset to
> > their original values. Same about vidioc_g_ext_ctrls: the mutex must be
> > taken only once for the values do not change. You also do not check if
> > the controls are in a same control class.
> 
> I see, it was my first shot and I just wanted to start the discussion
> with a "works here" implementation. I think that using some v4l2
> infrastructure like the control-framework like Hans proposes could be
> better in the long run. IIUC this could also prevent drivers having to
> handle menu entries themselves like we are doing now in sd_querymenu(),
> right?

Correct.

> If you two reach an agreement and he gets to do it I'll surely port
> over drivers using V4L2_CID_EXPOSURE_AUTO.
> 
> > Anyway, are these ioctl's needed?
> > 
> 
> Whether they are really needed or not, that depends on the definition of
> webcam, the definition of "camera" in V4L2, and the relationship
> between the two.
> 
> If a webcam IS-A v4l2 camera, then I'd expect it to be able to use
> V4L2_CTRL_CLASS_CAMERA controls, and then EXT controls should be made
> accessible by gspca somehow.

The UVC driver definitely uses the camera class controls. So they are really
meant for webcam use :-)

Note that for technical reasons the UVC driver is the only driver that will
not use the control framework (partially due to design of the datastructures
and partially due to the complex control handling/mapping and the support of
dynamic controls in UVC).

Perhaps in the future we might try to get it into UVC anyway, but not before
all other drivers have been converted.

Regards,

	Hans

> 
> Thanks,
>    Antonio
> 
>