[libcamera-devel,2/2] libcamera: v4l2_device: Return a unique pointer from fromEntityName()
diff mbox series

Message ID 20201208014951.30989-2-laurent.pinchart@ideasonboard.com
State Accepted
Commit e638ffde530440ec3515f40aa75a414ea1100231
Headers show
Series
  • [libcamera-devel,1/2] libcamera: v4l2_subdevice: Return a unique pointer from fromEntityName()
Related show

Commit Message

Laurent Pinchart Dec. 8, 2020, 1:49 a.m. UTC
The fromEntityName() function returns a pointer to a newly allocated
V4L2Device instance, which must be deleted by the caller. This opens the
door to memory leaks. Return a unique pointer instead, which conveys the
API semantics better than a sentence in the documentation.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/internal/v4l2_videodevice.h |  4 ++--
 src/libcamera/pipeline/ipu3/cio2.cpp          |  3 +--
 src/libcamera/pipeline/ipu3/cio2.h            |  2 +-
 src/libcamera/pipeline/ipu3/imgu.cpp          | 10 ++++------
 src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 13 +++----------
 src/libcamera/pipeline/rkisp1/rkisp1_path.cpp |  7 +------
 src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  3 +--
 src/libcamera/v4l2_videodevice.cpp            | 10 ++++------
 test/libtest/buffer_source.cpp                |  2 +-
 9 files changed, 18 insertions(+), 36 deletions(-)

Comments

Kieran Bingham Dec. 8, 2020, 10:25 p.m. UTC | #1
Hi Laurent,

On 08/12/2020 01:49, Laurent Pinchart wrote:
> The fromEntityName() function returns a pointer to a newly allocated
> V4L2Device instance, which must be deleted by the caller. This opens the
> door to memory leaks. Return a unique pointer instead, which conveys the
> API semantics better than a sentence in the documentation.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/internal/v4l2_videodevice.h |  4 ++--
>  src/libcamera/pipeline/ipu3/cio2.cpp          |  3 +--
>  src/libcamera/pipeline/ipu3/cio2.h            |  2 +-
>  src/libcamera/pipeline/ipu3/imgu.cpp          | 10 ++++------
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 13 +++----------
>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp |  7 +------
>  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  3 +--
>  src/libcamera/v4l2_videodevice.cpp            | 10 ++++------
>  test/libtest/buffer_source.cpp                |  2 +-
>  9 files changed, 18 insertions(+), 36 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index 661503d1565a..529ca0e359d6 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -210,8 +210,8 @@ public:
>  	int streamOn();
>  	int streamOff();
>  
> -	static V4L2VideoDevice *fromEntityName(const MediaDevice *media,
> -					       const std::string &entity);
> +	static std::unique_ptr<V4L2VideoDevice>
> +	fromEntityName(const MediaDevice *media, const std::string &entity);
>  
>  	V4L2PixelFormat toV4L2PixelFormat(const PixelFormat &pixelFormat);
>  
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index e43ec70fe3e4..821715e325b4 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -33,13 +33,12 @@ const std::map<uint32_t, PixelFormat> mbusCodesToPixelFormat = {
>  } /* namespace */
>  
>  CIO2Device::CIO2Device()
> -	: sensor_(nullptr), csi2_(nullptr), output_(nullptr)
> +	: sensor_(nullptr), csi2_(nullptr)
>  {
>  }
>  
>  CIO2Device::~CIO2Device()
>  {
> -	delete output_;
>  	delete csi2_;
>  	delete sensor_;
>  }
> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> index fa813a989fd2..0dca9673ca07 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.h
> +++ b/src/libcamera/pipeline/ipu3/cio2.h
> @@ -63,7 +63,7 @@ private:
>  
>  	CameraSensor *sensor_;
>  	V4L2Subdevice *csi2_;
> -	V4L2VideoDevice *output_;
> +	std::unique_ptr<V4L2VideoDevice> output_;
>  
>  	std::vector<std::unique_ptr<FrameBuffer>> buffers_;
>  	std::queue<FrameBuffer *> availableBuffers_;
> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> index bfe9624c7797..5b1c0318b426 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> @@ -348,24 +348,22 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)
>  	if (ret)
>  		return ret;
>  
> -	input_.reset(V4L2VideoDevice::fromEntityName(media, name_ + " input"));
> +	input_ = V4L2VideoDevice::fromEntityName(media, name_ + " input");
>  	ret = input_->open();
>  	if (ret)
>  		return ret;
>  
> -	output_.reset(V4L2VideoDevice::fromEntityName(media,
> -						      name_ + " output"));
> +	output_ = V4L2VideoDevice::fromEntityName(media, name_ + " output");
>  	ret = output_->open();
>  	if (ret)
>  		return ret;
>  
> -	viewfinder_.reset(V4L2VideoDevice::fromEntityName(media,
> -							  name_ + " viewfinder"));
> +	viewfinder_ = V4L2VideoDevice::fromEntityName(media, name_ + " viewfinder");
>  	ret = viewfinder_->open();
>  	if (ret)
>  		return ret;
>  
> -	stat_.reset(V4L2VideoDevice::fromEntityName(media, name_ + " 3a stat"));
> +	stat_ = V4L2VideoDevice::fromEntityName(media, name_ + " 3a stat");

