[libcamera-devel] [PATCH 10/14] android: camera_hal_manager: Stop thread when destroying

[Laurent Pinchart]

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 at 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 at 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);

-- 
Regards,

Laurent Pinchart