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

[Christian Rauch]

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 at 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 at 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
>>