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

[Laurent Pinchart]

On Thu, Aug 12, 2021 at 01:50:10AM +0300, Laurent Pinchart wrote:
> 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.

Note that you can initialize members of the private class in the public
class constructor. For instance, the Camera constructor is implemented
as

Camera::Camera(std::unique_ptr<Private> d, const std::string &id,
	       const std::set<Stream *> &streams)
	: Extensible(std::move(d))
{
	_d()->id_ = id;
	_d()->streams_ = streams;
}

You could store a

	std::unique_ptr<CameraControlValidator> validator_;

in Camera::Private, and construct and assign it in Camera::Camera() with

	_d()->validator_ = std::make_unique<CameraControlValidator>(this);

(it may make sense to store the return value of _d() in a local
variable, and no, you can't use the d parameter, as it's moved before
the body of the Camera constructor is executed).

> > > > 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