<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, May 24, 2021 at 6:32 PM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Laurent,<br>
<br>
On Sun, May 23, 2021 at 05:33:42AM +0300, Laurent Pinchart wrote:<br>
> When stopping the camera worker, queuedRequest() calls may have queued<br>
> asynchronous function invocation messages to the worker thread, and some<br>
> of those messages may not have been processed yet. The messages will<br>
> stay in the thread's queue until the camera worker is restarted (when<br>
> the camera service will start a new capture session). At that point,<br>
> they will be dispatched, which will cause a crash due to the<br>
> CaptureRequest passed to processRequest() having been deleted by<br>
> CameraDevice::stop() calling descriptors_.clear().<br>
><br>
> Fix this by forcing dispatching of all function invocation messages when<br>
> stopping the camera worker thread. Note that this is inherently racy, as<br>
> more queueRequest() calls may arrive from the camera service while we're<br>
> stopping. This race condition will be addressed by a subsequent patch<br>
> series.<br>
><br>
> Signed-off-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br></blockquote><div><br></div><div>It looks working as far as I tested.</div><div><br></div><div>Tested-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org">hiroh@chromium.org</a>></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> ---<br>
> I haven't tested this patch, as I'm currently rebuilding a new Soraka<br>
> image. Hiro, could you give it a try to see if it fixes the problem<br>
> you've reported ?<br>
> ---<br>
>  src/android/camera_worker.cpp | 14 ++++++++++----<br>
>  src/android/camera_worker.h   |  6 ++++--<br>
>  2 files changed, 14 insertions(+), 6 deletions(-)<br>
><br>
> diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp<br>
> index 300ddde03645..9f727826e23f 100644<br>
> --- a/src/android/camera_worker.cpp<br>
> +++ b/src/android/camera_worker.cpp<br>
> @@ -52,18 +52,24 @@ void CaptureRequest::queue()<br>
>   */<br>
>  CameraWorker::CameraWorker()<br>
>  {<br>
> -     worker_.moveToThread(&thread_);<br>
> +     worker_.moveToThread(this);<br>
>  }<br>
><br>
>  void CameraWorker::start()<br>
>  {<br>
> -     thread_.start();<br>
> +     Thread::start();<br>
>  }<br>
><br>
>  void CameraWorker::stop()<br>
>  {<br>
> -     thread_.exit();<br>
> -     thread_.wait();<br>
> +     exit();<br>
> +     wait();<br>
> +}<br>
> +<br>
> +void CameraWorker::run()<br>
> +{<br>
> +     exec();<br>
> +     dispatchMessages(Message::Type::InvokeMessage);<br>
>  }<br>
><br>
>  void CameraWorker::queueRequest(CaptureRequest *request)<br>
> diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h<br>
> index 64b1658b61b4..e289ef9b8655 100644<br>
> --- a/src/android/camera_worker.h<br>
> +++ b/src/android/camera_worker.h<br>
> @@ -42,7 +42,7 @@ private:<br>
>       std::unique_ptr<libcamera::Request> request_;<br>
>  };<br>
><br>
> -class CameraWorker<br>
> +class CameraWorker : private libcamera::Thread<br>
<br>
There is one thing I don't get here<br>
<br>
Now the whole CameraWorker runs in it own thread.<br>
Before this change CameraWorker had a Worker that was run a separate<br>
thread.<br>
<br>
CameraDevice calls CameraWorker::queueRequest, which runs on a<br>
different thread.<br>
<br>
CameraWorker::queueRequest<br>
<br>
        void CameraWorker::queueRequest(CaptureRequest *request)<br>
        {<br>
                /* Async process the request on the worker which runs its own thread. */<br>
                worker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued,<br>
                                     request);<br>
        }<br>
<br>
1) How does direct method calls on CameraWorker work now that it's a<br>
Thread<br>
2) If 1) is ok, then queueRequest should not need to call invokeMethod<br>
on worker_ as it runs in the same thread CameraWorker itself.<br>
<br>
Would it be possible to keep CameraWorker a simple class and make<br>
Worker a thread ?<br>
<br>
Thanks<br>
   j<br>
<br>
>  {<br>
>  public:<br>
>       CameraWorker();<br>
> @@ -52,6 +52,9 @@ public:<br>
><br>
>       void queueRequest(CaptureRequest *request);<br>
><br>
> +protected:<br>
> +     void run() override;<br>
> +<br>
>  private:<br>
>       class Worker : public libcamera::Object<br>
>       {<br>
> @@ -63,7 +66,6 @@ private:<br>
>       };<br>
><br>
>       Worker worker_;<br>
> -     libcamera::Thread thread_;<br>
>  };<br>
><br>
>  #endif /* __ANDROID_CAMERA_WORKER_H__ */<br>
> --<br>
> Regards,<br>
><br>
> Laurent Pinchart<br>
><br>
</blockquote></div></div>