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

[Jacopo Mondi]

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

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)

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