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

[Jacopo Mondi]

Hi Laurent,

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.

CameraWorker::queueRequest

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

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 ?

Thanks
   j

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