[libcamera-devel,v3] libcamera: controls: validate all ControlInfo values

17388 diff mbox series

ControlInfoMap::validate only checks the 'min' type against the ControlId
type. Extend this with checks against the 'max' type and the 'def' type,
if a default is specified. This forces the min/max bounds to have the same
type as the controlled value, but leaves the default optional.

Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/libcamera/controls.cpp | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

--
2.34.1

Hi Christian,

Thank you for the patch, and sorry for the delay.

On Thu, Sep 22, 2022 at 10:22:13PM +0200, Christian Rauch via libcamera-devel wrote:
> ControlInfoMap::validate only checks the 'min' type against the ControlId
> type. Extend this with checks against the 'max' type and the 'def' type,
> if a default is specified. This forces the min/max bounds to have the same
> type as the controlled value, but leaves the default optional.
> 
> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/libcamera/controls.cpp | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index bc3db4f6..1e54a712 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -701,9 +701,32 @@ bool ControlInfoMap::validate()
>  				      ? ControlTypeInteger32 : id->type();
>  		const ControlInfo &info = ctrl.second;
> 
> +		if (!(info.min().isArray() == info.max().isArray() &&
> +		      info.min().numElements() == info.max().numElements())) {
> +			LOG(Controls, Error)
> +				<< "Control " << id->name()
> +				<< " range must have the same dimension.";
> +			return false;
> +		}

This I'm not too sure about, we still haven't reached a conclusion on
how min and max should be interpreted for array controls. Could you
split this change to a separate patch, to continue that discussion
without blocking the rest ?

> +
> +		if (info.min().type() != info.max().type()) {

This will fail to validate if the control has a minimum and no maximum,
or the other way around. We don't clearly document if that's allowed,
and I think it should be. Let's add the following documentation change
to this patch:

diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
index bc3db4f69388..5318930aa0fe 100644
--- a/src/libcamera/controls.cpp
+++ b/src/libcamera/controls.cpp
@@ -477,6 +477,11 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen
  * The constraints depend on the object the control applies to, and are
  * constant for the lifetime of that object. They are typically constructed by
  * pipeline handlers to describe the controls they support.
+ *
+ * The min(), max() and def() values may be of type ControlTypeNone if the
+ * control has no minimum bound, maximum bound or default value respectively.
+ * They shall otherwise store a value of the same type as the control that the
+ * ControlInfo instance refers to.
  */
 
 /**

> +			LOG(Controls, Error)
> +				<< "Control " << id->name()
> +				<< " range types mismatch";
> +			return false;
> +		}
> +
> +		if (info.def().type() != ControlTypeNone &&
> +			info.min().type() != info.def().type()) {

There's a convenient isNone() function that you can use here.

		if (!info.def().isNone() && info.min().type() != info.def().type()) {

> +			LOG(Controls, Error)
> +				<< "Control " << id->name()
> +				<< " default value and info type mismatch";
> +			return false;
> +		}
> +
>  		if (info.min().type() != rangeType) {

But I think we can simplify the min, max and def checks and accept the
none type for any of them with this:

		if ((!info.min().isNone() && info.min().type() != rangeType) ||
		    (!info.max().isNone() && info.max().type() != rangeType) ||
		    (!info.def().isNone() && info.def().type() != rangeType)) {

>  			LOG(Controls, Error)
> -				<< "Control " << utils::hex(id->id())
> +				<< "Control " << id->name()
>  				<< " type and info type mismatch";
>  			return false;
>  		}

Hi Laurent,

Thanks for the review.

Am 07.10.22 um 00:11 schrieb Laurent Pinchart:
> Hi Christian,
>
> Thank you for the patch, and sorry for the delay.
>
> On Thu, Sep 22, 2022 at 10:22:13PM +0200, Christian Rauch via libcamera-devel wrote:
>> ControlInfoMap::validate only checks the 'min' type against the ControlId
>> type. Extend this with checks against the 'max' type and the 'def' type,
>> if a default is specified. This forces the min/max bounds to have the same
>> type as the controlled value, but leaves the default optional.
>>
>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>   src/libcamera/controls.cpp | 25 ++++++++++++++++++++++++-
>>   1 file changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
>> index bc3db4f6..1e54a712 100644
>> --- a/src/libcamera/controls.cpp
>> +++ b/src/libcamera/controls.cpp
>> @@ -701,9 +701,32 @@ bool ControlInfoMap::validate()
>>   				      ? ControlTypeInteger32 : id->type();
>>   		const ControlInfo &info = ctrl.second;
>>
>> +		if (!(info.min().isArray() == info.max().isArray() &&
>> +		      info.min().numElements() == info.max().numElements())) {
>> +			LOG(Controls, Error)
>> +				<< "Control " << id->name()
>> +				<< " range must have the same dimension.";
>> +			return false;
>> +		}
>
> This I'm not too sure about, we still haven't reached a conclusion on
> how min and max should be interpreted for array controls. Could you
> split this change to a separate patch, to continue that discussion
> without blocking the rest ?

We do not make this decision here. This only checks that both are either
scalar or array and have the same dimensions. In both cases, "isArray"
and "numElements" are expected to return the same values, but the
ControlInfo can describe a scalar or an array control value.

>> +
>> +		if (info.min().type() != info.max().type()) {
>
> This will fail to validate if the control has a minimum and no maximum,
> or the other way around. We don't clearly document if that's allowed,

Are there real cases where only the minimum or maximum make sense? For
now, I wanted to make sure that min/max are correctly formatted. That
means that either they are both not provided or they both have the same
dimensionality.
Currently, I do not see such mixed cases and would wait until those
arise in practice. Until then, we can verify that the controls follow
the conventions that we have currently in place (i.e. all controls have
either min and max or neither).

> and I think it should be. Let's add the following documentation change
> to this patch:
>
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index bc3db4f69388..5318930aa0fe 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -477,6 +477,11 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen
>    * The constraints depend on the object the control applies to, and are
>    * constant for the lifetime of that object. They are typically constructed by
>    * pipeline handlers to describe the controls they support.
> + *
> + * The min(), max() and def() values may be of type ControlTypeNone if the
> + * control has no minimum bound, maximum bound or default value respectively.
> + * They shall otherwise store a value of the same type as the control that the
> + * ControlInfo instance refers to.
>    */
>
>   /**
>
>> +			LOG(Controls, Error)
>> +				<< "Control " << id->name()
>> +				<< " range types mismatch";
>> +			return false;
>> +		}
>> +
>> +		if (info.def().type() != ControlTypeNone &&
>> +			info.min().type() != info.def().type()) {
>
> There's a convenient isNone() function that you can use here.
>
> 		if (!info.def().isNone() && info.min().type() != info.def().type()) {
>
>> +			LOG(Controls, Error)
>> +				<< "Control " << id->name()
>> +				<< " default value and info type mismatch";
>> +			return false;
>> +		}
>> +
>>   		if (info.min().type() != rangeType) {
>
> But I think we can simplify the min, max and def checks and accept the
> none type for any of them with this:
>
> 		if ((!info.min().isNone() && info.min().type() != rangeType) ||
> 		    (!info.max().isNone() && info.max().type() != rangeType) ||
> 		    (!info.def().isNone() && info.def().type() != rangeType)) {
>
>>   			LOG(Controls, Error)
>> -				<< "Control " << utils::hex(id->id())
>> +				<< "Control " << id->name()
>>   				<< " type and info type mismatch";
>>   			return false;
>>   		}
>