[libcamera-devel,v8,2/4] libcamera: controls: Define size of array controls as a shape vector
diff mbox series

Message ID 20220610120338.96883-3-Rauch.Christian@gmx.de
State Superseded
Headers show
Series
  • generate and use fixed-sized Span Control types
Related show

Commit Message

Christian Rauch June 10, 2022, 12:03 p.m. UTC 
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>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/control_ids.yaml  | 4 ++--
 src/libcamera/property_ids.yaml | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

--
2.34.1

Comments

Laurent Pinchart July 4, 2022, 11:37 p.m. UTC | #1 
Hi Christian,

Thank you for the patch.

On Fri, Jun 10, 2022 at 01:03:36PM +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.

I like the new syntax, even if it doesn't make it clear how we'll handle
multi-dimensional arrays with variable numbers of elements in one or
multiple dimensions, but we can worry about that later when/if needed.

This patch by itself introduces compilation errors, for instance, with
clang-14,

In file included from ../../src/libcamera/camera_sensor.cpp:8:
In file included from ../../include/libcamera/internal/camera_sensor.h:17:
In file included from include/libcamera/control_ids.h:15:
../../include/libcamera/controls.h:403:8: error: no matching member function for call to 'set'
                val->set<T>(Span<const typename std::remove_cv_t<V>>{ value.begin(), value.size() });
                ~~~~~^~~~~~
../../src/libcamera/camera_sensor.cpp:421:14: note: in instantiation of function template specialization 'libcamera::ControlList::set<libcamera::Rectangle, libcamera::Rectangle>' requested here
        properties_.set(properties::PixelArrayActiveAreas, { activeArea_ });
                    ^
../../include/libcamera/controls.h:178:7: note: candidate function template not viable: no known conversion from 'Span<const typename std::remove_cv_t<Rectangle>>' (aka 'Span<const libcamera::Rectangle>') to 'const libcamera::Rectangle' for 1st argument
        void set(const T &value)
             ^
../../include/libcamera/controls.h:190:7: note: candidate template ignored: requirement 'details::is_span<libcamera::Rectangle>::value || std::is_same<std::string, libcamera::Rectangle>::value' was not satisfied [with T = libcamera::Rectangle]
        void set(const T &value)
             ^
1 error generated.

I'll update the Python generator script here to fix this, it will be
part of v9.

> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/control_ids.yaml  | 4 ++--
>  src/libcamera/property_ids.yaml | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index cd1d4512..f707c1f5 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
> @@ -515,7 +515,7 @@ controls:
>          the window where the focal distance for the objects shown in that part
>          of the image are closest to the camera.
> 
> -      size: [n]
> +      size: []
> 
>    - AfTrigger:
>        type: int32_t
> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> index 11b7ebdc..a87485d7 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
Laurent Pinchart July 4, 2022, 11:50 p.m. UTC | #2 
On Tue, Jul 05, 2022 at 02:37:57AM +0300, Laurent Pinchart via libcamera-devel wrote:
> Hi Christian,
> 
> Thank you for the patch.
> 
> On Fri, Jun 10, 2022 at 01:03:36PM +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.
> 
> I like the new syntax, even if it doesn't make it clear how we'll handle
> multi-dimensional arrays with variable numbers of elements in one or
> multiple dimensions, but we can worry about that later when/if needed.

Just a quick note on this. In the review of previous versions, a syntax
such as [,] was proposed for a 2D NxN array, and [,3] for a Nx3 array.
This won't work correctly, as neither are valid YAML syntax.

One workaround for this would be to turn the size element into a string,
e.g.

	size: '[,]'

I don't like this much. Another option would be to reuse 'n', e.g. [n,n]
and [n,3]. If we go that way, we should keep [n] already to denote a
dynamic 1D array.

Any other option ?

> This patch by itself introduces compilation errors, for instance, with
> clang-14,
> 
> In file included from ../../src/libcamera/camera_sensor.cpp:8:
> In file included from ../../include/libcamera/internal/camera_sensor.h:17:
> In file included from include/libcamera/control_ids.h:15:
> ../../include/libcamera/controls.h:403:8: error: no matching member function for call to 'set'
>                 val->set<T>(Span<const typename std::remove_cv_t<V>>{ value.begin(), value.size() });
>                 ~~~~~^~~~~~
> ../../src/libcamera/camera_sensor.cpp:421:14: note: in instantiation of function template specialization 'libcamera::ControlList::set<libcamera::Rectangle, libcamera::Rectangle>' requested here
>         properties_.set(properties::PixelArrayActiveAreas, { activeArea_ });
>                     ^
> ../../include/libcamera/controls.h:178:7: note: candidate function template not viable: no known conversion from 'Span<const typename std::remove_cv_t<Rectangle>>' (aka 'Span<const libcamera::Rectangle>') to 'const libcamera::Rectangle' for 1st argument
>         void set(const T &value)
>              ^
> ../../include/libcamera/controls.h:190:7: note: candidate template ignored: requirement 'details::is_span<libcamera::Rectangle>::value || std::is_same<std::string, libcamera::Rectangle>::value' was not satisfied [with T = libcamera::Rectangle]
>         void set(const T &value)
>              ^
> 1 error generated.
> 
> I'll update the Python generator script here to fix this, it will be
> part of v9.
> 
> > Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/control_ids.yaml  | 4 ++--
> >  src/libcamera/property_ids.yaml | 4 ++--
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index cd1d4512..f707c1f5 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
> > @@ -515,7 +515,7 @@ controls:
> >          the window where the focal distance for the objects shown in that part
> >          of the image are closest to the camera.
> > 
> > -      size: [n]
> > +      size: []
> > 
> >    - AfTrigger:
> >        type: int32_t
> > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> > index 11b7ebdc..a87485d7 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
Christian Rauch July 5, 2022, 12:14 a.m. UTC | #3 
Am 05.07.22 um 00:50 schrieb Laurent Pinchart:
> On Tue, Jul 05, 2022 at 02:37:57AM +0300, Laurent Pinchart via libcamera-devel wrote:
>> Hi Christian,
>>
>> Thank you for the patch.
>>
>> On Fri, Jun 10, 2022 at 01:03:36PM +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.
>>
>> I like the new syntax, even if it doesn't make it clear how we'll handle
>> multi-dimensional arrays with variable numbers of elements in one or
>> multiple dimensions, but we can worry about that later when/if needed.
>
> Just a quick note on this. In the review of previous versions, a syntax
> such as [,] was proposed for a 2D NxN array, and [,3] for a Nx3 array.
> This won't work correctly, as neither are valid YAML syntax.
>
> One workaround for this would be to turn the size element into a string,
> e.g.
>
> 	size: '[,]'
>
> I don't like this much. Another option would be to reuse 'n', e.g. [n,n]
> and [n,3]. If we go that way, we should keep [n] already to denote a
> dynamic 1D array.
>
> Any other option ?

In such a case, I would use "0" as the magic value to indicate a
variable size dimension, since a 0-size will never be used in practice.
With this syntax, "[0,3]" would translate into a "N x 3" sized array and
"[0,2,0]" into "N x 2 x M". We would need to parse this and replace all
occurrences of 0 with a dynamic-sized length, but the sum will then
still mean the "minimum total size".

I am not a fan of using a character (e.g. 'n') to indicate this, as this
is a bit arbitrary and "[n,3,n]" could mean that the first and last
dimension have to have the same length "n".

>
>> This patch by itself introduces compilation errors, for instance, with
>> clang-14,
>>
>> In file included from ../../src/libcamera/camera_sensor.cpp:8:
>> In file included from ../../include/libcamera/internal/camera_sensor.h:17:
>> In file included from include/libcamera/control_ids.h:15:
>> ../../include/libcamera/controls.h:403:8: error: no matching member function for call to 'set'
>>                 val->set<T>(Span<const typename std::remove_cv_t<V>>{ value.begin(), value.size() });
>>                 ~~~~~^~~~~~
>> ../../src/libcamera/camera_sensor.cpp:421:14: note: in instantiation of function template specialization 'libcamera::ControlList::set<libcamera::Rectangle, libcamera::Rectangle>' requested here
>>         properties_.set(properties::PixelArrayActiveAreas, { activeArea_ });
>>                     ^
>> ../../include/libcamera/controls.h:178:7: note: candidate function template not viable: no known conversion from 'Span<const typename std::remove_cv_t<Rectangle>>' (aka 'Span<const libcamera::Rectangle>') to 'const libcamera::Rectangle' for 1st argument
>>         void set(const T &value)
>>              ^
>> ../../include/libcamera/controls.h:190:7: note: candidate template ignored: requirement 'details::is_span<libcamera::Rectangle>::value || std::is_same<std::string, libcamera::Rectangle>::value' was not satisfied [with T = libcamera::Rectangle]
>>         void set(const T &value)
>>              ^
>> 1 error generated.
>>
>> I'll update the Python generator script here to fix this, it will be
>> part of v9.
>>
>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
>>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>>> ---
>>>  src/libcamera/control_ids.yaml  | 4 ++--
>>>  src/libcamera/property_ids.yaml | 4 ++--
>>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
>>> index cd1d4512..f707c1f5 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
>>> @@ -515,7 +515,7 @@ controls:
>>>          the window where the focal distance for the objects shown in that part
>>>          of the image are closest to the camera.
>>>
>>> -      size: [n]
>>> +      size: []
>>>
>>>    - AfTrigger:
>>>        type: int32_t
>>> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
>>> index 11b7ebdc..a87485d7 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
>
Kieran Bingham July 5, 2022, 9:18 a.m. UTC | #4 
Quoting Christian Rauch via libcamera-devel (2022-07-05 01:14:10)
> 
> 
> Am 05.07.22 um 00:50 schrieb Laurent Pinchart:
> > On Tue, Jul 05, 2022 at 02:37:57AM +0300, Laurent Pinchart via libcamera-devel wrote:
> >> Hi Christian,
> >>
> >> Thank you for the patch.
> >>
> >> On Fri, Jun 10, 2022 at 01:03:36PM +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.
> >>
> >> I like the new syntax, even if it doesn't make it clear how we'll handle
> >> multi-dimensional arrays with variable numbers of elements in one or
> >> multiple dimensions, but we can worry about that later when/if needed.
> >
> > Just a quick note on this. In the review of previous versions, a syntax
> > such as [,] was proposed for a 2D NxN array, and [,3] for a Nx3 array.
> > This won't work correctly, as neither are valid YAML syntax.
> >
> > One workaround for this would be to turn the size element into a string,
> > e.g.
> >
> >       size: '[,]'
> >
> > I don't like this much. Another option would be to reuse 'n', e.g. [n,n]
> > and [n,3]. If we go that way, we should keep [n] already to denote a
> > dynamic 1D array.
> >
> > Any other option ?
> 
> In such a case, I would use "0" as the magic value to indicate a
> variable size dimension, since a 0-size will never be used in practice.
> With this syntax, "[0,3]" would translate into a "N x 3" sized array and
> "[0,2,0]" into "N x 2 x M". We would need to parse this and replace all
> occurrences of 0 with a dynamic-sized length, but the sum will then
> still mean the "minimum total size".
> 
> I am not a fan of using a character (e.g. 'n') to indicate this, as this
> is a bit arbitrary and "[n,3,n]" could mean that the first and last
> dimension have to have the same length "n".


Throwing another option in, can we use '*' or '?'?

I dislike '0' as that has a 'real' value, so to me an array of [0,3]
while not 'useful' ... is conceptually valid, and therefore doesn't
confer that it could be variable.


Or otherwise, can we convey that any 'letter' is a variable, so that we
'can' support both 
	[n,n,n], (To state that it must be 3,3,3 or 4,4,4)
or
	[n,m,5], (to convey that there are two distinct variables ?)

Or even

	[w,h] if it's appropriate?

--
Kieran


> 
> >
> >> This patch by itself introduces compilation errors, for instance, with
> >> clang-14,
> >>
> >> In file included from ../../src/libcamera/camera_sensor.cpp:8:
> >> In file included from ../../include/libcamera/internal/camera_sensor.h:17:
> >> In file included from include/libcamera/control_ids.h:15:
> >> ../../include/libcamera/controls.h:403:8: error: no matching member function for call to 'set'
> >>                 val->set<T>(Span<const typename std::remove_cv_t<V>>{ value.begin(), value.size() });
> >>                 ~~~~~^~~~~~
> >> ../../src/libcamera/camera_sensor.cpp:421:14: note: in instantiation of function template specialization 'libcamera::ControlList::set<libcamera::Rectangle, libcamera::Rectangle>' requested here
> >>         properties_.set(properties::PixelArrayActiveAreas, { activeArea_ });
> >>                     ^
> >> ../../include/libcamera/controls.h:178:7: note: candidate function template not viable: no known conversion from 'Span<const typename std::remove_cv_t<Rectangle>>' (aka 'Span<const libcamera::Rectangle>') to 'const libcamera::Rectangle' for 1st argument
> >>         void set(const T &value)
> >>              ^
> >> ../../include/libcamera/controls.h:190:7: note: candidate template ignored: requirement 'details::is_span<libcamera::Rectangle>::value || std::is_same<std::string, libcamera::Rectangle>::value' was not satisfied [with T = libcamera::Rectangle]
> >>         void set(const T &value)
> >>              ^
> >> 1 error generated.
> >>
> >> I'll update the Python generator script here to fix this, it will be
> >> part of v9.
> >>
> >>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> >>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >>> ---
> >>>  src/libcamera/control_ids.yaml  | 4 ++--
> >>>  src/libcamera/property_ids.yaml | 4 ++--
> >>>  2 files changed, 4 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> >>> index cd1d4512..f707c1f5 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
> >>> @@ -515,7 +515,7 @@ controls:
> >>>          the window where the focal distance for the objects shown in that part
> >>>          of the image are closest to the camera.
> >>>
> >>> -      size: [n]
> >>> +      size: []
> >>>
> >>>    - AfTrigger:
> >>>        type: int32_t
> >>> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> >>> index 11b7ebdc..a87485d7 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
> >

Patch
diff mbox series 

diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
index cd1d4512..f707c1f5 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
@@ -515,7 +515,7 @@  controls:
         the window where the focal distance for the objects shown in that part
         of the image are closest to the camera.

-      size: [n]
+      size: []

   - AfTrigger:
       type: int32_t
diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
index 11b7ebdc..a87485d7 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