| Message ID | 20190818011329.14499-11-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Sun, Aug 18, 2019 at 04:13:25AM +0300, Laurent Pinchart wrote: > The CameraHalManager starts a thread that is never stopped. This leads > to the thread being destroyed while running, which causes a crash. Fix > this by stopping the thread and waiting for it to finish in the > destructor of the CameraHalManager. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/android/camera_hal_manager.cpp | 9 +++++++++ > src/android/camera_hal_manager.h | 2 ++ > 2 files changed, 11 insertions(+) > > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp > index 37ba01355258..063080a0746b 100644 > --- a/src/android/camera_hal_manager.cpp > +++ b/src/android/camera_hal_manager.cpp > @@ -28,6 +28,15 @@ LOG_DECLARE_CATEGORY(HAL); > * their static information and to open and close camera devices. > */ > > +CameraHalManager::~CameraHalManager() > +{ > + if (isRunning()) { > + exit(0); Took me some time to realize this is not the c library exit() function > + /* \todo Wait with a timeout, just in case. */ Doesn't wait() removes the need for a timeout? Or is this to shorten the waiting time? In any case: Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > + wait(); > + } > +} > + > int CameraHalManager::init() > { > /* > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h > index 8228623aba90..502115cf056f 100644 > --- a/src/android/camera_hal_manager.h > +++ b/src/android/camera_hal_manager.h > @@ -24,6 +24,8 @@ class CameraProxy; > class CameraHalManager : public libcamera::Thread > { > public: > + ~CameraHalManager(); > + > int init(); > > CameraProxy *open(unsigned int id, const hw_module_t *module); > -- > 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:16:12AM +0200, Jacopo Mondi wrote: > On Sun, Aug 18, 2019 at 04:13:25AM +0300, Laurent Pinchart wrote: > > The CameraHalManager starts a thread that is never stopped. This leads > > to the thread being destroyed while running, which causes a crash. Fix > > this by stopping the thread and waiting for it to finish in the > > destructor of the CameraHalManager. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/android/camera_hal_manager.cpp | 9 +++++++++ > > src/android/camera_hal_manager.h | 2 ++ > > 2 files changed, 11 insertions(+) > > > > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp > > index 37ba01355258..063080a0746b 100644 > > --- a/src/android/camera_hal_manager.cpp > > +++ b/src/android/camera_hal_manager.cpp > > @@ -28,6 +28,15 @@ LOG_DECLARE_CATEGORY(HAL); > > * their static information and to open and close camera devices. > > */ > > > > +CameraHalManager::~CameraHalManager() > > +{ > > + if (isRunning()) { > > + exit(0); > > Took me some time to realize this is not the c library exit() > function > > > + /* \todo Wait with a timeout, just in case. */ > > Doesn't wait() removes the need for a timeout? Or is this to shorten > the waiting time? If the thread run() function never stops (which shouldn't happen, but bugs exist), wait() will wait forever. I think a timeout would be useful. > In any case: > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > + wait(); > > + } > > +} > > + > > int CameraHalManager::init() > > { > > /* > > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h > > index 8228623aba90..502115cf056f 100644 > > --- a/src/android/camera_hal_manager.h > > +++ b/src/android/camera_hal_manager.h > > @@ -24,6 +24,8 @@ class CameraProxy; > > class CameraHalManager : public libcamera::Thread > > { > > public: > > + ~CameraHalManager(); > > + > > int init(); > > > > CameraProxy *open(unsigned int id, const hw_module_t *module);
diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp index 37ba01355258..063080a0746b 100644 --- a/src/android/camera_hal_manager.cpp +++ b/src/android/camera_hal_manager.cpp @@ -28,6 +28,15 @@ LOG_DECLARE_CATEGORY(HAL); * their static information and to open and close camera devices. */ +CameraHalManager::~CameraHalManager() +{ + if (isRunning()) { + exit(0); + /* \todo Wait with a timeout, just in case. */ + wait(); + } +} + int CameraHalManager::init() { /* diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h index 8228623aba90..502115cf056f 100644 --- a/src/android/camera_hal_manager.h +++ b/src/android/camera_hal_manager.h @@ -24,6 +24,8 @@ class CameraProxy; class CameraHalManager : public libcamera::Thread { public: + ~CameraHalManager(); + int init(); CameraProxy *open(unsigned int id, const hw_module_t *module);
The CameraHalManager starts a thread that is never stopped. This leads to the thread being destroyed while running, which causes a crash. Fix this by stopping the thread and waiting for it to finish in the destructor of the CameraHalManager. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/android/camera_hal_manager.cpp | 9 +++++++++ src/android/camera_hal_manager.h | 2 ++ 2 files changed, 11 insertions(+)