[libcamera-devel] [PATCH] android: camera_worker: Process all queued requests when stopping
Hirokazu Honda
hiroh at chromium.org
Tue May 25 04:14:14 CEST 2021
Hi Laurent,
On Mon, May 24, 2021 at 9:04 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> Hi Laurent,
>
> On Mon, May 24, 2021 at 02:50:21PM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > On Mon, May 24, 2021 at 11:33:10AM +0200, Jacopo Mondi wrote:
> > > 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>
> > > > ---
> > > > 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);
>
> Thanks for the explanation. This
> moveToThread(this);
>
> mislead me, but with the below explanation is all good now
>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> Thanks
> j
>
> > > > }
> > > >
> > > > 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.
> >
> > While the CameraWorker instance inherits from Thread, it isn't bound the
> > its own thread. The CameraWorker instance is an Object (as Thread
> > inherits from Object), and Object instance are bound by default to the
> > thread in which they're created. The can then be moved to another thread
> > by Object::moveToThread(). In this case, the CameraWorker instance is
> > created in the camera service thread (the main thread from the HAL's
> > point of view), and isn't moved to any other thread with moveToThread().
> > It thus stays bound to the camera service thread, not to the Thread
> > instance that it inherits from.
> >
>
This explanation should be correct.
But
> as Thread inherits from Object.
looking Thread class, it doesn't inherit from Object. What am I missing?
The code change looks correct to me.
Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> > > CameraWorker::queueRequest
> > >
> > > void CameraWorker::queueRequest(CaptureRequest *request)
> >
> > In any case, functions that are called directly still run in the thread
> > of the caller, in all cases. Only functions invoked through
> > Object::invokeMethod() or through signals with a queued connection type
> > will run in the thread to which the object is bound. This function is
> > called directly by the CameraDevice, so it runs in the caller's thread.
> >
> > > {
> > > /* Async process the request on the worker which runs
> its own thread. */
> > > worker_.invokeMethod(&Worker::processRequest,
> ConnectionTypeQueued,
> > > request);
> >
> > This invokes the Worker::processRequest() function with a queued
> > connection type. As the Worker instance has been moved to the
> > CameraWorker Thread, the call will be asynchronous and cross-threads.
> >
> > > }
> > >
> > > 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 ?
> >
> > I think we could do that, and move Worker to itself with
> > moveToThread(this). The Worker would then need to reimplement run() as
> > done above. Moving an object to "itself's thread" hasn't been well
> > tested though. Additionally, I think it's better conceptually speaking
> > to have CameraWorker inheriting from Thread. You can imagine it as
> > CameraWorker being a Thread, exposing an API to the CameraDevice to
> > control it, with the Worker running inside that thread.
> >
> > > > {
> > > > 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/20210525/05d0ecd0/attachment-0001.htm>
More information about the libcamera-devel
mailing list