[libcamera-devel] [PATCH 3/3] libcamera: pipeline: Manage resources with std::unique_ptr<>
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Tue Dec 15 05:10:14 CET 2020
Hi Laurent,
On Mon, Dec 14, 2020 at 03:47:39PM +0200, Laurent Pinchart wrote:
> Hi Paul,
>
> On Mon, Dec 14, 2020 at 11:20:51AM +0900, paul.elder at ideasonboard.com wrote:
> > On Sat, Dec 12, 2020 at 08:51:16PM +0200, Laurent Pinchart wrote:
> > > Replace manual resource destruction with std::unique_ptr<> where
> > > applicable. This removes the need for several destructors.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > > src/libcamera/pipeline/ipu3/cio2.cpp | 11 ++---------
> > > src/libcamera/pipeline/ipu3/cio2.h | 9 ++++-----
> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 +++++++------------
> > > src/libcamera/pipeline/simple/converter.cpp | 8 +-------
> > > src/libcamera/pipeline/simple/converter.h | 3 +--
> > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 12 ++++--------
> > > src/libcamera/pipeline/vimc/vimc.cpp | 11 +++--------
> > > 7 files changed, 22 insertions(+), 51 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> > > index 821715e325b4..bd5260d5b288 100644
> > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> > > @@ -33,16 +33,9 @@ const std::map<uint32_t, PixelFormat> mbusCodesToPixelFormat = {
> > > } /* namespace */
> > >
> > > CIO2Device::CIO2Device()
> > > - : sensor_(nullptr), csi2_(nullptr)
> > > {
> > > }
> > >
> > > -CIO2Device::~CIO2Device()
> > > -{
> > > - delete csi2_;
> > > - delete sensor_;
> > > -}
> > > -
> > > /**
> > > * \brief Retrieve the list of supported PixelFormats
> > > *
> > > @@ -117,7 +110,7 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
> > >
> > > MediaLink *link = links[0];
> > > MediaEntity *sensorEntity = link->source()->entity();
> > > - sensor_ = new CameraSensor(sensorEntity);
> > > + sensor_ = std::make_unique<CameraSensor>(sensorEntity);
> > > ret = sensor_->init();
> > > if (ret)
> > > return ret;
> > > @@ -148,7 +141,7 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
> > > * might impact on power consumption.
> > > */
> > >
> > > - csi2_ = new V4L2Subdevice(csi2Entity);
> > > + csi2_ = std::make_unique<V4L2Subdevice>(csi2Entity);
> > > ret = csi2_->open();
> > > if (ret)
> > > return ret;
> > > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> > > index 0dca9673ca07..236ad287354a 100644
> > > --- a/src/libcamera/pipeline/ipu3/cio2.h
> > > +++ b/src/libcamera/pipeline/ipu3/cio2.h
> > > @@ -33,7 +33,6 @@ public:
> > > static constexpr unsigned int CIO2_BUFFER_COUNT = 4;
> > >
> > > CIO2Device();
> > > - ~CIO2Device();
> > >
> > > std::vector<PixelFormat> formats() const;
> > > std::vector<SizeRange> sizes() const;
> > > @@ -49,8 +48,8 @@ public:
> > > int start();
> > > int stop();
> > >
> > > - CameraSensor *sensor() { return sensor_; }
> > > - const CameraSensor *sensor() const { return sensor_; }
> > > + CameraSensor *sensor() { return sensor_.get(); }
> > > + const CameraSensor *sensor() const { return sensor_.get(); }
> > >
> > > int queueBuffer(Request *request, FrameBuffer *rawBuffer);
> > > void tryReturnBuffer(FrameBuffer *buffer);
> > > @@ -61,8 +60,8 @@ private:
> > >
> > > void cio2BufferReady(FrameBuffer *buffer);
> > >
> > > - CameraSensor *sensor_;
> > > - V4L2Subdevice *csi2_;
> > > + std::unique_ptr<CameraSensor> sensor_;
> > > + std::unique_ptr<V4L2Subdevice> csi2_;
> > > std::unique_ptr<V4L2VideoDevice> output_;
> > >
> > > std::vector<std::unique_ptr<FrameBuffer>> buffers_;
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 4d98c9027f42..021d0ffe3ffb 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -121,21 +121,16 @@ class RkISP1CameraData : public CameraData
> > > public:
> > > RkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,
> > > RkISP1SelfPath *selfPath)
> > > - : CameraData(pipe), sensor_(nullptr), frame_(0),
> > > - frameInfo_(pipe), mainPath_(mainPath), selfPath_(selfPath)
> > > + : CameraData(pipe), frame_(0), frameInfo_(pipe),
> > > + mainPath_(mainPath), selfPath_(selfPath)
> > > {
> > > }
> > >
> > > - ~RkISP1CameraData()
> > > - {
> > > - delete sensor_;
> > > - }
> > > -
> > > int loadIPA();
> > >
> > > Stream mainPathStream_;
> > > Stream selfPathStream_;
> > > - CameraSensor *sensor_;
> > > + std::unique_ptr<CameraSensor> sensor_;
> > > unsigned int frame_;
> > > std::vector<IPABuffer> ipaBuffers_;
> > > RkISP1Frames frameInfo_;
> > > @@ -430,7 +425,7 @@ void RkISP1CameraData::queueFrameAction(unsigned int frame,
> > > case RKISP1_IPA_ACTION_V4L2_SET: {
> > > const ControlList &controls = action.controls[0];
> > > timeline_.scheduleAction(std::make_unique<RkISP1ActionSetSensor>(frame,
> > > - sensor_,
> > > + sensor_.get(),
> > > controls));
> > > break;
> > > }
> > > @@ -489,7 +484,7 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)
> > >
> > > CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> > > {
> > > - const CameraSensor *sensor = data_->sensor_;
> > > + const CameraSensor *sensor = data_->sensor_.get();
> > > Status status = Valid;
> >
> > What's wrong with using the unique_ptr here directly?
>
> Nothing as such. We could s/sensor/data_->sensor_/ in the function. The
> local sensor variable is just a shorthand. We could also write
>
> const std::unique_ptr<CameraSensor> &sensor = data_->sensor_;
>
> but that seems a bit more complicated. Would you prefer that ?
Ah yeah, true. Nah, what you have is fine.
Paul
> > Just wondering though, so
> >
> > Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> >
> > > if (config_.empty())
> > > @@ -660,7 +655,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> > > RkISP1CameraConfiguration *config =
> > > static_cast<RkISP1CameraConfiguration *>(c);
> > > RkISP1CameraData *data = cameraData(camera);
> > > - CameraSensor *sensor = data->sensor_;
> > > + CameraSensor *sensor = data->sensor_.get();
> > > int ret;
> > >
> > > ret = initLinks(camera, sensor, *config);
> > > @@ -1035,7 +1030,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> > >
> > > data->controlInfo_ = std::move(ctrls);
> > >
> > > - data->sensor_ = new CameraSensor(sensor);
> > > + data->sensor_ = std::make_unique<CameraSensor>(sensor);
> > > ret = data->sensor_->init();
> > > if (ret)
> > > return ret;
> > > diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
> > > index 67e6e864aa0a..a6a8e139cb3e 100644
> > > --- a/src/libcamera/pipeline/simple/converter.cpp
> > > +++ b/src/libcamera/pipeline/simple/converter.cpp
> > > @@ -24,7 +24,6 @@ namespace libcamera {
> > > LOG_DECLARE_CATEGORY(SimplePipeline)
> > >
> > > SimpleConverter::SimpleConverter(MediaDevice *media)
> > > - : m2m_(nullptr)
> > > {
> > > /*
> > > * Locate the video node. There's no need to validate the pipeline
> > > @@ -38,17 +37,12 @@ SimpleConverter::SimpleConverter(MediaDevice *media)
> > > if (it == entities.end())
> > > return;
> > >
> > > - m2m_ = new V4L2M2MDevice((*it)->deviceNode());
> > > + m2m_ = std::make_unique<V4L2M2MDevice>((*it)->deviceNode());
> > >
> > > m2m_->output()->bufferReady.connect(this, &SimpleConverter::outputBufferReady);
> > > m2m_->capture()->bufferReady.connect(this, &SimpleConverter::captureBufferReady);
> > > }
> > >
> > > -SimpleConverter::~SimpleConverter()
> > > -{
> > > - delete m2m_;
> > > -}
> > > -
> > > int SimpleConverter::open()
> > > {
> > > if (!m2m_)
> > > diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h
> > > index 78296680aa14..a3c4d899cfc8 100644
> > > --- a/src/libcamera/pipeline/simple/converter.h
> > > +++ b/src/libcamera/pipeline/simple/converter.h
> > > @@ -29,7 +29,6 @@ class SimpleConverter
> > > {
> > > public:
> > > SimpleConverter(MediaDevice *media);
> > > - ~SimpleConverter();
> > >
> > > int open();
> > > void close();
> > > @@ -56,7 +55,7 @@ private:
> > > void captureBufferReady(FrameBuffer *buffer);
> > > void outputBufferReady(FrameBuffer *buffer);
> > >
> > > - V4L2M2MDevice *m2m_;
> > > + std::unique_ptr<V4L2M2MDevice> m2m_;
> > >
> > > std::queue<FrameBuffer *> captureDoneQueue_;
> > > std::queue<FrameBuffer *> outputDoneQueue_;
> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > index 87b0f03d143a..e54f45bb8825 100644
> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > @@ -9,6 +9,7 @@
> > > #include <fstream>
> > > #include <iomanip>
> > > #include <math.h>
> > > +#include <memory>
> > > #include <tuple>
> > >
> > > #include <libcamera/camera.h>
> > > @@ -35,21 +36,16 @@ class UVCCameraData : public CameraData
> > > {
> > > public:
> > > UVCCameraData(PipelineHandler *pipe)
> > > - : CameraData(pipe), video_(nullptr)
> > > + : CameraData(pipe)
> > > {
> > > }
> > >
> > > - ~UVCCameraData()
> > > - {
> > > - delete video_;
> > > - }
> > > -
> > > int init(MediaDevice *media);
> > > void addControl(uint32_t cid, const ControlInfo &v4l2info,
> > > ControlInfoMap::Map *ctrls);
> > > void bufferReady(FrameBuffer *buffer);
> > >
> > > - V4L2VideoDevice *video_;
> > > + std::unique_ptr<V4L2VideoDevice> video_;
> > > Stream stream_;
> > > };
> > >
> > > @@ -499,7 +495,7 @@ int UVCCameraData::init(MediaDevice *media)
> > > }
> > >
> > > /* Create and open the video device. */
> > > - video_ = new V4L2VideoDevice(*entity);
> > > + video_ = std::make_unique<V4L2VideoDevice>(*entity);
> > > ret = video_->open();
> > > if (ret)
> > > return ret;
> > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > > index 72256f5b4190..8bda746f3136 100644
> > > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > > @@ -42,20 +42,15 @@ class VimcCameraData : public CameraData
> > > {
> > > public:
> > > VimcCameraData(PipelineHandler *pipe, MediaDevice *media)
> > > - : CameraData(pipe), media_(media), sensor_(nullptr)
> > > + : CameraData(pipe), media_(media)
> > > {
> > > }
> > >
> > > - ~VimcCameraData()
> > > - {
> > > - delete sensor_;
> > > - }
> > > -
> > > int init();
> > > void bufferReady(FrameBuffer *buffer);
> > >
> > > MediaDevice *media_;
> > > - CameraSensor *sensor_;
> > > + std::unique_ptr<CameraSensor> sensor_;
> > > std::unique_ptr<V4L2Subdevice> debayer_;
> > > std::unique_ptr<V4L2Subdevice> scaler_;
> > > std::unique_ptr<V4L2VideoDevice> video_;
> > > @@ -461,7 +456,7 @@ int VimcCameraData::init()
> > > return ret;
> > >
> > > /* Create and open the camera sensor, debayer, scaler and video device. */
> > > - sensor_ = new CameraSensor(media_->getEntityByName("Sensor B"));
> > > + sensor_ = std::make_unique<CameraSensor>(media_->getEntityByName("Sensor B"));
> > > ret = sensor_->init();
> > > if (ret)
> > > return ret;
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list