[libcamera-devel,v5,2/7] libcamera: pipeline: uvcvideo: Add support for AeEnable
diff mbox series

Message ID 20200425004533.26907-3-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • Patchset for libcamera controls
Related show

Commit Message

Laurent Pinchart April 25, 2020, 12:45 a.m. UTC 
UVC devices support auto-exposure, expose the feature through the
AeEnable control.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 42 +++++++++++++++-----
 1 file changed, 33 insertions(+), 9 deletions(-)

Comments

Jacopo Mondi April 26, 2020, 1:02 p.m. UTC | #1 
Hi Laurent,

On Sat, Apr 25, 2020 at 03:45:28AM +0300, Laurent Pinchart wrote:
> UVC devices support auto-exposure, expose the feature through the
> AeEnable control.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 42 +++++++++++++++-----
>  1 file changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 92777b9f5fe4..b040f2460b1c 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -244,9 +244,6 @@ void PipelineHandlerUVC::stop(Camera *camera)
>  int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
>  				       const ControlValue &value)
>  {
> -	if (value.type() != ControlTypeInteger32)
> -		return -EINVAL;
> -
>  	uint32_t cid;
>
>  	if (id == controls::Brightness)
> @@ -255,6 +252,8 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
>  		cid = V4L2_CID_CONTRAST;
>  	else if (id == controls::Saturation)
>  		cid = V4L2_CID_SATURATION;
> +	else if (id == controls::AeEnable)
> +		cid = V4L2_CID_EXPOSURE_AUTO;
>  	else if (id == controls::ManualExposure)
>  		cid = V4L2_CID_EXPOSURE_ABSOLUTE;
>  	else if (id == controls::ManualGain)
> @@ -262,12 +261,21 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
>  	else
>  		return -EINVAL;
>
> -	int32_t ivalue = value.get<int32_t>();
> +	switch (cid) {
> +	case V4L2_CID_EXPOSURE_AUTO: {
> +		int32_t ivalue = value.get<bool>()
> +			       ? V4L2_EXPOSURE_APERTURE_PRIORITY
> +			       : V4L2_EXPOSURE_MANUAL;
> +		controls->set(V4L2_CID_EXPOSURE_AUTO, ivalue);
> +		break;
> +	}
>
> -	if (cid == V4L2_CID_EXPOSURE_ABSOLUTE)
> -		controls->set(V4L2_CID_EXPOSURE_AUTO, static_cast<int32_t>(1));

Which one between AeEnable and ManualExposure has priority ? Here it
seems to me the last processed one overwrites the other...

Anyway, am I wrong or if you receive ManualExposure, AeEnable is not
disabled anymore?  And, this comment applies to patch #1 but I'm just
noticing it now, if cid == V4L2_CID_EXPOSURE_ABSOLUTE, it means we have
received ManualExposure, shouldn't then EXPOSURE_AUTO be set to 0 ?

> -
> -	controls->set(cid, ivalue);
> +	default: {
> +		int32_t ivalue = value.get<int32_t>();
> +		controls->set(cid, ivalue);
> +		break;
> +	}
> +	}
>
>  	return 0;
>  }
> @@ -388,7 +396,9 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
>  			       ControlInfoMap::Map *ctrls)
>  {
>  	const ControlId *id;
> +	ControlInfo info;
>
> +	/* Map the control ID. */

it's fine here, but could have gone in #1. Nitpicking...

>  	switch (cid) {
>  	case V4L2_CID_BRIGHTNESS:
>  		id = &controls::Brightness;
> @@ -399,6 +409,9 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
>  	case V4L2_CID_SATURATION:
>  		id = &controls::Saturation;
>  		break;
> +	case V4L2_CID_EXPOSURE_AUTO:
> +		id = &controls::AeEnable;
> +		break;
>  	case V4L2_CID_EXPOSURE_ABSOLUTE:
>  		id = &controls::ManualExposure;
>  		break;
> @@ -409,7 +422,18 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
>  		return;
>  	}
>
> -	ctrls->emplace(id, v4l2Info);
> +	/* Map the control info. */
> +	switch (cid) {
> +	case V4L2_CID_EXPOSURE_AUTO:
> +		info = ControlInfo{ false, true, true };

Why can't we use the v4l2Info here ? what will it report that needs to
be manually contructed ?

> +		break;
> +
> +	default:
> +		info = v4l2Info;
> +		break;
> +	}
> +
> +	ctrls->emplace(id, info);
>  }
>
>  void UVCCameraData::bufferReady(FrameBuffer *buffer)
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart April 26, 2020, 5:04 p.m. UTC | #2 
Hi Jacopo,

