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

[Jacopo Mondi]

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

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 at jmondi.org>

Thanks
  j

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