Resetting to something just feels awkward, so I certainly like all that.


>  	ret = stat_->open();
>  	if (ret)
>  		return ret;
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index bcfe6c0514ab..064ab01057f4 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -178,7 +178,6 @@ class PipelineHandlerRkISP1 : public PipelineHandler
>  {
>  public:
>  	PipelineHandlerRkISP1(CameraManager *manager);
> -	~PipelineHandlerRkISP1();
>  
>  	CameraConfiguration *generateConfiguration(Camera *camera,
>  		const StreamRoles &roles) override;
> @@ -218,8 +217,8 @@ private:
>  
>  	MediaDevice *media_;
>  	std::unique_ptr<V4L2Subdevice> isp_;
> -	V4L2VideoDevice *param_;
> -	V4L2VideoDevice *stat_;
> +	std::unique_ptr<V4L2VideoDevice> param_;
> +	std::unique_ptr<V4L2VideoDevice> stat_;
>  
>  	RkISP1MainPath mainPath_;
>  	RkISP1SelfPath selfPath_;
> @@ -599,16 +598,10 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  }
>  
>  PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
> -	: PipelineHandler(manager), param_(nullptr), stat_(nullptr)
> +	: PipelineHandler(manager)
>  {
>  }
>  
> -PipelineHandlerRkISP1::~PipelineHandlerRkISP1()
> -{
> -	delete param_;
> -	delete stat_;
> -}

