Message ID | 20201208014951.30989-2-laurent.pinchart@ideasonboard.com |
---|---|
State | Accepted |
Commit | e638ffde530440ec3515f40aa75a414ea1100231 |
Headers | show |
Series |
|
Related | show |
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; >
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
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;
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(-)