| Message ID | 20210323014226.3211412-2-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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); Reviewed-by: Laurent Pinchart <laurent.pinchart@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_; >
Hi Laurent, thanks for reviewing. On Tue, Mar 23, 2021 at 10:52 AM Laurent Pinchart <laurent.pinchart@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@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@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
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@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@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_; > > >
Hi Laurent, On Tue, Mar 23, 2021 at 11:00 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > 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@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. > Right. I agree with you. -Hiro > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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
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_;
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> --- 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.rc2.261.g7f71774620-goog