[v3,2/4] libcamera: pipeline: uvcvideo: Fix `ExposureTimeMode` control setting
diff mbox series

Message ID 20250314174248.1015718-3-barnabas.pocze@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: pipeline: uvcvideo: Fix `ExposureTimeMode` control
Related show

Commit Message

Barnabás Pőcze March 14, 2025, 5:42 p.m. UTC 
The mapping in `UVCCameraData::processControl()` is not entirely correct
because the control value is retrieved as a `bool` instead of `int32_t`.
Additionally, the available modes are not taken into account.

Retrieve the control value with the right type, `int32_t`, check if the
requested mode is available, and if so, set the appropriate v4l2 control
value selected by `addControl()` earlier.

Fixes: bad8d591f8acfa ("libcamera: uvcvideo: Register ExposureTimeMode control")
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 29 ++++++++++++++------
 1 file changed, 20 insertions(+), 9 deletions(-)

Comments

Jacopo Mondi March 21, 2025, 1:57 p.m. UTC | #1 
Hi Barnabás

On Fri, Mar 14, 2025 at 06:42:46PM +0100, Barnabás Pőcze wrote:
> The mapping in `UVCCameraData::processControl()` is not entirely correct
> because the control value is retrieved as a `bool` instead of `int32_t`.
> Additionally, the available modes are not taken into account.
>
> Retrieve the control value with the right type, `int32_t`, check if the
> requested mode is available, and if so, set the appropriate v4l2 control
> value selected by `addControl()` earlier.
>
> Fixes: bad8d591f8acfa ("libcamera: uvcvideo: Register ExposureTimeMode control")
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 29 ++++++++++++++------
>  1 file changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 5c9025d9b..4ff79c291 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -99,8 +99,8 @@ public:
>  	bool match(DeviceEnumerator *enumerator) override;
>
>  private:
> -	int processControl(ControlList *controls, unsigned int id,
> -			   const ControlValue &value);
> +	int processControl(const UVCCameraData *data, ControlList *controls,
> +			   unsigned int id, const ControlValue &value);
>  	int processControls(UVCCameraData *data, Request *request);
>
>  	bool acquireDevice(Camera *camera) override;
> @@ -313,8 +313,8 @@ void PipelineHandlerUVC::stopDevice(Camera *camera)
>  	data->video_->releaseBuffers();
>  }
>
> -int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> -				       const ControlValue &value)
> +int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *controls,
> +				       unsigned int id, const ControlValue &value)
>  {
>  	uint32_t cid;
>
> @@ -358,10 +358,21 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
>  	}
>
>  	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);
> +		std::optional<v4l2_exposure_auto_type> mode;
> +
> +		switch (value.get<int32_t>()) {
> +		case controls::ExposureTimeModeAuto:
> +			mode = data->autoExposureMode_;
> +			break;
> +		case controls::ExposureTimeModeManual:
> +			mode = data->manualExposureMode_;
> +			break;
> +		}
> +
> +		if (!mode)
> +			return -EINVAL;

Here as well I don't think mode can be not initialized.

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> +
> +		controls->set(V4L2_CID_EXPOSURE_AUTO, static_cast<int32_t>(*mode));
>  		break;
>  	}
>
> @@ -399,7 +410,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
>  	ControlList controls(data->video_->controls());
>
>  	for (const auto &[id, value] : request->controls())
> -		processControl(&controls, id, value);
> +		processControl(data, &controls, id, value);
>
>  	for (const auto &ctrl : controls)
>  		LOG(UVC, Debug)
> --
> 2.48.1
>
Barnabás Pőcze March 21, 2025, 2:04 p.m. UTC | #2 
Hi

