[libcamera-devel] [PATCH v5 2/7] libcamera: pipeline: uvcvideo: Add support for AeEnable

[Jacopo Mondi]

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 at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel