[libcamera-devel] [PATCH 2/2] [DNI] test: Test serialization of Rectangle and Size controls

[Laurent Pinchart]

Hi Jacopo,

On Sun, Apr 26, 2020 at 05:10:03PM +0200, Jacopo Mondi wrote:
> Hi Laurent,
>   I start from here to comment on the API
> 
> On Sat, Apr 25, 2020 at 11:56:39PM +0300, Laurent Pinchart wrote:
> > This patch should be rebased on real controls once available.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/libcamera/control_ids.yaml               | 18 ++++++++++++++++++
> >  test/serialization/control_serialization.cpp |  5 +++++
> >  2 files changed, 23 insertions(+)
> >
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index 4befec746a59..b1ae03e5b0ff 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -50,4 +50,22 @@ controls:
> >        type: int32_t
> >        description: Specify a fixed gain parameter
> >
> > +  - TheRectangle:
> > +      type: Rectangle
> > +      description: A Rectangle property
> > +
> > +  - TheRectangles:
> > +      type: Rectangle
> > +      description: A Rectangle array property
> > +      size: [n]
> > +
> > +  - TheSize:
> > +      type: Size
> > +      description: A Size property
> > +
> > +  - TheSizes:
> > +      type: Size
> > +      description: A Size array property
> > +      size: [n]
> > +
> >  ...
> > diff --git a/test/serialization/control_serialization.cpp b/test/serialization/control_serialization.cpp
> > index 2989b52774fb..789e0d83f4e4 100644
> > --- a/test/serialization/control_serialization.cpp
> > +++ b/test/serialization/control_serialization.cpp
> > @@ -45,6 +45,11 @@ protected:
> >  		list.set(controls::Brightness, 255);
> >  		list.set(controls::Contrast, 128);
> >  		list.set(controls::Saturation, 50);
> > +		list.set(controls::TheRectangle, Rectangle{ 100, 100, 640, 480 });
> > +		list.set(controls::TheRectangles, { Rectangle{ 100, 100, 640, 480 },
> > +						    Rectangle{ 200, 200, 1280, 720 } });
> > +		list.set(controls::TheSize, Size{ 640, 480 });
> > +		list.set(controls::TheSizes, { Size{ 640, 480 }, Size{ 1280, 720 } });
> 
> Is this a good idea ? How easy is that get it wrong assuming
> 
> 		list.set(controls::TheSizes, { 640, 480 }};
> 
> does what one expects ? We'll get a type assertion error, which is not
> nice to debug.. I'm debated, this is useful, but might be really a
> bleeding edge of the API...

Adding

 		list.set(controls::TheSizes, { 640, 480 }};

the compiler tells me

In file included from ../../test/serialization/control_serialization.cpp:10:
In file included from ../../include/libcamera/camera.h:15:
../../include/libcamera/controls.h:393: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() });
                ~~~~~^~~~~~
../../test/serialization/control_serialization.cpp:53:8: note: in instantiation of function template specialization 'libcamera::ControlList::set<libcamera::Span<const libcamera::Size, 18446744073709551615>, int>' requested here
                list.set(controls::TheSizes, { 640, 480 });
                     ^
../../include/libcamera/controls.h:185:7: note: candidate function template not viable: no known conversion from 'Span<const typename std::remove_cv_t<int>, [...]>' to 'const Span<const libcamera::Size, [...]>' for 1st argument
        void set(const T &value)
             ^
../../include/libcamera/controls.h:173:7: note: candidate template ignored: requirement '!details::is_span<libcamera::Span<const libcamera::Size, 18446744073709551615> >::value' was not satisfied [with T = libcamera::Span<const libcamera::Size, 18446744073709551615>]
        void set(const T &value)
             ^
1 error generated.

That's the thing I like about the controls API, it has compile-time type
checking. We don't get this for V4L2 controls though, as they're
specified by integer ID, and I'm really tempted to add Control<>
instances for each of the V4L2 controls we need, but that's not for now.

> >
> >  		/*
> >  		 * Serialize the control list, this should fail as the control

-- 
Regards,

Laurent Pinchart