[PATCH v3 4/4] libcamera: pipeline: uvcvideo: Fix `ExposureTime` control handling

[Jacopo Mondi]

Hi Barnabás

On Fri, Mar 14, 2025 at 06:42:48PM +0100, Barnabás Pőcze wrote:
> The documentation of `ExposureTime` states that its value is ignored if
> `ExposureTimeMode` is not manual. This is currently not handled, and if
> `ExposureTimeMode` is automatic and `ExposureTime` is set, then the
> controls will be rejected, as expected.
>
> Only try to set `V4L2_CID_EXPOSURE_ABSOLUTE` if the current exposure mode
> is manual. To be able to handle requests that set both `ExposureTime{,Mode}`,
> process `ExposureTimeMode` first directly in `processControls()`, and store
> this new mode in a temporary location. Then have `processControl()` act on
> this temporary state, and only persist the temporary state if `setControls()`
> on the video device succeeds.
>
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=242
> Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
> ---
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 52 ++++++++++++--------
>  1 file changed, 31 insertions(+), 21 deletions(-)
>
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 7d882ebe1..22d286e49 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -108,7 +108,7 @@ public:
>  	bool match(DeviceEnumerator *enumerator) override;
>
>  private:
> -	int processControl(const UVCCameraData *data, ControlList *controls,
> +	int processControl(const UVCCameraData::State &state, ControlList *controls,
>  			   unsigned int id, const ControlValue &value);
>  	int processControls(UVCCameraData *data, Request *request);
>
> @@ -333,7 +333,7 @@ void PipelineHandlerUVC::stopDevice(Camera *camera)
>  	data->state_.reset();
>  }
>
> -int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *controls,
> +int PipelineHandlerUVC::processControl(const UVCCameraData::State &state, ControlList *controls,
>  				       unsigned int id, const ControlValue &value)
>  {
>  	uint32_t cid;
> @@ -378,26 +378,13 @@ int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c
>  	}
>
>  	case V4L2_CID_EXPOSURE_AUTO: {
> -		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));
> +		/* Handled directly in `processControls()`. */
>  		break;
>  	}
>
>  	case V4L2_CID_EXPOSURE_ABSOLUTE:
> -		controls->set(cid, value.get<int32_t>() / 100);
> +		if (state.exp == controls::ExposureTimeModeManual)
> +			controls->set(cid, value.get<int32_t>() / 100);
>  		break;
>
>  	case V4L2_CID_CONTRAST:
> @@ -428,9 +415,30 @@ int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c
>  int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
>  {
>  	ControlList controls(data->video_->controls());
> +	const auto &reqControls = request->controls();
> +	auto newState = data->state_;
> +
> +	if (const auto exp = reqControls.get(controls::ExposureTimeMode)) {
> +		std::optional<v4l2_exposure_auto_type> mode;
>
> -	for (const auto &[id, value] : request->controls())
> -		processControl(data, &controls, id, value);
> +		switch (*exp) {
> +		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));
> +		newState.exp = static_cast<controls::ExposureTimeModeEnum>(*exp);

Can't you just update data->state before calling processControl to
avoid passing newState to that function ?

> +	}
> +
> +	for (const auto &[id, value] : reqControls)
> +		processControl(newState, &controls, id, value);
>
>  	for (const auto &ctrl : controls)
>  		LOG(UVC, Debug)
> @@ -443,7 +451,9 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
>  		return ret < 0 ? ret : -EINVAL;
>  	}
>
> -	return ret;
> +	data->state_ = newState;
> +
> +	return 0;
>  }
>
>  int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request)
> --
> 2.48.1
>