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

11677 diff mbox series

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@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 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(-)

--
2.31.0.291.g576ba9dcdaf-goog

Hi Hiro,

On Wed, Mar 24, 2021 at 04:07:50PM +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@chromium.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Indeed!

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

> ---
>  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));
>  }
>
>  /*
> 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_;
>
> --
> 2.31.0.291.g576ba9dcdaf-goog
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel