[libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Return a unique pointer from fromEntityName()
Jacopo Mondi
jacopo at jmondi.org
Wed Dec 9 10:15:10 CET 2020
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 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_;
It would be worth to make the other two class members unique_ptr<>
too. On top anyway.
Reviewed-by: Jacopo Mondi <jacopo at 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 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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list