[{"id":16851,"web_url":"https://patchwork.libcamera.org/comment/16851/","msgid":"","date":"2021-05-10T06:03:05","subject":"Re: [libcamera-devel] [RFC PATCH] android: CameraWorker: Cancel\n\tpending messages in queue in stop()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Gentle ping for review.\n\nOn Fri, Apr 23, 2021 at 1:04 PM Hirokazu Honda wrote:\n\n> CameraWorker::stop() executes Thread::exit() and Thread::wait().\n> However, intuitively, the two functions doesn't clean up pending\n> messages in the message queue of the thread. They are rather\n> cleaned in the destruction of CameraWorker, but CameraWorker's\n> life time is the same as CameraDevice and thus CameraManager,\n> which is never destroyed. On the other hand,\n> Camera3RequestDescriptors are destroyed in CameraDevice::stop().\n>\n> This causes CameraWorker::Worker::queueRequest() starts to be\n> executed after CameraWorker::start(), which is after\n> CameraDevice::stop(). This obviously causes a segmentation fault,\n> because CameraWorker::Worker::queueRequest() touches\n> CaptureRequest which is owned by Camera3RequestDescriptor.\n>\n> This fixes the issue by cancel pending message in thread queue in\n> CameraWorker::stop() with Thread::dispatchMessages().\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/camera_worker.cpp | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp\n> index 300ddde0..b68ce9d5 100644\n> --- a/src/android/camera_worker.cpp\n> +++ b/src/android/camera_worker.cpp\n> @@ -62,6 +62,7 @@ void CameraWorker::start()\n>\n> void CameraWorker::stop()\n> {\n> + thread_.dispatchMessages(Message::Type::InvokeMessage);\n> thread_.exit();\n> thread_.wait();\n> }\n> --\n> 2.31.1.498.g6c1eba8ee3d-goog\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 8A908BF831\n\tfor ;\n\tMon, 10 May 2021 06:03:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4CAB168918;\n\tMon, 10 May 2021 08:03:18 +0200 (CEST)","from mail-ed1-x52f.google.com (mail-ed1-x52f.google.com\n\t[IPv6:2a00:1450:4864:20::52f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ECBE16153C\n\tfor ;\n\tMon, 10 May 2021 08:03:16 +0200 (CEST)","by mail-ed1-x52f.google.com with SMTP id b17so17320195ede.0\n\tfor ;\n\tSun, 09 May 2021 23:03:16 -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=\"MTDQPZMT\"; 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=AD5XY6oWGTLksvk+C4cZ0Uk/gXTdjs0LUen2qrzCKCw=;\n\tb=MTDQPZMTKtfJw6NRZ3b9TlQqOLNM2pfnVhl8U2yA/TPDqTIZfA6X1MuIX5XAVX8HMF\n\tRyh6tbdWITiCddBZmdb5TQefujofHdxtwdRqQ9D5IqUPnjK0mmbRrXFRclW2vCEToUA/\n\t3hvl9Id3h1JLdeZ38gVxng3JrqGBt4XlB++1Y=","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=AD5XY6oWGTLksvk+C4cZ0Uk/gXTdjs0LUen2qrzCKCw=;\n\tb=Vr5fE13wxy2+e2hi9xet21T1Ev3TkZ/RFN1oUt9EWl1GKWdOvKPuCNiZJ9xi2G2N/z\n\tuH1ddBB2G0pGcj45wtZEw8Y/6leVhNxgDi1AwEodE20MyAyVP0Kzs3DFkvLfu4pbdSgk\n\tGpEugmRoDWf6563Ml8PvD+sQm+LVTZZLTUgerSxJEV9im0+yGs7EREhKNAoGTLzCGCHv\n\tt4gWZVK/soY1N7veXJhyH39xV7Ez8IpNwlzXb+R8+OHcQTPMWwtH+wjMcLeqQm4TMylo\n\tVSFnpWbxO51RlmDg/vtpLZ+em/ou3iODRiDr4m/jLhKtXg+32rZ2uhpwInRnyWJ6EeOj\n\t97nA==","X-Gm-Message-State":"AOAM5317f5SHQLYhFoUmB6Gp2SfVGS9C8bfi4+8JuCoArIHbOilhho5q\n\tJcXD39RP+mxtDqi3wJZ2ZEhAZg8HnrVrVP7iMNN0Vd0m7u4=","X-Google-Smtp-Source":"ABdhPJy3Z26ydLJaS864K4/c1xkPvLgR6V6wwbHnxyObTIX6aCopiW0kLKwdECRGI3nMnKBz2znh0i5/zqgZHWsoM7k=","X-Received":"by 2002:a05:6402:3585:: with SMTP id\n\ty5mr27364418edc.233.1620626596589; \n\tSun, 09 May 2021 23:03:16 -0700 (PDT)","MIME-Version":"1.0","References":"<20210423040427.1226704-1-hiroh@chromium.org>","In-Reply-To":"<20210423040427.1226704-1-hiroh@chromium.org>","From":"Hirokazu Honda ","Date":"Mon, 10 May 2021 15:03:05 +0900","Message-ID":"","To":"libcamera devel ","Subject":"Re: [libcamera-devel] [RFC PATCH] android: CameraWorker: Cancel\n\tpending messages in queue in stop()","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","Content-Type":"multipart/mixed;\n\tboundary=\"===============5555030678003639303==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17044,"web_url":"https://patchwork.libcamera.org/comment/17044/","msgid":"","date":"2021-05-20T08:59:14","subject":"Re: [libcamera-devel] [RFC PATCH] android: CameraWorker: Cancel\n\tpending messages in queue in stop()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Gentle ping for review.\n\nOn Mon, May 10, 2021 at 3:03 PM Hirokazu Honda wrote:\n\n> Gentle ping for review.\n>\n> On Fri, Apr 23, 2021 at 1:04 PM Hirokazu Honda wrote:\n>\n>> CameraWorker::stop() executes Thread::exit() and Thread::wait().\n>> However, intuitively, the two functions doesn't clean up pending\n>> messages in the message queue of the thread. They are rather\n>> cleaned in the destruction of CameraWorker, but CameraWorker's\n>> life time is the same as CameraDevice and thus CameraManager,\n>> which is never destroyed. On the other hand,\n>> Camera3RequestDescriptors are destroyed in CameraDevice::stop().\n>>\n>> This causes CameraWorker::Worker::queueRequest() starts to be\n>> executed after CameraWorker::start(), which is after\n>> CameraDevice::stop(). This obviously causes a segmentation fault,\n>> because CameraWorker::Worker::queueRequest() touches\n>> CaptureRequest which is owned by Camera3RequestDescriptor.\n>>\n>> This fixes the issue by cancel pending message in thread queue in\n>> CameraWorker::stop() with Thread::dispatchMessages().\n>>\n>> Signed-off-by: Hirokazu Honda \n>> ---\n>> src/android/camera_worker.cpp | 1 +\n>> 1 file changed, 1 insertion(+)\n>>\n>> diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp\n>> index 300ddde0..b68ce9d5 100644\n>> --- a/src/android/camera_worker.cpp\n>> +++ b/src/android/camera_worker.cpp\n>> @@ -62,6 +62,7 @@ void CameraWorker::start()\n>>\n>> void CameraWorker::stop()\n>> {\n>> + thread_.dispatchMessages(Message::Type::InvokeMessage);\n>> thread_.exit();\n>> thread_.wait();\n>> }\n>> --\n>> 2.31.1.498.g6c1eba8ee3d-goog\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 AEFC6C31FB\n\tfor ;\n\tThu, 20 May 2021 08:59:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 30EDB68920;\n\tThu, 20 May 2021 10:59:28 +0200 (CEST)","from mail-ej1-x62f.google.com (mail-ej1-x62f.google.com\n\t[IPv6:2a00:1450:4864:20::62f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2DEE768919\n\tfor ;\n\tThu, 20 May 2021 10:59:27 +0200 (CEST)","by mail-ej1-x62f.google.com with SMTP id s22so23848818ejv.12\n\tfor ;\n\tThu, 20 May 2021 01:59:27 -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=\"GcQ4KS65\"; 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=OH0itRoCPnwVt1i5e+dBZUvIS2fhcvf0Gs5Uvya2tQ8=;\n\tb=GcQ4KS65gOsfVpMjgYoVAKLQQ9X6b9KXsBDuOlbWVRHK24eoRAlEUN4lIrz7fSzRpp\n\twaqWD/J+MSRZyRXJwNC0fvnZ7/Wnh14AAEhxl0hwdCVvbKwDMFQOC/m4D9Jx/p+Ymin/\n\tYDD3qqJMfNjapLPda10yHniWiBCw5sFuhCKRE=","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=OH0itRoCPnwVt1i5e+dBZUvIS2fhcvf0Gs5Uvya2tQ8=;\n\tb=PpmWKmE/gBQUMb6Hjm0rdTouLUcpdgZ+f/bQBM2lG5BYBCvCOi6jlfaqZyTcAs7GHv\n\tUBB7s5EvxotmdeewNe1C5qPPetG4Cu6ABMePLTThuPKCqh/Ub7fJGdTTJ+AChzJSnf1t\n\tZjafc0fUjraukCwyw21O57mXf879JRJJHMd5PiWhuIi1Kv+crjptt6jML//lGem1OdbX\n\tvLiCX4xmhu0c19xtRme+vhdZsPIOZptoCBeIQwoY4t8zAaoeewnwHIVpbJmgSYjcw3FR\n\tPoNC0VTvL6LKs057FA7x2rZNWMDfYxdw8nh6oL8DsUzgWHSw+FIMuOcMbQkjxZ/PbYNv\n\tW7mA==","X-Gm-Message-State":"AOAM531N7+W39p2gsgPGFZSyHDyew6dEb+RVzxL7RNBgOmHb4zenxCWM\n\tUIg4zqVGbYEbc8f396vPEI0APm6CwT3tt34Lal2S1/lNja8=","X-Google-Smtp-Source":"ABdhPJzMFFcYJyLSpA2oPeyWkt29loVonHx13/MtZdG4W2DIa7ILHMlpBp6saXWevA1EvKD4+di5AMMDPavVZ3kDF5Y=","X-Received":"by 2002:a17:907:1b06:: with SMTP id\n\tmp6mr3712506ejc.292.1621501166422; \n\tThu, 20 May 2021 01:59:26 -0700 (PDT)","MIME-Version":"1.0","References":"<20210423040427.1226704-1-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Thu, 20 May 2021 17:59:14 +0900","Message-ID":"","To":"libcamera devel ","Content-Type":"multipart/alternative; boundary=\"00000000000007335305c2bf290c\"","Subject":"Re: [libcamera-devel] [RFC PATCH] android: CameraWorker: Cancel\n\tpending messages in queue in stop()","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17048,"web_url":"https://patchwork.libcamera.org/comment/17048/","msgid":"<20210520111255.3vmdzu3tw2nbhvx7@uno.localdomain>","date":"2021-05-20T11:12:55","subject":"Re: [libcamera-devel] [RFC PATCH] android: CameraWorker: Cancel\n\tpending messages in queue in stop()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Fri, Apr 23, 2021 at 01:04:27PM +0900, Hirokazu Honda wrote:\n> CameraWorker::stop() executes Thread::exit() and Thread::wait().\n> However, intuitively, the two functions doesn't clean up pending\n> messages in the message queue of the thread. They are rather\n> cleaned in the destruction of CameraWorker, but CameraWorker's\n> life time is the same as CameraDevice and thus CameraManager,\n> which is never destroyed. On the other hand,\n> Camera3RequestDescriptors are destroyed in CameraDevice::stop().\n>\n> This causes CameraWorker::Worker::queueRequest() starts to be\n> executed after CameraWorker::start(), which is after\n\nI'm not sure I'm following right the sequence of Worker::queueRequest(),\nCameraWorker::start() and CameraDevice::stop()\n\nShould this be CameraWorker::stop() ?\n\nDid you ever triggered this failure ?\n\n> CameraDevice::stop(). This obviously causes a segmentation fault,\n> because CameraWorker::Worker::queueRequest() touches\n> CaptureRequest which is owned by Camera3RequestDescriptor.\n>\n> This fixes the issue by cancel pending message in thread queue in\n> CameraWorker::stop() with Thread::dispatchMessages().\n\nFor my limited understanding of the Object and Thread classes, I think\nit's ok to dispatch all pending method calls.\n\nHowever what concerns me is also Request that are pending in\nvoid CameraWorker::Worker::processRequest() when the Worker gets\nstopped. Should we wait for the queue of pending requests to have\nfinised waiting their fences (or instrument an interruption mechanism)\nbefore returning from stop() ?\n\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/camera_worker.cpp | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp\n> index 300ddde0..b68ce9d5 100644\n> --- a/src/android/camera_worker.cpp\n> +++ b/src/android/camera_worker.cpp\n> @@ -62,6 +62,7 @@ void CameraWorker::start()\n>\n> void CameraWorker::stop()\n> {\n> +\tthread_.dispatchMessages(Message::Type::InvokeMessage);\n> \tthread_.exit();\n> \tthread_.wait();\n> }\n> --\n> 2.31.1.498.g6c1eba8ee3d-goog\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 F00A3C31FB\n\tfor ;\n\tThu, 20 May 2021 11:12:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 141C36891C;\n\tThu, 20 May 2021 13:12:11 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A9E28602B2\n\tfor ;\n\tThu, 20 May 2021 13:12:09 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id DD0881BF205;\n\tThu, 20 May 2021 11:12:08 +0000 (UTC)"],"Date":"Thu, 20 May 2021 13:12:55 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210520111255.3vmdzu3tw2nbhvx7@uno.localdomain>","References":"<20210423040427.1226704-1-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210423040427.1226704-1-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH] android: CameraWorker: Cancel\n\tpending messages in queue in stop()","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":17052,"web_url":"https://patchwork.libcamera.org/comment/17052/","msgid":"","date":"2021-05-21T04:44:44","subject":"Re: [libcamera-devel] [RFC PATCH] android: CameraWorker: Cancel\n\tpending messages in queue in stop()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, thank you for reviewing.\n\nOn Thu, May 20, 2021 at 8:12 PM Jacopo Mondi wrote:\n\n> On Fri, Apr 23, 2021 at 01:04:27PM +0900, Hirokazu Honda wrote:\n> > CameraWorker::stop() executes Thread::exit() and Thread::wait().\n> > However, intuitively, the two functions doesn't clean up pending\n>\n\ns/intuitively/surprisingly/\n\n> messages in the message queue of the thread. They are rather\n> > cleaned in the destruction of CameraWorker, but CameraWorker's\n> > life time is the same as CameraDevice and thus CameraManager,\n> > which is never destroyed. On the other hand,\n> > Camera3RequestDescriptors are destroyed in CameraDevice::stop().\n> >\n> > This causes CameraWorker::Worker::queueRequest() starts to be\n> > executed after CameraWorker::start(), which is after\n>\n> I'm not sure I'm following right the sequence of Worker::queueRequest(),\n> CameraWorker::start() and CameraDevice::stop()\n>\n> Should this be CameraWorker::stop() ?\n>\n> Did you ever triggered this failure ?\n>\n>\nI have encountered this issue when I tested my flush().\nThe HAL API flow is CameraDevice::close() ->\n(CameraDevice::configureStreams() ->) CameraDevice::processCaptureRequest().\nThe internal flow is\n* in CameraDevice::close()\n - CameraWorker::stop()\n - Thread::exit()\n - Thread::wait()\n - Camera::stop()\n* in CameraDevice::processCaptureResult()\n - CameraWorker::start()\n\nAs I said, after CameraWorker::stop(), pending requests remain in the queue.\nCameraWorker::start() starts with the remained requests and because the\nobject of the requests are destroyed by clearing descriptors_, the SIGSEGV\nhappens.\n\n\n> > CameraDevice::stop(). This obviously causes a segmentation fault,\n> > because CameraWorker::Worker::queueRequest() touches\n> > CaptureRequest which is owned by Camera3RequestDescriptor.\n> >\n> > This fixes the issue by cancel pending message in thread queue in\n> > CameraWorker::stop() with Thread::dispatchMessages().\n>\n> For my limited understanding of the Object and Thread classes, I think\n> it's ok to dispatch all pending method calls.\n>\n> However what concerns me is also Request that are pending in\n> void CameraWorker::Worker::processRequest() when the Worker gets\n> stopped. Should we wait for the queue of pending requests to have\n> finised waiting their fences (or instrument an interruption mechanism)\n> before returning from stop() ?\n>\n>\nIf I understand correctly, Thread::dispatchMessages() executes\nCameraWorker::Worker::processRequest().\nThread::dispatchMessages() calls Object::message(), and it executes\nInvokeMessage::invoke().\nI am honestly unsure.\nLaurent should know as Laurent is the original author\nof Thread::dispatchMessages().\n\nThanks,\n-Hiro\n\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/android/camera_worker.cpp | 1 +\n> > 1 file changed, 1 insertion(+)\n> >\n> > diff --git a/src/android/camera_worker.cpp\n> b/src/android/camera_worker.cpp\n> > index 300ddde0..b68ce9d5 100644\n> > --- a/src/android/camera_worker.cpp\n> > +++ b/src/android/camera_worker.cpp\n> > @@ -62,6 +62,7 @@ void CameraWorker::start()\n> >\n> > void CameraWorker::stop()\n> > {\n> > + thread_.dispatchMessages(Message::Type::InvokeMessage);\n> > thread_.exit();\n> > thread_.wait();\n> > }\n> > --\n> > 2.31.1.498.g6c1eba8ee3d-goog\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 E18BBC31FB\n\tfor ;\n\tFri, 21 May 2021 04:44:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 43DAF6891C;\n\tFri, 21 May 2021 06:44:57 +0200 (CEST)","from mail-ej1-x62e.google.com (mail-ej1-x62e.google.com\n\t[IPv6:2a00:1450:4864:20::62e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D99C66050E\n\tfor ;\n\tFri, 21 May 2021 06:44:55 +0200 (CEST)","by mail-ej1-x62e.google.com with SMTP id l4so28535802ejc.10\n\tfor ;\n\tThu, 20 May 2021 21:44:55 -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=\"Sgb55SS4\"; 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=CqUbNgc3AZ9YSYondOkX+cU7cqy/iZCL8JqIm/ZqVZU=;\n\tb=Sgb55SS4dinTI9HMC+RLE64XdcyBBwewFMRL/9NtfEAEuxRWiIBxRyyqo/DwWb+fYf\n\tW/AGk9S/ck9ZGbhoKsfcrDx9JA8cyu3vtYVPiEzih0zpUjx9cmNWfJptKvlkgas0dNmc\n\t3PpbZyI4k/yvgKYa+l6vfKnZa9qXipxyZ1idY=","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=CqUbNgc3AZ9YSYondOkX+cU7cqy/iZCL8JqIm/ZqVZU=;\n\tb=TtdAj6ITSzc7f1Fo7loSIiva1I1MoIwZsh1cfUYqynEFmMY6RTnQLJ2KZoB2NwfFMz\n\tZA1MIoSpavoJUpxxBLWbNpPh1m5/CwMeXc1RLijrBHQJjxpxvb388rxTJ6OB3XgCIRc3\n\t1nRErrLGxoU8fQeE9TTN4tfuRNo1oNOg9moIYR03ORhZB70WMZIV3JV4cr0ABXbTvsY5\n\tBtwfc88mZz+Mv0emW73XOn1/WvYQRMmwZlbtHlePjdAV5jpEz4/XvVzVdtdqTH7IS6be\n\tA4lTSju4DyCLZBrwVfgWvqYmgMTNfG6RmPpRS34FHc8P0lNf0xRf2wcUPRIjyFvQVwY/\n\tfj3g==","X-Gm-Message-State":"AOAM533hQkvf2tSU6eVsAqxIT76ArENvcwsmSEk2mEr9mJvoCLdk+Ws6\n\tA+gx/o6vS3IoUcIZzjRfd3K3c3fF9Rdnvm/jsyF0tUdCCCS5Dg==","X-Google-Smtp-Source":"ABdhPJyXLzJCo8pcD/fKIeQOTR2LAuw7m6RuwzH7I5EfdboUrgJuer5wdtTvsHKwKugcZNrdYHTAwpDeAkSjK1o/krg=","X-Received":"by 2002:a17:907:1b06:: with SMTP id\n\tmp6mr8449482ejc.292.1621572295514; \n\tThu, 20 May 2021 21:44:55 -0700 (PDT)","MIME-Version":"1.0","References":"<20210423040427.1226704-1-hiroh@chromium.org>\n\t<20210520111255.3vmdzu3tw2nbhvx7@uno.localdomain>","In-Reply-To":"<20210520111255.3vmdzu3tw2nbhvx7@uno.localdomain>","From":"Hirokazu Honda ","Date":"Fri, 21 May 2021 13:44:44 +0900","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"multipart/alternative; boundary=\"000000000000a6ed3a05c2cfb86b\"","Subject":"Re: [libcamera-devel] [RFC PATCH] android: CameraWorker: Cancel\n\tpending messages in queue in stop()","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":17054,"web_url":"https://patchwork.libcamera.org/comment/17054/","msgid":"<20210521072541.on6bcxxezbcdj7km@uno.localdomain>","date":"2021-05-21T07:25:41","subject":"Re: [libcamera-devel] [RFC PATCH] android: CameraWorker: Cancel\n\tpending messages in queue in stop()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro\n\nOn Fri, May 21, 2021 at 01:44:44PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo, thank you for reviewing.\n\nCC Laurent for his opinion\n\nThanks\n j\n\n>\n> On Thu, May 20, 2021 at 8:12 PM Jacopo Mondi wrote:\n>\n> > On Fri, Apr 23, 2021 at 01:04:27PM +0900, Hirokazu Honda wrote:\n> > > CameraWorker::stop() executes Thread::exit() and Thread::wait().\n> > > However, intuitively, the two functions doesn't clean up pending\n> >\n>\n> s/intuitively/surprisingly/\n>\n> > messages in the message queue of the thread. They are rather\n> > > cleaned in the destruction of CameraWorker, but CameraWorker's\n> > > life time is the same as CameraDevice and thus CameraManager,\n> > > which is never destroyed. On the other hand,\n> > > Camera3RequestDescriptors are destroyed in CameraDevice::stop().\n> > >\n> > > This causes CameraWorker::Worker::queueRequest() starts to be\n> > > executed after CameraWorker::start(), which is after\n> >\n> > I'm not sure I'm following right the sequence of Worker::queueRequest(),\n> > CameraWorker::start() and CameraDevice::stop()\n> >\n> > Should this be CameraWorker::stop() ?\n> >\n> > Did you ever triggered this failure ?\n> >\n> >\n> I have encountered this issue when I tested my flush().\n> The HAL API flow is CameraDevice::close() ->\n> (CameraDevice::configureStreams() ->) CameraDevice::processCaptureRequest().\n> The internal flow is\n> * in CameraDevice::close()\n> - CameraWorker::stop()\n> - Thread::exit()\n> - Thread::wait()\n> - Camera::stop()\n> * in CameraDevice::processCaptureResult()\n> - CameraWorker::start()\n>\n> As I said, after CameraWorker::stop(), pending requests remain in the queue.\n> CameraWorker::start() starts with the remained requests and because the\n> object of the requests are destroyed by clearing descriptors_, the SIGSEGV\n> happens.\n>\n>\n> > > CameraDevice::stop(). This obviously causes a segmentation fault,\n> > > because CameraWorker::Worker::queueRequest() touches\n> > > CaptureRequest which is owned by Camera3RequestDescriptor.\n> > >\n> > > This fixes the issue by cancel pending message in thread queue in\n> > > CameraWorker::stop() with Thread::dispatchMessages().\n> >\n> > For my limited understanding of the Object and Thread classes, I think\n> > it's ok to dispatch all pending method calls.\n> >\n> > However what concerns me is also Request that are pending in\n> > void CameraWorker::Worker::processRequest() when the Worker gets\n> > stopped. Should we wait for the queue of pending requests to have\n> > finised waiting their fences (or instrument an interruption mechanism)\n> > before returning from stop() ?\n> >\n> >\n> If I understand correctly, Thread::dispatchMessages() executes\n> CameraWorker::Worker::processRequest().\n> Thread::dispatchMessages() calls Object::message(), and it executes\n> InvokeMessage::invoke().\n> I am honestly unsure.\n> Laurent should know as Laurent is the original author\n> of Thread::dispatchMessages().\n>\n> Thanks,\n> -Hiro\n>\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > src/android/camera_worker.cpp | 1 +\n> > > 1 file changed, 1 insertion(+)\n> > >\n> > > diff --git a/src/android/camera_worker.cpp\n> > b/src/android/camera_worker.cpp\n> > > index 300ddde0..b68ce9d5 100644\n> > > --- a/src/android/camera_worker.cpp\n> > > +++ b/src/android/camera_worker.cpp\n> > > @@ -62,6 +62,7 @@ void CameraWorker::start()\n> > >\n> > > void CameraWorker::stop()\n> > > {\n> > > + thread_.dispatchMessages(Message::Type::InvokeMessage);\n> > > thread_.exit();\n> > > thread_.wait();\n> > > }\n> > > --\n> > > 2.31.1.498.g6c1eba8ee3d-goog\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 331FCC31FB\n\tfor ;\n\tFri, 21 May 2021 07:24:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6C0706891D;\n\tFri, 21 May 2021 09:24:57 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7F98468918\n\tfor ;\n\tFri, 21 May 2021 09:24:56 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 8079A200006;\n\tFri, 21 May 2021 07:24:54 +0000 (UTC)"],"Date":"Fri, 21 May 2021 09:25:41 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ,\n\tLaurent Pinchart ","Message-ID":"<20210521072541.on6bcxxezbcdj7km@uno.localdomain>","References":"<20210423040427.1226704-1-hiroh@chromium.org>\n\t<20210520111255.3vmdzu3tw2nbhvx7@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] [RFC PATCH] android: CameraWorker: Cancel\n\tpending messages in queue in stop()","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":17104,"web_url":"https://patchwork.libcamera.org/comment/17104/","msgid":"","date":"2021-05-21T13:30:42","subject":"Re: [libcamera-devel] [RFC PATCH] android: CameraWorker: Cancel\n\tpending messages in queue in stop()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Fri, May 21, 2021 at 01:44:44PM +0900, Hirokazu Honda wrote:\n> On Thu, May 20, 2021 at 8:12 PM Jacopo Mondi wrote:\n> > On Fri, Apr 23, 2021 at 01:04:27PM +0900, Hirokazu Honda wrote:\n> > > CameraWorker::stop() executes Thread::exit() and Thread::wait().\n> > > However, intuitively, the two functions doesn't clean up pending\n> \n> s/intuitively/surprisingly/\n> \n> > messages in the message queue of the thread. They are rather\n> > > cleaned in the destruction of CameraWorker, but CameraWorker's\n> > > life time is the same as CameraDevice and thus CameraManager,\n> > > which is never destroyed. On the other hand,\n> > > Camera3RequestDescriptors are destroyed in CameraDevice::stop().\n> > >\n> > > This causes CameraWorker::Worker::queueRequest() starts to be\n> > > executed after CameraWorker::start(), which is after\n> >\n> > I'm not sure I'm following right the sequence of Worker::queueRequest(),\n> > CameraWorker::start() and CameraDevice::stop()\n> >\n> > Should this be CameraWorker::stop() ?\n> >\n> > Did you ever triggered this failure ?\n>\n> I have encountered this issue when I tested my flush().\n> The HAL API flow is CameraDevice::close() ->\n> (CameraDevice::configureStreams() ->) CameraDevice::processCaptureRequest().\n> The internal flow is\n> * in CameraDevice::close()\n> - CameraWorker::stop()\n> - Thread::exit()\n> - Thread::wait()\n> - Camera::stop()\n> * in CameraDevice::processCaptureResult()\n> - CameraWorker::start()\n> \n> As I said, after CameraWorker::stop(), pending requests remain in the queue.\n> CameraWorker::start() starts with the remained requests and because the\n> object of the requests are destroyed by clearing descriptors_, the SIGSEGV\n> happens.\n\nYou're right. We can have messages in the queue when the thread is\nstopped, and they're not removed. I won't say it's by design :-), but\nthinking about it, forcing automatic dispatching of all messages when\nthe thread is stopped would prevent fast stop of threads, so I think it\nshouldn't be done by default (or at least not without a way to prevent\nit).\n\n> > > CameraDevice::stop(). This obviously causes a segmentation fault,\n> > > because CameraWorker::Worker::queueRequest() touches\n> > > CaptureRequest which is owned by Camera3RequestDescriptor.\n> > >\n> > > This fixes the issue by cancel pending message in thread queue in\n> > > CameraWorker::stop() with Thread::dispatchMessages().\n> >\n> > For my limited understanding of the Object and Thread classes, I think\n> > it's ok to dispatch all pending method calls.\n> >\n> > However what concerns me is also Request that are pending in\n> > void CameraWorker::Worker::processRequest() when the Worker gets\n> > stopped. Should we wait for the queue of pending requests to have\n> > finised waiting their fences (or instrument an interruption mechanism)\n> > before returning from stop() ?\n>\n> If I understand correctly, Thread::dispatchMessages() executes\n> CameraWorker::Worker::processRequest().\n> Thread::dispatchMessages() calls Object::message(), and it executes\n> InvokeMessage::invoke().\n> I am honestly unsure.\n> Laurent should know as Laurent is the original author\n> of Thread::dispatchMessages().\n>\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > src/android/camera_worker.cpp | 1 +\n> > > 1 file changed, 1 insertion(+)\n> > >\n> > > diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp\n> > > index 300ddde0..b68ce9d5 100644\n> > > --- a/src/android/camera_worker.cpp\n> > > +++ b/src/android/camera_worker.cpp\n> > > @@ -62,6 +62,7 @@ void CameraWorker::start()\n> > >\n> > > void CameraWorker::stop()\n> > > {\n> > > + thread_.dispatchMessages(Message::Type::InvokeMessage);\n\nThread::dispatchMessages() is meant to be called from within the thread\nonly. This isn't documented, I'll send a patch for that.\n\nOne option here to call dispatchMessage() from the thread would be the\nfollowing (untested):\n\ndiff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp\nindex 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)\ndiff --git a/src/android/camera_worker.h b/src/android/camera_worker.h\nindex 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 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\nCould you test this to see if it fixes your issue ?\n\nI'm also considering deleting all posted messages when the thread is\nstopped, as I really can't see a use case for keeping messages to be\nprocessed when the thread is restarted. Does anyone have an opinion ?\nI'll likely post a patch as an RFC.\n\n> > > thread_.exit();\n> > > thread_.wait();\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 090E2C31FF\n\tfor ;\n\tFri, 21 May 2021 13:30:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4BC396891D;\n\tFri, 21 May 2021 15:30:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0DFF568911\n\tfor ;\n\tFri, 21 May 2021 15:30:45 +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 6C0DA8D8;\n\tFri, 21 May 2021 15:30:44 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"kCOp26FS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621603844;\n\tbh=ypzPpVv376pFYFV2Rj6r2J76XKuDdh4X4tg3cL4Kkns=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kCOp26FSPdFmeDiB1G/AEwp6oluBXCrkqijn8+tqu/EYAMJh4+Aapi8H8+Rb1TLLQ\n\tZmRtn+STq+D3Z93BEss3r8BPzh3YETI5SS/wW1ZMMd5VWdwDGtgQe7BZR2qMD0lxMp\n\t3v8llZuAbUN91QdlWrav3MLvYPxaWLGymoGNuTOw=","Date":"Fri, 21 May 2021 16:30:42 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210423040427.1226704-1-hiroh@chromium.org>\n\t<20210520111255.3vmdzu3tw2nbhvx7@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] [RFC PATCH] android: CameraWorker: Cancel\n\tpending messages in queue in stop()","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\" "}}]