<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, May 24, 2021 at 9:04 PM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Laurent,<br>
<br>
On Mon, May 24, 2021 at 02:50:21PM +0300, Laurent Pinchart wrote:<br>
> Hi Jacopo,<br>
><br>
> On Mon, May 24, 2021 at 11:33:10AM +0200, Jacopo Mondi wrote:<br>
> > On Sun, May 23, 2021 at 05:33:42AM +0300, Laurent Pinchart wrote:<br>
> > > When stopping the camera worker, queuedRequest() calls may have queued<br>
> > > asynchronous function invocation messages to the worker thread, and some<br>
> > > of those messages may not have been processed yet. The messages will<br>
> > > stay in the thread's queue until the camera worker is restarted (when<br>
> > > the camera service will start a new capture session). At that point,<br>
> > > they will be dispatched, which will cause a crash due to the<br>
> > > CaptureRequest passed to processRequest() having been deleted by<br>
> > > CameraDevice::stop() calling descriptors_.clear().<br>
> > ><br>
> > > Fix this by forcing dispatching of all function invocation messages when<br>
> > > stopping the camera worker thread. Note that this is inherently racy, as<br>
> > > more queueRequest() calls may arrive from the camera service while we're<br>
> > > stopping. This race condition will be addressed by a subsequent patch<br>
> > > series.<br>
> > ><br>
> > > Signed-off-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
> > > ---<br>
> > > I haven't tested this patch, as I'm currently rebuilding a new Soraka<br>
> > > image. Hiro, could you give it a try to see if it fixes the problem<br>
> > > you've reported ?<br>
> > > ---<br>
> > > src/android/camera_worker.cpp | 14 ++++++++++----<br>
> > > src/android/camera_worker.h | 6 ++++--<br>
> > > 2 files changed, 14 insertions(+), 6 deletions(-)<br>
> > ><br>
> > > diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp<br>
> > > index 300ddde03645..9f727826e23f 100644<br>
> > > --- a/src/android/camera_worker.cpp<br>
> > > +++ b/src/android/camera_worker.cpp<br>
> > > @@ -52,18 +52,24 @@ void CaptureRequest::queue()<br>
> > > */<br>
> > > CameraWorker::CameraWorker()<br>
> > > {<br>
> > > - worker_.moveToThread(&thread_);<br>
> > > + worker_.moveToThread(this);<br>
<br>
Thanks for the explanation. This<br>
moveToThread(this);<br>
<br>
mislead me, but with the below explanation is all good now<br>
<br>
Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
<br>
Thanks<br>
j<br>
<br>
> > > }<br>
> > ><br>
> > > void CameraWorker::start()<br>
> > > {<br>
> > > - thread_.start();<br>
> > > + Thread::start();<br>
> > > }<br>
> > ><br>
> > > void CameraWorker::stop()<br>
> > > {<br>
> > > - thread_.exit();<br>
> > > - thread_.wait();<br>
> > > + exit();<br>
> > > + wait();<br>
> > > +}<br>
> > > +<br>
> > > +void CameraWorker::run()<br>
> > > +{<br>
> > > + exec();<br>
> > > + dispatchMessages(Message::Type::InvokeMessage);<br>
> > > }<br>
> > ><br>
> > > void CameraWorker::queueRequest(CaptureRequest *request)<br>
> > > diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h<br>
> > > index 64b1658b61b4..e289ef9b8655 100644<br>
> > > --- a/src/android/camera_worker.h<br>
> > > +++ b/src/android/camera_worker.h<br>
> > > @@ -42,7 +42,7 @@ private:<br>
> > > std::unique_ptr<libcamera::Request> request_;<br>
> > > };<br>
> > ><br>
> > > -class CameraWorker<br>
> > > +class CameraWorker : private libcamera::Thread<br>
> ><br>
> > There is one thing I don't get here<br>
> ><br>
> > Now the whole CameraWorker runs in it own thread.<br>
> ><br>
> > Before this change CameraWorker had a Worker that was run a separate<br>
> > thread.<br>
> ><br>
> > CameraDevice calls CameraWorker::queueRequest, which runs on a<br>
> > different thread.<br>
><br>
> While the CameraWorker instance inherits from Thread, it isn't bound the<br>
> its own thread. The CameraWorker instance is an Object (as Thread<br>
> inherits from Object), and Object instance are bound by default to the<br>
> thread in which they're created. The can then be moved to another thread<br>
> by Object::moveToThread(). In this case, the CameraWorker instance is<br>
> created in the camera service thread (the main thread from the HAL's<br>
> point of view), and isn't moved to any other thread with moveToThread().<br>
> It thus stays bound to the camera service thread, not to the Thread<br>
> instance that it inherits from.<br>
><br></blockquote><div><br></div><div><br></div><div>This explanation should be correct.</div><div>But </div><div>> as Thread inherits from Object.</div><div>looking Thread class, it doesn't inherit from Object. What am I missing?</div><div><br></div><div>The code change looks correct to me.</div><div><br></div><div>Reviewed-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org">hiroh@chromium.org</a>></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> > CameraWorker::queueRequest<br>
> ><br>
> > void CameraWorker::queueRequest(CaptureRequest *request)<br>
><br>
> In any case, functions that are called directly still run in the thread<br>
> of the caller, in all cases. Only functions invoked through<br>
> Object::invokeMethod() or through signals with a queued connection type<br>
> will run in the thread to which the object is bound. This function is<br>
> called directly by the CameraDevice, so it runs in the caller's thread.<br>
><br>
> > {<br>
> > /* Async process the request on the worker which runs its own thread. */<br>
> > worker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued,<br>
> > request);<br>
><br>
> This invokes the Worker::processRequest() function with a queued<br>
> connection type. As the Worker instance has been moved to the<br>
> CameraWorker Thread, the call will be asynchronous and cross-threads.<br>
><br>
> > }<br>
> ><br>
> > 1) How does direct method calls on CameraWorker work now that it's a<br>
> > Thread<br>
> > 2) If 1) is ok, then queueRequest should not need to call invokeMethod<br>
> > on worker_ as it runs in the same thread CameraWorker itself.<br>
> ><br>
> > Would it be possible to keep CameraWorker a simple class and make<br>
> > Worker a thread ?<br>
><br>
> I think we could do that, and move Worker to itself with<br>
> moveToThread(this). The Worker would then need to reimplement run() as<br>
> done above. Moving an object to "itself's thread" hasn't been well<br>
> tested though. Additionally, I think it's better conceptually speaking<br>
> to have CameraWorker inheriting from Thread. You can imagine it as<br>
> CameraWorker being a Thread, exposing an API to the CameraDevice to<br>
> control it, with the Worker running inside that thread.<br>
><br>
> > > {<br>
> > > public:<br>
> > > CameraWorker();<br>
> > > @@ -52,6 +52,9 @@ public:<br>
> > ><br>
> > > void queueRequest(CaptureRequest *request);<br>
> > ><br>
> > > +protected:<br>
> > > + void run() override;<br>
> > > +<br>
> > > private:<br>
> > > class Worker : public libcamera::Object<br>
> > > {<br>
> > > @@ -63,7 +66,6 @@ private:<br>
> > > };<br>
> > ><br>
> > > Worker worker_;<br>
> > > - libcamera::Thread thread_;<br>
> > > };<br>
> > ><br>
> > > #endif /* __ANDROID_CAMERA_WORKER_H__ */<br>
><br>
> --<br>
> Regards,<br>
><br>
> Laurent Pinchart<br>
</blockquote></div></div>