| Message ID | 20190818011329.14499-13-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent 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@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? Thanks j > + cameraManager_->stop(); > } > > CameraProxy *CameraHalManager::open(unsigned int id, > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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. > > + cameraManager_->stop(); > > } > > > > CameraProxy *CameraHalManager::open(unsigned int id,
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@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
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@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,
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@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@jmondi.org> Thanks j > > >>> + cameraManager_->stop(); > > >>> } > > >>> > > >>> CameraProxy *CameraHalManager::open(unsigned int id, > > -- > Regards, > > Laurent Pinchart
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(); + cameraManager_->stop(); } CameraProxy *CameraHalManager::open(unsigned int id,
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@ideasonboard.com> --- src/android/camera_hal_manager.cpp | 4 ++++ 1 file changed, 4 insertions(+)