[v1,3/4] libcamera: v4l2_device: Use `bool` for boolean controls
diff mbox series

Message ID 20260325092659.79453-3-barnabas.pocze@ideasonboard.com
State New
Headers show
Series
  • [v1,1/4] libcamera: v4l2_device: Ensure consistent control types
Related show

Commit Message

Barnabás Pőcze March 25, 2026, 9:26 a.m. UTC 
The `ControlInfo` generated by `V4L2Device::v4l2ControlInfo()` already
uses the `bool` type, and there is no reason to disallow setting proper
"bool" values.

So adjust `updateControls()` and `setControls()` accordingly, and also
do the necessary adjustments wrt. `V4L2_CID_{V,H}FLIP`.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/libcamera/sensor/camera_sensor_legacy.cpp | 10 ++++------
 src/libcamera/sensor/camera_sensor_raw.cpp    |  6 ++----
 src/libcamera/v4l2_device.cpp                 | 14 ++++++++++++++
 3 files changed, 20 insertions(+), 10 deletions(-)

Comments

Jacopo Mondi March 25, 2026, 4:50 p.m. UTC | #1 
Hi Barnabás

On Wed, Mar 25, 2026 at 10:26:58AM +0100, Barnabás Pőcze wrote:
> The `ControlInfo` generated by `V4L2Device::v4l2ControlInfo()` already
> uses the `bool` type, and there is no reason to disallow setting proper
> "bool" values.
>
> So adjust `updateControls()` and `setControls()` accordingly, and also
> do the necessary adjustments wrt. `V4L2_CID_{V,H}FLIP`.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/libcamera/sensor/camera_sensor_legacy.cpp | 10 ++++------
>  src/libcamera/sensor/camera_sensor_raw.cpp    |  6 ++----
>  src/libcamera/v4l2_device.cpp                 | 14 ++++++++++++++
>  3 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
> index 6a683821f..99ce7ffb0 100644
> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp
> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp
> @@ -212,9 +212,9 @@ int CameraSensorLegacy::init()
>  	 */
>  	ControlList ctrls(subdev_->controls());
>  	if (subdev_->controls().find(V4L2_CID_HFLIP) != subdev_->controls().end())
> -		ctrls.set(V4L2_CID_HFLIP, 0);
> +		ctrls.set(V4L2_CID_HFLIP, false);
>  	if (subdev_->controls().find(V4L2_CID_VFLIP) != subdev_->controls().end())
> -		ctrls.set(V4L2_CID_VFLIP, 0);
> +		ctrls.set(V4L2_CID_VFLIP, false);
>  	subdev_->setControls(&ctrls);
>
>  	/* Enumerate, sort and cache media bus codes and sizes. */
> @@ -762,10 +762,8 @@ int CameraSensorLegacy::setFormat(V4L2SubdeviceFormat *format, Transform transfo
>  	if (supportFlips_) {
>  		ControlList flipCtrls(subdev_->controls());
>
> -		flipCtrls.set(V4L2_CID_HFLIP,
> -			      static_cast<int32_t>(!!(transform & Transform::HFlip)));
> -		flipCtrls.set(V4L2_CID_VFLIP,
> -			      static_cast<int32_t>(!!(transform & Transform::VFlip)));
> +		flipCtrls.set(V4L2_CID_HFLIP, !!(transform & Transform::HFlip));
> +		flipCtrls.set(V4L2_CID_VFLIP, !!(transform & Transform::VFlip));
>
>  		int ret = subdev_->setControls(&flipCtrls);
>  		if (ret)
> diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp
> index 10eba0331..882c1cc1a 100644
> --- a/src/libcamera/sensor/camera_sensor_raw.cpp
> +++ b/src/libcamera/sensor/camera_sensor_raw.cpp
> @@ -822,10 +822,8 @@ int CameraSensorRaw::setFormat(V4L2SubdeviceFormat *format, Transform transform)
>  	if (supportFlips_) {
>  		ControlList flipCtrls(subdev_->controls());
>
> -		flipCtrls.set(V4L2_CID_HFLIP,
> -			      static_cast<int32_t>(!!(transform & Transform::HFlip)));
> -		flipCtrls.set(V4L2_CID_VFLIP,
> -			      static_cast<int32_t>(!!(transform & Transform::VFlip)));
> +		flipCtrls.set(V4L2_CID_HFLIP, !!(transform & Transform::HFlip));
> +		flipCtrls.set(V4L2_CID_VFLIP, !!(transform & Transform::VFlip));
>
>  		int ret = subdev_->setControls(&flipCtrls);
>  		if (ret)

Again, I'm not sure if the below changes are required to fix these as
the control type of V4L2_CID_VFLIP and V4L2_CID_HFLIP is V4L2_CTRL_TYPE_BOOLEAN
already

> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index db1eb70e4..426664b83 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -327,6 +327,17 @@ int V4L2Device::setControls(ControlList *ctrls, const V4L2Request *request)
>  		/* Set the v4l2_ext_control value for the write operation. */
>  		ControlValue &value = ctrl->second;
>  		switch (iter->first->type()) {
> +		case ControlTypeBool: {
> +			if (value.isArray()) {
> +				LOG(V4L2, Error)
> +					<< "Array of bool not supported for control " << utils::hex(id);
> +				return -ENOTSUP;
> +			}
> +
> +			v4l2Ctrl.value = value.get<bool>();
> +			break;
> +		}
> +
>  		case ControlTypeUnsigned16: {
>  			if (value.isArray()) {
>  				Span<uint8_t> data = value.data();
> @@ -824,6 +835,9 @@ void V4L2Device::updateControls(ControlList *ctrls,
>  		ASSERT(iter != controls_.end());
>
>  		switch (iter->first->type()) {
> +		case ControlTypeBool:
> +			value.set<bool>(v4l2Ctrl.value);
> +			break;

but this is certainly more correct

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

Thanks
  j

>  		case ControlTypeByte:
>  			value.set<uint8_t>(v4l2Ctrl.value);
>  			break;
> --
> 2.53.0
>
Barnabás Pőcze March 27, 2026, 7:47 a.m. UTC | #2 
2026. 03. 25. 17:50 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Wed, Mar 25, 2026 at 10:26:58AM +0100, Barnabás Pőcze wrote:
>> The `ControlInfo` generated by `V4L2Device::v4l2ControlInfo()` already
>> uses the `bool` type, and there is no reason to disallow setting proper
>> "bool" values.
>>
>> So adjust `updateControls()` and `setControls()` accordingly, and also
>> do the necessary adjustments wrt. `V4L2_CID_{V,H}FLIP`.
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   src/libcamera/sensor/camera_sensor_legacy.cpp | 10 ++++------
>>   src/libcamera/sensor/camera_sensor_raw.cpp    |  6 ++----
>>   src/libcamera/v4l2_device.cpp                 | 14 ++++++++++++++
>>   3 files changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
>> index 6a683821f..99ce7ffb0 100644
>> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp
>> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp
>> @@ -212,9 +212,9 @@ int CameraSensorLegacy::init()
>>   	 */
>>   	ControlList ctrls(subdev_->controls());
>>   	if (subdev_->controls().find(V4L2_CID_HFLIP) != subdev_->controls().end())
>> -		ctrls.set(V4L2_CID_HFLIP, 0);
>> +		ctrls.set(V4L2_CID_HFLIP, false);
>>   	if (subdev_->controls().find(V4L2_CID_VFLIP) != subdev_->controls().end())
>> -		ctrls.set(V4L2_CID_VFLIP, 0);
>> +		ctrls.set(V4L2_CID_VFLIP, false);
>>   	subdev_->setControls(&ctrls);
>>
>>   	/* Enumerate, sort and cache media bus codes and sizes. */
>> @@ -762,10 +762,8 @@ int CameraSensorLegacy::setFormat(V4L2SubdeviceFormat *format, Transform transfo
>>   	if (supportFlips_) {
>>   		ControlList flipCtrls(subdev_->controls());
>>
>> -		flipCtrls.set(V4L2_CID_HFLIP,
>> -			      static_cast<int32_t>(!!(transform & Transform::HFlip)));
>> -		flipCtrls.set(V4L2_CID_VFLIP,
>> -			      static_cast<int32_t>(!!(transform & Transform::VFlip)));
>> +		flipCtrls.set(V4L2_CID_HFLIP, !!(transform & Transform::HFlip));
>> +		flipCtrls.set(V4L2_CID_VFLIP, !!(transform & Transform::VFlip));
>>
>>   		int ret = subdev_->setControls(&flipCtrls);
>>   		if (ret)
>> diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp
>> index 10eba0331..882c1cc1a 100644
>> --- a/src/libcamera/sensor/camera_sensor_raw.cpp
>> +++ b/src/libcamera/sensor/camera_sensor_raw.cpp
>> @@ -822,10 +822,8 @@ int CameraSensorRaw::setFormat(V4L2SubdeviceFormat *format, Transform transform)
>>   	if (supportFlips_) {
>>   		ControlList flipCtrls(subdev_->controls());
>>
>> -		flipCtrls.set(V4L2_CID_HFLIP,
>> -			      static_cast<int32_t>(!!(transform & Transform::HFlip)));
>> -		flipCtrls.set(V4L2_CID_VFLIP,
>> -			      static_cast<int32_t>(!!(transform & Transform::VFlip)));
>> +		flipCtrls.set(V4L2_CID_HFLIP, !!(transform & Transform::HFlip));
>> +		flipCtrls.set(V4L2_CID_VFLIP, !!(transform & Transform::VFlip));
>>
>>   		int ret = subdev_->setControls(&flipCtrls);
>>   		if (ret)
> 
> Again, I'm not sure if the below changes are required to fix these as
> the control type of V4L2_CID_VFLIP and V4L2_CID_HFLIP is V4L2_CTRL_TYPE_BOOLEAN
> already

That's true, but in libcamera bool controls are handled as V4L2_CTRL_TYPE_INTEGER,
and v4l2 controls don't have `Control<...>` objects, so the types are
entirely deduced from the argument used in `ControlList::set()`, so they
must be adjusted from int to bool.


> 
>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
>> index db1eb70e4..426664b83 100644
>> --- a/src/libcamera/v4l2_device.cpp
>> +++ b/src/libcamera/v4l2_device.cpp
>> @@ -327,6 +327,17 @@ int V4L2Device::setControls(ControlList *ctrls, const V4L2Request *request)
>>   		/* Set the v4l2_ext_control value for the write operation. */
>>   		ControlValue &value = ctrl->second;
>>   		switch (iter->first->type()) {
>> +		case ControlTypeBool: {
>> +			if (value.isArray()) {
>> +				LOG(V4L2, Error)
>> +					<< "Array of bool not supported for control " << utils::hex(id);
>> +				return -ENOTSUP;
>> +			}
>> +
>> +			v4l2Ctrl.value = value.get<bool>();
>> +			break;
>> +		}
>> +
>>   		case ControlTypeUnsigned16: {
>>   			if (value.isArray()) {
>>   				Span<uint8_t> data = value.data();
>> @@ -824,6 +835,9 @@ void V4L2Device::updateControls(ControlList *ctrls,
>>   		ASSERT(iter != controls_.end());
>>
>>   		switch (iter->first->type()) {
>> +		case ControlTypeBool:
>> +			value.set<bool>(v4l2Ctrl.value);
>> +			break;
> 
> but this is certainly more correct
> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> Thanks
>    j
> 
>>   		case ControlTypeByte:
>>   			value.set<uint8_t>(v4l2Ctrl.value);
>>   			break;
>> --
>> 2.53.0
>>

Patch
diff mbox series 

diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
index 6a683821f..99ce7ffb0 100644
--- a/src/libcamera/sensor/camera_sensor_legacy.cpp
+++ b/src/libcamera/sensor/camera_sensor_legacy.cpp
@@ -212,9 +212,9 @@  int CameraSensorLegacy::init()
 	 */
 	ControlList ctrls(subdev_->controls());
 	if (subdev_->controls().find(V4L2_CID_HFLIP) != subdev_->controls().end())
-		ctrls.set(V4L2_CID_HFLIP, 0);
+		ctrls.set(V4L2_CID_HFLIP, false);
 	if (subdev_->controls().find(V4L2_CID_VFLIP) != subdev_->controls().end())
-		ctrls.set(V4L2_CID_VFLIP, 0);
+		ctrls.set(V4L2_CID_VFLIP, false);
 	subdev_->setControls(&ctrls);
 
 	/* Enumerate, sort and cache media bus codes and sizes. */
@@ -762,10 +762,8 @@  int CameraSensorLegacy::setFormat(V4L2SubdeviceFormat *format, Transform transfo
 	if (supportFlips_) {
 		ControlList flipCtrls(subdev_->controls());
 
-		flipCtrls.set(V4L2_CID_HFLIP,
-			      static_cast<int32_t>(!!(transform & Transform::HFlip)));
-		flipCtrls.set(V4L2_CID_VFLIP,
-			      static_cast<int32_t>(!!(transform & Transform::VFlip)));
+		flipCtrls.set(V4L2_CID_HFLIP, !!(transform & Transform::HFlip));
+		flipCtrls.set(V4L2_CID_VFLIP, !!(transform & Transform::VFlip));
 
 		int ret = subdev_->setControls(&flipCtrls);
 		if (ret)
diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp
index 10eba0331..882c1cc1a 100644
--- a/src/libcamera/sensor/camera_sensor_raw.cpp
+++ b/src/libcamera/sensor/camera_sensor_raw.cpp
@@ -822,10 +822,8 @@  int CameraSensorRaw::setFormat(V4L2SubdeviceFormat *format, Transform transform)
 	if (supportFlips_) {
 		ControlList flipCtrls(subdev_->controls());
 
-		flipCtrls.set(V4L2_CID_HFLIP,
-			      static_cast<int32_t>(!!(transform & Transform::HFlip)));
-		flipCtrls.set(V4L2_CID_VFLIP,
-			      static_cast<int32_t>(!!(transform & Transform::VFlip)));
+		flipCtrls.set(V4L2_CID_HFLIP, !!(transform & Transform::HFlip));
+		flipCtrls.set(V4L2_CID_VFLIP, !!(transform & Transform::VFlip));
 
 		int ret = subdev_->setControls(&flipCtrls);
 		if (ret)
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index db1eb70e4..426664b83 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -327,6 +327,17 @@  int V4L2Device::setControls(ControlList *ctrls, const V4L2Request *request)
 		/* Set the v4l2_ext_control value for the write operation. */
 		ControlValue &value = ctrl->second;
 		switch (iter->first->type()) {
+		case ControlTypeBool: {
+			if (value.isArray()) {
+				LOG(V4L2, Error)
+					<< "Array of bool not supported for control " << utils::hex(id);
+				return -ENOTSUP;
+			}
+
+			v4l2Ctrl.value = value.get<bool>();
+			break;
+		}
+
 		case ControlTypeUnsigned16: {
 			if (value.isArray()) {
 				Span<uint8_t> data = value.data();
@@ -824,6 +835,9 @@  void V4L2Device::updateControls(ControlList *ctrls,
 		ASSERT(iter != controls_.end());
 
 		switch (iter->first->type()) {
+		case ControlTypeBool:
+			value.set<bool>(v4l2Ctrl.value);
+			break;
 		case ControlTypeByte:
 			value.set<uint8_t>(v4l2Ctrl.value);
 			break;