<div dir="ltr"><div>Hi Jacopo, thank you for reviewing.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, May 20, 2021 at 8:12 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">On Fri, Apr 23, 2021 at 01:04:27PM +0900, Hirokazu Honda wrote:<br>
> CameraWorker::stop() executes Thread::exit() and Thread::wait().<br>
> However, intuitively, the two functions doesn't clean up pending<br></blockquote><div><br></div><div>s/intuitively/surprisingly/ </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> messages in the message queue of the thread. They are rather<br>
> cleaned in the destruction of CameraWorker, but CameraWorker's<br>
> life time is the same as CameraDevice and thus CameraManager,<br>
> which is never destroyed. On the other hand,<br>
> Camera3RequestDescriptors are destroyed in CameraDevice::stop().<br>
><br>
> This causes CameraWorker::Worker::queueRequest() starts to be<br>
> executed after CameraWorker::start(), which is after<br>
<br>
I'm not sure I'm following right the sequence of Worker::queueRequest(),<br>
CameraWorker::start() and CameraDevice::stop()<br>
<br>
Should this be CameraWorker::stop() ?<br>
<br>
Did you ever triggered this failure ?<br>
<br></blockquote><div><br></div><div>I have encountered this issue when I tested my flush().</div><div>The HAL API flow is CameraDevice::close() -> (CameraDevice::configureStreams() ->) CameraDevice::processCaptureRequest().</div><div>The internal flow is </div><div>* in CameraDevice::close()</div><div>   - CameraWorker::stop()</div><div>     - Thread::exit()</div><div>     - Thread::wait()</div><div>   - Camera::stop()</div><div>* in CameraDevice::processCaptureResult() </div><div>  - CameraWorker::start() </div><div><br></div><div>As I said, after CameraWorker::stop(), pending requests remain in the queue.</div><div>CameraWorker::start() starts with the remained requests and because the object of the requests are destroyed by clearing descriptors_, the SIGSEGV happens.</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">
> CameraDevice::stop(). This obviously causes a segmentation fault,<br>
> because CameraWorker::Worker::queueRequest() touches<br>
> CaptureRequest which is owned by Camera3RequestDescriptor.<br>
><br>
> This fixes the issue by cancel pending message in thread queue in<br>
> CameraWorker::stop() with Thread::dispatchMessages().<br>
<br>
For my limited understanding of the Object and Thread classes, I think<br>
it's ok to dispatch all pending method calls.<br>
<br>
However what concerns me is also Request that are pending in<br>
void CameraWorker::Worker::processRequest() when the Worker gets<br>
stopped. Should we wait for the queue of pending requests to have<br>
finised waiting their fences (or instrument an interruption mechanism)<br>
before returning from stop() ?<br>
<br></blockquote><div><br></div><div>If I understand correctly, Thread::dispatchMessages() executes CameraWorker::Worker::processRequest(). </div><div>Thread::dispatchMessages() calls Object::message(), and it executes InvokeMessage::invoke(). </div><div>I am honestly unsure. </div><div>Laurent should know as Laurent is the original author of Thread::dispatchMessages().</div><div><br></div><div>Thanks,</div><div>-Hiro</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
> Signed-off-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org" target="_blank">hiroh@chromium.org</a>><br>
> ---<br>
>  src/android/camera_worker.cpp | 1 +<br>
>  1 file changed, 1 insertion(+)<br>
><br>
> diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp<br>
> index 300ddde0..b68ce9d5 100644<br>
> --- a/src/android/camera_worker.cpp<br>
> +++ b/src/android/camera_worker.cpp<br>
> @@ -62,6 +62,7 @@ void CameraWorker::start()<br>
><br>
>  void CameraWorker::stop()<br>
>  {<br>
> +     thread_.dispatchMessages(Message::Type::InvokeMessage);<br>
>       thread_.exit();<br>
>       thread_.wait();<br>
>  }<br>
> --<br>
> 2.31.1.498.g6c1eba8ee3d-goog<br>
><br>
</blockquote></div></div>