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

[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 at gmx.de>
> Reviewed-by: Kieran Bingham <kieran.bingham at 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;
>  		}

-- 
Regards,

Laurent Pinchart