[libcamera-devel] [PATCH 1/8] android: CameraHalManager: Hold CameraDevice with std::unique_ptr

[Laurent Pinchart]

Hi Hiro,

On Tue, Mar 23, 2021 at 11:24:11AM +0900, Hirokazu Honda wrote:
> On Tue, Mar 23, 2021 at 10:52 AM Laurent Pinchart wrote:
> > On Tue, Mar 23, 2021 at 10:42:19AM +0900, Hirokazu Honda wrote:
> > > CameraDevice is owned by CameraHalManager. The ownership of the
> > > object is not shared with other classes. So CameraHalManager
> > > should manage CameraDevice with std::unique_ptr.
> > >
> > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > >
> > > ---
> > >  src/android/camera_device.cpp      |  5 ++---
> > >  src/android/camera_device.h        |  2 +-
> > >  src/android/camera_hal_manager.cpp | 10 ++++------
> > >  src/android/camera_hal_manager.h   |  2 +-
> > >  4 files changed, 8 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index 72a89258..d0955de7 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -350,11 +350,10 @@ CameraDevice::~CameraDevice()
> > >               delete it.second;
> > >  }
> > >
> > > -std::shared_ptr<CameraDevice> CameraDevice::create(unsigned int id,
> > > +std::unique_ptr<CameraDevice> CameraDevice::create(unsigned int id,
> > >                                                  const std::shared_ptr<Camera> &cam)
> > >  {
> > > -     CameraDevice *camera = new CameraDevice(id, cam);
> > > -     return std::shared_ptr<CameraDevice>(camera);
> > > +     return std::unique_ptr<CameraDevice>(new CameraDevice(id, cam));
> >
> > This could use
> >
> >         return std::make_unique<CameraDevice>(id, cam);
> 
> std::make_uniue cannot be used because CameraDevice ctor is private.

Ah, good point. Let's leave it as-is then.

> By the way, chromium is base::WrapUnique for this.
> https://source.chromium.org/chromium/chromium/src/+/master:base/memory/ptr_util.h;l=17;drc=0e26b21394de64e30cdeebe308df851129dd5eff

Do I understand correctly that it would result in the following change ?

-	return std::unique_ptr<CameraDevice>(new CameraDevice(id, cam));
+	return base::WrapUnique(new CameraDevice(id, cam));

It's a bit shorter, but possibly not worth creating a specific helper in
libcamera given how uncommon private constructors are in our code base.

> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > >  }
> > >
> > >  /*
> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > index 823d561c..8be7f305 100644
> > > --- a/src/android/camera_device.h
> > > +++ b/src/android/camera_device.h
> > > @@ -32,7 +32,7 @@
> > >  class CameraDevice : protected libcamera::Loggable
> > >  {
> > >  public:
> > > -     static std::shared_ptr<CameraDevice> create(unsigned int id,
> > > +     static std::unique_ptr<CameraDevice> create(unsigned int id,
> > >                                                   const std::shared_ptr<libcamera::Camera> &cam);
> > >       ~CameraDevice();
> > >
> > > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> > > index 189eda2b..b3c85406 100644
> > > --- a/src/android/camera_hal_manager.cpp
> > > +++ b/src/android/camera_hal_manager.cpp
> > > @@ -36,8 +36,6 @@ CameraHalManager::CameraHalManager()
> > >
> > >  CameraHalManager::~CameraHalManager()
> > >  {
> > > -     cameras_.clear();
> > > -
> > >       if (cameraManager_) {
> > >               cameraManager_->stop();
> > >               delete cameraManager_;
> > > @@ -124,7 +122,7 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
> > >       }
> > >
> > >       /* Create a CameraDevice instance to wrap the libcamera Camera. */
> > > -     std::shared_ptr<CameraDevice> camera = CameraDevice::create(id, std::move(cam));
> > > +     std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, std::move(cam));
> > >       int ret = camera->initialize();
> > >       if (ret) {
> > >               LOG(HAL, Error) << "Failed to initialize camera: " << cam->id();
> > > @@ -154,7 +152,7 @@ void CameraHalManager::cameraRemoved(std::shared_ptr<Camera> cam)
> > >       MutexLocker locker(mutex_);
> > >
> > >       auto iter = std::find_if(cameras_.begin(), cameras_.end(),
> > > -                              [&cam](std::shared_ptr<CameraDevice> &camera) {
> > > +                              [&cam](const std::unique_ptr<CameraDevice> &camera) {
> > >                                        return cam == camera->camera();
> > >                                });
> > >       if (iter == cameras_.end())
> > > @@ -191,7 +189,7 @@ int32_t CameraHalManager::cameraLocation(const Camera *cam)
> > >  CameraDevice *CameraHalManager::cameraDeviceFromHalId(unsigned int id)
> > >  {
> > >       auto iter = std::find_if(cameras_.begin(), cameras_.end(),
> > > -                              [id](std::shared_ptr<CameraDevice> &camera) {
> > > +                              [id](const std::unique_ptr<CameraDevice> &camera) {
> > >                                        return camera->id() == id;
> > >                                });
> > >       if (iter == cameras_.end())
> > > @@ -243,7 +241,7 @@ void CameraHalManager::setCallbacks(const camera_module_callbacks_t *callbacks)
> > >        * Internal cameras are already assumed to be present at module load
> > >        * time by the Android framework.
> > >        */
> > > -     for (std::shared_ptr<CameraDevice> &camera : cameras_) {
> > > +     for (const std::unique_ptr<CameraDevice> &camera : cameras_) {
> > >               unsigned int id = camera->id();
> > >               if (id >= firstExternalCameraId_)
> > >                       callbacks_->camera_device_status_change(callbacks_, id,
> > > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h
> > > index a91decc7..65bb3554 100644
> > > --- a/src/android/camera_hal_manager.h
> > > +++ b/src/android/camera_hal_manager.h
> > > @@ -50,7 +50,7 @@ private:
> > >       libcamera::CameraManager *cameraManager_;
> > >
> > >       const camera_module_callbacks_t *callbacks_;
> > > -     std::vector<std::shared_ptr<CameraDevice>> cameras_;
> > > +     std::vector<std::unique_ptr<CameraDevice>> cameras_;
> > >       std::map<std::string, unsigned int> cameraIdsMap_;
> > >       Mutex mutex_;
> > >

-- 
Regards,

Laurent Pinchart