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

[Hirokazu Honda]

Hi Laurent, thanks for reviewing.

On Tue, Mar 23, 2021 at 10:52 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> 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.
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

Regards,
-Hiro

> 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