[libcamera-devel] [PATCH] android: camera_worker: Process all queued requests when stopping

[Laurent Pinchart]

Hi Jacopo,

On Mon, May 24, 2021 at 11:33:10AM +0200, Jacopo Mondi wrote:
> On Sun, May 23, 2021 at 05:33:42AM +0300, Laurent Pinchart wrote:
> > When stopping the camera worker, queuedRequest() calls may have queued
> > asynchronous function invocation messages to the worker thread, and some
> > of those messages may not have been processed yet. The messages will
> > stay in the thread's queue until the camera worker is restarted (when
> > the camera service will start a new capture session). At that point,
> > they will be dispatched, which will cause a crash due to the
> > CaptureRequest passed to processRequest() having been deleted by
> > CameraDevice::stop() calling descriptors_.clear().
> >
> > Fix this by forcing dispatching of all function invocation messages when
> > stopping the camera worker thread. Note that this is inherently racy, as
> > more queueRequest() calls may arrive from the camera service while we're
> > stopping. This race condition will be addressed by a subsequent patch
> > series.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > I haven't tested this patch, as I'm currently rebuilding a new Soraka
> > image. Hiro, could you give it a try to see if it fixes the problem
> > you've reported ?
> > ---
> >  src/android/camera_worker.cpp | 14 ++++++++++----
> >  src/android/camera_worker.h   |  6 ++++--
> >  2 files changed, 14 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp
> > index 300ddde03645..9f727826e23f 100644
> > --- a/src/android/camera_worker.cpp
> > +++ b/src/android/camera_worker.cpp
> > @@ -52,18 +52,24 @@ void CaptureRequest::queue()
> >   */
> >  CameraWorker::CameraWorker()
> >  {
> > -	worker_.moveToThread(&thread_);
> > +	worker_.moveToThread(this);
> >  }
> >
> >  void CameraWorker::start()
> >  {
> > -	thread_.start();
> > +	Thread::start();
> >  }
> >
> >  void CameraWorker::stop()
> >  {
> > -	thread_.exit();
> > -	thread_.wait();
> > +	exit();
> > +	wait();
> > +}
> > +
> > +void CameraWorker::run()
> > +{
> > +	exec();
> > +	dispatchMessages(Message::Type::InvokeMessage);
> >  }
> >
> >  void CameraWorker::queueRequest(CaptureRequest *request)
> > diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h
> > index 64b1658b61b4..e289ef9b8655 100644
> > --- a/src/android/camera_worker.h
> > +++ b/src/android/camera_worker.h
> > @@ -42,7 +42,7 @@ private:
> >  	std::unique_ptr<libcamera::Request> request_;
> >  };
> >
> > -class CameraWorker
> > +class CameraWorker : private libcamera::Thread
> 
> There is one thing I don't get here
> 
> Now the whole CameraWorker runs in it own thread.
>
> Before this change CameraWorker had a Worker that was run a separate
> thread.
> 
> CameraDevice calls CameraWorker::queueRequest, which runs on a
> different thread.

While the CameraWorker instance inherits from Thread, it isn't bound the
its own thread. The CameraWorker instance is an Object (as Thread
inherits from Object), and Object instance are bound by default to the
thread in which they're created. The can then be moved to another thread
by Object::moveToThread(). In this case, the CameraWorker instance is
created in the camera service thread (the main thread from the HAL's
point of view), and isn't moved to any other thread with moveToThread().
It thus stays bound to the camera service thread, not to the Thread
instance that it inherits from.

> CameraWorker::queueRequest
> 
>         void CameraWorker::queueRequest(CaptureRequest *request)

In any case, functions that are called directly still run in the thread
of the caller, in all cases. Only functions invoked through
Object::invokeMethod() or through signals with a queued connection type
will run in the thread to which the object is bound. This function is
called directly by the CameraDevice, so it runs in the caller's thread.

>         {
>                 /* Async process the request on the worker which runs its own thread. */
>                 worker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued,
>                                      request);

This invokes the Worker::processRequest() function with a queued
connection type. As the Worker instance has been moved to the
CameraWorker Thread, the call will be asynchronous and cross-threads.

>         }
> 
> 1) How does direct method calls on CameraWorker work now that it's a
> Thread
> 2) If 1) is ok, then queueRequest should not need to call invokeMethod
> on worker_ as it runs in the same thread CameraWorker itself.
> 
> Would it be possible to keep CameraWorker a simple class and make
> Worker a thread ?

I think we could do that, and move Worker to itself with
moveToThread(this). The Worker would then need to reimplement run() as
done above. Moving an object to "itself's thread" hasn't been well
tested though. Additionally, I think it's better conceptually speaking
to have CameraWorker inheriting from Thread. You can imagine it as
CameraWorker being a Thread, exposing an API to the CameraDevice to
control it, with the Worker running inside that thread.

> >  {
> >  public:
> >  	CameraWorker();
> > @@ -52,6 +52,9 @@ public:
> >
> >  	void queueRequest(CaptureRequest *request);
> >
> > +protected:
> > +	void run() override;
> > +
> >  private:
> >  	class Worker : public libcamera::Object
> >  	{
> > @@ -63,7 +66,6 @@ private:
> >  	};
> >
> >  	Worker worker_;
> > -	libcamera::Thread thread_;
> >  };
> >
> >  #endif /* __ANDROID_CAMERA_WORKER_H__ */

-- 
Regards,

Laurent Pinchart