[libcamera-devel] [RFC PATCH] android: CameraWorker: Cancel pending messages in queue in stop()

[Hirokazu Honda]

Hi Jacopo, thank you for reviewing.

On Thu, May 20, 2021 at 8:12 PM Jacopo Mondi <jacopo at jmondi.org> wrote:

> On Fri, Apr 23, 2021 at 01:04:27PM +0900, Hirokazu Honda wrote:
> > CameraWorker::stop() executes Thread::exit() and Thread::wait().
> > However, intuitively, the two functions doesn't clean up pending
>

s/intuitively/surprisingly/

> messages in the message queue of the thread. They are rather
> > cleaned in the destruction of CameraWorker, but CameraWorker's
> > life time is the same as CameraDevice and thus CameraManager,
> > which is never destroyed. On the other hand,
> > Camera3RequestDescriptors are destroyed in CameraDevice::stop().
> >
> > This causes CameraWorker::Worker::queueRequest() starts to be
> > executed after CameraWorker::start(), which is after
>
> I'm not sure I'm following right the sequence of Worker::queueRequest(),
> CameraWorker::start() and CameraDevice::stop()
>
> Should this be CameraWorker::stop() ?
>
> Did you ever triggered this failure ?
>
>
I have encountered this issue when I tested my flush().
The HAL API flow is CameraDevice::close() ->
(CameraDevice::configureStreams() ->) CameraDevice::processCaptureRequest().
The internal flow is
* in CameraDevice::close()
   - CameraWorker::stop()
     - Thread::exit()
     - Thread::wait()
   - Camera::stop()
* in CameraDevice::processCaptureResult()
  - CameraWorker::start()

As I said, after CameraWorker::stop(), pending requests remain in the queue.
CameraWorker::start() starts with the remained requests and because the
object of the requests are destroyed by clearing descriptors_, the SIGSEGV
happens.


> > CameraDevice::stop(). This obviously causes a segmentation fault,
> > because CameraWorker::Worker::queueRequest() touches
> > CaptureRequest which is owned by Camera3RequestDescriptor.
> >
> > This fixes the issue by cancel pending message in thread queue in
> > CameraWorker::stop() with Thread::dispatchMessages().
>
> For my limited understanding of the Object and Thread classes, I think
> it's ok to dispatch all pending method calls.
>
> However what concerns me is also Request that are pending in
> void CameraWorker::Worker::processRequest() when the Worker gets
> stopped. Should we wait for the queue of pending requests to have
> finised waiting their fences (or instrument an interruption mechanism)
> before returning from stop() ?
>
>
If I understand correctly, Thread::dispatchMessages() executes
CameraWorker::Worker::processRequest().
Thread::dispatchMessages() calls Object::message(), and it executes
InvokeMessage::invoke().
I am honestly unsure.
Laurent should know as Laurent is the original author
of Thread::dispatchMessages().

Thanks,
-Hiro

> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> >  src/android/camera_worker.cpp | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/src/android/camera_worker.cpp
> b/src/android/camera_worker.cpp
> > index 300ddde0..b68ce9d5 100644
> > --- a/src/android/camera_worker.cpp
> > +++ b/src/android/camera_worker.cpp
> > @@ -62,6 +62,7 @@ void CameraWorker::start()
> >
> >  void CameraWorker::stop()
> >  {
> > +     thread_.dispatchMessages(Message::Type::InvokeMessage);
> >       thread_.exit();
> >       thread_.wait();
> >  }
> > --
> > 2.31.1.498.g6c1eba8ee3d-goog
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210521/dc63bb01/attachment.htm>