[v3,4/4] libcamera: pipeline: uvcvideo: Fix `ExposureTime` control handling
diff mbox series

Message ID 20250314174248.1015718-5-barnabas.pocze@ideasonboard.com
State Deferred
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 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@ideasonboard.com>
---
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 52 ++++++++++++--------
 1 file changed, 31 insertions(+), 21 deletions(-)

Comments

Jacopo Mondi March 21, 2025, 2:17 p.m. UTC | #1 
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@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
>
Barnabás Pőcze March 21, 2025, 2:33 p.m. UTC | #2 
Hi

2025. 03. 21. 15:17 keltezéssel, Jacopo Mondi írta:
> 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@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 ?

If the controls cannot be set, then I think the state should not be changed, this
is why it operates on a copy, which is only persisted after `setControls()` succeeds.


Regards,
Barnabás Pőcze

> 
>> +	}
>> +
>> +	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
>>
Jacopo Mondi March 21, 2025, 2:41 p.m. UTC | #3 
On Fri, Mar 21, 2025 at 03:33:59PM +0100, Barnabás Pőcze wrote:
> Hi
>
> 2025. 03. 21. 15:17 keltezéssel, Jacopo Mondi írta:
> > 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@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 ?
>
> If the controls cannot be set, then I think the state should not be changed, this
> is why it operates on a copy, which is only persisted after `setControls()` succeeds.

Which control ? Even if ABSOLUTE cannot be set (because we're in AUTO)
if the mode is changed, it should anyway be

Ah you mean to protect against a failure in

	int ret = data->video_->setControls(&controls);

?

Then yes, maybe keep the state update after this makes sense

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

Thanks
  j


>
>
> Regards,
> Barnabás Pőcze
>
> >
> > > +	}
> > > +
> > > +	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
> > >
>

Patch
diff mbox series 

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);
+	}
+
+	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)