[libcamera-devel,v2,2/7] libcamera: pipeline: uvcvideo: Support the new AE controls
diff mbox series

Message ID 20211001103325.1077590-3-paul.elder@ideasonboard.com
State Superseded
Delegated to: Paul Elder
Headers show
Series
  • The Great AE Changes
Related show

Commit Message

Paul Elder Oct. 1, 2021, 10:33 a.m. UTC 
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@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(-)

Comments

Jacopo Mondi Oct. 1, 2021, 4:54 p.m. UTC | #1 
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@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 ?

>  	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 ?
> +
> +		/* \todo Make this nicer. */

I don't get what happens down here, maybe a most explicative comment
might help ?

> +		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
>
Paul Elder Oct. 4, 2021, 8:11 a.m. UTC | #2 
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@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
> >

Patch
diff mbox series 

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;
 	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);
+
+		/* \todo Make this nicer. */
+		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: