[v3,3/8] libcamera: uvcvideo: Register ExposureTimeMode control
diff mbox series

Message ID 20241113131256.3170817-4-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • AEGC controls
Related show

Commit Message

Paul Elder Nov. 13, 2024, 1:12 p.m. UTC 
From: Jacopo Mondi <jacopo@jmondi.org>

Port the UVC pipeline handler to use the new ExposureTimeMode control
when processing Camera controls in place of the AeEnable control.

The V4L2_CID_EXPOSURE_AUTO control allows 4 possible values, which
map to ExposureTimeModeAuto and ExposureTimeModeManual.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

---
No change in v3
---
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 54 ++++++++++++++++++--
 1 file changed, 49 insertions(+), 5 deletions(-)

Comments

Laurent Pinchart Nov. 21, 2024, 3:21 a.m. UTC | #1 
Hi Paul, Jacopo,

Thank you for the patch.

On Wed, Nov 13, 2024 at 10:12:51PM +0900, Paul Elder wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> Port the UVC pipeline handler to use the new ExposureTimeMode control
> when processing Camera controls in place of the AeEnable control.
> 
> The V4L2_CID_EXPOSURE_AUTO control allows 4 possible values, which
> map to ExposureTimeModeAuto and ExposureTimeModeManual.

Should we fix https://bugs.libcamera.org/show_bug.cgi?id=242 while at it
?

> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> No change in v3
> ---
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 54 ++++++++++++++++++--
>  1 file changed, 49 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 8c2c6baf3575..a2f0935181d4 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -298,7 +298,7 @@ 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::ExposureTime)
>  		cid = V4L2_CID_EXPOSURE_ABSOLUTE;
> @@ -647,7 +647,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;
> @@ -660,6 +660,7 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
>  	}
>  
>  	/* Map the control info. */
> +	const std::vector<ControlValue> &v4l2Values = v4l2Info.values();
>  	int32_t min = v4l2Info.min().get<int32_t>();
>  	int32_t max = v4l2Info.max().get<int32_t>();
>  	int32_t def = v4l2Info.def().get<int32_t>();
> @@ -697,10 +698,53 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
>  		};
>  		break;
>  
> -	case V4L2_CID_EXPOSURE_AUTO:
> -		info = ControlInfo{ false, true, true };
> +	case V4L2_CID_EXPOSURE_AUTO: {
> +		/*
> +		 * From the V4L2_CID_EXPOSURE_AUTO documentation:
> +		 *
> +		 * ------------------------------------------------------------
> +		 * V4L2_EXPOSURE_AUTO:
> +		 * Automatic exposure time, automatic iris aperture.
> +		 *
> +		 * V4L2_EXPOSURE_MANUAL:
> +		 * Manual exposure time, manual iris.
> +		 *
> +		 * V4L2_EXPOSURE_SHUTTER_PRIORITY:
> +		 * Manual exposure time, auto iris.
> +		 *
> +		 * V4L2_EXPOSURE_APERTURE_PRIORITY:
> +		 * Auto exposure time, manual iris.
> +		 *-------------------------------------------------------------
> +		 *
> +		 * ExposureTimeModeAuto = { V4L2_EXPOSURE_AUTO,
> +		 * 			    V4L2_EXPOSURE_APERTURE_PRIORITY }
> +		 *
> +		 *
> +		 * ExposureTimeModeManual = { V4L2_EXPOSURE_MANUAL,
> +		 *			      V4L2_EXPOSURE_SHUTTER_PRIORITY }
> +		 */
> +		std::array<int32_t, 2> values{};
> +
> +		auto it = std::find_if(v4l2Values.begin(), v4l2Values.end(),
> +			[&](const ControlValue &val) {
> +				return (val.get<int32_t>() == V4L2_EXPOSURE_APERTURE_PRIORITY ||
> +					val.get<int32_t>() == V4L2_EXPOSURE_AUTO) ? true : false;
> +			});
> +		if (it != v4l2Values.end())
> +			values.back() = static_cast<int32_t>(controls::ExposureTimeModeAuto);
> +
> +		it = std::find_if(v4l2Values.begin(), v4l2Values.end(),
> +			[&](const ControlValue &val) {
> +				return (val.get<int32_t>() == V4L2_EXPOSURE_SHUTTER_PRIORITY ||
> +					val.get<int32_t>() == V4L2_EXPOSURE_MANUAL) ? true : false;
> +			});
> +		if (it != v4l2Values.end())
> +			values.back() = static_cast<int32_t>(controls::ExposureTimeModeManual);
> +
> +		int32_t *data = values.data();
> +		info = ControlInfo{Span<int32_t>(data, 2), values[0]};
>  		break;
> -
> +	}
>  	case V4L2_CID_EXPOSURE_ABSOLUTE:
>  		/*
>  		 * ExposureTime is in units of 1 µs, and UVC expects
Paul Elder Dec. 3, 2024, 10:54 a.m. UTC | #2 
On Thu, Nov 21, 2024 at 05:21:19AM +0200, Laurent Pinchart wrote:
> Hi Paul, Jacopo,
> 
> Thank you for the patch.
> 
> On Wed, Nov 13, 2024 at 10:12:51PM +0900, Paul Elder wrote:
> > From: Jacopo Mondi <jacopo@jmondi.org>
> > 
> > Port the UVC pipeline handler to use the new ExposureTimeMode control
> > when processing Camera controls in place of the AeEnable control.
> > 
> > The V4L2_CID_EXPOSURE_AUTO control allows 4 possible values, which
> > map to ExposureTimeModeAuto and ExposureTimeModeManual.
> 
> Should we fix https://bugs.libcamera.org/show_bug.cgi?id=242 while at it
> ?

I'd rather do it on top than delay this even further.


Paul

> 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > No change in v3
> > ---
> >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 54 ++++++++++++++++++--
> >  1 file changed, 49 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index 8c2c6baf3575..a2f0935181d4 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -298,7 +298,7 @@ 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::ExposureTime)
> >  		cid = V4L2_CID_EXPOSURE_ABSOLUTE;
> > @@ -647,7 +647,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;
> > @@ -660,6 +660,7 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> >  	}
> >  
> >  	/* Map the control info. */
> > +	const std::vector<ControlValue> &v4l2Values = v4l2Info.values();
> >  	int32_t min = v4l2Info.min().get<int32_t>();
> >  	int32_t max = v4l2Info.max().get<int32_t>();
> >  	int32_t def = v4l2Info.def().get<int32_t>();
> > @@ -697,10 +698,53 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> >  		};
> >  		break;
> >  
> > -	case V4L2_CID_EXPOSURE_AUTO:
> > -		info = ControlInfo{ false, true, true };
> > +	case V4L2_CID_EXPOSURE_AUTO: {
> > +		/*
> > +		 * From the V4L2_CID_EXPOSURE_AUTO documentation:
> > +		 *
> > +		 * ------------------------------------------------------------
> > +		 * V4L2_EXPOSURE_AUTO:
> > +		 * Automatic exposure time, automatic iris aperture.
> > +		 *
> > +		 * V4L2_EXPOSURE_MANUAL:
> > +		 * Manual exposure time, manual iris.
> > +		 *
> > +		 * V4L2_EXPOSURE_SHUTTER_PRIORITY:
> > +		 * Manual exposure time, auto iris.
> > +		 *
> > +		 * V4L2_EXPOSURE_APERTURE_PRIORITY:
> > +		 * Auto exposure time, manual iris.
> > +		 *-------------------------------------------------------------
> > +		 *
> > +		 * ExposureTimeModeAuto = { V4L2_EXPOSURE_AUTO,
> > +		 * 			    V4L2_EXPOSURE_APERTURE_PRIORITY }
> > +		 *
> > +		 *
> > +		 * ExposureTimeModeManual = { V4L2_EXPOSURE_MANUAL,
> > +		 *			      V4L2_EXPOSURE_SHUTTER_PRIORITY }
> > +		 */
> > +		std::array<int32_t, 2> values{};
> > +
> > +		auto it = std::find_if(v4l2Values.begin(), v4l2Values.end(),
> > +			[&](const ControlValue &val) {
> > +				return (val.get<int32_t>() == V4L2_EXPOSURE_APERTURE_PRIORITY ||
> > +					val.get<int32_t>() == V4L2_EXPOSURE_AUTO) ? true : false;
> > +			});
> > +		if (it != v4l2Values.end())
> > +			values.back() = static_cast<int32_t>(controls::ExposureTimeModeAuto);
> > +
> > +		it = std::find_if(v4l2Values.begin(), v4l2Values.end(),
> > +			[&](const ControlValue &val) {
> > +				return (val.get<int32_t>() == V4L2_EXPOSURE_SHUTTER_PRIORITY ||
> > +					val.get<int32_t>() == V4L2_EXPOSURE_MANUAL) ? true : false;
> > +			});
> > +		if (it != v4l2Values.end())
> > +			values.back() = static_cast<int32_t>(controls::ExposureTimeModeManual);
> > +
> > +		int32_t *data = values.data();
> > +		info = ControlInfo{Span<int32_t>(data, 2), values[0]};
> >  		break;
> > -
> > +	}
> >  	case V4L2_CID_EXPOSURE_ABSOLUTE:
> >  		/*
> >  		 * ExposureTime is in units of 1 µs, and UVC expects
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Laurent Pinchart Dec. 3, 2024, 10:59 a.m. UTC | #3 
On Tue, Dec 03, 2024 at 07:54:52PM +0900, Paul Elder wrote:
> On Thu, Nov 21, 2024 at 05:21:19AM +0200, Laurent Pinchart wrote:
> > On Wed, Nov 13, 2024 at 10:12:51PM +0900, Paul Elder wrote:
> > > From: Jacopo Mondi <jacopo@jmondi.org>
> > > 
> > > Port the UVC pipeline handler to use the new ExposureTimeMode control
> > > when processing Camera controls in place of the AeEnable control.
> > > 
> > > The V4L2_CID_EXPOSURE_AUTO control allows 4 possible values, which
> > > map to ExposureTimeModeAuto and ExposureTimeModeManual.
> > 
> > Should we fix https://bugs.libcamera.org/show_bug.cgi?id=242 while at it
> > ?
> 
> I'd rather do it on top than delay this even further.

I was thinking that fixing the issue would be useful to show that the
API can work for this use case. I'm fine with a separate patch on top,
maybe you could first send v4 of this series and then fix the UVC issue
while I review the new version ?

> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > > 
> > > ---
> > > No change in v3
> > > ---
> > >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 54 ++++++++++++++++++--
> > >  1 file changed, 49 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > index 8c2c6baf3575..a2f0935181d4 100644
> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > @@ -298,7 +298,7 @@ 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::ExposureTime)
> > >  		cid = V4L2_CID_EXPOSURE_ABSOLUTE;
> > > @@ -647,7 +647,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;
> > > @@ -660,6 +660,7 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> > >  	}
> > >  
> > >  	/* Map the control info. */
> > > +	const std::vector<ControlValue> &v4l2Values = v4l2Info.values();
> > >  	int32_t min = v4l2Info.min().get<int32_t>();
> > >  	int32_t max = v4l2Info.max().get<int32_t>();
> > >  	int32_t def = v4l2Info.def().get<int32_t>();
> > > @@ -697,10 +698,53 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> > >  		};
> > >  		break;
> > >  
> > > -	case V4L2_CID_EXPOSURE_AUTO:
> > > -		info = ControlInfo{ false, true, true };
> > > +	case V4L2_CID_EXPOSURE_AUTO: {
> > > +		/*
> > > +		 * From the V4L2_CID_EXPOSURE_AUTO documentation:
> > > +		 *
> > > +		 * ------------------------------------------------------------
> > > +		 * V4L2_EXPOSURE_AUTO:
> > > +		 * Automatic exposure time, automatic iris aperture.
> > > +		 *
> > > +		 * V4L2_EXPOSURE_MANUAL:
> > > +		 * Manual exposure time, manual iris.
> > > +		 *
> > > +		 * V4L2_EXPOSURE_SHUTTER_PRIORITY:
> > > +		 * Manual exposure time, auto iris.
> > > +		 *
> > > +		 * V4L2_EXPOSURE_APERTURE_PRIORITY:
> > > +		 * Auto exposure time, manual iris.
> > > +		 *-------------------------------------------------------------
> > > +		 *
> > > +		 * ExposureTimeModeAuto = { V4L2_EXPOSURE_AUTO,
> > > +		 * 			    V4L2_EXPOSURE_APERTURE_PRIORITY }
> > > +		 *
> > > +		 *
> > > +		 * ExposureTimeModeManual = { V4L2_EXPOSURE_MANUAL,
> > > +		 *			      V4L2_EXPOSURE_SHUTTER_PRIORITY }
> > > +		 */
> > > +		std::array<int32_t, 2> values{};
> > > +
> > > +		auto it = std::find_if(v4l2Values.begin(), v4l2Values.end(),
> > > +			[&](const ControlValue &val) {
> > > +				return (val.get<int32_t>() == V4L2_EXPOSURE_APERTURE_PRIORITY ||
> > > +					val.get<int32_t>() == V4L2_EXPOSURE_AUTO) ? true : false;
> > > +			});
> > > +		if (it != v4l2Values.end())
> > > +			values.back() = static_cast<int32_t>(controls::ExposureTimeModeAuto);
> > > +
> > > +		it = std::find_if(v4l2Values.begin(), v4l2Values.end(),
> > > +			[&](const ControlValue &val) {
> > > +				return (val.get<int32_t>() == V4L2_EXPOSURE_SHUTTER_PRIORITY ||
> > > +					val.get<int32_t>() == V4L2_EXPOSURE_MANUAL) ? true : false;
> > > +			});
> > > +		if (it != v4l2Values.end())
> > > +			values.back() = static_cast<int32_t>(controls::ExposureTimeModeManual);
> > > +
> > > +		int32_t *data = values.data();
> > > +		info = ControlInfo{Span<int32_t>(data, 2), values[0]};
> > >  		break;
> > > -
> > > +	}
> > >  	case V4L2_CID_EXPOSURE_ABSOLUTE:
> > >  		/*
> > >  		 * ExposureTime is in units of 1 µs, and UVC expects
> > 
> > -- 
> > Regards,
> > 
> > Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
index 8c2c6baf3575..a2f0935181d4 100644
--- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
+++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
@@ -298,7 +298,7 @@  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::ExposureTime)
 		cid = V4L2_CID_EXPOSURE_ABSOLUTE;
@@ -647,7 +647,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;
@@ -660,6 +660,7 @@  void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
 	}
 
 	/* Map the control info. */
