[{"id":17181,"web_url":"https://patchwork.libcamera.org/comment/17181/","msgid":"<20210524093310.x5g4zyvozifcu6fu@uno.localdomain>","date":"2021-05-24T09:33:10","subject":"Re: [libcamera-devel] [PATCH] android: camera_worker: Process all\n\tqueued requests when stopping","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sun, May 23, 2021 at 05:33:42AM +0300, Laurent Pinchart wrote:\n> When stopping the camera worker, queuedRequest() calls may have queued\n> asynchronous function invocation messages to the worker thread, and some\n> of those messages may not have been processed yet. The messages will\n> stay in the thread's queue until the camera worker is restarted (when\n> the camera service will start a new capture session). At that point,\n> they will be dispatched, which will cause a crash due to the\n> CaptureRequest passed to processRequest() having been deleted by\n> CameraDevice::stop() calling descriptors_.clear().\n>\n> Fix this by forcing dispatching of all function invocation messages when\n> stopping the camera worker thread. Note that this is inherently racy, as\n> more queueRequest() calls may arrive from the camera service while we're\n> stopping. This race condition will be addressed by a subsequent patch\n> series.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> I haven't tested this patch, as I'm currently rebuilding a new Soraka\n> image. Hiro, could you give it a try to see if it fixes the problem\n> you've reported ?\n> ---\n> src/android/camera_worker.cpp | 14 ++++++++++----\n> src/android/camera_worker.h | 6 ++++--\n> 2 files changed, 14 insertions(+), 6 deletions(-)\n>\n> diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp\n> index 300ddde03645..9f727826e23f 100644\n> --- a/src/android/camera_worker.cpp\n> +++ b/src/android/camera_worker.cpp\n> @@ -52,18 +52,24 @@ void CaptureRequest::queue()\n> */\n> CameraWorker::CameraWorker()\n> {\n> -\tworker_.moveToThread(&thread_);\n> +\tworker_.moveToThread(this);\n> }\n>\n> void CameraWorker::start()\n> {\n> -\tthread_.start();\n> +\tThread::start();\n> }\n>\n> void CameraWorker::stop()\n> {\n> -\tthread_.exit();\n> -\tthread_.wait();\n> +\texit();\n> +\twait();\n> +}\n> +\n> +void CameraWorker::run()\n> +{\n> +\texec();\n> +\tdispatchMessages(Message::Type::InvokeMessage);\n> }\n>\n> void CameraWorker::queueRequest(CaptureRequest *request)\n> diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h\n> index 64b1658b61b4..e289ef9b8655 100644\n> --- a/src/android/camera_worker.h\n> +++ b/src/android/camera_worker.h\n> @@ -42,7 +42,7 @@ private:\n> \tstd::unique_ptr request_;\n> };\n>\n> -class CameraWorker\n> +class CameraWorker : private libcamera::Thread\n\nThere is one thing I don't get here\n\nNow the whole CameraWorker runs in it own thread.\nBefore this change CameraWorker had a Worker that was run a separate\nthread.\n\nCameraDevice calls CameraWorker::queueRequest, which runs on a\ndifferent thread.\n\nCameraWorker::queueRequest\n\n void CameraWorker::queueRequest(CaptureRequest *request)\n {\n /* Async process the request on the worker which runs its own thread. */\n worker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued,\n request);\n }\n\n1) How does direct method calls on CameraWorker work now that it's a\nThread\n2) If 1) is ok, then queueRequest should not need to call invokeMethod\non worker_ as it runs in the same thread CameraWorker itself.\n\nWould it be possible to keep CameraWorker a simple class and make\nWorker a thread ?\n\nThanks\n j\n\n> {\n> public:\n> \tCameraWorker();\n> @@ -52,6 +52,9 @@ public:\n>\n> \tvoid queueRequest(CaptureRequest *request);\n>\n> +protected:\n> +\tvoid run() override;\n> +\n> private:\n> \tclass Worker : public libcamera::Object\n> \t{\n> @@ -63,7 +66,6 @@ private:\n> \t};\n>\n> \tWorker worker_;\n> -\tlibcamera::Thread thread_;\n> };\n>\n> #endif /* __ANDROID_CAMERA_WORKER_H__ */\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DDADAC3200\n\tfor ;\n\tMon, 24 May 2021 09:32:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5030C68919;\n\tMon, 24 May 2021 11:32:25 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D7B89602B1\n\tfor ;\n\tMon, 24 May 2021 11:32:24 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 55A2D1C0004;\n\tMon, 24 May 2021 09:32:24 +0000 (UTC)"],"Date":"Mon, 24 May 2021 11:33:10 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210524093310.x5g4zyvozifcu6fu@uno.localdomain>","References":"<20210523023342.13141-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210523023342.13141-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] android: camera_worker: Process all\n\tqueued requests when stopping","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17187,"web_url":"https://patchwork.libcamera.org/comment/17187/","msgid":"","date":"2021-05-24T11:09:23","subject":"Re: [libcamera-devel] [PATCH] android: camera_worker: Process all\n\tqueued requests when stopping","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Mon, May 24, 2021 at 6:32 PM Jacopo Mondi wrote:\n\n> Hi Laurent,\n>\n> On Sun, May 23, 2021 at 05:33:42AM +0300, Laurent Pinchart wrote:\n> > When stopping the camera worker, queuedRequest() calls may have queued\n> > asynchronous function invocation messages to the worker thread, and some\n> > of those messages may not have been processed yet. The messages will\n> > stay in the thread's queue until the camera worker is restarted (when\n> > the camera service will start a new capture session). At that point,\n> > they will be dispatched, which will cause a crash due to the\n> > CaptureRequest passed to processRequest() having been deleted by\n> > CameraDevice::stop() calling descriptors_.clear().\n> >\n> > Fix this by forcing dispatching of all function invocation messages when\n> > stopping the camera worker thread. Note that this is inherently racy, as\n> > more queueRequest() calls may arrive from the camera service while we're\n> > stopping. This race condition will be addressed by a subsequent patch\n> > series.\n> >\n> > Signed-off-by: Laurent Pinchart \n>\n\nIt looks working as far as I tested.\n\nTested-by: Hirokazu Honda \n\n\n> > ---\n> > I haven't tested this patch, as I'm currently rebuilding a new Soraka\n> > image. Hiro, could you give it a try to see if it fixes the problem\n> > you've reported ?\n> > ---\n> > src/android/camera_worker.cpp | 14 ++++++++++----\n> > src/android/camera_worker.h | 6 ++++--\n> > 2 files changed, 14 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/src/android/camera_worker.cpp\n> b/src/android/camera_worker.cpp\n> > index 300ddde03645..9f727826e23f 100644\n> > --- a/src/android/camera_worker.cpp\n> > +++ b/src/android/camera_worker.cpp\n> > @@ -52,18 +52,24 @@ void CaptureRequest::queue()\n> > */\n> > CameraWorker::CameraWorker()\n> > {\n> > - worker_.moveToThread(&thread_);\n> > + worker_.moveToThread(this);\n> > }\n> >\n> > void CameraWorker::start()\n> > {\n> > - thread_.start();\n> > + Thread::start();\n> > }\n> >\n> > void CameraWorker::stop()\n> > {\n> > - thread_.exit();\n> > - thread_.wait();\n> > + exit();\n> > + wait();\n> > +}\n> > +\n> > +void CameraWorker::run()\n> > +{\n> > + exec();\n> > + dispatchMessages(Message::Type::InvokeMessage);\n> > }\n> >\n> > void CameraWorker::queueRequest(CaptureRequest *request)\n> > diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h\n> > index 64b1658b61b4..e289ef9b8655 100644\n> > --- a/src/android/camera_worker.h\n> > +++ b/src/android/camera_worker.h\n> > @@ -42,7 +42,7 @@ private:\n> > std::unique_ptr request_;\n> > };\n> >\n> > -class CameraWorker\n> > +class CameraWorker : private libcamera::Thread\n>\n> There is one thing I don't get here\n>\n> Now the whole CameraWorker runs in it own thread.\n> Before this change CameraWorker had a Worker that was run a separate\n> thread.\n>\n> CameraDevice calls CameraWorker::queueRequest, which runs on a\n> different thread.\n>\n> CameraWorker::queueRequest\n>\n> void CameraWorker::queueRequest(CaptureRequest *request)\n> {\n> /* Async process the request on the worker which runs its\n> own thread. */\n> worker_.invokeMethod(&Worker::processRequest,\n> ConnectionTypeQueued,\n> request);\n> }\n>\n> 1) How does direct method calls on CameraWorker work now that it's a\n> Thread\n> 2) If 1) is ok, then queueRequest should not need to call invokeMethod\n> on worker_ as it runs in the same thread CameraWorker itself.\n>\n> Would it be possible to keep CameraWorker a simple class and make\n> Worker a thread ?\n>\n> Thanks\n> j\n>\n> > {\n> > public:\n> > CameraWorker();\n> > @@ -52,6 +52,9 @@ public:\n> >\n> > void queueRequest(CaptureRequest *request);\n> >\n> > +protected:\n> > + void run() override;\n> > +\n> > private:\n> > class Worker : public libcamera::Object\n> > {\n> > @@ -63,7 +66,6 @@ private:\n> > };\n> >\n> > Worker worker_;\n> > - libcamera::Thread thread_;\n> > };\n> >\n> > #endif /* __ANDROID_CAMERA_WORKER_H__ */\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 64A17C3200\n\tfor ;\n\tMon, 24 May 2021 11:09:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B9FE568919;\n\tMon, 24 May 2021 13:09:36 +0200 (CEST)","from mail-ed1-x532.google.com (mail-ed1-x532.google.com\n\t[IPv6:2a00:1450:4864:20::532])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 34E32601AA\n\tfor ;\n\tMon, 24 May 2021 13:09:35 +0200 (CEST)","by mail-ed1-x532.google.com with SMTP id h16so31415193edr.6\n\tfor ;\n\tMon, 24 May 2021 04:09:35 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"AbMyFgxB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=hF39qBKfhkjFCqD9ETGNhc1QpYhrrJS/44do4Ldy23o=;\n\tb=AbMyFgxBckIxO2mjvv95XmRro44cen5HRGHAGDhKbhoI8ilYO//bj+VDBSvjZ0sMmL\n\tp+ivMFTyjMpREwmi2d9/9M3tODSxcx5RcodEiYa65ET2KE6Wk0rCiHfhfqsFFxLGoe20\n\ts76LYX5zt7thHYdrz+OUEaJvVP37WiVBjMLNw=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=hF39qBKfhkjFCqD9ETGNhc1QpYhrrJS/44do4Ldy23o=;\n\tb=aLpYQAtIUTvo+bMg15Oa1scTmOGBHOw29HNYFzBHT9Ou8XCVcpPFmwd1uRWsOv3mI6\n\tqIHWEQ7hbCXib5vg/NaNE0qBdHaeEckffbIp12cAg/dlTp552oIdlXREyOt5ZKFt5AUp\n\tag6AdaVRjiwTETSYRbYI/TwvHFjJLA+S+Oxsaq0YQoeJGAhl7mvPWT0UkYEL3hMQlOQ/\n\tdGAgD+Sq0kkeMOvhcg96Pn5pKNnq68v9hpz6JtPUsoMuHTEX6/a4W34E3Z4NBvlcblMP\n\tpkf/GikRbwaVGXiB6TaKBSnodnu0PPJlA7JxCCmIH3YgPPVLri62sqSK+kESDuArcBB+\n\tsNMg==","X-Gm-Message-State":"AOAM533TX1JBeetS0Bhb2Y+X7KnG8OmRPP6EeyaQgu32qtK9SwIVyyf7\n\thHtv80l54k9K4c0sQ+cI7oTaYcCqYx4bUcUiAGp5Qunpgs8=","X-Google-Smtp-Source":"ABdhPJx3SpKy9vNZntLNkzpKWUeyoPJzYgLCqO+/Yhr/pxSwUdsjbca+HWAtkgcQUSndvtFmMr1Sa1wqrfxmVeJeB3g=","X-Received":"by 2002:a05:6402:95d:: with SMTP id\n\th29mr23093337edz.233.1621854574782; \n\tMon, 24 May 2021 04:09:34 -0700 (PDT)","MIME-Version":"1.0","References":"<20210523023342.13141-1-laurent.pinchart@ideasonboard.com>\n\t<20210524093310.x5g4zyvozifcu6fu@uno.localdomain>","In-Reply-To":"<20210524093310.x5g4zyvozifcu6fu@uno.localdomain>","From":"Hirokazu Honda ","Date":"Mon, 24 May 2021 20:09:23 +0900","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"multipart/alternative; boundary=\"000000000000ceb92305c31171f4\"","Subject":"Re: [libcamera-devel] [PATCH] android: camera_worker: Process all\n\tqueued requests when stopping","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17189,"web_url":"https://patchwork.libcamera.org/comment/17189/","msgid":"","date":"2021-05-24T11:50:21","subject":"Re: [libcamera-devel] [PATCH] android: camera_worker: Process all\n\tqueued requests when stopping","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, May 24, 2021 at 11:33:10AM +0200, Jacopo Mondi wrote:\n> On Sun, May 23, 2021 at 05:33:42AM +0300, Laurent Pinchart wrote:\n> > When stopping the camera worker, queuedRequest() calls may have queued\n> > asynchronous function invocation messages to the worker thread, and some\n> > of those messages may not have been processed yet. The messages will\n> > stay in the thread's queue until the camera worker is restarted (when\n> > the camera service will start a new capture session). At that point,\n> > they will be dispatched, which will cause a crash due to the\n> > CaptureRequest passed to processRequest() having been deleted by\n> > CameraDevice::stop() calling descriptors_.clear().\n> >\n> > Fix this by forcing dispatching of all function invocation messages when\n> > stopping the camera worker thread. Note that this is inherently racy, as\n> > more queueRequest() calls may arrive from the camera service while we're\n> > stopping. This race condition will be addressed by a subsequent patch\n> > series.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > I haven't tested this patch, as I'm currently rebuilding a new Soraka\n> > image. Hiro, could you give it a try to see if it fixes the problem\n> > you've reported ?\n> > ---\n> > src/android/camera_worker.cpp | 14 ++++++++++----\n> > src/android/camera_worker.h | 6 ++++--\n> > 2 files changed, 14 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp\n> > index 300ddde03645..9f727826e23f 100644\n> > --- a/src/android/camera_worker.cpp\n> > +++ b/src/android/camera_worker.cpp\n> > @@ -52,18 +52,24 @@ void CaptureRequest::queue()\n> > */\n> > CameraWorker::CameraWorker()\n> > {\n> > -\tworker_.moveToThread(&thread_);\n> > +\tworker_.moveToThread(this);\n> > }\n> >\n> > void CameraWorker::start()\n> > {\n> > -\tthread_.start();\n> > +\tThread::start();\n> > }\n> >\n> > void CameraWorker::stop()\n> > {\n> > -\tthread_.exit();\n> > -\tthread_.wait();\n> > +\texit();\n> > +\twait();\n> > +}\n> > +\n> > +void CameraWorker::run()\n> > +{\n> > +\texec();\n> > +\tdispatchMessages(Message::Type::InvokeMessage);\n> > }\n> >\n> > void CameraWorker::queueRequest(CaptureRequest *request)\n> > diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h\n> > index 64b1658b61b4..e289ef9b8655 100644\n> > --- a/src/android/camera_worker.h\n> > +++ b/src/android/camera_worker.h\n> > @@ -42,7 +42,7 @@ private:\n> > \tstd::unique_ptr request_;\n> > };\n> >\n> > -class CameraWorker\n> > +class CameraWorker : private libcamera::Thread\n> \n> There is one thing I don't get here\n> \n> Now the whole CameraWorker runs in it own thread.\n>\n> Before this change CameraWorker had a Worker that was run a separate\n> thread.\n> \n> CameraDevice calls CameraWorker::queueRequest, which runs on a\n> different thread.\n\nWhile the CameraWorker instance inherits from Thread, it isn't bound the\nits own thread. The CameraWorker instance is an Object (as Thread\ninherits from Object), and Object instance are bound by default to the\nthread in which they're created. The can then be moved to another thread\nby Object::moveToThread(). In this case, the CameraWorker instance is\ncreated in the camera service thread (the main thread from the HAL's\npoint of view), and isn't moved to any other thread with moveToThread().\nIt thus stays bound to the camera service thread, not to the Thread\ninstance that it inherits from.\n\n> CameraWorker::queueRequest\n> \n> void CameraWorker::queueRequest(CaptureRequest *request)\n\nIn any case, functions that are called directly still run in the thread\nof the caller, in all cases. Only functions invoked through\nObject::invokeMethod() or through signals with a queued connection type\nwill run in the thread to which the object is bound. This function is\ncalled directly by the CameraDevice, so it runs in the caller's thread.\n\n> {\n> /* Async process the request on the worker which runs its own thread. */\n> worker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued,\n> request);\n\nThis invokes the Worker::processRequest() function with a queued\nconnection type. As the Worker instance has been moved to the\nCameraWorker Thread, the call will be asynchronous and cross-threads.\n\n> }\n> \n> 1) How does direct method calls on CameraWorker work now that it's a\n> Thread\n> 2) If 1) is ok, then queueRequest should not need to call invokeMethod\n> on worker_ as it runs in the same thread CameraWorker itself.\n> \n> Would it be possible to keep CameraWorker a simple class and make\n> Worker a thread ?\n\nI think we could do that, and move Worker to itself with\nmoveToThread(this). The Worker would then need to reimplement run() as\ndone above. Moving an object to \"itself's thread\" hasn't been well\ntested though. Additionally, I think it's better conceptually speaking\nto have CameraWorker inheriting from Thread. You can imagine it as\nCameraWorker being a Thread, exposing an API to the CameraDevice to\ncontrol it, with the Worker running inside that thread.\n\n> > {\n> > public:\n> > \tCameraWorker();\n> > @@ -52,6 +52,9 @@ public:\n> >\n> > \tvoid queueRequest(CaptureRequest *request);\n> >\n> > +protected:\n> > +\tvoid run() override;\n> > +\n> > private:\n> > \tclass Worker : public libcamera::Object\n> > \t{\n> > @@ -63,7 +66,6 @@ private:\n> > \t};\n> >\n> > \tWorker worker_;\n> > -\tlibcamera::Thread thread_;\n> > };\n> >\n> > #endif /* __ANDROID_CAMERA_WORKER_H__ */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 557EBC3200\n\tfor ;\n\tMon, 24 May 2021 11:50:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C61AE601AA;\n\tMon, 24 May 2021 13:50:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1BA5E601AA\n\tfor ;\n\tMon, 24 May 2021 13:50:25 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 788C4ED;\n\tMon, 24 May 2021 13:50:24 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"D7DhL+Ln\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621857024;\n\tbh=l+z+X3Er/X/Bm/5+xT/w3ZJdqvB3WQKbyng2x+wQUvU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=D7DhL+Lnbspf7zp3M+04LnCZHFGkHj9cIE5NkI5vL6DfULT1Ft+83M/ZUV+/K/2eZ\n\tileUJIMqcKq63HZ5Q5H4nVyA5kVWD2xEI5BH+mWPP6/XiQ74iMlr3vdcLLQNbWqmJ2\n\tgGjBgVZ+lVVyEKSPqY0v+7ywvXHyz0oemlBooPpE=","Date":"Mon, 24 May 2021 14:50:21 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210523023342.13141-1-laurent.pinchart@ideasonboard.com>\n\t<20210524093310.x5g4zyvozifcu6fu@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210524093310.x5g4zyvozifcu6fu@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH] android: camera_worker: Process all\n\tqueued requests when stopping","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17191,"web_url":"https://patchwork.libcamera.org/comment/17191/","msgid":"<20210524120529.ja7jfphsqgkvwa3z@uno.localdomain>","date":"2021-05-24T12:05:29","subject":"Re: [libcamera-devel] [PATCH] android: camera_worker: Process all\n\tqueued requests when stopping","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, May 24, 2021 at 02:50:21PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, May 24, 2021 at 11:33:10AM +0200, Jacopo Mondi wrote:\n> > On Sun, May 23, 2021 at 05:33:42AM +0300, Laurent Pinchart wrote:\n> > > When stopping the camera worker, queuedRequest() calls may have queued\n> > > asynchronous function invocation messages to the worker thread, and some\n> > > of those messages may not have been processed yet. The messages will\n> > > stay in the thread's queue until the camera worker is restarted (when\n> > > the camera service will start a new capture session). At that point,\n> > > they will be dispatched, which will cause a crash due to the\n> > > CaptureRequest passed to processRequest() having been deleted by\n> > > CameraDevice::stop() calling descriptors_.clear().\n> > >\n> > > Fix this by forcing dispatching of all function invocation messages when\n> > > stopping the camera worker thread. Note that this is inherently racy, as\n> > > more queueRequest() calls may arrive from the camera service while we're\n> > > stopping. This race condition will be addressed by a subsequent patch\n> > > series.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > I haven't tested this patch, as I'm currently rebuilding a new Soraka\n> > > image. Hiro, could you give it a try to see if it fixes the problem\n> > > you've reported ?\n> > > ---\n> > > src/android/camera_worker.cpp | 14 ++++++++++----\n> > > src/android/camera_worker.h | 6 ++++--\n> > > 2 files changed, 14 insertions(+), 6 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp\n> > > index 300ddde03645..9f727826e23f 100644\n> > > --- a/src/android/camera_worker.cpp\n> > > +++ b/src/android/camera_worker.cpp\n> > > @@ -52,18 +52,24 @@ void CaptureRequest::queue()\n> > > */\n> > > CameraWorker::CameraWorker()\n> > > {\n> > > -\tworker_.moveToThread(&thread_);\n> > > +\tworker_.moveToThread(this);\n\nThanks for the explanation. This\n moveToThread(this);\n\nmislead me, but with the below explanation is all good now\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> > > }\n> > >\n> > > void CameraWorker::start()\n> > > {\n> > > -\tthread_.start();\n> > > +\tThread::start();\n> > > }\n> > >\n> > > void CameraWorker::stop()\n> > > {\n> > > -\tthread_.exit();\n> > > -\tthread_.wait();\n> > > +\texit();\n> > > +\twait();\n> > > +}\n> > > +\n> > > +void CameraWorker::run()\n> > > +{\n> > > +\texec();\n> > > +\tdispatchMessages(Message::Type::InvokeMessage);\n> > > }\n> > >\n> > > void CameraWorker::queueRequest(CaptureRequest *request)\n> > > diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h\n> > > index 64b1658b61b4..e289ef9b8655 100644\n> > > --- a/src/android/camera_worker.h\n> > > +++ b/src/android/camera_worker.h\n> > > @@ -42,7 +42,7 @@ private:\n> > > \tstd::unique_ptr request_;\n> > > };\n> > >\n> > > -class CameraWorker\n> > > +class CameraWorker : private libcamera::Thread\n> >\n> > There is one thing I don't get here\n> >\n> > Now the whole CameraWorker runs in it own thread.\n> >\n> > Before this change CameraWorker had a Worker that was run a separate\n> > thread.\n> >\n> > CameraDevice calls CameraWorker::queueRequest, which runs on a\n> > different thread.\n>\n> While the CameraWorker instance inherits from Thread, it isn't bound the\n> its own thread. The CameraWorker instance is an Object (as Thread\n> inherits from Object), and Object instance are bound by default to the\n> thread in which they're created. The can then be moved to another thread\n> by Object::moveToThread(). In this case, the CameraWorker instance is\n> created in the camera service thread (the main thread from the HAL's\n> point of view), and isn't moved to any other thread with moveToThread().\n> It thus stays bound to the camera service thread, not to the Thread\n> instance that it inherits from.\n>\n> > CameraWorker::queueRequest\n> >\n> > void CameraWorker::queueRequest(CaptureRequest *request)\n>\n> In any case, functions that are called directly still run in the thread\n> of the caller, in all cases. Only functions invoked through\n> Object::invokeMethod() or through signals with a queued connection type\n> will run in the thread to which the object is bound. This function is\n> called directly by the CameraDevice, so it runs in the caller's thread.\n>\n> > {\n> > /* Async process the request on the worker which runs its own thread. */\n> > worker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued,\n> > request);\n>\n> This invokes the Worker::processRequest() function with a queued\n> connection type. As the Worker instance has been moved to the\n> CameraWorker Thread, the call will be asynchronous and cross-threads.\n>\n> > }\n> >\n> > 1) How does direct method calls on CameraWorker work now that it's a\n> > Thread\n> > 2) If 1) is ok, then queueRequest should not need to call invokeMethod\n> > on worker_ as it runs in the same thread CameraWorker itself.\n> >\n> > Would it be possible to keep CameraWorker a simple class and make\n> > Worker a thread ?\n>\n> I think we could do that, and move Worker to itself with\n> moveToThread(this). The Worker would then need to reimplement run() as\n> done above. Moving an object to \"itself's thread\" hasn't been well\n> tested though. Additionally, I think it's better conceptually speaking\n> to have CameraWorker inheriting from Thread. You can imagine it as\n> CameraWorker being a Thread, exposing an API to the CameraDevice to\n> control it, with the Worker running inside that thread.\n>\n> > > {\n> > > public:\n> > > \tCameraWorker();\n> > > @@ -52,6 +52,9 @@ public:\n> > >\n> > > \tvoid queueRequest(CaptureRequest *request);\n> > >\n> > > +protected:\n> > > +\tvoid run() override;\n> > > +\n> > > private:\n> > > \tclass Worker : public libcamera::Object\n> > > \t{\n> > > @@ -63,7 +66,6 @@ private:\n> > > \t};\n> > >\n> > > \tWorker worker_;\n> > > -\tlibcamera::Thread thread_;\n> > > };\n> > >\n> > > #endif /* __ANDROID_CAMERA_WORKER_H__ */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 67A8DC3201\n\tfor ;\n\tMon, 24 May 2021 12:04:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D781068919;\n\tMon, 24 May 2021 14:04:45 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 364D7601AA\n\tfor ;\n\tMon, 24 May 2021 14:04:44 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 9A0A240004;\n\tMon, 24 May 2021 12:04:43 +0000 (UTC)"],"Date":"Mon, 24 May 2021 14:05:29 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210524120529.ja7jfphsqgkvwa3z@uno.localdomain>","References":"<20210523023342.13141-1-laurent.pinchart@ideasonboard.com>\n\t<20210524093310.x5g4zyvozifcu6fu@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] android: camera_worker: Process all\n\tqueued requests when stopping","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17220,"web_url":"https://patchwork.libcamera.org/comment/17220/","msgid":"","date":"2021-05-25T02:14:14","subject":"Re: [libcamera-devel] [PATCH] android: camera_worker: Process all\n\tqueued requests when stopping","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Mon, May 24, 2021 at 9:04 PM Jacopo Mondi wrote:\n\n> Hi Laurent,\n>\n> On Mon, May 24, 2021 at 02:50:21PM +0300, Laurent Pinchart wrote:\n> > Hi Jacopo,\n> >\n> > On Mon, May 24, 2021 at 11:33:10AM +0200, Jacopo Mondi wrote:\n> > > On Sun, May 23, 2021 at 05:33:42AM +0300, Laurent Pinchart wrote:\n> > > > When stopping the camera worker, queuedRequest() calls may have\n> queued\n> > > > asynchronous function invocation messages to the worker thread, and\n> some\n> > > > of those messages may not have been processed yet. The messages will\n> > > > stay in the thread's queue until the camera worker is restarted (when\n> > > > the camera service will start a new capture session). At that point,\n> > > > they will be dispatched, which will cause a crash due to the\n> > > > CaptureRequest passed to processRequest() having been deleted by\n> > > > CameraDevice::stop() calling descriptors_.clear().\n> > > >\n> > > > Fix this by forcing dispatching of all function invocation messages\n> when\n> > > > stopping the camera worker thread. Note that this is inherently\n> racy, as\n> > > > more queueRequest() calls may arrive from the camera service while\n> we're\n> > > > stopping. This race condition will be addressed by a subsequent patch\n> > > > series.\n> > > >\n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > I haven't tested this patch, as I'm currently rebuilding a new Soraka\n> > > > image. Hiro, could you give it a try to see if it fixes the problem\n> > > > you've reported ?\n> > > > ---\n> > > > src/android/camera_worker.cpp | 14 ++++++++++----\n> > > > src/android/camera_worker.h | 6 ++++--\n> > > > 2 files changed, 14 insertions(+), 6 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_worker.cpp\n> b/src/android/camera_worker.cpp\n> > > > index 300ddde03645..9f727826e23f 100644\n> > > > --- a/src/android/camera_worker.cpp\n> > > > +++ b/src/android/camera_worker.cpp\n> > > > @@ -52,18 +52,24 @@ void CaptureRequest::queue()\n> > > > */\n> > > > CameraWorker::CameraWorker()\n> > > > {\n> > > > - worker_.moveToThread(&thread_);\n> > > > + worker_.moveToThread(this);\n>\n> Thanks for the explanation. This\n> moveToThread(this);\n>\n> mislead me, but with the below explanation is all good now\n>\n> Reviewed-by: Jacopo Mondi \n>\n> Thanks\n> j\n>\n> > > > }\n> > > >\n> > > > void CameraWorker::start()\n> > > > {\n> > > > - thread_.start();\n> > > > + Thread::start();\n> > > > }\n> > > >\n> > > > void CameraWorker::stop()\n> > > > {\n> > > > - thread_.exit();\n> > > > - thread_.wait();\n> > > > + exit();\n> > > > + wait();\n> > > > +}\n> > > > +\n> > > > +void CameraWorker::run()\n> > > > +{\n> > > > + exec();\n> > > > + dispatchMessages(Message::Type::InvokeMessage);\n> > > > }\n> > > >\n> > > > void CameraWorker::queueRequest(CaptureRequest *request)\n> > > > diff --git a/src/android/camera_worker.h\n> b/src/android/camera_worker.h\n> > > > index 64b1658b61b4..e289ef9b8655 100644\n> > > > --- a/src/android/camera_worker.h\n> > > > +++ b/src/android/camera_worker.h\n> > > > @@ -42,7 +42,7 @@ private:\n> > > > std::unique_ptr request_;\n> > > > };\n> > > >\n> > > > -class CameraWorker\n> > > > +class CameraWorker : private libcamera::Thread\n> > >\n> > > There is one thing I don't get here\n> > >\n> > > Now the whole CameraWorker runs in it own thread.\n> > >\n> > > Before this change CameraWorker had a Worker that was run a separate\n> > > thread.\n> > >\n> > > CameraDevice calls CameraWorker::queueRequest, which runs on a\n> > > different thread.\n> >\n> > While the CameraWorker instance inherits from Thread, it isn't bound the\n> > its own thread. The CameraWorker instance is an Object (as Thread\n> > inherits from Object), and Object instance are bound by default to the\n> > thread in which they're created. The can then be moved to another thread\n> > by Object::moveToThread(). In this case, the CameraWorker instance is\n> > created in the camera service thread (the main thread from the HAL's\n> > point of view), and isn't moved to any other thread with moveToThread().\n> > It thus stays bound to the camera service thread, not to the Thread\n> > instance that it inherits from.\n> >\n>\n\n\nThis explanation should be correct.\nBut\n> as Thread inherits from Object.\nlooking Thread class, it doesn't inherit from Object. What am I missing?\n\nThe code change looks correct to me.\n\nReviewed-by: Hirokazu Honda \n\n\n> > > CameraWorker::queueRequest\n> > >\n> > > void CameraWorker::queueRequest(CaptureRequest *request)\n> >\n> > In any case, functions that are called directly still run in the thread\n> > of the caller, in all cases. Only functions invoked through\n> > Object::invokeMethod() or through signals with a queued connection type\n> > will run in the thread to which the object is bound. This function is\n> > called directly by the CameraDevice, so it runs in the caller's thread.\n> >\n> > > {\n> > > /* Async process the request on the worker which runs\n> its own thread. */\n> > > worker_.invokeMethod(&Worker::processRequest,\n> ConnectionTypeQueued,\n> > > request);\n> >\n> > This invokes the Worker::processRequest() function with a queued\n> > connection type. As the Worker instance has been moved to the\n> > CameraWorker Thread, the call will be asynchronous and cross-threads.\n> >\n> > > }\n> > >\n> > > 1) How does direct method calls on CameraWorker work now that it's a\n> > > Thread\n> > > 2) If 1) is ok, then queueRequest should not need to call invokeMethod\n> > > on worker_ as it runs in the same thread CameraWorker itself.\n> > >\n> > > Would it be possible to keep CameraWorker a simple class and make\n> > > Worker a thread ?\n> >\n> > I think we could do that, and move Worker to itself with\n> > moveToThread(this). The Worker would then need to reimplement run() as\n> > done above. Moving an object to \"itself's thread\" hasn't been well\n> > tested though. Additionally, I think it's better conceptually speaking\n> > to have CameraWorker inheriting from Thread. You can imagine it as\n> > CameraWorker being a Thread, exposing an API to the CameraDevice to\n> > control it, with the Worker running inside that thread.\n> >\n> > > > {\n> > > > public:\n> > > > CameraWorker();\n> > > > @@ -52,6 +52,9 @@ public:\n> > > >\n> > > > void queueRequest(CaptureRequest *request);\n> > > >\n> > > > +protected:\n> > > > + void run() override;\n> > > > +\n> > > > private:\n> > > > class Worker : public libcamera::Object\n> > > > {\n> > > > @@ -63,7 +66,6 @@ private:\n> > > > };\n> > > >\n> > > > Worker worker_;\n> > > > - libcamera::Thread thread_;\n> > > > };\n> > > >\n> > > > #endif /* __ANDROID_CAMERA_WORKER_H__ */\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 43405C3200\n\tfor ;\n\tTue, 25 May 2021 02:14:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8FF926891C;\n\tTue, 25 May 2021 04:14:26 +0200 (CEST)","from mail-ej1-x636.google.com (mail-ej1-x636.google.com\n\t[IPv6:2a00:1450:4864:20::636])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 54D3D601A9\n\tfor ;\n\tTue, 25 May 2021 04:14:25 +0200 (CEST)","by mail-ej1-x636.google.com with SMTP id c20so44819815ejm.3\n\tfor ;\n\tMon, 24 May 2021 19:14:25 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"nLKRxQmP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=kmku2bqE7Kah4JhBWlpkvBmCOLyuHhd+L3fBaxACYCw=;\n\tb=nLKRxQmPIZfqurdjBFyP2vdLIjTPx/hlwD2zfkQ6eMaY3mPeu4KkbitFUmxH1nXo/y\n\tnOR2ic65AjV9SyqOYjMiJfztXH8RxU3eKqqhn79jQBCVecvk8YTCVZq3ERFZlJZp51nQ\n\tFcpJl/opkUt2EjN0DQjr/pbRJ5HF1RmgGUl70=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=kmku2bqE7Kah4JhBWlpkvBmCOLyuHhd+L3fBaxACYCw=;\n\tb=eZSXHlGj8myg0igtyNeGLEcWMsCQoGDZCwVwDgjQsRYlv1K5NW3tDjgoo3A51eAQbT\n\tmD9r2YdOrZLRi5qRAC7+mQp5PgaxOu/+UdWDzl6oaKOOhfdQmSWsZQaxBSr2cmCoWy1I\n\tjUSx6/+6GwEJsaPx9sPEMoKXYsj7PQH3oEGX9YzkV9J1gsjIbDUhGcaWV6njztg+/eIZ\n\tltV7TQtz0532vqcy+BekhrhBpmmN9UN+dWqk+HW2lJHekm8ATBDaeGbWCubaNb304Mmu\n\teUSAYqkoNW04Ql606AGTLe3q/pPdtX5u9Tgd2LKbwAmNHB243VDTzvy+fJWwJgYRHn3n\n\t/3EQ==","X-Gm-Message-State":"AOAM532UXIGwS8whoXzRHUou0fggjEJJT7IulLLooFMccnPwODHsONlU\n\t5M3MlIUrZ1bQpBFdR91iY5TnoR/G4eFbndWLKGt5NA==","X-Google-Smtp-Source":"ABdhPJxGtWQlg+HE+CUuC7/JUtyuvI9pQft6gjeIuli+Xt/LoW7rFSIjJS0mwoeOSrS1nZ6M2Vmd7/9i7SFC8JOt5TE=","X-Received":"by 2002:a17:906:b84b:: with SMTP id\n\tga11mr13868280ejb.243.1621908864830; \n\tMon, 24 May 2021 19:14:24 -0700 (PDT)","MIME-Version":"1.0","References":"<20210523023342.13141-1-laurent.pinchart@ideasonboard.com>\n\t<20210524093310.x5g4zyvozifcu6fu@uno.localdomain>\n\t\n\t<20210524120529.ja7jfphsqgkvwa3z@uno.localdomain>","In-Reply-To":"<20210524120529.ja7jfphsqgkvwa3z@uno.localdomain>","From":"Hirokazu Honda ","Date":"Tue, 25 May 2021 11:14:14 +0900","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"multipart/alternative; boundary=\"000000000000bf1df905c31e15ba\"","Subject":"Re: [libcamera-devel] [PATCH] android: camera_worker: Process all\n\tqueued requests when stopping","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]