| Message ID | 20200425205639.30566-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Not Applicable |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
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@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... > > /* > * Serialize the control list, this should fail as the control > -- > Regards, > > 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@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
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 } }); /* * Serialize the control list, this should fail as the control
This patch should be rebased on real controls once available. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/control_ids.yaml | 18 ++++++++++++++++++ test/serialization/control_serialization.cpp | 5 +++++ 2 files changed, 23 insertions(+)