[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:18:55PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> 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

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/63d46151/attachment.sig>