[libcamera-devel] [PATCH 12/14] android: camera_hal_manager: Clean up resources when terminating

[Laurent Pinchart]

Hi Jacopo,

On Mon, Aug 19, 2019 at 05:40:41PM +0200, Jacopo Mondi wrote:
> On Mon, Aug 19, 2019 at 06:18:55PM +0300, Laurent Pinchart wrote:
> > On Mon, Aug 19, 2019 at 11:23:57AM +0200, Jacopo Mondi wrote:
> >> On Sun, Aug 18, 2019 at 04:13:27AM +0300, Laurent Pinchart wrote:
> >>> The CameraHalManager starts the libcamera CameraManager and creates
> >>> CameraProxy instances for each camera in the system. Clean up those
> >>> resources when the CameraHalManager terminates.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>> ---
> >>>  src/android/camera_hal_manager.cpp | 4 ++++
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> >>> index a1ffb3713d7e..cf981720bca4 100644
> >>> --- a/src/android/camera_hal_manager.cpp
> >>> +++ b/src/android/camera_hal_manager.cpp
> >>> @@ -90,6 +90,10 @@ void CameraHalManager::run()
> >>>
> >>>  	/* Now start processing events and messages. */
> >>>  	exec();
> >>> +
> >>> +	/* Clean up the resources we have allocated. */
> >>> +	proxies_.clear();
> >>
> >> Proxies are unique_ptr
> >> 	std::vector<std::unique_ptr<CameraProxy>> proxies_;
> >>
> >> and they are unique_ptr so that they should be deleted at
> >> CameraHalManager deletion time. However, the CameraHalManager instance
> >> is defined as a static one in camera3_hal.cpp and the destructor of
> >> the static instance should be called at library teardown time. However
> >> I noticed with the help of valgrind the proxies memory did not get
> >> released properly (according to valgrind, which might fail to detect
> >> deletion after the library as been unloaded). So if you recall, I
> >> tried to address this with __attribute((destructor)) withtout seeing
> >> any change.
> >>
> >> With your change, which I think is good, I guess proxies_ can now be
> >> made a vector of plain pointers, doesn't it?
> >
> > clear() will delete all items in the vector. Deleting the
> > std::unique_ptr<CameraProxy> will thus delete the CameraProxy instances.
> > If we made them plain pointers, proxies_.clear() would clear the vector,
> > but not delete the objects pointed by the vector members. We thus need
> > eo keep unique_ptr.
> 
> Yeah, of course we would need to delete each single proxy if we make
> them plain pointers. So is it worth keeping them as unique_ptr just to
> be able to delete all of them with a single .clear ?
> 
> True that the overhead is minimal, but I don't see a reason apart for
> the above mentioned one to have them as unique_ptr

Yes, that's the whole point, std::vector<std::unique_ptr<T>> is a
self-deleting vector of T* :-) With std::vector<CameraProxy *> I would
have to write

	for (CameraProxy *proxy : proxies_)
		delete proxy;
	proxies_.clear();

and make sure not to forget any path where the vector is cleared (either
explicitly like here, or implicitly in the destructor).

Do you think that would be better ?

> >>> +	cameraManager_->stop();
> >>>  }
> >>>
> >>>  CameraProxy *CameraHalManager::open(unsigned int id,

-- 
Regards,

Laurent Pinchart