[{"id":11214,"web_url":"https://patchwork.libcamera.org/comment/11214/","msgid":"<20200706114913.mlyzvhfee5dluo56@uno.localdomain>","date":"2020-07-06T11:49:13","subject":"Re: [libcamera-devel] [PATCH] libcamera: ipa_proxy: Allow stop() on\n\ta stopped IPA","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n just a small nit\n\nOn Mon, Jul 06, 2020 at 02:08:47PM +0300, Laurent Pinchart wrote:\n> To make error handling easier in callers, allow the stop() function to\n> be called when the proxy is already stopped, or not started yet.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/internal/ipa_proxy.h | 2 ++\n> src/libcamera/ipa_proxy.cpp | 10 ++++++++++\n> src/libcamera/proxy/ipa_proxy_thread.cpp | 3 +++\n> 3 files changed, 15 insertions(+)\n>\n> diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h\n> index aec8f04ffc15..b429ce5a68a3 100644\n> --- a/include/libcamera/internal/ipa_proxy.h\n> +++ b/include/libcamera/internal/ipa_proxy.h\n> @@ -27,6 +27,8 @@ public:\n>\n> \tstd::string configurationFile(const std::string &file) const;\n>\n> +\tvoid stop() override = 0;\n> +\n> protected:\n> \tstd::string resolvePath(const std::string &file) const;\n>\n> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp\n> index 23be24ad9bf1..01740a6e39ec 100644\n> --- a/src/libcamera/ipa_proxy.cpp\n> +++ b/src/libcamera/ipa_proxy.cpp\n> @@ -145,6 +145,16 @@ std::string IPAProxy::configurationFile(const std::string &name) const\n> \treturn std::string();\n> }\n>\n> +/**\n> + * \\fn IPAProxy::stop()\n> + * \\brief Stop the IPA proxy\n> + *\n> + * This method informs the IPA module that the camera is stopped. The IPA module\n> + * shall release resources prepared in start(). Calling stop() when the IPA\n\ns/prepared/acquired ?\n\n> + * hasn't been started or has already been stopped is valid, and the IPA shall\n> + * treat this as a no-op.\n> + */\n> +\n> /**\n> * \\brief Find a valid full path for a proxy worker for a given executable name\n> * \\param[in] file File name of proxy worker executable\n> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> index aa403e22f297..eead2883708d 100644\n> --- a/src/libcamera/proxy/ipa_proxy_thread.cpp\n> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> @@ -121,6 +121,9 @@ int IPAProxyThread::start()\n>\n> void IPAProxyThread::stop()\n> {\n> +\tif (!running_)\n> +\t\treturn;\n> +\n\nIn case the IPA is then stopped already, IPA won't receive the call,\nmaking the above \"hasn't been started or has already been stopped is\nvalid\" possibly confusing.\n\nWith out without these addressed\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> \trunning_ = false;\n>\n> \tproxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);\n> --\n> Regards,\n>\n> Laurent Pinchart\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 E66B6BD790\n\tfor ;\n\tMon, 6 Jul 2020 11:45:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C65860E01;\n\tMon, 6 Jul 2020 13:45:42 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 19F7F603AA\n\tfor ;\n\tMon, 6 Jul 2020 13:45:41 +0200 (CEST)","from uno.localdomain (host-79-34-235-173.business.telecomitalia.it\n\t[79.34.235.173]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 45FE0C000B;\n\tMon, 6 Jul 2020 11:45:40 +0000 (UTC)"],"X-Originating-IP":"79.34.235.173","Date":"Mon, 6 Jul 2020 13:49:13 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20200706114913.mlyzvhfee5dluo56@uno.localdomain>","References":"<20200706110847.11324-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200706110847.11324-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: ipa_proxy: Allow stop() on\n\ta stopped IPA","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11215,"web_url":"https://patchwork.libcamera.org/comment/11215/","msgid":"<20200706115129.GG5912@pendragon.ideasonboard.com>","date":"2020-07-06T11:51:29","subject":"Re: [libcamera-devel] [PATCH] libcamera: ipa_proxy: Allow stop() on\n\ta stopped IPA","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Jul 06, 2020 at 01:49:13PM +0200, Jacopo Mondi wrote:\n> On Mon, Jul 06, 2020 at 02:08:47PM +0300, Laurent Pinchart wrote:\n> > To make error handling easier in callers, allow the stop() function to\n> > be called when the proxy is already stopped, or not started yet.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/internal/ipa_proxy.h | 2 ++\n> > src/libcamera/ipa_proxy.cpp | 10 ++++++++++\n> > src/libcamera/proxy/ipa_proxy_thread.cpp | 3 +++\n> > 3 files changed, 15 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h\n> > index aec8f04ffc15..b429ce5a68a3 100644\n> > --- a/include/libcamera/internal/ipa_proxy.h\n> > +++ b/include/libcamera/internal/ipa_proxy.h\n> > @@ -27,6 +27,8 @@ public:\n> >\n> > \tstd::string configurationFile(const std::string &file) const;\n> >\n> > +\tvoid stop() override = 0;\n> > +\n> > protected:\n> > \tstd::string resolvePath(const std::string &file) const;\n> >\n> > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp\n> > index 23be24ad9bf1..01740a6e39ec 100644\n> > --- a/src/libcamera/ipa_proxy.cpp\n> > +++ b/src/libcamera/ipa_proxy.cpp\n> > @@ -145,6 +145,16 @@ std::string IPAProxy::configurationFile(const std::string &name) const\n> > \treturn std::string();\n> > }\n> >\n> > +/**\n> > + * \\fn IPAProxy::stop()\n> > + * \\brief Stop the IPA proxy\n> > + *\n> > + * This method informs the IPA module that the camera is stopped. The IPA module\n> > + * shall release resources prepared in start(). Calling stop() when the IPA\n> \n> s/prepared/acquired ?\n> \n> > + * hasn't been started or has already been stopped is valid, and the IPA shall\n> > + * treat this as a no-op.\n> > + */\n> > +\n> > /**\n> > * \\brief Find a valid full path for a proxy worker for a given executable name\n> > * \\param[in] file File name of proxy worker executable\n> > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > index aa403e22f297..eead2883708d 100644\n> > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > @@ -121,6 +121,9 @@ int IPAProxyThread::start()\n> >\n> > void IPAProxyThread::stop()\n> > {\n> > +\tif (!running_)\n> > +\t\treturn;\n> > +\n> \n> In case the IPA is then stopped already, IPA won't receive the call,\n> making the above \"hasn't been started or has already been stopped is\n> valid\" possibly confusing.\n\nI agree with you. How about the following ?\n\n * This function stops the IPA and releases all the resources acquired by the\n * proxy in start(). Calling stop() when the IPA proxy hasn't been started or\n * has already been stopped is valid, the proxy shall treat this as a no-op and\n * shall not forward the call to the IPA.\n\n> With out without these addressed\n> Reviewed-by: Jacopo Mondi \n> \n> > \trunning_ = false;\n> >\n> > \tproxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);","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 8554FBD792\n\tfor ;\n\tMon, 6 Jul 2020 11:51:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 24504603AB;\n\tMon, 6 Jul 2020 13:51:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BD77E603AA\n\tfor ;\n\tMon, 6 Jul 2020 13:51:35 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E517997E;\n\tMon, 6 Jul 2020 13:51:34 +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=\"ouDIvfI+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1594036295;\n\tbh=YqE2noa3+XzgjFpZYECEMUjv3YDkr9yO2+HjHeK+CMA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ouDIvfI+tAj3I0OiSGnPjJNsYPOi4nY0/FhfK+40/kl/VS/e/oF9LTEorKUvECi4c\n\t0dpXasFjqMLDA9tpcvMW716QkbFh0rejSonHNjZeXeQUnWqQqTNbGpwjg5nN1CpntF\n\tmxaFKH/8qQVTfIYzJ0yGQeOCKmkft09VGyuiel1k=","Date":"Mon, 6 Jul 2020 14:51:29 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200706115129.GG5912@pendragon.ideasonboard.com>","References":"<20200706110847.11324-1-laurent.pinchart@ideasonboard.com>\n\t<20200706114913.mlyzvhfee5dluo56@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200706114913.mlyzvhfee5dluo56@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: ipa_proxy: Allow stop() on\n\ta stopped IPA","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11216,"web_url":"https://patchwork.libcamera.org/comment/11216/","msgid":"<20200706115648.5lx7dl42u6tpyijo@uno.localdomain>","date":"2020-07-06T11:56:48","subject":"Re: [libcamera-devel] [PATCH] libcamera: ipa_proxy: Allow stop() on\n\ta stopped IPA","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Jul 06, 2020 at 02:51:29PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Jul 06, 2020 at 01:49:13PM +0200, Jacopo Mondi wrote:\n> > On Mon, Jul 06, 2020 at 02:08:47PM +0300, Laurent Pinchart wrote:\n> > > To make error handling easier in callers, allow the stop() function to\n> > > be called when the proxy is already stopped, or not started yet.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > include/libcamera/internal/ipa_proxy.h | 2 ++\n> > > src/libcamera/ipa_proxy.cpp | 10 ++++++++++\n> > > src/libcamera/proxy/ipa_proxy_thread.cpp | 3 +++\n> > > 3 files changed, 15 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h\n> > > index aec8f04ffc15..b429ce5a68a3 100644\n> > > --- a/include/libcamera/internal/ipa_proxy.h\n> > > +++ b/include/libcamera/internal/ipa_proxy.h\n> > > @@ -27,6 +27,8 @@ public:\n> > >\n> > > \tstd::string configurationFile(const std::string &file) const;\n> > >\n> > > +\tvoid stop() override = 0;\n> > > +\n> > > protected:\n> > > \tstd::string resolvePath(const std::string &file) const;\n> > >\n> > > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp\n> > > index 23be24ad9bf1..01740a6e39ec 100644\n> > > --- a/src/libcamera/ipa_proxy.cpp\n> > > +++ b/src/libcamera/ipa_proxy.cpp\n> > > @@ -145,6 +145,16 @@ std::string IPAProxy::configurationFile(const std::string &name) const\n> > > \treturn std::string();\n> > > }\n> > >\n> > > +/**\n> > > + * \\fn IPAProxy::stop()\n> > > + * \\brief Stop the IPA proxy\n> > > + *\n> > > + * This method informs the IPA module that the camera is stopped. The IPA module\n> > > + * shall release resources prepared in start(). Calling stop() when the IPA\n> >\n> > s/prepared/acquired ?\n> >\n> > > + * hasn't been started or has already been stopped is valid, and the IPA shall\n> > > + * treat this as a no-op.\n> > > + */\n> > > +\n> > > /**\n> > > * \\brief Find a valid full path for a proxy worker for a given executable name\n> > > * \\param[in] file File name of proxy worker executable\n> > > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > > index aa403e22f297..eead2883708d 100644\n> > > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > > @@ -121,6 +121,9 @@ int IPAProxyThread::start()\n> > >\n> > > void IPAProxyThread::stop()\n> > > {\n> > > +\tif (!running_)\n> > > +\t\treturn;\n> > > +\n> >\n> > In case the IPA is then stopped already, IPA won't receive the call,\n> > making the above \"hasn't been started or has already been stopped is\n> > valid\" possibly confusing.\n>\n> I agree with you. How about the following ?\n>\n> * This function stops the IPA and releases all the resources acquired by the\n> * proxy in start(). Calling stop() when the IPA proxy hasn't been started or\n> * has already been stopped is valid, the proxy shall treat this as a no-op and\n> * shall not forward the call to the IPA.\n>\n\nLooks good, thanks!\n\n> > With out without these addressed\n> > Reviewed-by: Jacopo Mondi \n> >\n> > > \trunning_ = false;\n> > >\n> > > \tproxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);\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 B3727BD792\n\tfor ;\n\tMon, 6 Jul 2020 11:53:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 89FA8603AB;\n\tMon, 6 Jul 2020 13:53:17 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2B023603AA\n\tfor ;\n\tMon, 6 Jul 2020 13:53:16 +0200 (CEST)","from uno.localdomain (host-79-34-235-173.business.telecomitalia.it\n\t[79.34.235.173]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 5B7FF40006;\n\tMon, 6 Jul 2020 11:53:15 +0000 (UTC)"],"X-Originating-IP":"79.34.235.173","Date":"Mon, 6 Jul 2020 13:56:48 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20200706115648.5lx7dl42u6tpyijo@uno.localdomain>","References":"<20200706110847.11324-1-laurent.pinchart@ideasonboard.com>\n\t<20200706114913.mlyzvhfee5dluo56@uno.localdomain>\n\t<20200706115129.GG5912@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200706115129.GG5912@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: ipa_proxy: Allow stop() on\n\ta stopped IPA","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]