[libcamera-devel,v2,1/5] define Span size as shape vector

15615 diff mbox series

This follows the convention in other Tensor APIs. Since all tensors are
represented as a Span with a single dimension, values provided in 'size'
are interpreted as fixed-size Spans, while an empty array ("[]") will be
interpreted as variable-sized Span.

Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
---
 src/libcamera/control_ids.yaml  | 2 +-
 src/libcamera/property_ids.yaml | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

--
2.25.1

Hi Christian,

Thank you for the patch.

The subject line should be

libcamera: controls: Define size of array controls as a shape vector

(for the right prefix, and because you're modifying controls, not the
Span class)

On Tue, Apr 05, 2022 at 01:42:11AM +0100, Christian Rauch via libcamera-devel wrote:
> This follows the convention in other Tensor APIs. Since all tensors are
> represented as a Span with a single dimension, values provided in 'size'
> are interpreted as fixed-size Spans, while an empty array ("[]") will be
> interpreted as variable-sized Span.

In tensorflow notation (and I suppose other tensor APIs), the [] shape
describes a scalar. Should we keep "n" to denote a variable number of
elements, but still apply the change that uses the comma as a separator
instead of the 'x' ? Otherwise a variable-length array of objects that
have three components would have "[3,]" as a shape, which looks quite
weird.

There may be alternatives, such as using "*" instead of "n" for
instance. I don't know if there's any standard(ish) notation.

> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> ---
>  src/libcamera/control_ids.yaml  | 2 +-
>  src/libcamera/property_ids.yaml | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index 9d4638ae..c3f593a1 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -291,7 +291,7 @@ controls:
>          transformation. The 3x3 matrix is stored in conventional reading
>          order in an array of 9 floating point values.
> 
> -      size: [3x3]
> +      size: [3,3]
> 
>    - ScalerCrop:
>        type: Rectangle
> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> index 12ecbce5..47c350ed 100644
> --- a/src/libcamera/property_ids.yaml
> +++ b/src/libcamera/property_ids.yaml
> @@ -497,7 +497,7 @@ controls:
> 
>    - PixelArrayOpticalBlackRectangles:
>        type: Rectangle
> -      size: [n]
> +      size: []
>        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: [n]
> +      size: []
>        description: |
>          The PixelArrayActiveAreas property defines the (possibly multiple and
>          overlapping) portions of the camera sensor readable pixel matrix

Hi Laurent,

Am 05.04.22 um 17:13 schrieb Laurent Pinchart:
> Hi Christian,
>
> Thank you for the patch.
>
> The subject line should be
>
> libcamera: controls: Define size of array controls as a shape vector
>
> (for the right prefix, and because you're modifying controls, not the
> Span class)
>
> On Tue, Apr 05, 2022 at 01:42:11AM +0100, Christian Rauch via libcamera-devel wrote:
>> This follows the convention in other Tensor APIs. Since all tensors are
>> represented as a Span with a single dimension, values provided in 'size'
>> are interpreted as fixed-size Spans, while an empty array ("[]") will be
>> interpreted as variable-sized Span.
>
> In tensorflow notation (and I suppose other tensor APIs), the [] shape
> describes a scalar. Should we keep "n" to denote a variable number of
> elements, but still apply the change that uses the comma as a separator
> instead of the 'x' ? Otherwise a variable-length array of objects that
> have three components would have "[3,]" as a shape, which looks quite
> weird.

We don't have to follow an established notation. For simplicity, I would
suggest that the "size" should be an array of the dimension sizes, since
this already matches the yaml format and is simple to parse.
In numpy, an empty array will give an empty shape ("np.empty([]).shape"
gives "()"). This would align with the proposal above. One-dimensional
shapes "[3,]" and "[3]" would be equivalent. I would not use a special
character to indicate variable-size Spans as this makes parsing more
complex.

>
> There may be alternatives, such as using "*" instead of "n" for
> instance. I don't know if there's any standard(ish) notation.
>
>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
>> ---
>>  src/libcamera/control_ids.yaml  | 2 +-
>>  src/libcamera/property_ids.yaml | 4 ++--
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
>> index 9d4638ae..c3f593a1 100644
>> --- a/src/libcamera/control_ids.yaml
>> +++ b/src/libcamera/control_ids.yaml
>> @@ -291,7 +291,7 @@ controls:
>>          transformation. The 3x3 matrix is stored in conventional reading
>>          order in an array of 9 floating point values.
>>
>> -      size: [3x3]
>> +      size: [3,3]
>>
>>    - ScalerCrop:
>>        type: Rectangle
>> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
>> index 12ecbce5..47c350ed 100644
>> --- a/src/libcamera/property_ids.yaml
>> +++ b/src/libcamera/property_ids.yaml
>> @@ -497,7 +497,7 @@ controls:
>>
>>    - PixelArrayOpticalBlackRectangles:
>>        type: Rectangle
>> -      size: [n]
>> +      size: []
>>        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: [n]
>> +      size: []
>>        description: |
>>          The PixelArrayActiveAreas property defines the (possibly multiple and
>>          overlapping) portions of the camera sensor readable pixel matrix
>