[libcamera-devel] [PATCH 2/8] android: CameraHalManager: Hold CameraManager with std::unique_ptr

[Hirokazu Honda]

Hi Laurent, thanks for reviewing.

On Tue, Mar 23, 2021 at 11:00 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Tue, Mar 23, 2021 at 10:42:20AM +0900, Hirokazu Honda wrote:
> > CameraManager is owned by CameraHalManager. The ownership of the
> > object is not shared with other classes. So CameraHalManager
> > should manage CameraManager with std::unique_ptr.
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> >
> > ---
> >  src/android/camera_hal_manager.cpp | 14 +++-----------
> >  src/android/camera_hal_manager.h   |  2 +-
> >  2 files changed, 4 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> > index b3c85406..fa398fea 100644
> > --- a/src/android/camera_hal_manager.cpp
> > +++ b/src/android/camera_hal_manager.cpp
> > @@ -34,18 +34,11 @@ CameraHalManager::CameraHalManager()
> >  {
> >  }
> >
> > -CameraHalManager::~CameraHalManager()
> > -{
> > -     if (cameraManager_) {
> > -             cameraManager_->stop();
>
> Shouldn't this line be kept ? Apart from that, the patch looks good to
> me.

I removed it because libcamera::CameraManager's dtor calls stop().

-Hiro
>
> > -             delete cameraManager_;
> > -             cameraManager_ = nullptr;
> > -     }
> > -}
> > +CameraHalManager::~CameraHalManager() = default;
> >
> >  int CameraHalManager::init()
> >  {
> > -     cameraManager_ = new CameraManager();
> > +     cameraManager_ = std::make_unique<CameraManager>();
> >
> >       /* Support camera hotplug. */
> >       cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded);
> > @@ -55,8 +48,7 @@ int CameraHalManager::init()
> >       if (ret) {
> >               LOG(HAL, Error) << "Failed to start camera manager: "
> >                               << strerror(-ret);
> > -             delete cameraManager_;
> > -             cameraManager_ = nullptr;
> > +             cameraManager_.reset();
> >               return ret;
> >       }
> >
> > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h
> > index 65bb3554..dc4e37e5 100644
> > --- a/src/android/camera_hal_manager.h
> > +++ b/src/android/camera_hal_manager.h
> > @@ -47,7 +47,7 @@ private:
> >
> >       CameraDevice *cameraDeviceFromHalId(unsigned int id);
> >
> > -     libcamera::CameraManager *cameraManager_;
> > +     std::unique_ptr<libcamera::CameraManager> cameraManager_;
> >
> >       const camera_module_callbacks_t *callbacks_;
> >       std::vector<std::unique_ptr<CameraDevice>> cameras_;
>
> --
> Regards,
>
> Laurent Pinchart