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

[Jacopo Mondi]

Hi Laurent,

On Mon, Aug 19, 2019 at 06:49:04PM +0300, Laurent Pinchart wrote:
> 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 ?
>

Let's say that if I had to write it from scratch I would not use
unique_ptr, but they do not harm if you want to keep them there, no
issues...

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

Thanks
   j

> > >>> +	cameraManager_->stop();
> > >>>  }
> > >>>
> > >>>  CameraProxy *CameraHalManager::open(unsigned int id,
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190819/5f57e09f/attachment.sig>