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

[Laurent Pinchart]

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

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)

-- 
Regards,

Laurent Pinchart