[libcamera-devel,2/8] android: CameraHalManager: Hold CameraManager with std::unique_ptr
diff mbox series

Message ID 20210323014226.3211412-3-hiroh@chromium.org
State Superseded
Headers show
Series
  • Improve pointer types in android HAL adaptation layer
Related show

Commit Message

Hirokazu Honda March 23, 2021, 1:42 a.m. UTC 
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@chromium.org>

---
 src/android/camera_hal_manager.cpp | 14 +++-----------
 src/android/camera_hal_manager.h   |  2 +-
 2 files changed, 4 insertions(+), 12 deletions(-)

--
2.31.0.rc2.261.g7f71774620-goog

Comments

Laurent Pinchart March 23, 2021, 1:59 a.m. UTC | #1 
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@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.

> -		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_;
Hirokazu Honda March 23, 2021, 2:25 a.m. UTC | #2 
Hi Laurent, thanks for reviewing.

On Tue, Mar 23, 2021 at 11:00 AM Laurent Pinchart
<laurent.pinchart@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@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
Laurent Pinchart March 23, 2021, 2:05 p.m. UTC | #3 
Hi Hiro,

On Tue, Mar 23, 2021 at 11:25:45AM +0900, Hirokazu Honda wrote:
> On Tue, Mar 23, 2021 at 11:00 AM Laurent Pinchart wrote:
> > 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@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().

It makes sense. Could you mention this in the commit message though ?

~CameraManager() calling stop() is an implementation detail at this
moment, but it makes sense, so I'll send a documentation patch to make
this official.

> > > -             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_;

Patch
diff mbox series 

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();
-		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_;