And we no longer need a destructor - even better.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> -
>  /* -----------------------------------------------------------------------------
>   * Pipeline Operations
>   */
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index e05d9dd657cd..25f482eb8d8e 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -24,15 +24,10 @@ RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
>  		       const Size &minResolution, const Size &maxResolution)
>  	: name_(name), running_(false), formats_(formats),
>  	  minResolution_(minResolution), maxResolution_(maxResolution),
> -	  video_(nullptr), link_(nullptr)
> +	  link_(nullptr)
>  {
>  }
>  
> -RkISP1Path::~RkISP1Path()
> -{
> -	delete video_;
> -}
> -
>  bool RkISP1Path::init(MediaDevice *media)
>  {
>  	std::string resizer = std::string("rkisp1_resizer_") + name_ + "path";
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> index f06ac5a731cc..3b3e37d258d0 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> @@ -31,7 +31,6 @@ class RkISP1Path
>  public:
>  	RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
>  		   const Size &minResolution, const Size &maxResolution);
> -	~RkISP1Path();
>  
>  	bool init(MediaDevice *media);
>  
> @@ -67,7 +66,7 @@ private:
>  	const Size maxResolution_;
>  
>  	std::unique_ptr<V4L2Subdevice> resizer_;
> -	V4L2VideoDevice *video_;
> +	std::unique_ptr<V4L2VideoDevice> video_;
>  	MediaLink *link_;
>  };
>  
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index e76fe2dd1f3a..e2b582842a9b 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1598,19 +1598,17 @@ int V4L2VideoDevice::streamOff()
>   * \param[in] media The media device where the entity is registered
>   * \param[in] entity The media entity name
>   *
> - * Releasing memory of the newly created instance is responsibility of the
> - * caller of this function.
> - *
>   * \return A newly created V4L2VideoDevice on success, nullptr otherwise
>   */
> -V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,
> -						 const std::string &entity)
> +std::unique_ptr<V4L2VideoDevice>
> +V4L2VideoDevice::fromEntityName(const MediaDevice *media,
> +				const std::string &entity)
>  {
>  	MediaEntity *mediaEntity = media->getEntityByName(entity);
>  	if (!mediaEntity)
>  		return nullptr;
>  
> -	return new V4L2VideoDevice(mediaEntity);
> +	return std::make_unique<V4L2VideoDevice>(mediaEntity);
>  }
>  
>  /**
> diff --git a/test/libtest/buffer_source.cpp b/test/libtest/buffer_source.cpp
> index ee87c8cd821c..73563f2fc39d 100644
> --- a/test/libtest/buffer_source.cpp
> +++ b/test/libtest/buffer_source.cpp
> @@ -50,7 +50,7 @@ int BufferSource::allocate(const StreamConfiguration &config)
>  		return TestSkip;
>  	}
>  
> -	std::unique_ptr<V4L2VideoDevice> video{ V4L2VideoDevice::fromEntityName(media_.get(), videoDeviceName) };
> +	std::unique_ptr<V4L2VideoDevice> video = V4L2VideoDevice::fromEntityName(media_.get(), videoDeviceName);
>  	if (!video) {
>  		std::cout << "Failed to get video device from entity "
>  			  << videoDeviceName << std::endl;
>
Jacopo Mondi Dec. 9, 2020, 9:15 a.m. UTC | #2
Hi Laurent,

On Tue, Dec 08, 2020 at 10:25:00PM +0000, Kieran Bingham wrote:
> Hi Laurent,
>
> On 08/12/2020 01:49, Laurent Pinchart wrote:
> > The fromEntityName() function returns a pointer to a newly allocated
> > V4L2Device instance, which must be deleted by the caller. This opens the
> > door to memory leaks. Return a unique pointer instead, which conveys the
> > API semantics better than a sentence in the documentation.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/internal/v4l2_videodevice.h |  4 ++--
> >  src/libcamera/pipeline/ipu3/cio2.cpp          |  3 +--
> >  src/libcamera/pipeline/ipu3/cio2.h            |  2 +-
> >  src/libcamera/pipeline/ipu3/imgu.cpp          | 10 ++++------
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 13 +++----------
> >  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp |  7 +------
> >  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  3 +--
> >  src/libcamera/v4l2_videodevice.cpp            | 10 ++++------
> >  test/libtest/buffer_source.cpp                |  2 +-
> >  9 files changed, 18 insertions(+), 36 deletions(-)
> >
> > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> > index 661503d1565a..529ca0e359d6 100644
> > --- a/include/libcamera/internal/v4l2_videodevice.h
> > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > @@ -210,8 +210,8 @@ public:
> >  	int streamOn();
> >  	int streamOff();
> >
> > -	static V4L2VideoDevice *fromEntityName(const MediaDevice *media,
> > -					       const std::string &entity);
> > +	static std::unique_ptr<V4L2VideoDevice>
> > +	fromEntityName(const MediaDevice *media, const std::string &entity);
> >
> >  	V4L2PixelFormat toV4L2PixelFormat(const PixelFormat &pixelFormat);
> >
> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> > index e43ec70fe3e4..821715e325b4 100644
> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> > @@ -33,13 +33,12 @@ const std::map<uint32_t, PixelFormat> mbusCodesToPixelFormat = {
> >  } /* namespace */
> >
> >  CIO2Device::CIO2Device()
> > -	: sensor_(nullptr), csi2_(nullptr), output_(nullptr)
> > +	: sensor_(nullptr), csi2_(nullptr)
> >  {
> >  }
> >
> >  CIO2Device::~CIO2Device()
> >  {
> > -	delete output_;
> >  	delete csi2_;
> >  	delete sensor_;
> >  }
> > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> > index fa813a989fd2..0dca9673ca07 100644
> > --- a/src/libcamera/pipeline/ipu3/cio2.h
> > +++ b/src/libcamera/pipeline/ipu3/cio2.h
> > @@ -63,7 +63,7 @@ private:
> >
> >  	CameraSensor *sensor_;
> >  	V4L2Subdevice *csi2_;
> > -	V4L2VideoDevice *output_;
> > +	std::unique_ptr<V4L2VideoDevice> output_;