2025. 03. 21. 14:57 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Fri, Mar 14, 2025 at 06:42:46PM +0100, Barnabás Pőcze wrote:
>> The mapping in `UVCCameraData::processControl()` is not entirely correct
>> because the control value is retrieved as a `bool` instead of `int32_t`.
>> Additionally, the available modes are not taken into account.
>>
>> Retrieve the control value with the right type, `int32_t`, check if the
>> requested mode is available, and if so, set the appropriate v4l2 control
>> value selected by `addControl()` earlier.
>>
>> Fixes: bad8d591f8acfa ("libcamera: uvcvideo: Register ExposureTimeMode control")
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 29 ++++++++++++++------
>>   1 file changed, 20 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>> index 5c9025d9b..4ff79c291 100644
>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>> @@ -99,8 +99,8 @@ public:
>>   	bool match(DeviceEnumerator *enumerator) override;
>>
>>   private:
>> -	int processControl(ControlList *controls, unsigned int id,
>> -			   const ControlValue &value);
>> +	int processControl(const UVCCameraData *data, ControlList *controls,
>> +			   unsigned int id, const ControlValue &value);
>>   	int processControls(UVCCameraData *data, Request *request);
>>
>>   	bool acquireDevice(Camera *camera) override;
>> @@ -313,8 +313,8 @@ void PipelineHandlerUVC::stopDevice(Camera *camera)
>>   	data->video_->releaseBuffers();
>>   }
>>
>> -int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
>> -				       const ControlValue &value)
>> +int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *controls,
>> +				       unsigned int id, const ControlValue &value)
>>   {
>>   	uint32_t cid;
>>
>> @@ -358,10 +358,21 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
>>   	}
>>
>>   	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);
>> +		std::optional<v4l2_exposure_auto_type> mode;
>> +
>> +		switch (value.get<int32_t>()) {
>> +		case controls::ExposureTimeModeAuto:
>> +			mode = data->autoExposureMode_;
>> +			break;
>> +		case controls::ExposureTimeModeManual:
>> +			mode = data->manualExposureMode_;
>> +			break;
>> +		}
>> +
>> +		if (!mode)
>> +			return -EINVAL;
> 
> Here as well I don't think mode can be not initialized.

I believe it is, because the `CameraControlValidator` class does not check
actual values, only whether or not the control itself is supported. Maybe
it should be extended to do so.


Regards,
Barnabás Pőcze


> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
>> +
>> +		controls->set(V4L2_CID_EXPOSURE_AUTO, static_cast<int32_t>(*mode));
>>   		break;
>>   	}
>>
>> @@ -399,7 +410,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
>>   	ControlList controls(data->video_->controls());
>>
>>   	for (const auto &[id, value] : request->controls())
>> -		processControl(&controls, id, value);
>> +		processControl(data, &controls, id, value);
>>
>>   	for (const auto &ctrl : controls)
>>   		LOG(UVC, Debug)
>> --
>> 2.48.1
>>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
index 5c9025d9b..4ff79c291 100644
--- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
+++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
@@ -99,8 +99,8 @@  public:
 	bool match(DeviceEnumerator *enumerator) override;
 
 private:
-	int processControl(ControlList *controls, unsigned int id,
-			   const ControlValue &value);
+	int processControl(const UVCCameraData *data, ControlList *controls,
+			   unsigned int id, const ControlValue &value);
 	int processControls(UVCCameraData *data, Request *request);
 
 	bool acquireDevice(Camera *camera) override;
@@ -313,8 +313,8 @@  void PipelineHandlerUVC::stopDevice(Camera *camera)
 	data->video_->releaseBuffers();
 }
 
-int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
-				       const ControlValue &value)
+int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *controls,
+				       unsigned int id, const ControlValue &value)
 {
 	uint32_t cid;
 
@@ -358,10 +358,21 @@  int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
 	}
 
 	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);
+		std::optional<v4l2_exposure_auto_type> mode;
+
+		switch (value.get<int32_t>()) {
+		case controls::ExposureTimeModeAuto:
+			mode = data->autoExposureMode_;
+			break;
+		case controls::ExposureTimeModeManual:
+			mode = data->manualExposureMode_;
+			break;
+		}
+
+		if (!mode)
+			return -EINVAL;
+
+		controls->set(V4L2_CID_EXPOSURE_AUTO, static_cast<int32_t>(*mode));
 		break;
 	}
 
@@ -399,7 +410,7 @@  int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
 	ControlList controls(data->video_->controls());
 
 	for (const auto &[id, value] : request->controls())
-		processControl(&controls, id, value);
+		processControl(data, &controls, id, value);
 
 	for (const auto &ctrl : controls)
 		LOG(UVC, Debug)