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

[Barnabás Pőcze]

Hi


2025. 03. 04. 11:09 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Mon, Mar 03, 2025 at 02:42:34PM +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.
>>
>> To fix this, retrieve the control with the proper type - `int32_t` -, and
>> use the V4L2 mode stored in `UVCCameraData` that was selected earlier in
>> `UVCCameraData::addControl()` for the given libcamera exposure time mode.
>>
>> 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 | 32 ++++++++++++++------
>>   1 file changed, 23 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>> index a6cc37366..dc3e85bd8 100644
>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>> @@ -97,8 +97,8 @@ public:
>>   	bool match(DeviceEnumerator *enumerator) override;
>>
>>   private:
>> -	int processControl(ControlList *controls, unsigned int id,
>> -			   const ControlValue &value);
>> +	int processControl(UVCCameraData *data, ControlList *controls,
>> +			   unsigned int id, const ControlValue &value);
>>   	int processControls(UVCCameraData *data, Request *request);
>>
>>   	bool acquireDevice(Camera *camera) override;
>> @@ -291,8 +291,8 @@ void PipelineHandlerUVC::stopDevice(Camera *camera)
>>   	data->video_->releaseBuffers();
>>   }
>>
>> -int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
>> -				       const ControlValue &value)
>> +int PipelineHandlerUVC::processControl(UVCCameraData *data, ControlList *controls,
>> +				       unsigned int id, const ControlValue &value)
>>   {
>>   	uint32_t cid;
>>
>> @@ -336,10 +336,24 @@ 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);
>> +		int32_t exposureMode;
>> +
>> +		switch (value.get<int32_t>()) {
>> +		case controls::ExposureTimeModeAuto:
>> +			if (!data->autoExposureMode_)
>> +				return -EINVAL;
>> +			exposureMode = *data->autoExposureMode_;
> 
> That's more an open question, but according to your previous patch
> 
>                 if (exposureModes[V4L2_EXPOSURE_AUTO])
>                         autoExposureMode_ = V4L2_EXPOSURE_AUTO;
>                 else if (exposureModes[V4L2_EXPOSURE_APERTURE_PRIORITY])
>                         autoExposureMode_ = V4L2_EXPOSURE_APERTURE_PRIORITY;
> 
> If the camera supported both V4L2_EXPOSURE_AUTO and
> V4L2_EXPOSURE_APERTURE_PRIORITY autoExposureMode_ will always be
> V4L2_EXPOSURE_AUTO meaning that we effectively make it impossible to
> support APERTURE_PRIORITY by using auto exposure and manual gain.

I based the order on the comment in the code:

		 * ExposureTimeModeAuto = { V4L2_EXPOSURE_AUTO,
		 * 			    V4L2_EXPOSURE_APERTURE_PRIORITY }
		 *
		 *
		 * ExposureTimeModeManual = { V4L2_EXPOSURE_MANUAL,
		 *			      V4L2_EXPOSURE_SHUTTER_PRIORITY }


> 
> I wonder if the above condition shouldn't be changed to
> 
>                 if (exposureModes[V4L2_EXPOSURE_APERTURE_PRIORITY])
>                         autoExposureMode_ = V4L2_EXPOSURE_APERTURE_PRIORITY;
>                 else if (exposureModes[V4L2_EXPOSURE_AUTO])
>                         autoExposureMode_ = V4L2_EXPOSURE_AUTO;
> 
> so that if the camera supports _PRIORITY modes we use them to allow
> implementing _PRIORITY modes in libcamera with ExposureTimeMode and
> AnalogueGainMode.
> 
> True that, if a user sets controls::ExposureTimeModeAuto and
> controls::AnalogueGainModeAuto and here we set APERTURE_PRIORITY,
> we're breaking it.
> 
> Again, I'm not sure how many uvc cameras support PRIORITY modes, but
> if we want to support the full handling of PRIORITY we should also
> inspect AnalogueGainMode.

I only have a single UVC camera that supports `V4L2_CID_EXPOSURE_AUTO`, and
it implements `V4L2_EXPOSURE_MANUAL` and `V4L2_EXPOSURE_APERTURE_PRIORITY`.


> 
> The pipeline at the moment doesn't even register AnalogueGainMode so I
> guess we can don't care about PRIORITIES, so what you have here is
> fine.
> 
> If someone wants to actually support priority modes in uvc, then there
> is a bigger rework to be done most probably. I wonder if we should
> capture it in a comment at least (can be a separate patch)

I will open a bug report about this if that's fine?

If I understand it correctly:

                   gain
                A         M
exposure A   auto    aperture
          M  shutter   manual

This is how the two libcamera controls would be mapped to the v4l2 control.
Will it not be an issue that you cannot express the supported feature set
with ControlInfo exactly? E.g. if three are supported, or if two that are
placed diagonally in the above table. I don't know if this is just theoretical,
if such scenario is even possible/allowed.


Regards,
Barnabás Pőcze

> 
>> +			break;
>> +		case controls::ExposureTimeModeManual:
>> +			if (!data->manualExposureMode_)
>> +				return -EINVAL;
>> +			exposureMode = *data->manualExposureMode_;
>> +			break;
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +
>> +		controls->set(V4L2_CID_EXPOSURE_AUTO, exposureMode);
> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> 
> Thanks
>    j
> 
>>   		break;
>>   	}
>>
>> @@ -377,7 +391,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
>>