It would be worth to make the other two class members unique_ptr<>
too. On top anyway.

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

Thanks
  j
> >
> >  	std::vector<std::unique_ptr<FrameBuffer>> buffers_;
> >  	std::queue<FrameBuffer *> availableBuffers_;
> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> > index bfe9624c7797..5b1c0318b426 100644
> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> > @@ -348,24 +348,22 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)
> >  	if (ret)
> >  		return ret;
> >
> > -	input_.reset(V4L2VideoDevice::fromEntityName(media, name_ + " input"));
> > +	input_ = V4L2VideoDevice::fromEntityName(media, name_ + " input");
> >  	ret = input_->open();
> >  	if (ret)
> >  		return ret;
> >
> > -	output_.reset(V4L2VideoDevice::fromEntityName(media,
> > -						      name_ + " output"));
> > +	output_ = V4L2VideoDevice::fromEntityName(media, name_ + " output");
> >  	ret = output_->open();
> >  	if (ret)
> >  		return ret;
> >
> > -	viewfinder_.reset(V4L2VideoDevice::fromEntityName(media,
> > -							  name_ + " viewfinder"));
> > +	viewfinder_ = V4L2VideoDevice::fromEntityName(media, name_ + " viewfinder");
> >  	ret = viewfinder_->open();
> >  	if (ret)
> >  		return ret;
> >
> > -	stat_.reset(V4L2VideoDevice::fromEntityName(media, name_ + " 3a stat"));
> > +	stat_ = V4L2VideoDevice::fromEntityName(media, name_ + " 3a stat");
>
> Resetting to something just feels awkward, so I certainly like all that.
>
>
> >  	ret = stat_->open();
> >  	if (ret)
> >  		return ret;
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index bcfe6c0514ab..064ab01057f4 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -178,7 +178,6 @@ class PipelineHandlerRkISP1 : public PipelineHandler
> >  {
> >  public:
> >  	PipelineHandlerRkISP1(CameraManager *manager);
> > -	~PipelineHandlerRkISP1();
> >
> >  	CameraConfiguration *generateConfiguration(Camera *camera,
> >  		const StreamRoles &roles) override;
> > @@ -218,8 +217,8 @@ private:
> >
> >  	MediaDevice *media_;
> >  	std::unique_ptr<V4L2Subdevice> isp_;
> > -	V4L2VideoDevice *param_;
> > -	V4L2VideoDevice *stat_;
> > +	std::unique_ptr<V4L2VideoDevice> param_;
> > +	std::unique_ptr<V4L2VideoDevice> stat_;
> >
> >  	RkISP1MainPath mainPath_;
> >  	RkISP1SelfPath selfPath_;
> > @@ -599,16 +598,10 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> >  }
> >
> >  PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
> > -	: PipelineHandler(manager), param_(nullptr), stat_(nullptr)
> > +	: PipelineHandler(manager)
> >  {
> >  }
> >
> > -PipelineHandlerRkISP1::~PipelineHandlerRkISP1()
> > -{
> > -	delete param_;
> > -	delete stat_;
> > -}
>
> And we no longer need a destructor - even better.
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> > -
> >  /* -----------------------------------------------------------------------------
> >   * Pipeline Operations
> >   */
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > index e05d9dd657cd..25f482eb8d8e 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > @@ -24,15 +24,10 @@ RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
> >  		       const Size &minResolution, const Size &maxResolution)
> >  	: name_(name), running_(false), formats_(formats),
> >  	  minResolution_(minResolution), maxResolution_(maxResolution),
> > -	  video_(nullptr), link_(nullptr)
> > +	  link_(nullptr)
> >  {
> >  }
> >
> > -RkISP1Path::~RkISP1Path()
> > -{
> > -	delete video_;
> > -}
> > -
> >  bool RkISP1Path::init(MediaDevice *media)
> >  {
> >  	std::string resizer = std::string("rkisp1_resizer_") + name_ + "path";
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > index f06ac5a731cc..3b3e37d258d0 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > @@ -31,7 +31,6 @@ class RkISP1Path
> >  public:
> >  	RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
> >  		   const Size &minResolution, const Size &maxResolution);
> > -	~RkISP1Path();
> >
> >  	bool init(MediaDevice *media);
> >
> > @@ -67,7 +66,7 @@ private:
> >  	const Size maxResolution_;
> >
> >  	std::unique_ptr<V4L2Subdevice> resizer_;
> > -	V4L2VideoDevice *video_;
> > +	std::unique_ptr<V4L2VideoDevice> video_;
> >  	MediaLink *link_;
> >  };
> >
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index e76fe2dd1f3a..e2b582842a9b 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -1598,19 +1598,17 @@ int V4L2VideoDevice::streamOff()
> >   * \param[in] media The media device where the entity is registered
> >   * \param[in] entity The media entity name
> >   *
> > - * Releasing memory of the newly created instance is responsibility of the
> > - * caller of this function.
> > - *
> >   * \return A newly created V4L2VideoDevice on success, nullptr otherwise
> >   */
> > -V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,
> > -						 const std::string &entity)
> > +std::unique_ptr<V4L2VideoDevice>
> > +V4L2VideoDevice::fromEntityName(const MediaDevice *media,
> > +				const std::string &entity)
> >  {
> >  	MediaEntity *mediaEntity = media->getEntityByName(entity);
> >  	if (!mediaEntity)
> >  		return nullptr;
> >
> > -	return new V4L2VideoDevice(mediaEntity);
> > +	return std::make_unique<V4L2VideoDevice>(mediaEntity);
> >  }
> >
> >  /**
> > diff --git a/test/libtest/buffer_source.cpp b/test/libtest/buffer_source.cpp
> > index ee87c8cd821c..73563f2fc39d 100644
> > --- a/test/libtest/buffer_source.cpp
> > +++ b/test/libtest/buffer_source.cpp
> > @@ -50,7 +50,7 @@ int BufferSource::allocate(const StreamConfiguration &config)
> >  		return TestSkip;
> >  	}
> >
> > -	std::unique_ptr<V4L2VideoDevice> video{ V4L2VideoDevice::fromEntityName(media_.get(), videoDeviceName) };
> > +	std::unique_ptr<V4L2VideoDevice> video = V4L2VideoDevice::fromEntityName(media_.get(), videoDeviceName);
> >  	if (!video) {
> >  		std::cout << "Failed to get video device from entity "
> >  			  << videoDeviceName << std::endl;
> >
>
> --
> Regards
> --
> Kieran
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series

diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
index 661503d1565a..529ca0e359d6 100644
--- a/include/libcamera/internal/v4l2_videodevice.h
+++ b/include/libcamera/internal/v4l2_videodevice.h
@@ -210,8 +210,8 @@  public:
 	int streamOn();
 	int streamOff();
 
-	static V4L2VideoDevice *fromEntityName(const MediaDevice *media,
-					       const std::string &entity);
+	static std::unique_ptr<V4L2VideoDevice>
+	fromEntityName(const MediaDevice *media, const std::string &entity);
 
 	V4L2PixelFormat toV4L2PixelFormat(const PixelFormat &pixelFormat);
 
diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
index e43ec70fe3e4..821715e325b4 100644
--- a/src/libcamera/pipeline/ipu3/cio2.cpp
+++ b/src/libcamera/pipeline/ipu3/cio2.cpp
@@ -33,13 +33,12 @@  const std::map<uint32_t, PixelFormat> mbusCodesToPixelFormat = {
 } /* namespace */
 
 CIO2Device::CIO2Device()
