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

[Jacopo Mondi]

Hi Hiro

On Fri, May 21, 2021 at 01:44:44PM +0900, Hirokazu Honda wrote:
> Hi Jacopo, thank you for reviewing.

CC Laurent for his opinion

Thanks
  j

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