+	const std::vector<ControlValue> &v4l2Values = v4l2Info.values();
 	int32_t min = v4l2Info.min().get<int32_t>();
 	int32_t max = v4l2Info.max().get<int32_t>();
 	int32_t def = v4l2Info.def().get<int32_t>();
@@ -697,10 +698,53 @@  void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
 		};
 		break;
 
-	case V4L2_CID_EXPOSURE_AUTO:
-		info = ControlInfo{ false, true, true };
+	case V4L2_CID_EXPOSURE_AUTO: {
+		/*
+		 * From the V4L2_CID_EXPOSURE_AUTO documentation:
+		 *
+		 * ------------------------------------------------------------
+		 * V4L2_EXPOSURE_AUTO:
+		 * Automatic exposure time, automatic iris aperture.
+		 *
+		 * V4L2_EXPOSURE_MANUAL:
+		 * Manual exposure time, manual iris.
+		 *
+		 * V4L2_EXPOSURE_SHUTTER_PRIORITY:
+		 * Manual exposure time, auto iris.
+		 *
+		 * V4L2_EXPOSURE_APERTURE_PRIORITY:
+		 * Auto exposure time, manual iris.
+		 *-------------------------------------------------------------
+		 *
+		 * ExposureTimeModeAuto = { V4L2_EXPOSURE_AUTO,
+		 * 			    V4L2_EXPOSURE_APERTURE_PRIORITY }
+		 *
+		 *
+		 * ExposureTimeModeManual = { V4L2_EXPOSURE_MANUAL,
+		 *			      V4L2_EXPOSURE_SHUTTER_PRIORITY }
+		 */
+		std::array<int32_t, 2> values{};
+
+		auto it = std::find_if(v4l2Values.begin(), v4l2Values.end(),
+			[&](const ControlValue &val) {
+				return (val.get<int32_t>() == V4L2_EXPOSURE_APERTURE_PRIORITY ||
+					val.get<int32_t>() == V4L2_EXPOSURE_AUTO) ? true : false;
+			});
+		if (it != v4l2Values.end())
+			values.back() = static_cast<int32_t>(controls::ExposureTimeModeAuto);
+
+		it = std::find_if(v4l2Values.begin(), v4l2Values.end(),
+			[&](const ControlValue &val) {
+				return (val.get<int32_t>() == V4L2_EXPOSURE_SHUTTER_PRIORITY ||
+					val.get<int32_t>() == V4L2_EXPOSURE_MANUAL) ? true : false;
+			});
+		if (it != v4l2Values.end())
+			values.back() = static_cast<int32_t>(controls::ExposureTimeModeManual);
+
+		int32_t *data = values.data();
+		info = ControlInfo{Span<int32_t>(data, 2), values[0]};
 		break;
-
+	}
 	case V4L2_CID_EXPOSURE_ABSOLUTE:
 		/*
 		 * ExposureTime is in units of 1 µs, and UVC expects