[v1,1/4] libcamera: v4l2_device: Ensure consistent control types
diff mbox series

Message ID 20260325092659.79453-1-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 
While `setControls()` uses the specific types, `updateControls()` converts
everything to 32-bit integers (except 64-bit integers). This causes, for
example, that a 16-bit unsigned integer is retrieved as `int32_t` even
though there is a specific libcamera control type for it. What's even
more surprising is that setting such a control requires a `ControlValue`
with `uint16_t`, but when `setControls()` updates the list to contain
the actually applied values, it will now have type `int32_t`.

So make sure that `updateControls()` uses the appropriate type.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/libcamera/v4l2_device.cpp | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

Comments

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

On Wed, Mar 25, 2026 at 10:26:56AM +0100, Barnabás Pőcze wrote:
> While `setControls()` uses the specific types, `updateControls()` converts
> everything to 32-bit integers (except 64-bit integers). This causes, for
> example, that a 16-bit unsigned integer is retrieved as `int32_t` even
> though there is a specific libcamera control type for it. What's even
> more surprising is that setting such a control requires a `ControlValue`
> with `uint16_t`, but when `setControls()` updates the list to contain
> the actually applied values, it will now have type `int32_t`.

I think this boils down the fact struct v4l2_ext_control has only

	union {
		__s32 value;
		__s64 value64;

for non-matrix controls.

The v4l2-ctrl framework populates "value" for all control types which
are < V4L2_CTRL_TYPE_INTEGER64.


>
> So make sure that `updateControls()` uses the appropriate type.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/libcamera/v4l2_device.cpp | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 67bbaa037..e0099cb7a 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -823,19 +823,26 @@ void V4L2Device::updateControls(ControlList *ctrls,
>  		ASSERT(iter != controls_.end());
>
>  		switch (iter->first->type()) {
> +		case ControlTypeByte:
> +			value.set<uint8_t>(v4l2Ctrl.value);
> +			break;
> +		case ControlTypeUnsigned16:
> +			value.set<uint16_t>(v4l2Ctrl.value);
> +			break;
> +		case ControlTypeInteger32:
> +			value.set<int32_t>(v4l2Ctrl.value);
> +			break;
> +		case ControlTypeUnsigned32:
> +			value.set<uint32_t>(v4l2Ctrl.value);
> +			break;

However, since we have types for the libcamera controls, I would say
that unless I'm missing something obvious it is fine to cast to the
actual libcamera control size even if it shouldn't practically change
anything ?

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

Thanks
   j
>  		case ControlTypeInteger64:
>  			value.set<int64_t>(v4l2Ctrl.value64);
>  			break;
> -
>  		default:
> -			/*
> -			 * Note: this catches the ControlTypeInteger32 case.
> -			 *
> -			 * \todo To be changed when support for string controls
> -			 * will be added.
> -			 */
> -			value.set<int32_t>(v4l2Ctrl.value);
> -			break;
> +			LOG(V4L2, Error)
> +				<< "Control " << utils::hex(id)
> +				<< " has unsupported type";
> +			continue;
>  		}
>
>  		ctrls->set(id, value);
> --
> 2.53.0
>
Jacopo Mondi March 25, 2026, 4:56 p.m. UTC | #2 
Hi Barnabás

On Wed, Mar 25, 2026 at 10:26:59AM +0100, Barnabás Pőcze wrote:
> When setting a control, if its type is not explicitly supported, do not fall
> back to int32_t as that is unlikely to be correct and will abort the program.
> Instead print an error and return -ENOTSUP.

If I understand correctly all v4l2-control types matched to libcamera
control types in  V4L2Device::v4l2CtrlType() are now handled.

>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/libcamera/v4l2_device.cpp | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 426664b83..6ad1d178e 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -394,9 +394,10 @@ int V4L2Device::setControls(ControlList *ctrls, const V4L2Request *request)
>  		}
>
>  		default:
> -			/* \todo To be changed to support strings. */

This mention strings, but strigs are deflected to ControlTypeNone in
V4L2Device::v4l2CtrlType()

> -			v4l2Ctrl.value = value.get<int32_t>();

and this wasn't correct.

> -			break;
> +			LOG(V4L2, Error)
> +				<< "Control " << utils::hex(id)
> +				<< " has unsupported type";
> +			return -ENOTSUP;

Will it break some users ? It shouldn't because of the above reasoning
around the v4l2-ctrl-to-libcamera-ctrl type translation

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

Thanks
  j
>  		}
>  	}
>
> --
> 2.53.0
>
Barnabás Pőcze March 27, 2026, 7:47 a.m. UTC | #3 
2026. 03. 25. 17:39 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Wed, Mar 25, 2026 at 10:26:56AM +0100, Barnabás Pőcze wrote:
>> While `setControls()` uses the specific types, `updateControls()` converts
>> everything to 32-bit integers (except 64-bit integers). This causes, for
>> example, that a 16-bit unsigned integer is retrieved as `int32_t` even
>> though there is a specific libcamera control type for it. What's even
>> more surprising is that setting such a control requires a `ControlValue`
>> with `uint16_t`, but when `setControls()` updates the list to contain
>> the actually applied values, it will now have type `int32_t`.
> 
> I think this boils down the fact struct v4l2_ext_control has only
> 
> 	union {
> 		__s32 value;
> 		__s64 value64;
> 
> for non-matrix controls.
> 
> The v4l2-ctrl framework populates "value" for all control types which
> are < V4L2_CTRL_TYPE_INTEGER64.
> 
> 
>>
>> So make sure that `updateControls()` uses the appropriate type.
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   src/libcamera/v4l2_device.cpp | 25 ++++++++++++++++---------
>>   1 file changed, 16 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
>> index 67bbaa037..e0099cb7a 100644
>> --- a/src/libcamera/v4l2_device.cpp
>> +++ b/src/libcamera/v4l2_device.cpp
>> @@ -823,19 +823,26 @@ void V4L2Device::updateControls(ControlList *ctrls,
>>   		ASSERT(iter != controls_.end());
>>
>>   		switch (iter->first->type()) {
>> +		case ControlTypeByte:
>> +			value.set<uint8_t>(v4l2Ctrl.value);
>> +			break;
>> +		case ControlTypeUnsigned16:
>> +			value.set<uint16_t>(v4l2Ctrl.value);
>> +			break;
>> +		case ControlTypeInteger32:
>> +			value.set<int32_t>(v4l2Ctrl.value);
>> +			break;
>> +		case ControlTypeUnsigned32:
>> +			value.set<uint32_t>(v4l2Ctrl.value);
>> +			break;
> 
> However, since we have types for the libcamera controls, I would say
> that unless I'm missing something obvious it is fine to cast to the
> actual libcamera control size even if it shouldn't practically change
> anything ?

It does change things, e.g. now `getControls()` will return controls
with potentially smaller integer types, not just int32_t and int64_t.
But as far as I could check every `getControls()` call, this does not
affect any existing code.


> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> Thanks
>     j
>>   		case ControlTypeInteger64:
>>   			value.set<int64_t>(v4l2Ctrl.value64);
>>   			break;
>> -
>>   		default:
>> -			/*
>> -			 * Note: this catches the ControlTypeInteger32 case.
>> -			 *
>> -			 * \todo To be changed when support for string controls
>> -			 * will be added.
>> -			 */
>> -			value.set<int32_t>(v4l2Ctrl.value);
>> -			break;
>> +			LOG(V4L2, Error)
>> +				<< "Control " << utils::hex(id)
>> +				<< " has unsupported type";
>> +			continue;
>>   		}
>>
>>   		ctrls->set(id, value);
>> --
>> 2.53.0
>>
Barnabás Pőcze March 27, 2026, 7:47 a.m. UTC | #4 
2026. 03. 25. 17:56 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Wed, Mar 25, 2026 at 10:26:59AM +0100, Barnabás Pőcze wrote:
>> When setting a control, if its type is not explicitly supported, do not fall
>> back to int32_t as that is unlikely to be correct and will abort the program.
>> Instead print an error and return -ENOTSUP.
> 
> If I understand correctly all v4l2-control types matched to libcamera
> control types in  V4L2Device::v4l2CtrlType() are now handled.
> 
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   src/libcamera/v4l2_device.cpp | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
>> index 426664b83..6ad1d178e 100644
>> --- a/src/libcamera/v4l2_device.cpp
>> +++ b/src/libcamera/v4l2_device.cpp
>> @@ -394,9 +394,10 @@ int V4L2Device::setControls(ControlList *ctrls, const V4L2Request *request)
>>   		}
>>
>>   		default:
>> -			/* \todo To be changed to support strings. */
> 
> This mention strings, but strigs are deflected to ControlTypeNone in
> V4L2Device::v4l2CtrlType()
> 
>> -			v4l2Ctrl.value = value.get<int32_t>();
> 
> and this wasn't correct.
> 
>> -			break;
>> +			LOG(V4L2, Error)
>> +				<< "Control " << utils::hex(id)
>> +				<< " has unsupported type";
>> +			return -ENOTSUP;
> 
> Will it break some users ? It shouldn't because of the above reasoning
> around the v4l2-ctrl-to-libcamera-ctrl type translation

As far I can tell, it shouldn't. Because as you said everything that is
handled in `v4l2CtrlType()` should be handled here as well. And other
types would have aborted the process in `value.get<int32_t>()` that
was previously here.


> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> Thanks
>    j
>>   		}
>>   	}
>>
>> --
>> 2.53.0
>>

Patch
diff mbox series 

diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index 67bbaa037..e0099cb7a 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -823,19 +823,26 @@  void V4L2Device::updateControls(ControlList *ctrls,
 		ASSERT(iter != controls_.end());
 
 		switch (iter->first->type()) {
+		case ControlTypeByte:
+			value.set<uint8_t>(v4l2Ctrl.value);
+			break;
+		case ControlTypeUnsigned16:
+			value.set<uint16_t>(v4l2Ctrl.value);
+			break;
+		case ControlTypeInteger32:
+			value.set<int32_t>(v4l2Ctrl.value);
+			break;
+		case ControlTypeUnsigned32:
+			value.set<uint32_t>(v4l2Ctrl.value);
+			break;
 		case ControlTypeInteger64:
 			value.set<int64_t>(v4l2Ctrl.value64);
 			break;
-
 		default:
-			/*
-			 * Note: this catches the ControlTypeInteger32 case.
-			 *
-			 * \todo To be changed when support for string controls
-			 * will be added.
-			 */
-			value.set<int32_t>(v4l2Ctrl.value);
-			break;
+			LOG(V4L2, Error)
+				<< "Control " << utils::hex(id)
+				<< " has unsupported type";
+			continue;
 		}
 
 		ctrls->set(id, value);