-	: sensor_(nullptr), csi2_(nullptr), output_(nullptr)
+	: sensor_(nullptr), csi2_(nullptr)
 {
 }
 
 CIO2Device::~CIO2Device()
 {
-	delete output_;
 	delete csi2_;
 	delete sensor_;
 }
diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
index fa813a989fd2..0dca9673ca07 100644
--- a/src/libcamera/pipeline/ipu3/cio2.h
+++ b/src/libcamera/pipeline/ipu3/cio2.h
@@ -63,7 +63,7 @@  private:
 
 	CameraSensor *sensor_;
 	V4L2Subdevice *csi2_;
-	V4L2VideoDevice *output_;
+	std::unique_ptr<V4L2VideoDevice> output_;
 
 	std::vector<std::unique_ptr<FrameBuffer>> buffers_;
 	std::queue<FrameBuffer *> availableBuffers_;
diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
index bfe9624c7797..5b1c0318b426 100644
--- a/src/libcamera/pipeline/ipu3/imgu.cpp
+++ b/src/libcamera/pipeline/ipu3/imgu.cpp
@@ -348,24 +348,22 @@  int ImgUDevice::init(MediaDevice *media, unsigned int index)
 	if (ret)
 		return ret;
 
-	input_.reset(V4L2VideoDevice::fromEntityName(media, name_ + " input"));
+	input_ = V4L2VideoDevice::fromEntityName(media, name_ + " input");
 	ret = input_->open();
 	if (ret)
 		return ret;
 
