libcamera: v4l2_device: Return a unique pointer from fromEntityName()

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>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

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

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

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

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;

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
  */

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

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

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

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;