[libcamera-devel,v2,08/11] libcamera: pipeline: ipu3: Migrate to Camera::Private
diff mbox series

Message ID 20210805175848.24188-9-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: Replace CameraData with Camera::Private
Related show

Commit Message

Laurent Pinchart Aug. 5, 2021, 5:58 p.m. UTC 
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@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 38 +++++++++++++---------------
 1 file changed, 18 insertions(+), 20 deletions(-)

Comments

Jacopo Mondi Aug. 10, 2021, 1:53 p.m. UTC | #1 
Hi Laurent

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@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@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 ?

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

>  		}
>
> -		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
>
Laurent Pinchart Aug. 11, 2021, 5:14 p.m. UTC | #2 
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@ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@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@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;
> >  	}

Patch
diff mbox series 

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