| Message ID | 20210523023342.13141-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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); > } > > 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 >
Hi Laurent, On Mon, May 24, 2021 at 6:32 PM Jacopo Mondi <jacopo@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@ideasonboard.com> > It looks working as far as I tested. Tested-by: Hirokazu Honda <hiroh@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 > > >
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@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); > > } > > > > 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. > 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__ */
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@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@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. > > > 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
Hi Laurent, On Mon, May 24, 2021 at 9:04 PM Jacopo Mondi <jacopo@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@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@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@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 >
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 { 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__ */
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@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(-)