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

[Hirokazu Honda]

Hi Laurent,

On Mon, May 24, 2021 at 6:32 PM Jacopo Mondi <jacopo at jmondi.org> wrote:

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

It looks working as far as I tested.

Tested-by: Hirokazu Honda <hiroh at chromium.org>


> > ---
> > 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210524/1890f856/attachment.htm>