[libcamera-devel] [RFC PATCH 15/17] libcamera: pipeline: ipu3: Migrate to Camera::Private

[Laurent Pinchart]

Hi Jacopo,

On Thu, Jul 29, 2021 at 11:14:59PM +0200, Jacopo Mondi wrote:
> On Fri, Jul 23, 2021 at 07:00:34AM +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/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
> 
> A very trivial comment, shouldn't this class now just be IPU3Camera ?

I think that would be a bit misleading, as the name implies (for me at
least) that it would be a subclass of Camera.

This being said, I'm open to considering letting pipeline handlers
subclass the Camera class. We haven't allowed this up to now in order
mainly to avoid exposing the Camera constructor as a public function of
the Camera class, but it's not an issue anymore as applications can't
construct a Camera::Private instance to pass to the Camera constructor.
I haven't however considered all the implications, and Niklas didn't
seem to be thrilled by the idea, so I haven't really investigated it.

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