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

[Jacopo Mondi]

Hi Kieran,

On Wed, Aug 11, 2021 at 04:03:50PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 11/08/2021 13:35, Jacopo Mondi wrote:
> > Hi Kieran,
> >
> > 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?
> >
> > 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 ;-)
>
> --
> Kieran
>
>
>
> >
> >>
> >>
> >>  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>())
> >>  {
> >>  }
> >>
> >> --
> >> 2.30.2
> >>