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

Message ID 20210805175848.24188-4-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>
---
 src/libcamera/pipeline/simple/simple.cpp | 25 ++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

Comments

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

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@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

>  	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 ?

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

Thanks
  j

>  		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 Aug. 11, 2021, 5:08 p.m. UTC | #2 
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@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@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;
> >  	}
> >

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