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

[Kieran Bingham]

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


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