On Sun, Apr 26, 2020 at 03:02:17PM +0200, Jacopo Mondi wrote:
> On Sat, Apr 25, 2020 at 03:45:28AM +0300, Laurent Pinchart wrote:
> > UVC devices support auto-exposure, expose the feature through the
> > AeEnable control.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 42 +++++++++++++++-----
> >  1 file changed, 33 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index 92777b9f5fe4..b040f2460b1c 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -244,9 +244,6 @@ void PipelineHandlerUVC::stop(Camera *camera)
> >  int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> >  				       const ControlValue &value)
> >  {
> > -	if (value.type() != ControlTypeInteger32)
> > -		return -EINVAL;
> > -
> >  	uint32_t cid;
> >
> >  	if (id == controls::Brightness)
> > @@ -255,6 +252,8 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> >  		cid = V4L2_CID_CONTRAST;
> >  	else if (id == controls::Saturation)
> >  		cid = V4L2_CID_SATURATION;
> > +	else if (id == controls::AeEnable)
> > +		cid = V4L2_CID_EXPOSURE_AUTO;
> >  	else if (id == controls::ManualExposure)
> >  		cid = V4L2_CID_EXPOSURE_ABSOLUTE;
> >  	else if (id == controls::ManualGain)
> > @@ -262,12 +261,21 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> >  	else
> >  		return -EINVAL;
> >
> > -	int32_t ivalue = value.get<int32_t>();
> > +	switch (cid) {
> > +	case V4L2_CID_EXPOSURE_AUTO: {
> > +		int32_t ivalue = value.get<bool>()
> > +			       ? V4L2_EXPOSURE_APERTURE_PRIORITY
> > +			       : V4L2_EXPOSURE_MANUAL;
> > +		controls->set(V4L2_CID_EXPOSURE_AUTO, ivalue);
> > +		break;
> > +	}
> >
> > -	if (cid == V4L2_CID_EXPOSURE_ABSOLUTE)
> > -		controls->set(V4L2_CID_EXPOSURE_AUTO, static_cast<int32_t>(1));
> 
> Which one between AeEnable and ManualExposure has priority ? Here it
> seems to me the last processed one overwrites the other...

The V4L2_CID_EXPOSURE_ABSOLUTE control is ignored by the device when
V4L2_CID_EXPOSURE_AUTO is set to automatic, which is what we want. We
could make it explicit in the code here by skipping the
V4L2_CID_EXPOSURE_ABSOLUTE control when auto-exposure is enabled, but I
think that's a bit overkill as the hardware ignores it anyway.

> Anyway, am I wrong or if you receive ManualExposure, AeEnable is not
> disabled anymore?  And, this comment applies to patch #1 but I'm just
> noticing it now, if cid == V4L2_CID_EXPOSURE_ABSOLUTE, it means we have
> received ManualExposure, shouldn't then EXPOSURE_AUTO be set to 0 ?

AeEnable controls whether the auto-exposure algorithm is enabled or not.
When enabled, manual changes to the exposure time shall be ignored by
the pipeline handler. In this specific case the pipeline handler
implementation relies on the device ignoring the manual exposure time.

> > -
> > -	controls->set(cid, ivalue);
> > +	default: {
> > +		int32_t ivalue = value.get<int32_t>();
> > +		controls->set(cid, ivalue);
> > +		break;
> > +	}
> > +	}
> >
> >  	return 0;
> >  }
> > @@ -388,7 +396,9 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> >  			       ControlInfoMap::Map *ctrls)
> >  {
> >  	const ControlId *id;
> > +	ControlInfo info;
> >
> > +	/* Map the control ID. */
> 
> it's fine here, but could have gone in #1. Nitpicking...
> 
> >  	switch (cid) {
> >  	case V4L2_CID_BRIGHTNESS:
> >  		id = &controls::Brightness;
> > @@ -399,6 +409,9 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> >  	case V4L2_CID_SATURATION:
> >  		id = &controls::Saturation;
> >  		break;
> > +	case V4L2_CID_EXPOSURE_AUTO:
> > +		id = &controls::AeEnable;
> > +		break;
> >  	case V4L2_CID_EXPOSURE_ABSOLUTE:
> >  		id = &controls::ManualExposure;
> >  		break;
> > @@ -409,7 +422,18 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> >  		return;
> >  	}
> >
> > -	ctrls->emplace(id, v4l2Info);
> > +	/* Map the control info. */
> > +	switch (cid) {
> > +	case V4L2_CID_EXPOSURE_AUTO:
> > +		info = ControlInfo{ false, true, true };
> 
> Why can't we use the v4l2Info here ? what will it report that needs to
> be manually contructed ?

V4L2_CID_EXPOSURE_AUTO is a menu control, so v4l2Info contains int32_t
values, while controls::AeEnable is a boolean control.

> > +		break;
> > +
> > +	default:
> > +		info = v4l2Info;
> > +		break;
> > +	}
> > +
> > +	ctrls->emplace(id, info);
> >  }
> >
> >  void UVCCameraData::bufferReady(FrameBuffer *buffer)
Jacopo Mondi April 26, 2020, 7:36 p.m. UTC | #3 
Hi Laurent,

