[libcamera-devel] [PATCH 2/3] libcamera: camera: Create a CameraControlValidator

[Laurent Pinchart]

Hello,

On Wed, Aug 11, 2021 at 05:12:24PM +0200, Jacopo Mondi wrote:
> On Wed, Aug 11, 2021 at 04:03:50PM +0100, Kieran Bingham wrote:
> > On 11/08/2021 13:35, Jacopo Mondi wrote:
> > > On Tue, Aug 10, 2021 at 05:11:33PM +0100, Kieran Bingham wrote:
> > >> Create a Camera specific CameraControlValidator for the Camera instance.
> > >> This will allow requests to use a single validor instance without having
> > >> to construct their own.
> > >>
> > >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >> ---
> > >>
> > >> Laurent:
> > >>  - Is the use of the _o<Public>() reference valid here in the
> > >>    initialiser list?

The _o() function is defined as

	template<typename T>
	T *_o()
	{
		return static_cast<T *>(o_);
	}

Since the recent change to decouple construction of Extensible and
Extensible::Private, the Extensible::Private::o_ member variable is
initialized by the Extensible constructor. With the pending patch series
that makes the pipeline handlers subclass Camera::Private, construction
of Camera::Private will happen before Camera is constructed. _o() is
thus not valid here I'm afraid.

> > > Could the requirement to pass a Camera * to the CameraControlValidator
> > > constructor be dropped ? The Request class has access to the internal
> > > Camera::Private representation and the need for the Validator to have
> > > a Camera * is only to access the Camera's control info map.
> >
> > It looks like Camera is used for both:
> >   camera_->name()
> > and
> >   camera_->controls()
> >
> > indeed.
> >
> > the name is a little trivial, but still there.
> > I'm not sure what the fear is about passing the Camera *
> >
> >
> > Indeed, if we can pass the Camera::Private we can dig the
> > pipe_->controls() ... but .. ahh.
> >
> > To get the ControlInfoMap - it's indexed from the Pipe based on the
> > Camera * ... which is because it's in the CameraData ...
> >
> > Aha - Now I see where you're going with that - Laurent's series moves
> > that CameraData into Camera::Private. So - at that point, yes we could.
> >
> > But I haven't got that series applied here yet ;-)
> 
> Yes indeed, I was already projected to when the control info and
> properties will be stored in Camera::Private...
> 
> > > IOW: Can the CameraControlValidator be made to accept a
> > > Camera::Private ? There shouldn't be a need to expose it to
> > > applications, right ?
> >
> > Perhaps I need to rebase on top of Laurent's patches, or give this
> > series to him ;-)

Please feel free to rebase :-) Please also feel free to change the way
control validators are designed. The current implementation is more of a
skeleton than anything else, and its design dates back to a time when
libcamera was very different. Don't feel constrained by how control
validators work today.

> > >>  include/libcamera/internal/camera.h | 6 ++++++
> > >>  src/libcamera/camera.cpp            | 2 +-
> > >>  2 files changed, 7 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h
> > >> index b60ed140356a..f14bafc75e05 100644
> > >> --- a/include/libcamera/internal/camera.h
> > >> +++ b/include/libcamera/internal/camera.h
> > >> @@ -16,6 +16,8 @@
> > >>
> > >>  #include <libcamera/camera.h>
> > >>
> > >> +#include "libcamera/internal/camera_controls.h"
> > >> +
> > >>  namespace libcamera {
> > >>
> > >>  class PipelineHandler;
> > >> @@ -30,6 +32,8 @@ public:
> > >>  		const std::set<Stream *> &streams);
> > >>  	~Private();
> > >>
> > >> +	const CameraControlValidator &validator() const { return validator_; }
> > >> +
> > >>  private:
> > >>  	enum State {
> > >>  		CameraAvailable,
> > >> @@ -56,6 +60,8 @@ private:
> > >>
> > >>  	bool disconnected_;
> > >>  	std::atomic<State> state_;
> > >> +
> > >> +	CameraControlValidator validator_;
> > >>  };
> > >>
> > >>  } /* namespace libcamera */
> > >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > >> index 6281e92057e4..b914bf188d57 100644
> > >> --- a/src/libcamera/camera.cpp
> > >> +++ b/src/libcamera/camera.cpp
> > >> @@ -336,7 +336,7 @@ Camera::Private::Private(PipelineHandler *pipe,
> > >>  			 const std::string &id,
> > >>  			 const std::set<Stream *> &streams)
> > >>  	: pipe_(pipe->shared_from_this()), id_(id), streams_(streams),
> > >> -	  disconnected_(false), state_(CameraAvailable)
> > >> +	  disconnected_(false), state_(CameraAvailable), validator_(_o<Public>())
> > >>  {
> > >>  }
> > >>

-- 
Regards,

Laurent Pinchart