[PATCH v3 2/4] libcamera: pipeline: uvcvideo: Fix `ExposureTimeMode` control setting

[Barnabás Pőcze]

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