[libcamera-devel] [PATCH v2 2/4] utils: gen-controls: Improve YAML notation for variable-size array controls

[Umang Jain]

Hi Laurent,

Thank you for the patch.

On 10/7/22 4:37 AM, Laurent Pinchart via libcamera-devel wrote:
> Array controls specify the array size through the YAML 'size' element,
> which stores a list of values, one per dimension. Variable-size arrays
> currently use an empty 'size' list, which prevents describing the number
> of dimensions of the array.
>
> Improve this by using the same notation for fixed-size and variable-size
> array controls. Dimensions that are not fixed are described as a string
> instead of an integer, such as [n], [n,3] or [w,h]. The strings have
> currently no special meaning, this may change in the future.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>   src/libcamera/control_ids.yaml  |  2 +-
>   src/libcamera/property_ids.yaml |  4 ++--
>   utils/gen-controls.py           | 31 +++++++++++++++++++++++++------
>   3 files changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index c93f362463dd..a456e6c0358f 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -525,7 +525,7 @@ controls:
>           the window where the focal distance for the objects shown in that part
>           of the image are closest to the camera.
>   
> -      size: []
> +      size: [n]
>   
>     - AfTrigger:
>         type: int32_t
> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> index 960e254495b7..cb55e0ed2283 100644
> --- a/src/libcamera/property_ids.yaml
> +++ b/src/libcamera/property_ids.yaml
> @@ -497,7 +497,7 @@ controls:
>   
>     - PixelArrayOpticalBlackRectangles:
>         type: Rectangle
> -      size: []
> +      size: [n]
>         description: |
>           The pixel array region(s) which contain optical black pixels
>           considered valid for calibration purposes.
> @@ -592,7 +592,7 @@ controls:
>   
>     - PixelArrayActiveAreas:
>         type: Rectangle
> -      size: []
> +      size: [n]
>         description: |
>           The PixelArrayActiveAreas property defines the (possibly multiple and
>           overlapping) portions of the camera sensor readable pixel matrix
> diff --git a/utils/gen-controls.py b/utils/gen-controls.py
> index d6b4e30001b2..45b7081e6b1d 100755
> --- a/utils/gen-controls.py
> +++ b/utils/gen-controls.py
> @@ -39,11 +39,33 @@ class Control(object):
>           self.__name = name
>           self.__data = data
>           self.__enum_values = None
> +        self.__size = None
>   
>           enum_values = data.get('enum')
>           if enum_values is not None:
>               self.__enum_values = [ControlEnum(enum) for enum in enum_values]
>   
> +        size = self.__data.get('size')
> +        if size is not None:
> +            if len(size) == 0:
> +                raise RuntimeError(f'Control `{self.__name}` size must have at least one dimension')

Not sure if there's a case where size is not None, but has len(size) == 
0; Anyways, it's not harmful to have a guard here
> +
> +            # Compute the total number of elemens in the array. If any of the

s/elemens/elements/
> +            # array dimension is a string, the array is variable-sized.
> +            num_elems = 1
> +            for dim in size:
> +                if type(dim) is str:
> +                    num_elems = 0
> +                    break
> +
> +                dim = int(dim)
> +                if dim <= 0:
> +                    raise RuntimeError(f'Control `{self.__name}` size must have positive values only')
> +
> +                num_elems *= dim
> +
> +            self.__size = num_elems
> +

Looks good!

Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>

>       @property
>       def description(self):
>           """The control description"""
> @@ -86,15 +108,12 @@ class Control(object):
>           if typ == 'string':
>               return 'std::string'
>   
> -        if size is None:
> +        if self.__size is None:
>               return typ
>   
> -        if len(size) > 0:
> -            # fixed-sized Span
> -            span_size = reduce(operator.mul, size)
> -            return f"Span<const {typ}, {span_size}>"
> +        if self.__size:
> +            return f"Span<const {typ}, {self.__size}>"
>           else:
> -            # variable-sized Span
>               return f"Span<const {typ}>"
>   
>