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

17387 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>
---
 src/libcamera/controls.cpp | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

--
2.34.1

Quoting Christian Rauch via libcamera-devel (2022-09-21 21:31:48)
> 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>
> ---
>  src/libcamera/controls.cpp | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index bc3db4f6..e7251507 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -701,6 +701,28 @@ bool ControlInfoMap::validate()
>                                       ? ControlTypeInteger32 : id->type();
>                 const ControlInfo &info = ctrl.second;
> 
> +               if (!(info.min().isArray() == info.max().isArray() &&
> +                     info.min().numElements() == info.max().numElements())) {

Does numElements work even if the type is not an array? (i.e. will it
return '1'?)

If so - that could perhaps be simplified to just:
		if (info.min().numElements() != info.max().numElements())



> +                       LOG(Controls, Error)
> +                               << "Control " << utils::hex(id->id())
> +                               << " range must have the same dimension.";
> +                       return false;
> +               }
> +
> +               if (info.min().type() != info.max().type()) {

This will already enforce the types are the same. But perhaps the type
checking should be before the size checking above.

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

Perhaps neater over two lines, and I believe the second parenthesis
isn't required:

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



> +                       LOG(Controls, Error)
> +                               << "Control " << utils::hex(id->id())

Do we have access to any human readable string here instead of a hex of
the id()?

If so - It would be better to use that (but it may not be available)

With those considered:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

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

Hi Kieran,

Am 22.09.22 um 13:23 schrieb Kieran Bingham:
> Quoting Christian Rauch via libcamera-devel (2022-09-21 21:31:48)
>> 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>
>> ---
>>  src/libcamera/controls.cpp | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
>> index bc3db4f6..e7251507 100644
>> --- a/src/libcamera/controls.cpp
>> +++ b/src/libcamera/controls.cpp
>> @@ -701,6 +701,28 @@ bool ControlInfoMap::validate()
>>                                       ? ControlTypeInteger32 : id->type();
>>                 const ControlInfo &info = ctrl.second;
>>
>> +               if (!(info.min().isArray() == info.max().isArray() &&
>> +                     info.min().numElements() == info.max().numElements())) {
>
> Does numElements work even if the type is not an array? (i.e. will it
> return '1'?)
>
> If so - that could perhaps be simplified to just:
> 		if (info.min().numElements() != info.max().numElements())

numElements() will return a value in any case. I think it will be 0 for
scalars, but without checking isArray() it is not clear if this is a
scalar or empty array.

>
>> +                       LOG(Controls, Error)
>> +                               << "Control " << utils::hex(id->id())
>> +                               << " range must have the same dimension.";
>> +                       return false;
>> +               }
>> +
>> +               if (info.min().type() != info.max().type()) {
>
> This will already enforce the types are the same. But perhaps the type
> checking should be before the size checking above.

Not sure why "already". I think this is the first place we check the
min/max types.

>
>> +                       LOG(Controls, Error)
>> +                               << "Control " << utils::hex(id->id())
>> +                               << " range types mismatch";
>> +                       return false;
>> +               }
>> +
>> +               if (info.def().type() != ControlTypeNone && (info.min().type() != info.def().type())) {
>
> Perhaps neater over two lines, and I believe the second parenthesis
> isn't required:
>
> 		if (info.def().type() != ControlTypeNone &&
> 		    info.min().type() != info.def().type()) {
>
>
>
>> +                       LOG(Controls, Error)
>> +                               << "Control " << utils::hex(id->id())
>
> Do we have access to any human readable string here instead of a hex of
> the id()?
>
> If so - It would be better to use that (but it may not be available)

I used the hex-id as in the other test, but changed this to the 'name'.

>
> With those considered:
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>> +                               << " default value and info type mismatch";
>> +                       return false;
>> +               }
>> +
>>                 if (info.min().type() != rangeType) {
>>                         LOG(Controls, Error)
>>                                 << "Control " << utils::hex(id->id())
>> --
>> 2.34.1
>>