-	output_.reset(V4L2VideoDevice::fromEntityName(media,
-						      name_ + " output"));
+	output_ = V4L2VideoDevice::fromEntityName(media, name_ + " output");
 	ret = output_->open();
 	if (ret)
 		return ret;
 
-	viewfinder_.reset(V4L2VideoDevice::fromEntityName(media,
-							  name_ + " viewfinder"));
+	viewfinder_ = V4L2VideoDevice::fromEntityName(media, name_ + " viewfinder");
 	ret = viewfinder_->open();
 	if (ret)
 		return ret;
 
-	stat_.reset(V4L2VideoDevice::fromEntityName(media, name_ + " 3a stat"));
+	stat_ = V4L2VideoDevice::fromEntityName(media, name_ + " 3a stat");
 	ret = stat_->open();
 	if (ret)
 		return ret;
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index bcfe6c0514ab..064ab01057f4 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -178,7 +178,6 @@  class PipelineHandlerRkISP1 : public PipelineHandler
 {
 public:
 	PipelineHandlerRkISP1(CameraManager *manager);
-	~PipelineHandlerRkISP1();
 
 	CameraConfiguration *generateConfiguration(Camera *camera,
 		const StreamRoles &roles) override;
@@ -218,8 +217,8 @@  private:
 
 	MediaDevice *media_;
 	std::unique_ptr<V4L2Subdevice> isp_;
-	V4L2VideoDevice *param_;
-	V4L2VideoDevice *stat_;
+	std::unique_ptr<V4L2VideoDevice> param_;
+	std::unique_ptr<V4L2VideoDevice> stat_;
 
 	RkISP1MainPath mainPath_;
 	RkISP1SelfPath selfPath_;
@@ -599,16 +598,10 @@  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
 }
 
 PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
