[libcamera-devel] [PATCH v2 2/7] libcamera: pipeline: uvcvideo: Support the new AE controls

[paul.elder at ideasonboard.com]

Hi Jacopo,

On Fri, Oct 01, 2021 at 06:54:02PM +0200, Jacopo Mondi wrote:
> Hi Paul,
> 
> On Fri, Oct 01, 2021 at 07:33:20PM +0900, Paul Elder wrote:
> > Add support for the new AE controls in the uvcvideo pipeline handler.
> >
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=42
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=43
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> >
> > ---
> > Changes in v2:
> > - fix the rebase error where some uvc stuff was in rasberrypi
> > ---
> >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 36 +++++++++++++++++---
> >  1 file changed, 32 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index 264f5370..3a9c3b8d 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -268,7 +268,9 @@ 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)
> > +	else if (id == controls::ExposureTimeMode)
> > +		cid = V4L2_CID_EXPOSURE_AUTO;
> > +	else if (id == controls::AnalogueGainMode)
> >  		cid = V4L2_CID_EXPOSURE_AUTO;
> 
> Shouldn't AnalogueGainMode map to V4L2_CID_AUTOGAIN ?

Oh probably. I'm not yet familiar with V4L2 controls.

> 
> >  	else if (id == controls::ExposureTime)
> >  		cid = V4L2_CID_EXPOSURE_ABSOLUTE;
> > @@ -302,9 +304,33 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> >  	}
> >
> >  	case V4L2_CID_EXPOSURE_AUTO: {
> > -		int32_t ivalue = value.get<bool>()
> > +		bool exposureSet = controls->contains(V4L2_CID_EXPOSURE_AUTO);
> 
> Uh, I don't get this. We add V4L2_CID_EXPOSURE_AUTO to controls at the
> end of the function, don't we ?

Oh you're right. I think I thought I needed and exception and then
didn't and forgot to fix it back.

> > +
> > +		/* \todo Make this nicer. */
> 
> I don't get what happens down here, maybe a most explicative comment
> might help ?

Ah :D

I thought that V4L2_CID_EXPOSURE_AUTO was "the" AE control, as it had
both aperture priority and shutter priority.

The end goal of this complex block is to:
exposure |  gain  || value
---------+--------++------
auto     | auto   || auto
auto     | manual || aperture priority
auto     | none   || aperture priority
manual   | auto   || shutter priority
none     | auto   || shutter priority
manual   | manual || manual

(I'm pretty sure, this is so disgusting that even I lost track...)


Paul

> 
> > +		int32_t ivalue;
> > +		if (id == controls::ExposureTimeMode && exposureSet) {
> > +			int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO).get<int32_t>();
> > +			ivalue = value.get<int32_t>() == controls::ExposureTimeModeAuto
> > +			       ? (exposureMode == V4L2_EXPOSURE_SHUTTER_PRIORITY
> > +				  ? V4L2_EXPOSURE_AUTO
> > +				  : V4L2_EXPOSURE_APERTURE_PRIORITY)
> > +			       : V4L2_EXPOSURE_MANUAL;
> > +		} else if (id == controls::ExposureTimeMode && !exposureSet) {
> > +			ivalue = value.get<int32_t>() == controls::ExposureTimeModeAuto
> >  			       ? V4L2_EXPOSURE_APERTURE_PRIORITY
> >  			       : V4L2_EXPOSURE_MANUAL;
> > +		} else if (id == controls::AnalogueGainMode && exposureSet) {
> > +			int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO).get<int32_t>();
> > +			ivalue = value.get<int32_t>() == controls::AnalogueGainModeAuto
> > +			       ? (exposureMode == V4L2_EXPOSURE_APERTURE_PRIORITY
> > +				  ? V4L2_EXPOSURE_AUTO
> > +				  : V4L2_EXPOSURE_SHUTTER_PRIORITY)
> > +			       : V4L2_EXPOSURE_MANUAL;
> > +		} else if (id == controls::AnalogueGainMode && !exposureSet) {
> > +			ivalue = value.get<int32_t>() == controls::AnalogueGainModeAuto
> > +			       ? V4L2_EXPOSURE_SHUTTER_PRIORITY
> > +			       : V4L2_EXPOSURE_MANUAL;
> > +		}
> >  		controls->set(V4L2_CID_EXPOSURE_AUTO, ivalue);
> >  		break;
> >  	}
> > @@ -559,7 +585,7 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> >  		id = &controls::Saturation;
> >  		break;
> >  	case V4L2_CID_EXPOSURE_AUTO:
> > -		id = &controls::AeEnable;
> > +		id = &controls::ExposureTimeMode;
> >  		break;
> >  	case V4L2_CID_EXPOSURE_ABSOLUTE:
> >  		id = &controls::ExposureTime;
> > @@ -610,7 +636,9 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> >  		break;
> >
> >  	case V4L2_CID_EXPOSURE_AUTO:
> > -		info = ControlInfo{ false, true, true };
> > +		info = ControlInfo{ { controls::ExposureTimeModeAuto,
> > +				      controls::ExposureTimeModeDisabled },
> > +				    controls::ExposureTimeModeDisabled };
> 
> >  		break;
> >
> >  	case V4L2_CID_EXPOSURE_ABSOLUTE:
> > --
> > 2.27.0
> >