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

[Umang Jain]

Hi,

On 10/7/22 8:14 PM, Laurent Pinchart wrote:
> Hi Umang,
>
> On Fri, Oct 07, 2022 at 08:06:55PM +0530, Umang Jain wrote:
>> 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
> It happens with '[]'.

Ah that makes sense now... my python chops a bit rusty..
>
>>> +
>>> +            # 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}>"
>>>    
>>>