[libcamera-devel,RFC,10/17] libcamera: pipeline: simple: Migrate to Camera::Private
diff mbox series

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

Commit Message

Laurent Pinchart July 23, 2021, 4 a.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>
---
 src/libcamera/pipeline/simple/simple.cpp | 25 ++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

Comments

Niklas Söderlund July 24, 2021, 7:24 a.m. UTC | #1 
Hi Laurent,

Thanks for your work.

On 2021-07-23 07:00:29 +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>
> ---
>  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_;

This looks odd, I can't figure out why const is dropped. Looking back 
should not Camera::Private::pipe() have a const version? It should be OK 
to get the pipe for RO operations.

>  
>  	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());
>  	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();
>  		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
>
Laurent Pinchart July 24, 2021, 7:34 p.m. UTC | #2 
Hi Niklas,

On Sat, Jul 24, 2021 at 09:24:45AM +0200, Niklas Söderlund wrote:
> On 2021-07-23 07:00:29 +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>
> > ---
> >  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_;
> 
> This looks odd, I can't figure out why const is dropped. Looking back 
> should not Camera::Private::pipe() have a const version? It should be OK 
> to get the pipe for RO operations.

The following changes end up being needed:

diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h
index 44242332a52f..a20ace75fe8b 100644
--- a/include/libcamera/internal/camera.h
+++ b/include/libcamera/internal/camera.h
@@ -30,6 +30,7 @@ public:
 	~Private();

 	PipelineHandler *pipe() { return pipe_.get(); }
+	const PipelineHandler *pipe() const { return pipe_.get(); }

 	std::list<Request *> queuedRequests_;
 	ControlInfoMap controlInfo_;
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
index 72a42dbeb3d3..dec4efbfe993 100644
--- a/src/libcamera/camera.cpp
+++ b/src/libcamera/camera.cpp
@@ -361,11 +361,15 @@ Camera::Private::~Private()
 }

 /**
- * \fn Camera::Private::pipe()
+ * \fn Camera::Private::pipe() const
  * \brief Retrieve the pipeline handler related to this camera
  * \return The pipeline handler for this camera
  */

+/**
+ * \fn Camera::Private::pipe()
+ * \copydoc Camera::Private::pipe() const
+ */
 /**
  * \var Camera::Private::queuedRequests_
  * \brief The list of queued and not yet completed request
diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
index b5e34c4cd0c5..6a92e89a1867 100644
--- a/src/libcamera/pipeline/simple/converter.cpp
+++ b/src/libcamera/pipeline/simple/converter.cpp
@@ -278,7 +278,7 @@ SizeRange SimpleConverter::sizes(const Size &input)

 std::tuple<unsigned int, unsigned int>
 SimpleConverter::strideAndFrameSize(const PixelFormat &pixelFormat,
-				    const Size &size)
+				    const Size &size) const
 {
 	V4L2DeviceFormat format;
 	format.fourcc = m2m_->capture()->toV4L2PixelFormat(pixelFormat);
diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h
index 276a2a291c21..960c102091be 100644
--- a/src/libcamera/pipeline/simple/converter.h
+++ b/src/libcamera/pipeline/simple/converter.h
@@ -40,7 +40,7 @@ public:
 	SizeRange sizes(const Size &input);

 	std::tuple<unsigned int, unsigned int>
-	strideAndFrameSize(const PixelFormat &pixelFormat, const Size &size);
+	strideAndFrameSize(const PixelFormat &pixelFormat, const Size &size) const;

 	int configure(const StreamConfiguration &inputCfg,
 		      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfg);
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 8594520f520c..84bc2723f42b 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -213,7 +213,7 @@ private:
 	 * reference to the camera data, store a new reference to the camera.
 	 */
 	std::shared_ptr<Camera> camera_;
-	SimpleCameraData *data_;
+	const SimpleCameraData *data_;

 	const SimpleCameraData::Configuration *pipeConfig_;
 	bool needConversion_;
@@ -239,6 +239,7 @@ public:
 	V4L2VideoDevice *video(const MediaEntity *entity);
 	V4L2Subdevice *subdev(const MediaEntity *entity);
 	SimpleConverter *converter() { return converter_.get(); }
+	const SimpleConverter *converter() const { return converter_.get(); }

 protected:
 	int queueRequestDevice(Camera *camera, Request *request) override;
@@ -582,8 +583,8 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
 	}

 	/* Adjust the requested streams. */
-	SimplePipelineHandler *pipe = static_cast<SimplePipelineHandler *>(data_->pipe());
-	SimpleConverter *converter = pipe->converter();
+	const SimplePipelineHandler *pipe = static_cast<const SimplePipelineHandler *>(data_->pipe());
+	const SimpleConverter *converter = pipe->converter();

 	/*
 	 * Enable usage of the converter when producing multiple streams, as

I'm thinking about storing a pointer to the Camera in
CameraConfiguration, which will allow accessing the pipeline handler to
be accessed through Camera::Private. That's why I didn't want to
introduce this change yet.

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

Patch
diff mbox series 

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