[libcamera-devel] [PATCH v2 03/11] libcamera: pipeline: simple: Migrate to Camera::Private

[Laurent Pinchart]

Hi Jacopo,

On Tue, Aug 10, 2021 at 03:37:48PM +0200, Jacopo Mondi wrote:
> On Thu, Aug 05, 2021 at 08:58:40PM +0300, Laurent Pinchart wrote:
> > As part of the effort to remove the CameraData class, migrate the
> > pipeline handler-specific camera data from CameraData to the
> > Camera::Private class.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/simple/simple.cpp | 25 ++++++++++++------------
> >  1 file changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 43af3fafa475..81d342a3fa73 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -147,7 +147,7 @@ static const SimplePipelineInfo supportedDevices[] = {
> >
> >  } /* namespace */
> >
> > -class SimpleCameraData : public CameraData
> > +class SimpleCameraData : public Camera::Private
> >  {
> >  public:
> >  	SimpleCameraData(SimplePipelineHandler *pipe,
> > @@ -213,7 +213,7 @@ private:
> >  	 * reference to the camera data, store a new reference to the camera.
> >  	 */
> >  	std::shared_ptr<Camera> camera_;
> > -	const SimpleCameraData *data_;
> > +	SimpleCameraData *data_;
> >
> >  	const SimpleCameraData::Configuration *pipeConfig_;
> >  	bool needConversion_;
> > @@ -246,10 +246,9 @@ protected:
> >  private:
> >  	static constexpr unsigned int kNumInternalBuffers = 3;
> >
> > -	SimpleCameraData *cameraData(const Camera *camera)
> > +	SimpleCameraData *cameraData(Camera *camera)
> >  	{
> > -		return static_cast<SimpleCameraData *>(
> > -			PipelineHandler::cameraData(camera));
> > +		return static_cast<SimpleCameraData *>(camera->_d());
> >  	}
> >
> >  	std::vector<MediaEntity *> locateSensors();
> > @@ -274,7 +273,7 @@ private:
> >  SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
> >  				   unsigned int numStreams,
> >  				   MediaEntity *sensor)
> > -	: CameraData(pipe), streams_(numStreams)
> > +	: Camera::Private(pipe), streams_(numStreams)
> >  {
> >  	int ret;
> >
> > @@ -355,7 +354,8 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
> >
> >  int SimpleCameraData::init()
> >  {
> > -	SimplePipelineHandler *pipe = static_cast<SimplePipelineHandler *>(pipe_);
> > +	SimplePipelineHandler *pipe =
> > +		static_cast<SimplePipelineHandler *>(this->pipe());
> >  	SimpleConverter *converter = pipe->converter();
> >  	int ret;
> >
> > @@ -480,7 +480,8 @@ int SimpleCameraData::setupLinks()
> >  int SimpleCameraData::setupFormats(V4L2SubdeviceFormat *format,
> >  				   V4L2Subdevice::Whence whence)
> >  {
> > -	SimplePipelineHandler *pipe = static_cast<SimplePipelineHandler *>(pipe_);
> > +	SimplePipelineHandler *pipe =
> > +		static_cast<SimplePipelineHandler *>(this->pipe());
> 
> An helper could be considered

Good point. I'll give it a try, possibly on top, as it's logically
separate from this patch I think, or in here if it makes the patch more
readable.

> >  	int ret;
> >
> >  	/*
> > @@ -581,7 +582,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
> >  	}
> >
> >  	/* Adjust the requested streams. */
> > -	SimplePipelineHandler *pipe = static_cast<SimplePipelineHandler *>(data_->pipe_);
> > +	SimplePipelineHandler *pipe = static_cast<SimplePipelineHandler *>(data_->pipe());
> >  	SimpleConverter *converter = pipe->converter();
> >
> >  	/*
> > @@ -1046,10 +1047,10 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> >  			       std::inserter(streams, streams.end()),
> >  			       [](Stream &stream) { return &stream; });
> >
> > +		const std::string &id = data->sensor_->id();
> 
> What do you need id for ?

To pass it to Camera::create() :-) Writing

			Camera::create(data.release(),
				       data->sensor_->id(), streams);

would reference a null pointer as data.release() makes data null.

> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> >  		std::shared_ptr<Camera> camera =
> > -			Camera::create(new Camera::Private(this),
> > -				       data->sensor_->id(), streams);
> > -		registerCamera(std::move(camera), std::move(data));
> > +			Camera::create(data.release(), id, streams);
> > +		registerCamera(std::move(camera), nullptr);
> >  		registered = true;
> >  	}
> >

-- 
Regards,

Laurent Pinchart