[libcamera-devel] [PATCH v2 08/11] libcamera: pipeline: ipu3: Migrate to Camera::Private

[Laurent Pinchart]

Hi Jacopo,

On Tue, Aug 10, 2021 at 03:53:55PM +0200, Jacopo Mondi wrote:
> On Thu, Aug 05, 2021 at 08:58:45PM +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>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 38 +++++++++++++---------------
> >  1 file changed, 18 insertions(+), 20 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 15d6cca609ff..6d097ac91d2e 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -53,11 +53,11 @@ static const ControlInfoMap::Map IPU3Controls = {
> >  	{ &controls::draft::PipelineDepth, ControlInfo(2, 3) },
> >  };
> >
> > -class IPU3CameraData : public CameraData
> > +class IPU3CameraData : public Camera::Private
> >  {
> >  public:
> >  	IPU3CameraData(PipelineHandler *pipe)
> > -		: CameraData(pipe), exposureTime_(0), supportsFlips_(false)
> > +		: Camera::Private(pipe), exposureTime_(0), supportsFlips_(false)
> >  	{
> >  	}
> >
> > @@ -146,10 +146,9 @@ public:
> >  	bool match(DeviceEnumerator *enumerator) override;
> >
> >  private:
> > -	IPU3CameraData *cameraData(const Camera *camera)
> > +	IPU3CameraData *cameraData(Camera *camera)
> >  	{
> > -		return static_cast<IPU3CameraData *>(
> > -			PipelineHandler::cameraData(camera));
> > +		return static_cast<IPU3CameraData *>(camera->_d());
> >  	}
> >
> >  	int initControls(IPU3CameraData *data);
> > @@ -800,10 +799,10 @@ void IPU3CameraData::cancelPendingRequests()
> >  		for (auto it : request->buffers()) {
> >  			FrameBuffer *buffer = it.second;
> >  			buffer->cancel();
> > -			pipe_->completeBuffer(request, buffer);
> > +			pipe()->completeBuffer(request, buffer);
> 
> I wonder, being the Private class only visible internally, do we need
> to keep Camera::Private::pipe_ private ?

Making it private makes it impossible to inadvertently modify the
pointer in the derived class. This brings a bit of additional safety.

> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> >  		}
> >
> > -		pipe_->completeRequest(request);
> > +		pipe()->completeRequest(request);
> >  		pendingRequests_.pop();
> >  	}
> >  }
> > @@ -1184,12 +1183,11 @@ int PipelineHandlerIPU3::registerCameras()
> >  					&IPU3CameraData::statBufferReady);
> >
> >  		/* Create and register the Camera instance. */
> > -		std::string cameraId = cio2->sensor()->id();
> > +		const std::string &cameraId = cio2->sensor()->id();
> >  		std::shared_ptr<Camera> camera =
> > -			Camera::create(new Camera::Private(this), cameraId,
> > -				       streams);
> > +			Camera::create(data.release(), cameraId, streams);
> >
> > -		registerCamera(std::move(camera), std::move(data));
> > +		registerCamera(std::move(camera), nullptr);
> >
> >  		LOG(IPU3, Info)
> >  			<< "Registered Camera[" << numCameras << "] \""
> > @@ -1204,7 +1202,7 @@ int PipelineHandlerIPU3::registerCameras()
> >
> >  int IPU3CameraData::loadIPA()
> >  {
> > -	ipa_ = IPAManager::createIPA<ipa::ipu3::IPAProxyIPU3>(pipe_, 1, 1);
> > +	ipa_ = IPAManager::createIPA<ipa::ipu3::IPAProxyIPU3>(pipe(), 1, 1);
> >  	if (!ipa_)
> >  		return -ENOENT;
> >
> > @@ -1261,7 +1259,7 @@ void IPU3CameraData::queueFrameAction(unsigned int id,
> >
> >  		info->metadataProcessed = true;
> >  		if (frameInfos_.tryComplete(info))
> > -			pipe_->completeRequest(request);
> > +			pipe()->completeRequest(request);
> >
> >  		break;
> >  	}
> > @@ -1289,7 +1287,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
> >
> >  	Request *request = info->request;
> >
> > -	pipe_->completeBuffer(request, buffer);
> > +	pipe()->completeBuffer(request, buffer);
> >
> >  	request->metadata().set(controls::draft::PipelineDepth, 3);
> >  	/* \todo Move the ExposureTime control to the IPA. */
> > @@ -1300,7 +1298,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
> >  	request->metadata().set(controls::ScalerCrop, cropRegion_);
> >
> >  	if (frameInfos_.tryComplete(info))
> > -		pipe_->completeRequest(request);
> > +		pipe()->completeRequest(request);
> >  }
> >
> >  /**
> > @@ -1332,16 +1330,16 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
> >  		for (auto it : request->buffers()) {
> >  			FrameBuffer *b = it.second;
> >  			b->cancel();
> > -			pipe_->completeBuffer(request, b);
> > +			pipe()->completeBuffer(request, b);
> >  		}
> >
> >  		frameInfos_.remove(info);
> > -		pipe_->completeRequest(request);
> > +		pipe()->completeRequest(request);
> >  		return;
> >  	}
> >
> >  	if (request->findBuffer(&rawStream_))
> > -		pipe_->completeBuffer(request, buffer);
> > +		pipe()->completeBuffer(request, buffer);
> >
> >  	ipa::ipu3::IPU3Event ev;
> >  	ev.op = ipa::ipu3::EventFillParams;
> > @@ -1367,7 +1365,7 @@ void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)
> >  	Request *request = info->request;
> >
> >  	if (frameInfos_.tryComplete(info))
> > -		pipe_->completeRequest(request);
> > +		pipe()->completeRequest(request);
> >  }
> >
> >  void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
> > @@ -1386,7 +1384,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
> >  		 * In that event, we must have obtained the Request before hand.
> >  		 */
> >  		if (frameInfos_.tryComplete(info))
> > -			pipe_->completeRequest(request);
> > +			pipe()->completeRequest(request);
> >
> >  		return;
> >  	}

-- 
Regards,

Laurent Pinchart