-	: PipelineHandler(manager), param_(nullptr), stat_(nullptr)
+	: PipelineHandler(manager)
 {
 }
 
-PipelineHandlerRkISP1::~PipelineHandlerRkISP1()
-{
-	delete param_;
-	delete stat_;
-}
-
 /* -----------------------------------------------------------------------------
  * Pipeline Operations
  */
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
index e05d9dd657cd..25f482eb8d8e 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
@@ -24,15 +24,10 @@  RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
 		       const Size &minResolution, const Size &maxResolution)
 	: name_(name), running_(false), formats_(formats),
 	  minResolution_(minResolution), maxResolution_(maxResolution),
-	  video_(nullptr), link_(nullptr)
+	  link_(nullptr)
 {
 }
 
-RkISP1Path::~RkISP1Path()
-{
-	delete video_;
-}
-
 bool RkISP1Path::init(MediaDevice *media)
 {
 	std::string resizer = std::string("rkisp1_resizer_") + name_ + "path";
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
index f06ac5a731cc..3b3e37d258d0 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
+++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
@@ -31,7 +31,6 @@  class RkISP1Path
 public:
 	RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
 		   const Size &minResolution, const Size &maxResolution);
-	~RkISP1Path();
 
 	bool init(MediaDevice *media);
 
@@ -67,7 +66,7 @@  private:
 	const Size maxResolution_;
 
 	std::unique_ptr<V4L2Subdevice> resizer_;
-	V4L2VideoDevice *video_;
+	std::unique_ptr<V4L2VideoDevice> video_;
 	MediaLink *link_;
 };
 
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
index e76fe2dd1f3a..e2b582842a9b 100644
--- a/src/libcamera/v4l2_videodevice.cpp
+++ b/src/libcamera/v4l2_videodevice.cpp
@@ -1598,19 +1598,17 @@  int V4L2VideoDevice::streamOff()
  * \param[in] media The media device where the entity is registered
  * \param[in] entity The media entity name
  *
- * Releasing memory of the newly created instance is responsibility of the
- * caller of this function.
- *
  * \return A newly created V4L2VideoDevice on success, nullptr otherwise
  */
-V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,
-						 const std::string &entity)
+std::unique_ptr<V4L2VideoDevice>
+V4L2VideoDevice::fromEntityName(const MediaDevice *media,
+				const std::string &entity)
 {
 	MediaEntity *mediaEntity = media->getEntityByName(entity);
 	if (!mediaEntity)
 		return nullptr;
 
-	return new V4L2VideoDevice(mediaEntity);
+	return std::make_unique<V4L2VideoDevice>(mediaEntity);
 }
 
 /**
diff --git a/test/libtest/buffer_source.cpp b/test/libtest/buffer_source.cpp
index ee87c8cd821c..73563f2fc39d 100644
--- a/test/libtest/buffer_source.cpp
+++ b/test/libtest/buffer_source.cpp
@@ -50,7 +50,7 @@  int BufferSource::allocate(const StreamConfiguration &config)
 		return TestSkip;
 	}
 
-	std::unique_ptr<V4L2VideoDevice> video{ V4L2VideoDevice::fromEntityName(media_.get(), videoDeviceName) };
+	std::unique_ptr<V4L2VideoDevice> video = V4L2VideoDevice::fromEntityName(media_.get(), videoDeviceName);
 	if (!video) {
 		std::cout << "Failed to get video device from entity "
 			  << videoDeviceName << std::endl;