On Sun, Apr 26, 2020 at 08:04:27PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Sun, Apr 26, 2020 at 03:02:17PM +0200, Jacopo Mondi wrote:
> > On Sat, Apr 25, 2020 at 03:45:28AM +0300, Laurent Pinchart wrote:
> > > UVC devices support auto-exposure, expose the feature through the
> > > AeEnable control.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 42 +++++++++++++++-----
> > >  1 file changed, 33 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > index 92777b9f5fe4..b040f2460b1c 100644
> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > @@ -244,9 +244,6 @@ void PipelineHandlerUVC::stop(Camera *camera)
> > >  int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> > >  				       const ControlValue &value)
> > >  {
> > > -	if (value.type() != ControlTypeInteger32)
> > > -		return -EINVAL;
> > > -
> > >  	uint32_t cid;
> > >
> > >  	if (id == controls::Brightness)
> > > @@ -255,6 +252,8 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> > >  		cid = V4L2_CID_CONTRAST;
> > >  	else if (id == controls::Saturation)
> > >  		cid = V4L2_CID_SATURATION;
> > > +	else if (id == controls::AeEnable)
> > > +		cid = V4L2_CID_EXPOSURE_AUTO;
> > >  	else if (id == controls::ManualExposure)
> > >  		cid = V4L2_CID_EXPOSURE_ABSOLUTE;
> > >  	else if (id == controls::ManualGain)
> > > @@ -262,12 +261,21 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> > >  	else
> > >  		return -EINVAL;
> > >
> > > -	int32_t ivalue = value.get<int32_t>();
> > > +	switch (cid) {
> > > +	case V4L2_CID_EXPOSURE_AUTO: {
> > > +		int32_t ivalue = value.get<bool>()
> > > +			       ? V4L2_EXPOSURE_APERTURE_PRIORITY
> > > +			       : V4L2_EXPOSURE_MANUAL;
> > > +		controls->set(V4L2_CID_EXPOSURE_AUTO, ivalue);
> > > +		break;
> > > +	}
> > >
> > > -	if (cid == V4L2_CID_EXPOSURE_ABSOLUTE)
> > > -		controls->set(V4L2_CID_EXPOSURE_AUTO, static_cast<int32_t>(1));
> >
> > Which one between AeEnable and ManualExposure has priority ? Here it
> > seems to me the last processed one overwrites the other...
>
> The V4L2_CID_EXPOSURE_ABSOLUTE control is ignored by the device when
> V4L2_CID_EXPOSURE_AUTO is set to automatic, which is what we want. We
> could make it explicit in the code here by skipping the
> V4L2_CID_EXPOSURE_ABSOLUTE control when auto-exposure is enabled, but I
> think that's a bit overkill as the hardware ignores it anyway.
>
> > Anyway, am I wrong or if you receive ManualExposure, AeEnable is not
> > disabled anymore?  And, this comment applies to patch #1 but I'm just
> > noticing it now, if cid == V4L2_CID_EXPOSURE_ABSOLUTE, it means we have
> > received ManualExposure, shouldn't then EXPOSURE_AUTO be set to 0 ?
>
> AeEnable controls whether the auto-exposure algorithm is enabled or not.
> When enabled, manual changes to the exposure time shall be ignored by
> the pipeline handler. In this specific case the pipeline handler
> implementation relies on the device ignoring the manual exposure time.
>

Oh, I had no idea, this is fine then.

> > > -
> > > -	controls->set(cid, ivalue);
> > > +	default: {
> > > +		int32_t ivalue = value.get<int32_t>();
> > > +		controls->set(cid, ivalue);
> > > +		break;
> > > +	}
> > > +	}
> > >
> > >  	return 0;
> > >  }
> > > @@ -388,7 +396,9 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> > >  			       ControlInfoMap::Map *ctrls)
> > >  {
> > >  	const ControlId *id;
> > > +	ControlInfo info;
> > >
> > > +	/* Map the control ID. */
> >
> > it's fine here, but could have gone in #1. Nitpicking...
> >
> > >  	switch (cid) {
> > >  	case V4L2_CID_BRIGHTNESS:
> > >  		id = &controls::Brightness;
> > > @@ -399,6 +409,9 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> > >  	case V4L2_CID_SATURATION:
> > >  		id = &controls::Saturation;
> > >  		break;
> > > +	case V4L2_CID_EXPOSURE_AUTO:
> > > +		id = &controls::AeEnable;
> > > +		break;
> > >  	case V4L2_CID_EXPOSURE_ABSOLUTE:
> > >  		id = &controls::ManualExposure;
> > >  		break;
> > > @@ -409,7 +422,18 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> > >  		return;
> > >  	}
> > >
> > > -	ctrls->emplace(id, v4l2Info);
> > > +	/* Map the control info. */
> > > +	switch (cid) {
> > > +	case V4L2_CID_EXPOSURE_AUTO:
> > > +		info = ControlInfo{ false, true, true };
> >
> > Why can't we use the v4l2Info here ? what will it report that needs to
> > be manually contructed ?
>
> V4L2_CID_EXPOSURE_AUTO is a menu control, so v4l2Info contains int32_t
> values, while controls::AeEnable is a boolean control.
>

Ack.

With these questions clarified
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> > > +		break;
> > > +
> > > +	default:
> > > +		info = v4l2Info;
> > > +		break;
> > > +	}
> > > +
> > > +	ctrls->emplace(id, info);
> > >  }
> > >
> > >  void UVCCameraData::bufferReady(FrameBuffer *buffer)
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
index 92777b9f5fe4..b040f2460b1c 100644
--- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
+++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
@@ -244,9 +244,6 @@  void PipelineHandlerUVC::stop(Camera *camera)
 int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
 				       const ControlValue &value)
 {
-	if (value.type() != ControlTypeInteger32)
-		return -EINVAL;
-
 	uint32_t cid;
 
 	if (id == controls::Brightness)
@@ -255,6 +252,8 @@  int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
 		cid = V4L2_CID_CONTRAST;
 	else if (id == controls::Saturation)
 		cid = V4L2_CID_SATURATION;
+	else if (id == controls::AeEnable)
+		cid = V4L2_CID_EXPOSURE_AUTO;
 	else if (id == controls::ManualExposure)
 		cid = V4L2_CID_EXPOSURE_ABSOLUTE;
 	else if (id == controls::ManualGain)
@@ -262,12 +261,21 @@  int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
 	else
 		return -EINVAL;
 
-	int32_t ivalue = value.get<int32_t>();
+	switch (cid) {
+	case V4L2_CID_EXPOSURE_AUTO: {
+		int32_t ivalue = value.get<bool>()
+			       ? V4L2_EXPOSURE_APERTURE_PRIORITY
+			       : V4L2_EXPOSURE_MANUAL;
+		controls->set(V4L2_CID_EXPOSURE_AUTO, ivalue);
+		break;
+	}
 
-	if (cid == V4L2_CID_EXPOSURE_ABSOLUTE)
-		controls->set(V4L2_CID_EXPOSURE_AUTO, static_cast<int32_t>(1));
-
-	controls->set(cid, ivalue);
+	default: {
+		int32_t ivalue = value.get<int32_t>();
+		controls->set(cid, ivalue);
+		break;
+	}
+	}
 
 	return 0;
 }
@@ -388,7 +396,9 @@  void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
 			       ControlInfoMap::Map *ctrls)
 {
 	const ControlId *id;
+	ControlInfo info;
 
+	/* Map the control ID. */
 	switch (cid) {
 	case V4L2_CID_BRIGHTNESS:
 		id = &controls::Brightness;
@@ -399,6 +409,9 @@  void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
 	case V4L2_CID_SATURATION:
 		id = &controls::Saturation;
 		break;
+	case V4L2_CID_EXPOSURE_AUTO:
+		id = &controls::AeEnable;
+		break;
 	case V4L2_CID_EXPOSURE_ABSOLUTE:
 		id = &controls::ManualExposure;
 		break;
@@ -409,7 +422,18 @@  void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
 		return;
 	}
 
-	ctrls->emplace(id, v4l2Info);
+	/* Map the control info. */
+	switch (cid) {
+	case V4L2_CID_EXPOSURE_AUTO:
+		info = ControlInfo{ false, true, true };
+		break;
+
+	default:
+		info = v4l2Info;
+		break;
+	}
+
+	ctrls->emplace(id, info);
 }
 
 void UVCCameraData::bufferReady(FrameBuffer *buffer)