[libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Return a unique pointer from fromEntityName()
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Dec 8 23:25:00 CET 2020
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 at 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 at 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
More information about the libcamera-devel
mailing list