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

[Jacopo Mondi]

Hi Laurent,

On Mon, May 24, 2021 at 02:50:21PM +0300, Laurent Pinchart wrote:
> 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);

Thanks for the explanation. This
        moveToThread(this);

mislead me, but with the below explanation is all good now

Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

> > >  }
> > >
> > >  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