[{"id":16749,"web_url":"https://patchwork.libcamera.org/comment/16749/","msgid":"<20210504074155.lzrtelhpt7k2lgxr@uno.localdomain>","date":"2021-05-04T07:41:55","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/3] libcamera: camera: Make\n\tstop() idempotent","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Nicolas,\n\nOn Mon, May 03, 2021 at 04:33:05PM -0300, Nícolas F. R. A. Prado wrote:\n> Make Camera::stop() idempotent so that it can be called in any state and\n> consecutive times. When called in any state other than CameraRunning, it\n> is a no-op. This simplifies the cleanup path for applications.\n>\n> Signed-off-by: Nícolas F. R. A. Prado \n> ---\n> Changes in v2:\n> - Suggested by Kieran:\n> - Add new isRunning() function instead of relying on isAccessAllowed()\n> - Suggested by Laurent:\n> - Make stop()'s description clearer\n>\n> src/libcamera/camera.cpp | 20 +++++++++++++++++---\n> 1 file changed, 17 insertions(+), 3 deletions(-)\n>\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 1340c266cc5f..1c6c76c7c01e 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -347,6 +347,7 @@ public:\n> \t\tconst std::set &streams);\n> \t~Private();\n>\n> +\tbool isRunning() const;\n> \tint isAccessAllowed(State state, bool allowDisconnected = false,\n> \t\t\t const char *from = __builtin_FUNCTION()) const;\n> \tint isAccessAllowed(State low, State high,\n> @@ -388,6 +389,15 @@ static const char *const camera_state_names[] = {\n> \t\"Running\",\n> };\n>\n> +bool Camera::Private::isRunning() const\n> +{\n> +\tState currentState = state_.load(std::memory_order_acquire);\n> +\tif (currentState == CameraRunning)\n> +\t\treturn true;\n> +\n> +\treturn false;\n> +}\n> +\n> int Camera::Private::isAccessAllowed(State state, bool allowDisconnected,\n> \t\t\t\t const char *from) const\n> {\n> @@ -1061,9 +1071,10 @@ int Camera::start(const ControlList *controls)\n> * This method stops capturing and processing requests immediately. All pending\n> * requests are cancelled and complete synchronously in an error state.\n> *\n> - * \\context This function may only be called when the camera is in the Running\n> - * state as defined in \\ref camera_operation, and shall be synchronized by the\n> - * caller with other functions that affect the camera state.\n> + * \\context This function may be called in any camera state as defined in \\ref\n> + * camera_operation, and shall be synchronized by the caller with other\n> + * functions that affect the camera state. If called when the camera isn't\n> + * running, it is a no-op.\n> *\n> * \\return 0 on success or a negative error code otherwise\n> * \\retval -ENODEV The camera has been disconnected from the system\n> @@ -1073,6 +1084,9 @@ int Camera::stop()\n> {\n> \tPrivate *const d = LIBCAMERA_D_PTR();\n>\n> +\tif (!d->isRunning())\n> +\t\treturn 0;\n> +\n\nThis kind of defeats the purpose of the isAccessAllowed() here below.\n\nPlease note that isAccessAllowed(Private::CameraRunning) will return\ntrue only if the camera is running, which is the same thing your new\nfunction checks for. Let me guess, you wanted to get rid of the error\nmessage ?\n\nPersonally, I would be in favour of calling stop() only when it's\nappropriate instead of shortcutting the camera state machine. What is\nthe use case that makes it hard to call stop() only when actually\nrequired ?\n\nThanks\n j\n> \tint ret = d->isAccessAllowed(Private::CameraRunning);\n> \tif (ret < 0)\n> \t\treturn ret;\n> --\n> 2.31.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 8E80ABDE78\n\tfor ;\n\tTue, 4 May 2021 07:41:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F2F166890C;\n\tTue, 4 May 2021 09:41:13 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7C5E86890C\n\tfor ;\n\tTue, 4 May 2021 09:41:12 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 8828060011;\n\tTue, 4 May 2021 07:41:11 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 4 May 2021 09:41:55 +0200","From":"Jacopo Mondi ","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","Message-ID":"<20210504074155.lzrtelhpt7k2lgxr@uno.localdomain>","References":"<20210503193307.108607-1-nfraprado@collabora.com>\n\t<20210503193307.108607-2-nfraprado@collabora.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210503193307.108607-2-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/3] libcamera: camera: Make\n\tstop() idempotent","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, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16760,"web_url":"https://patchwork.libcamera.org/comment/16760/","msgid":"","date":"2021-05-04T14:31:03","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/3] libcamera: camera: Make\n\tstop() idempotent","submitter":{"id":84,"url":"https://patchwork.libcamera.org/api/people/84/","name":"Nícolas F. R. A. Prado","email":"nfraprado@collabora.com"},"content":"Hi Jacopo,\n\nEm 2021-05-04 04:41, Jacopo Mondi escreveu:\n\n> Hi Nicolas,\n>\n> On Mon, May 03, 2021 at 04:33:05PM -0300, Nícolas F. R. A. Prado wrote:\n> > Make Camera::stop() idempotent so that it can be called in any state and\n> > consecutive times. When called in any state other than CameraRunning, it\n> > is a no-op. This simplifies the cleanup path for applications.\n> >\n> > Signed-off-by: Nícolas F. R. A. Prado \n> > ---\n> > Changes in v2:\n> > - Suggested by Kieran:\n> > - Add new isRunning() function instead of relying on isAccessAllowed()\n> > - Suggested by Laurent:\n> > - Make stop()'s description clearer\n> >\n> > src/libcamera/camera.cpp | 20 +++++++++++++++++---\n> > 1 file changed, 17 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 1340c266cc5f..1c6c76c7c01e 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -347,6 +347,7 @@ public:\n> > \t\tconst std::set &streams);\n> > \t~Private();\n> >\n> > +\tbool isRunning() const;\n> > \tint isAccessAllowed(State state, bool allowDisconnected = false,\n> > \t\t\t const char *from = __builtin_FUNCTION()) const;\n> > \tint isAccessAllowed(State low, State high,\n> > @@ -388,6 +389,15 @@ static const char *const camera_state_names[] = {\n> > \t\"Running\",\n> > };\n> >\n> > +bool Camera::Private::isRunning() const\n> > +{\n> > +\tState currentState = state_.load(std::memory_order_acquire);\n> > +\tif (currentState == CameraRunning)\n> > +\t\treturn true;\n> > +\n> > +\treturn false;\n> > +}\n> > +\n> > int Camera::Private::isAccessAllowed(State state, bool allowDisconnected,\n> > \t\t\t\t const char *from) const\n> > {\n> > @@ -1061,9 +1071,10 @@ int Camera::start(const ControlList *controls)\n> > * This method stops capturing and processing requests immediately. All pending\n> > * requests are cancelled and complete synchronously in an error state.\n> > *\n> > - * \\context This function may only be called when the camera is in the Running\n> > - * state as defined in \\ref camera_operation, and shall be synchronized by the\n> > - * caller with other functions that affect the camera state.\n> > + * \\context This function may be called in any camera state as defined in \\ref\n> > + * camera_operation, and shall be synchronized by the caller with other\n> > + * functions that affect the camera state. If called when the camera isn't\n> > + * running, it is a no-op.\n> > *\n> > * \\return 0 on success or a negative error code otherwise\n> > * \\retval -ENODEV The camera has been disconnected from the system\n> > @@ -1073,6 +1084,9 @@ int Camera::stop()\n> > {\n> > \tPrivate *const d = LIBCAMERA_D_PTR();\n> >\n> > +\tif (!d->isRunning())\n> > +\t\treturn 0;\n> > +\n>\n> This kind of defeats the purpose of the isAccessAllowed() here below.\n>\n> Please note that isAccessAllowed(Private::CameraRunning) will return\n> true only if the camera is running, which is the same thing your new\n> function checks for. Let me guess, you wanted to get rid of the error\n> message ?\n\nIndeed, I missed that, sorry. In v1 [1] I added other parameters to\nisAccessAllowed() just to be able to exit silently, but Kieran pointed out that\nit would be cleaner with an isRunning() function. So I did that, but forgot to\nremove the isAccessAllowed() below as well.\n\n[1] https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019735.html\n\n>\n> Personally, I would be in favour of calling stop() only when it's\n> appropriate instead of shortcutting the camera state machine. What is\n> the use case that makes it hard to call stop() only when actually\n> required ?\n\nThe use case is always calling stop() in the destructor of the capture in\nlc-compliance, which makes the cleanup path simpler as we can safely fail an\nassert and still have the camera stopped.\n\nThat said, it wouldn't be hard to call stop() only when required. We could track\nwhether the camera was started internally in lc-compliance. But it seemed like\nthis would be a common pattern and handling it by making Camera::stop() itself\nbe idempotent would be beneficial.\n\nI'll wait for others to comment on this as well.\n\nThanks,\nNícolas\n\n>\n> Thanks\n> j\n> > \tint ret = d->isAccessAllowed(Private::CameraRunning);\n> > \tif (ret < 0)\n> > \t\treturn ret;\n> > --\n> > 2.31.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","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 92FABBDE79\n\tfor ;\n\tTue, 4 May 2021 14:33:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E9D6060511;\n\tTue, 4 May 2021 16:33:06 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 85E53602BF\n\tfor ;\n\tTue, 4 May 2021 16:33:05 +0200 (CEST)","from localhost (unknown\n\t[IPv6:2804:14c:1a9:2978:995d:672b:100f:2fd9])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits))\n\t(No client certificate requested) (Authenticated sender: nfraprado)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id 4F6CC1F4269F;\n\tTue, 4 May 2021 15:33:04 +0100 (BST)"],"Mime-Version":"1.0","To":"\"Jacopo Mondi\" ","From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= ","Date":"Tue, 04 May 2021 11:31:03 -0300","Message-Id":"","In-Reply-To":"<20210504074155.lzrtelhpt7k2lgxr@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/3] libcamera: camera: Make\n\tstop() idempotent","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, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?q?ndr=C3=A9_Almeida?= ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16761,"web_url":"https://patchwork.libcamera.org/comment/16761/","msgid":"<0c4d56aa-b34a-a02d-76c1-4c3902aa15ac@ideasonboard.com>","date":"2021-05-04T14:48:16","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/3] libcamera: camera: Make\n\tstop() idempotent","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Nicolas, Jacopo,\n\nOn 04/05/2021 15:31, Nícolas F. R. A. Prado wrote:\n> Hi Jacopo,\n> \n> Em 2021-05-04 04:41, Jacopo Mondi escreveu:\n> \n>> Hi Nicolas,\n>>\n>> On Mon, May 03, 2021 at 04:33:05PM -0300, Nícolas F. R. A. Prado wrote:\n>>> Make Camera::stop() idempotent so that it can be called in any state and\n>>> consecutive times. When called in any state other than CameraRunning, it\n>>> is a no-op. This simplifies the cleanup path for applications.\n>>>\n>>> Signed-off-by: Nícolas F. R. A. Prado \n>>> ---\n>>> Changes in v2:\n>>> - Suggested by Kieran:\n>>> - Add new isRunning() function instead of relying on isAccessAllowed()\n>>> - Suggested by Laurent:\n>>> - Make stop()'s description clearer\n>>>\n>>> src/libcamera/camera.cpp | 20 +++++++++++++++++---\n>>> 1 file changed, 17 insertions(+), 3 deletions(-)\n>>>\n>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n>>> index 1340c266cc5f..1c6c76c7c01e 100644\n>>> --- a/src/libcamera/camera.cpp\n>>> +++ b/src/libcamera/camera.cpp\n>>> @@ -347,6 +347,7 @@ public:\n>>> \t\tconst std::set &streams);\n>>> \t~Private();\n>>>\n>>> +\tbool isRunning() const;\n>>> \tint isAccessAllowed(State state, bool allowDisconnected = false,\n>>> \t\t\t const char *from = __builtin_FUNCTION()) const;\n>>> \tint isAccessAllowed(State low, State high,\n>>> @@ -388,6 +389,15 @@ static const char *const camera_state_names[] = {\n>>> \t\"Running\",\n>>> };\n>>>\n>>> +bool Camera::Private::isRunning() const\n>>> +{\n>>> +\tState currentState = state_.load(std::memory_order_acquire);\n>>> +\tif (currentState == CameraRunning)\n>>> +\t\treturn true;\n>>> +\n>>> +\treturn false;\n>>> +}\n>>> +\n>>> int Camera::Private::isAccessAllowed(State state, bool allowDisconnected,\n>>> \t\t\t\t const char *from) const\n>>> {\n>>> @@ -1061,9 +1071,10 @@ int Camera::start(const ControlList *controls)\n>>> * This method stops capturing and processing requests immediately. All pending\n>>> * requests are cancelled and complete synchronously in an error state.\n>>> *\n>>> - * \\context This function may only be called when the camera is in the Running\n>>> - * state as defined in \\ref camera_operation, and shall be synchronized by the\n>>> - * caller with other functions that affect the camera state.\n>>> + * \\context This function may be called in any camera state as defined in \\ref\n>>> + * camera_operation, and shall be synchronized by the caller with other\n>>> + * functions that affect the camera state. If called when the camera isn't\n>>> + * running, it is a no-op.\n>>> *\n>>> * \\return 0 on success or a negative error code otherwise\n>>> * \\retval -ENODEV The camera has been disconnected from the system\n>>> @@ -1073,6 +1084,9 @@ int Camera::stop()\n>>> {\n>>> \tPrivate *const d = LIBCAMERA_D_PTR();\n>>>\n>>> +\tif (!d->isRunning())\n>>> +\t\treturn 0;\n>>> +\n>>\n>> This kind of defeats the purpose of the isAccessAllowed() here below.\n>>\n>> Please note that isAccessAllowed(Private::CameraRunning) will return\n>> true only if the camera is running, which is the same thing your new\n>> function checks for. Let me guess, you wanted to get rid of the error\n>> message ?\n> \n> Indeed, I missed that, sorry. In v1 [1] I added other parameters to\n> isAccessAllowed() just to be able to exit silently, but Kieran pointed out that\n> it would be cleaner with an isRunning() function. So I did that, but forgot to\n> remove the isAccessAllowed() below as well.\n\nYes, the extra checks should be removed.\n\n\n> \n> [1] https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019735.html\n> \n>>\n>> Personally, I would be in favour of calling stop() only when it's\n>> appropriate instead of shortcutting the camera state machine. What is\n>> the use case that makes it hard to call stop() only when actually\n>> required ?\n> \n> The use case is always calling stop() in the destructor of the capture in\n> lc-compliance, which makes the cleanup path simpler as we can safely fail an\n> assert and still have the camera stopped.\n> \n> That said, it wouldn't be hard to call stop() only when required. We could track\n> whether the camera was started internally in lc-compliance. But it seemed like\n> this would be a common pattern and handling it by making Camera::stop() itself\n> be idempotent would be beneficial.\n> \n> I'll wait for others to comment on this as well.\n\nI think being able to clean up easily on shutdowns is beneficial.\n\nI would consider this use case to be that of being able to call\n\n\tp = NULL;\n\t...\n\tkfree(p);\n\nrather than\n\tif (p)\n\t\tkfree(p);\n\nIt's a convenience feature, so that extra state doesn't have to be\nrepeatedly maintained elsewhere.\n\n--\nKieran\n\n\n> \n> Thanks,\n> Nícolas\n> \n>>\n>> Thanks\n>> j\n>>> \tint ret = d->isAccessAllowed(Private::CameraRunning);\n>>> \tif (ret < 0)\n>>> \t\treturn ret;\n>>> --\n>>> 2.31.1\n>>>\n>>> _______________________________________________\n>>> libcamera-devel mailing list\n>>> libcamera-devel@lists.libcamera.org\n>>> https://lists.libcamera.org/listinfo/libcamera-devel\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\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 D0F03BDE7A\n\tfor ;\n\tTue, 4 May 2021 14:48:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 512F36890E;\n\tTue, 4 May 2021 16:48:20 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 800DE602BF\n\tfor ;\n\tTue, 4 May 2021 16:48:19 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DAD9D56B;\n\tTue, 4 May 2021 16:48:18 +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=\"ORHwMQgu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1620139699;\n\tbh=LvuQjTxa3WdoU8KqmuXm+dtzpRfbTsPVNSrJ4LA+v6Q=;\n\th=Reply-To:To:Cc:References:From:Subject:Date:In-Reply-To:From;\n\tb=ORHwMQgu09UKpex/eui5czvCvXQOdIGSH22a28IRuLhcugY30ibNM8TVk6+8XAR5M\n\tIRXlkW6JWrP2baxjYwEfsrkQ0l1G658GD4wFbOns3WOuMtEP8uIg3ed2XUJCUi9Gnl\n\tyeCB/P0/K5FxBlo0ivL55ob4bmMGe2qbimAqAA8c=","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= ,\n\tJacopo Mondi ","References":"","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"<0c4d56aa-b34a-a02d-76c1-4c3902aa15ac@ideasonboard.com>","Date":"Tue, 4 May 2021 15:48:16 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.7.1","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/3] libcamera: camera: Make\n\tstop() idempotent","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?q?ndr=C3=A9_Almeida?= ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16762,"web_url":"https://patchwork.libcamera.org/comment/16762/","msgid":"<20210504152936.4nkrc3tp42lcprpc@uno.localdomain>","date":"2021-05-04T15:29:36","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/3] libcamera: camera: Make\n\tstop() idempotent","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello,\n\nOn Tue, May 04, 2021 at 03:48:16PM +0100, Kieran Bingham wrote:\n> Hi Nicolas, Jacopo,\n>\n> On 04/05/2021 15:31, Nícolas F. R. A. Prado wrote:\n> > Hi Jacopo,\n> >\n> > Em 2021-05-04 04:41, Jacopo Mondi escreveu:\n> >\n\n[snip]\n\n> >> Personally, I would be in favour of calling stop() only when it's\n> >> appropriate instead of shortcutting the camera state machine. What is\n> >> the use case that makes it hard to call stop() only when actually\n> >> required ?\n> >\n> > The use case is always calling stop() in the destructor of the capture in\n> > lc-compliance, which makes the cleanup path simpler as we can safely fail an\n> > assert and still have the camera stopped.\n> >\n> > That said, it wouldn't be hard to call stop() only when required. We could track\n> > whether the camera was started internally in lc-compliance. But it seemed like\n> > this would be a common pattern and handling it by making Camera::stop() itself\n> > be idempotent would be beneficial.\n> >\n> > I'll wait for others to comment on this as well.\n>\n> I think being able to clean up easily on shutdowns is beneficial.\n>\n> I would consider this use case to be that of being able to call\n>\n> \tp = NULL;\n> \t...\n> \tkfree(p);\n>\n> rather than\n> \tif (p)\n> \t\tkfree(p);\n>\n> It's a convenience feature, so that extra state doesn't have to be\n> repeatedly maintained elsewhere.\n>\n\nMight be a personal opinion, but I would prefer, as a general idea,\na stricter API. I concur there's not much value in the error message\nwhen stop() is called from the wrong state, but sometime it helped me\nfinding a wrong patter when working on the camera HAL. I guess it\nmight be beneficial for other use cases too...\n\nAnyway, I won't push, whatever's best for the majority is fine with\nme.\n\nThanks\n j\n\n> --\n> Kieran\n>\n>\n> >\n> > Thanks,\n> > Nícolas\n> >\n> >>\n> >> Thanks\n> >> j\n> >>> \tint ret = d->isAccessAllowed(Private::CameraRunning);\n> >>> \tif (ret < 0)\n> >>> \t\treturn ret;\n> >>> --\n> >>> 2.31.1\n> >>>\n> >>> _______________________________________________\n> >>> libcamera-devel mailing list\n> >>> libcamera-devel@lists.libcamera.org\n> >>> https://lists.libcamera.org/listinfo/libcamera-devel\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n>\n> --\n> Regards\n> --\n> Kieran","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 1EF87BDE79\n\tfor ;\n\tTue, 4 May 2021 15:28:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9FAFB60511;\n\tTue, 4 May 2021 17:28:55 +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 217F4602BF\n\tfor ;\n\tTue, 4 May 2021 17:28:54 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id DE60A1C0006;\n\tTue, 4 May 2021 15:28:52 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 4 May 2021 17:29:36 +0200","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20210504152936.4nkrc3tp42lcprpc@uno.localdomain>","References":"\n\t<0c4d56aa-b34a-a02d-76c1-4c3902aa15ac@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<0c4d56aa-b34a-a02d-76c1-4c3902aa15ac@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/3] libcamera: camera: Make\n\tstop() idempotent","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, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16763,"web_url":"https://patchwork.libcamera.org/comment/16763/","msgid":"","date":"2021-05-04T19:15:17","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/3] libcamera: camera: Make\n\tstop() idempotent","submitter":{"id":84,"url":"https://patchwork.libcamera.org/api/people/84/","name":"Nícolas F. R. A. Prado","email":"nfraprado@collabora.com"},"content":"Hi,\n\nEm 2021-05-04 12:29, Jacopo Mondi escreveu:\n\n> Hello,\n>\n> On Tue, May 04, 2021 at 03:48:16PM +0100, Kieran Bingham wrote:\n> > Hi Nicolas, Jacopo,\n> >\n> > On 04/05/2021 15:31, Nícolas F. R. A. Prado wrote:\n> > > Hi Jacopo,\n> > >\n> > > Em 2021-05-04 04:41, Jacopo Mondi escreveu:\n> > >\n>\n> [snip]\n>\n> > >> Personally, I would be in favour of calling stop() only when it's\n> > >> appropriate instead of shortcutting the camera state machine. What is\n> > >> the use case that makes it hard to call stop() only when actually\n> > >> required ?\n> > >\n> > > The use case is always calling stop() in the destructor of the capture in\n> > > lc-compliance, which makes the cleanup path simpler as we can safely fail an\n> > > assert and still have the camera stopped.\n> > >\n> > > That said, it wouldn't be hard to call stop() only when required. We could track\n> > > whether the camera was started internally in lc-compliance. But it seemed like\n> > > this would be a common pattern and handling it by making Camera::stop() itself\n> > > be idempotent would be beneficial.\n> > >\n> > > I'll wait for others to comment on this as well.\n> >\n> > I think being able to clean up easily on shutdowns is beneficial.\n> >\n> > I would consider this use case to be that of being able to call\n> >\n> > \tp = NULL;\n> > \t...\n> > \tkfree(p);\n> >\n> > rather than\n> > \tif (p)\n> > \t\tkfree(p);\n> >\n> > It's a convenience feature, so that extra state doesn't have to be\n> > repeatedly maintained elsewhere.\n> >\n>\n> Might be a personal opinion, but I would prefer, as a general idea,\n> a stricter API. I concur there's not much value in the error message\n> when stop() is called from the wrong state, but sometime it helped me\n> finding a wrong patter when working on the camera HAL. I guess it\n> might be beneficial for other use cases too...\n\nI think adding a log at level DEBUG or INFO stating that \"stop() called for\nnon-running camera\" would help debug those cases while still allowing the\nconvenience of calling it in any state.\n\nThanks,\nNícolas\n\n>\n> Anyway, I won't push, whatever's best for the majority is fine with\n> me.\n>\n> Thanks\n> j\n>\n> > --\n> > Kieran\n> >\n> >\n> > >\n> > > Thanks,\n> > > Nícolas\n> > >\n> > >>\n> > >> Thanks\n> > >> j\n> > >>> \tint ret = d->isAccessAllowed(Private::CameraRunning);\n> > >>> \tif (ret < 0)\n> > >>> \t\treturn ret;\n> > >>> --\n> > >>> 2.31.1\n> > >>>\n> > >>> _______________________________________________\n> > >>> libcamera-devel mailing list\n> > >>> libcamera-devel@lists.libcamera.org\n> > >>> https://lists.libcamera.org/listinfo/libcamera-devel\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > >\n> >\n> > --\n> > Regards\n> > --\n> > Kieran\n>\n> --\n> To unsubscribe, send mail to kernel-unsubscribe@lists.collabora.co.uk.","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 946BCBDE7B\n\tfor ;\n\tTue, 4 May 2021 19:15:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CCB0C6891A;\n\tTue, 4 May 2021 21:15:54 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 11CFB60511\n\tfor ;\n\tTue, 4 May 2021 21:15:52 +0200 (CEST)","from localhost (unknown\n\t[IPv6:2804:14c:1a9:2978:995d:672b:100f:2fd9])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits))\n\t(No client certificate requested) (Authenticated sender: nfraprado)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id 6C5881F429CE;\n\tTue, 4 May 2021 20:15:51 +0100 (BST)"],"Mime-Version":"1.0","Date":"Tue, 04 May 2021 16:15:17 -0300","Message-Id":"","From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= ","To":"\"Jacopo Mondi\" , \"Kieran Bingham\"\n\t","References":"\n\t<0c4d56aa-b34a-a02d-76c1-4c3902aa15ac@ideasonboard.com>\n\t<20210504152936.4nkrc3tp42lcprpc@uno.localdomain>","In-Reply-To":"<20210504152936.4nkrc3tp42lcprpc@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/3] libcamera: camera: Make\n\tstop() idempotent","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, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?q?ndr=C3=A9_Almeida?= ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16838,"web_url":"https://patchwork.libcamera.org/comment/16838/","msgid":"","date":"2021-05-08T00:03:54","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/3] libcamera: camera: Make\n\tstop() idempotent","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Tue, May 04, 2021 at 04:15:17PM -0300, Nícolas F. R. A. Prado wrote:\n> Em 2021-05-04 12:29, Jacopo Mondi escreveu:\n> > On Tue, May 04, 2021 at 03:48:16PM +0100, Kieran Bingham wrote:\n> > > On 04/05/2021 15:31, Nícolas F. R. A. Prado wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > Em 2021-05-04 04:41, Jacopo Mondi escreveu:\n> > > >\n> >\n> > [snip]\n> >\n> > > >> Personally, I would be in favour of calling stop() only when it's\n> > > >> appropriate instead of shortcutting the camera state machine. What is\n> > > >> the use case that makes it hard to call stop() only when actually\n> > > >> required ?\n> > > >\n> > > > The use case is always calling stop() in the destructor of the capture in\n> > > > lc-compliance, which makes the cleanup path simpler as we can safely fail an\n> > > > assert and still have the camera stopped.\n> > > >\n> > > > That said, it wouldn't be hard to call stop() only when required. We could track\n> > > > whether the camera was started internally in lc-compliance. But it seemed like\n> > > > this would be a common pattern and handling it by making Camera::stop() itself\n> > > > be idempotent would be beneficial.\n> > > >\n> > > > I'll wait for others to comment on this as well.\n> > >\n> > > I think being able to clean up easily on shutdowns is beneficial.\n> > >\n> > > I would consider this use case to be that of being able to call\n> > >\n> > > \tp = NULL;\n> > > \t...\n> > > \tkfree(p);\n> > >\n> > > rather than\n> > > \tif (p)\n> > > \t\tkfree(p);\n> > >\n> > > It's a convenience feature, so that extra state doesn't have to be\n> > > repeatedly maintained elsewhere.\n> >\n> > Might be a personal opinion, but I would prefer, as a general idea,\n> > a stricter API. I concur there's not much value in the error message\n> > when stop() is called from the wrong state, but sometime it helped me\n> > finding a wrong patter when working on the camera HAL. I guess it\n> > might be beneficial for other use cases too...\n> \n> I think adding a log at level DEBUG or INFO stating that \"stop() called for\n> non-running camera\" would help debug those cases while still allowing the\n> convenience of calling it in any state.\n\nSorry about the conflicting directions :-S\n\nThere are two options. Calling Camera::stop() when stopped is either\nvalid, in which case the function shouldn't log any message with a level\nhigher than debug, or it's invalid, in which case we should log an\nerror. In the latter case, the caller needs to keep track of the camera\nstate, which is a bit of a hassle. Taking Niklas' example, it would be\nsimilar to requiring the caller to have a bool is_p_null variable, as\nthe Camera class doesn't expose its state, so a caller can do\n\n\tif (cam->state() == CameraRunning)\n\t\tcam->stop();\n\nWe could expose the camera state, but that increases the surface of the\npublic API, for little gain I believe.\n\nJacopo, what do you think ?\n\n> > Anyway, I won't push, whatever's best for the majority is fine with\n> > me.\n> >\n> > > >>> \tint ret = d->isAccessAllowed(Private::CameraRunning);\n> > > >>> \tif (ret < 0)\n> > > >>> \t\treturn ret;","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 0EBC7BF831\n\tfor ;\n\tSat, 8 May 2021 00:04:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4238768915;\n\tSat, 8 May 2021 02:04:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 683B368901\n\tfor ;\n\tSat, 8 May 2021 02:04:01 +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 6F26D3D7;\n\tSat, 8 May 2021 02:04:00 +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=\"ITyshmhy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1620432240;\n\tbh=QyV38HKQWpC6K36g/caW2cpeuVDs9WfKTSo5OOnx4uM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ITyshmhyrY5CGNsE1QvoZlBWohTJ28rDWI/FK7GGFaf6ijlGblDZcCrXIRKgb7PL+\n\tTjO++M+I2VGXa+J7hYuuyFYV2ddqaK3bDRZrstNWuDUlLmuG8lcl5rjS+K1AFnBnV4\n\tm1o9kCK4TKOphliyzHI9X4WBngkmJwdaep4wouFY=","Date":"Sat, 8 May 2021 03:03:54 +0300","From":"Laurent Pinchart ","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","Message-ID":"","References":"\n\t<0c4d56aa-b34a-a02d-76c1-4c3902aa15ac@ideasonboard.com>\n\t<20210504152936.4nkrc3tp42lcprpc@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/3] libcamera: camera: Make\n\tstop() idempotent","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, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16847,"web_url":"https://patchwork.libcamera.org/comment/16847/","msgid":"<20210508072402.sylp35irjojm2g5e@uno.localdomain>","date":"2021-05-08T07:24:02","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/3] libcamera: camera: Make\n\tstop() idempotent","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sat, May 08, 2021 at 03:03:54AM +0300, Laurent Pinchart wrote:\n> Hello,\n>\n> On Tue, May 04, 2021 at 04:15:17PM -0300, Nícolas F. R. A. Prado wrote:\n> > Em 2021-05-04 12:29, Jacopo Mondi escreveu:\n> > > On Tue, May 04, 2021 at 03:48:16PM +0100, Kieran Bingham wrote:\n> > > > On 04/05/2021 15:31, Nícolas F. R. A. Prado wrote:\n> > > > > Hi Jacopo,\n> > > > >\n> > > > > Em 2021-05-04 04:41, Jacopo Mondi escreveu:\n> > > > >\n> > >\n> > > [snip]\n> > >\n> > > > >> Personally, I would be in favour of calling stop() only when it's\n> > > > >> appropriate instead of shortcutting the camera state machine. What is\n> > > > >> the use case that makes it hard to call stop() only when actually\n> > > > >> required ?\n> > > > >\n> > > > > The use case is always calling stop() in the destructor of the capture in\n> > > > > lc-compliance, which makes the cleanup path simpler as we can safely fail an\n> > > > > assert and still have the camera stopped.\n> > > > >\n> > > > > That said, it wouldn't be hard to call stop() only when required. We could track\n> > > > > whether the camera was started internally in lc-compliance. But it seemed like\n> > > > > this would be a common pattern and handling it by making Camera::stop() itself\n> > > > > be idempotent would be beneficial.\n> > > > >\n> > > > > I'll wait for others to comment on this as well.\n> > > >\n> > > > I think being able to clean up easily on shutdowns is beneficial.\n> > > >\n> > > > I would consider this use case to be that of being able to call\n> > > >\n> > > > \tp = NULL;\n> > > > \t...\n> > > > \tkfree(p);\n> > > >\n> > > > rather than\n> > > > \tif (p)\n> > > > \t\tkfree(p);\n> > > >\n> > > > It's a convenience feature, so that extra state doesn't have to be\n> > > > repeatedly maintained elsewhere.\n> > >\n> > > Might be a personal opinion, but I would prefer, as a general idea,\n> > > a stricter API. I concur there's not much value in the error message\n> > > when stop() is called from the wrong state, but sometime it helped me\n> > > finding a wrong patter when working on the camera HAL. I guess it\n> > > might be beneficial for other use cases too...\n> >\n> > I think adding a log at level DEBUG or INFO stating that \"stop() called for\n> > non-running camera\" would help debug those cases while still allowing the\n> > convenience of calling it in any state.\n>\n> Sorry about the conflicting directions :-S\n>\n> There are two options. Calling Camera::stop() when stopped is either\n> valid, in which case the function shouldn't log any message with a level\n> higher than debug, or it's invalid, in which case we should log an\n> error. In the latter case, the caller needs to keep track of the camera\n> state, which is a bit of a hassle. Taking Niklas' example, it would be\n> similar to requiring the caller to have a bool is_p_null variable, as\n> the Camera class doesn't expose its state, so a caller can do\n>\n> \tif (cam->state() == CameraRunning)\n> \t\tcam->stop();\n>\n> We could expose the camera state, but that increases the surface of the\n> public API, for little gain I believe.\n>\n> Jacopo, what do you think ?\n>\n\nEither we change the camera state machine and allow Stop->Stop by\nadding a state\n Stopping -> Stopping [label = \"stop()\"];\n\nand change\n isAccessAllowed(Private::CameraStopped,\n Private::CameraRunning);\n\nnot to throw any error\n\nOr we mandate applications to track the state themeselves.\n\nExposing the state has a little gain, as application will\nunconditionally repeat the pattern\n\n \tif (cam->state() == CameraRunning)\n \t\tcam->stop();\n\nhence they could similarly be allowed to just call stop() without\nbothering about the state check.\n\nI would like application to be in charge of tracking the state and\nknow what they're doing precisely, but I'm afraid they will just\ncall stop() and ignore the error, and I'm not 100% sure it is fair to\nmandate the state tracking burden to apps.\n\nSo yes, allowing stop->stop seems fair, but it should be made part of\nthe Camera state machine and not worked around by not checking if the\naccess is actually allowed or not imho.\n\nThanks\n j\n\n> > > Anyway, I won't push, whatever's best for the majority is fine with\n> > > me.\n> > >\n> > > > >>> \tint ret = d->isAccessAllowed(Private::CameraRunning);\n> > > > >>> \tif (ret < 0)\n> > > > >>> \t\treturn ret;\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 A616BBF829\n\tfor ;\n\tSat, 8 May 2021 07:23:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1112F6891C;\n\tSat, 8 May 2021 09:23:22 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 009F0688E5\n\tfor ;\n\tSat, 8 May 2021 09:23:20 +0200 (CEST)","from uno.localdomain (host-79-53-131-195.retail.telecomitalia.it\n\t[79.53.131.195]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id D2A2E60004;\n\tSat, 8 May 2021 07:23:18 +0000 (UTC)"],"X-Originating-IP":"79.53.131.195","Date":"Sat, 8 May 2021 09:24:02 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210508072402.sylp35irjojm2g5e@uno.localdomain>","References":"\n\t<0c4d56aa-b34a-a02d-76c1-4c3902aa15ac@ideasonboard.com>\n\t<20210504152936.4nkrc3tp42lcprpc@uno.localdomain>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/3] libcamera: camera: Make\n\tstop() idempotent","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, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16861,"web_url":"https://patchwork.libcamera.org/comment/16861/","msgid":"<462fb876-749b-b426-6761-19105c702c52@ideasonboard.com>","date":"2021-05-10T14:15:03","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/3] libcamera: camera: Make\n\tstop() idempotent","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 08/05/2021 08:24, Jacopo Mondi wrote:\n> Hi Laurent,\n> \n> On Sat, May 08, 2021 at 03:03:54AM +0300, Laurent Pinchart wrote:\n>> Hello,\n>>\n>> On Tue, May 04, 2021 at 04:15:17PM -0300, Nícolas F. R. A. Prado wrote:\n>>> Em 2021-05-04 12:29, Jacopo Mondi escreveu:\n>>>> On Tue, May 04, 2021 at 03:48:16PM +0100, Kieran Bingham wrote:\n>>>>> On 04/05/2021 15:31, Nícolas F. R. A. Prado wrote:\n>>>>>> Hi Jacopo,\n>>>>>>\n>>>>>> Em 2021-05-04 04:41, Jacopo Mondi escreveu:\n>>>>>>\n>>>>\n>>>> [snip]\n>>>>\n>>>>>>> Personally, I would be in favour of calling stop() only when it's\n>>>>>>> appropriate instead of shortcutting the camera state machine. What is\n>>>>>>> the use case that makes it hard to call stop() only when actually\n>>>>>>> required ?\n>>>>>>\n>>>>>> The use case is always calling stop() in the destructor of the capture in\n>>>>>> lc-compliance, which makes the cleanup path simpler as we can safely fail an\n>>>>>> assert and still have the camera stopped.\n>>>>>>\n>>>>>> That said, it wouldn't be hard to call stop() only when required. We could track\n>>>>>> whether the camera was started internally in lc-compliance. But it seemed like\n>>>>>> this would be a common pattern and handling it by making Camera::stop() itself\n>>>>>> be idempotent would be beneficial.\n>>>>>>\n>>>>>> I'll wait for others to comment on this as well.\n>>>>>\n>>>>> I think being able to clean up easily on shutdowns is beneficial.\n>>>>>\n>>>>> I would consider this use case to be that of being able to call\n>>>>>\n>>>>> \tp = NULL;\n>>>>> \t...\n>>>>> \tkfree(p);\n>>>>>\n>>>>> rather than\n>>>>> \tif (p)\n>>>>> \t\tkfree(p);\n>>>>>\n>>>>> It's a convenience feature, so that extra state doesn't have to be\n>>>>> repeatedly maintained elsewhere.\n>>>>\n>>>> Might be a personal opinion, but I would prefer, as a general idea,\n>>>> a stricter API. I concur there's not much value in the error message\n>>>> when stop() is called from the wrong state, but sometime it helped me\n>>>> finding a wrong patter when working on the camera HAL. I guess it\n>>>> might be beneficial for other use cases too...\n>>>\n>>> I think adding a log at level DEBUG or INFO stating that \"stop() called for\n>>> non-running camera\" would help debug those cases while still allowing the\n>>> convenience of calling it in any state.\n>>\n>> Sorry about the conflicting directions :-S\n>>\n>> There are two options. Calling Camera::stop() when stopped is either\n>> valid, in which case the function shouldn't log any message with a level\n>> higher than debug, or it's invalid, in which case we should log an\n>> error. In the latter case, the caller needs to keep track of the camera\n>> state, which is a bit of a hassle. Taking Niklas' example, it would be\n>> similar to requiring the caller to have a bool is_p_null variable, as\n>> the Camera class doesn't expose its state, so a caller can do\n>>\n>> \tif (cam->state() == CameraRunning)\n>> \t\tcam->stop();\n>>\n>> We could expose the camera state, but that increases the surface of the\n>> public API, for little gain I believe.\n>>\n>> Jacopo, what do you think ?\n>>\n> \n> Either we change the camera state machine and allow Stop->Stop by\n> adding a state\n> Stopping -> Stopping [label = \"stop()\"];\n\nHrm - Are we missing some updates to our state diagram already?\nShouldn't this be 'Stopped'?\n\n(I bet that's my fault)\n\n\n> and change\n> isAccessAllowed(Private::CameraStopped,\n> Private::CameraRunning);\n> \n> not to throw any error\n> \n> Or we mandate applications to track the state themeselves.\n> \n> Exposing the state has a little gain, as application will\n> unconditionally repeat the pattern\n> \n> \tif (cam->state() == CameraRunning)\n> \t\tcam->stop();\n> \n> hence they could similarly be allowed to just call stop() without\n> bothering about the state check.\n> \n> I would like application to be in charge of tracking the state and\n> know what they're doing precisely, but I'm afraid they will just\n> call stop() and ignore the error, and I'm not 100% sure it is fair to\n> mandate the state tracking burden to apps.\n> \n> So yes, allowing stop->stop seems fair, but it should be made part of\n> the Camera state machine and not worked around by not checking if the\n> access is actually allowed or not imho.\n\nI think making it clear in the state machine diagrams is a very good point.\n\nBut doesn't it then mean allowing stop() from any of the other states?\n\nWhat happens if we call stop in {Available, Acquired, Configured} for\nexample.\n\nIn fact, indeed we shouldn't allow calling stop() on a camera which has\nnot been acquired, as that would imply that the caller could stop() a\ncamera which was in a running state, belonging to someone else.\n\nWe may still need to have the instrumentation in the stop function for\nisRunning() so that it doesn't try to stop, when already stopped though,\nbut I agree the isAccessAllowed() could in fact be a bit more restrictive.\n\n\n> \n> Thanks\n> j\n> \n>>>> Anyway, I won't push, whatever's best for the majority is fine with\n>>>> me.\n>>>>\n>>>>>>>> \tint ret = d->isAccessAllowed(Private::CameraRunning);\n>>>>>>>> \tif (ret < 0)\n>>>>>>>> \t\treturn ret;\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 68B4DBF829\n\tfor ;\n\tMon, 10 May 2021 14:15:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A4EC36891A;\n\tMon, 10 May 2021 16:15:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 488AD602BE\n\tfor ;\n\tMon, 10 May 2021 16:15:08 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5520E908;\n\tMon, 10 May 2021 16:15:06 +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=\"ANj0PvMA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1620656107;\n\tbh=SHKkBc/ndoc5FCbFKRZO+0Or36aCYepU/1hjAxXcYs0=;\n\th=Reply-To:To:Cc:References:From:Subject:Date:In-Reply-To:From;\n\tb=ANj0PvMAor/25GlDmC7HcJLb0TAxseNVzWxkyrojgclK4halEcgLBW3kSDljxIbiU\n\t7T3EskjIc3Ry2lRHT373pslDYzWfsZwr4WI3R2ka2yKojCN8/goYIlhZu0SPXW2KLF\n\tK8IyoIliDaIGrk1hnnvmclGdJioUxBx18Qq9feIY=","To":"Jacopo Mondi ,\n\tLaurent Pinchart ","References":"\n\t<0c4d56aa-b34a-a02d-76c1-4c3902aa15ac@ideasonboard.com>\n\t<20210504152936.4nkrc3tp42lcprpc@uno.localdomain>\n\t\n\t\n\t<20210508072402.sylp35irjojm2g5e@uno.localdomain>","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"<462fb876-749b-b426-6761-19105c702c52@ideasonboard.com>","Date":"Mon, 10 May 2021 15:15:03 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.7.1","MIME-Version":"1.0","In-Reply-To":"<20210508072402.sylp35irjojm2g5e@uno.localdomain>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/3] libcamera: camera: Make\n\tstop() idempotent","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?q?ndr=C3=A9_Almeida?= ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16862,"web_url":"https://patchwork.libcamera.org/comment/16862/","msgid":"","date":"2021-05-10T14:25:56","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/3] libcamera: camera: Make\n\tstop() idempotent","submitter":{"id":84,"url":"https://patchwork.libcamera.org/api/people/84/","name":"Nícolas F. R. A. Prado","email":"nfraprado@collabora.com"},"content":"Hi Jacopo,\n\nEm 2021-05-08 04:24, Jacopo Mondi escreveu:\n\n> Hi Laurent,\n>\n> On Sat, May 08, 2021 at 03:03:54AM +0300, Laurent Pinchart wrote:\n> > Hello,\n> >\n> > On Tue, May 04, 2021 at 04:15:17PM -0300, Nícolas F. R. A. Prado wrote:\n> > > Em 2021-05-04 12:29, Jacopo Mondi escreveu:\n> > > > On Tue, May 04, 2021 at 03:48:16PM +0100, Kieran Bingham wrote:\n> > > > > On 04/05/2021 15:31, Nícolas F. R. A. Prado wrote:\n> > > > > > Hi Jacopo,\n> > > > > >\n> > > > > > Em 2021-05-04 04:41, Jacopo Mondi escreveu:\n> > > > > >\n> > > >\n> > > > [snip]\n> > > >\n> > > > > >> Personally, I would be in favour of calling stop() only when it's\n> > > > > >> appropriate instead of shortcutting the camera state machine. What is\n> > > > > >> the use case that makes it hard to call stop() only when actually\n> > > > > >> required ?\n> > > > > >\n> > > > > > The use case is always calling stop() in the destructor of the capture in\n> > > > > > lc-compliance, which makes the cleanup path simpler as we can safely fail an\n> > > > > > assert and still have the camera stopped.\n> > > > > >\n> > > > > > That said, it wouldn't be hard to call stop() only when required. We could track\n> > > > > > whether the camera was started internally in lc-compliance. But it seemed like\n> > > > > > this would be a common pattern and handling it by making Camera::stop() itself\n> > > > > > be idempotent would be beneficial.\n> > > > > >\n> > > > > > I'll wait for others to comment on this as well.\n> > > > >\n> > > > > I think being able to clean up easily on shutdowns is beneficial.\n> > > > >\n> > > > > I would consider this use case to be that of being able to call\n> > > > >\n> > > > > \tp = NULL;\n> > > > > \t...\n> > > > > \tkfree(p);\n> > > > >\n> > > > > rather than\n> > > > > \tif (p)\n> > > > > \t\tkfree(p);\n> > > > >\n> > > > > It's a convenience feature, so that extra state doesn't have to be\n> > > > > repeatedly maintained elsewhere.\n> > > >\n> > > > Might be a personal opinion, but I would prefer, as a general idea,\n> > > > a stricter API. I concur there's not much value in the error message\n> > > > when stop() is called from the wrong state, but sometime it helped me\n> > > > finding a wrong patter when working on the camera HAL. I guess it\n> > > > might be beneficial for other use cases too...\n> > >\n> > > I think adding a log at level DEBUG or INFO stating that \"stop() called for\n> > > non-running camera\" would help debug those cases while still allowing the\n> > > convenience of calling it in any state.\n> >\n> > Sorry about the conflicting directions :-S\n> >\n> > There are two options. Calling Camera::stop() when stopped is either\n> > valid, in which case the function shouldn't log any message with a level\n> > higher than debug, or it's invalid, in which case we should log an\n> > error. In the latter case, the caller needs to keep track of the camera\n> > state, which is a bit of a hassle. Taking Niklas' example, it would be\n> > similar to requiring the caller to have a bool is_p_null variable, as\n> > the Camera class doesn't expose its state, so a caller can do\n> >\n> > \tif (cam->state() == CameraRunning)\n> > \t\tcam->stop();\n> >\n> > We could expose the camera state, but that increases the surface of the\n> > public API, for little gain I believe.\n> >\n> > Jacopo, what do you think ?\n> >\n>\n> Either we change the camera state machine and allow Stop->Stop by\n> adding a state\n> Stopping -> Stopping [label = \"stop()\"];\n>\n> and change\n> isAccessAllowed(Private::CameraStopped,\n> Private::CameraRunning);\n>\n> not to throw any error\n\nBut this wouldn't really solve the problem. The Stopping state only happens\nwhile requests are being finished, and the intention here is to make it possible\nto call stop() in any state to make cleanup easier.\n\nIf we were to add it as part of the state machine, it would mean making each\nstate (apart from Running) point to itself with label \"stop()\", as it wouldn't\nmake sense to transition Acquired to Stopping, for example.\n\nBut if we don't want to transition to the Stopping state from a state like\nAcquired, it means that we want to return early on stop() for the cases where\nthe camera isn't Running. This implies not making use of isAccessAllowed().\nBecause if we did use it, there would have to be two isAccessAllowed() calls in\nstop(), like I did in v1 [1]. But as Kieran noted there, this isn't a case of\nhaving access, since all states are accounted for.\n\nRegarding the state diagram, maybe it is okay to omit the fact that \"stop()\" can\nbe called from states other than Running (ie keep it the way it is), given that\nthose are no-ops and aren't there to change the operation but to make it more\nconvenient? (It will be documented in the stop() function, just not on the state\ndiagram)\n\nFinally, having an error message in isAccessAllowed() sounds important to warn\nabout invalid state transitions. It doesn't seem right to me to remove that\nmessage just because stop() is a special case.\n\nThese are my thoughts :).\n\nThanks,\nNícolas\n\n[1] https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019735.html\n\n>\n> Or we mandate applications to track the state themeselves.\n>\n> Exposing the state has a little gain, as application will\n> unconditionally repeat the pattern\n>\n> if (cam->state() == CameraRunning)\n> cam->stop();\n>\n> hence they could similarly be allowed to just call stop() without\n> bothering about the state check.\n>\n> I would like application to be in charge of tracking the state and\n> know what they're doing precisely, but I'm afraid they will just\n> call stop() and ignore the error, and I'm not 100% sure it is fair to\n> mandate the state tracking burden to apps.\n>\n> So yes, allowing stop->stop seems fair, but it should be made part of\n> the Camera state machine and not worked around by not checking if the\n> access is actually allowed or not imho.\n>\n> Thanks\n> j\n>\n> > > > Anyway, I won't push, whatever's best for the majority is fine with\n> > > > me.\n> > > >\n> > > > > >>> \tint ret = d->isAccessAllowed(Private::CameraRunning);\n> > > > > >>> \tif (ret < 0)\n> > > > > >>> \t\treturn ret;\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n>\n> --\n> To unsubscribe, send mail to kernel-unsubscribe@lists.collabora.co.uk.","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 0AD5BBF831\n\tfor ;\n\tMon, 10 May 2021 14:26:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 75AF868911;\n\tMon, 10 May 2021 16:26:36 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 23664602BE\n\tfor ;\n\tMon, 10 May 2021 16:26:35 +0200 (CEST)","from localhost (unknown\n\t[IPv6:2804:14c:1a9:2978:985c:7892:ebcf:7a90])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits))\n\t(No client certificate requested) (Authenticated sender: nfraprado)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id CCC8A1F41FAD;\n\tMon, 10 May 2021 15:26:32 +0100 (BST)"],"Mime-Version":"1.0","Date":"Mon, 10 May 2021 11:25:56 -0300","Message-Id":"","From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= ","To":"\"Jacopo Mondi\" , \"Laurent Pinchart\"\n\t","References":"\n\t<0c4d56aa-b34a-a02d-76c1-4c3902aa15ac@ideasonboard.com>\n\t<20210504152936.4nkrc3tp42lcprpc@uno.localdomain>\n\t\n\t\n\t<20210508072402.sylp35irjojm2g5e@uno.localdomain>","In-Reply-To":"<20210508072402.sylp35irjojm2g5e@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/3] libcamera: camera: Make\n\tstop() idempotent","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, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?q?ndr=C3=A9_Almeida?= ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]