[{"id":12779,"web_url":"https://patchwork.libcamera.org/comment/12779/","msgid":"<20200925153600.5bbotjnxzux6a6dr@uno.localdomain>","date":"2020-09-25T15:36:00","subject":"Re: [libcamera-devel] [PATCH v3 22/22] libcamera: pipeline: rkisp1:\n\tUse cleanup error paths for start()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Fri, Sep 25, 2020 at 03:42:07AM +0200, Niklas Söderlund wrote:\n> Use a lambda function to make the error paths a bit cleaner in start(),\n> no functional change.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 98 ++++++++++++------------\n> 1 file changed, 51 insertions(+), 47 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index c6c2e3aa3dc6d0dc..34196af3057b14bb 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -827,58 +827,62 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> \tRkISP1CameraData *data = cameraData(camera);\n> \tint ret;\n>\n> -\t/* Allocate buffers for internal pipeline usage. */\n> -\tret = allocateBuffers(camera);\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> -\tret = data->ipa_->start();\n> -\tif (ret) {\n> -\t\tfreeBuffers(camera);\n> -\t\tLOG(RkISP1, Error)\n> -\t\t\t<< \"Failed to start IPA \" << camera->id();\n> -\t\treturn ret;\n> -\t}\n> -\n> -\tdata->frame_ = 0;\n> -\n> -\tret = param_->streamOn();\n> -\tif (ret) {\n> -\t\tdata->ipa_->stop();\n> -\t\tfreeBuffers(camera);\n> -\t\tLOG(RkISP1, Error)\n> -\t\t\t<< \"Failed to start parameters \" << camera->id();\n> -\t\treturn ret;\n> -\t}\n> -\n> -\tret = stat_->streamOn();\n> -\tif (ret) {\n> -\t\tparam_->streamOff();\n> -\t\tdata->ipa_->stop();\n> -\t\tfreeBuffers(camera);\n> -\t\tLOG(RkISP1, Error)\n> -\t\t\t<< \"Failed to start statistics \" << camera->id();\n> -\t\treturn ret;\n> -\t}\n> -\n> -\tret = mainPath_.start();\n> -\tif (ret) {\n> -\t\tparam_->streamOff();\n> -\t\tstat_->streamOff();\n> -\t\tdata->ipa_->stop();\n> -\t\tfreeBuffers(camera);\n> -\t\treturn ret;\n> -\t}\n> -\n> -\tret = selfPath_.start();\n> -\tif (ret) {\n> +\tauto startDevices = [this](Camera *camera, RkISP1CameraData *data) {\n\nWhy a lamda function for a function that is only called for a single place as a\nregulr function ?\n\nAlso, shouldn't re-working the existing code to use labels be better ?\n\n> +\t\tint ret;\n> +\n> +\t\t/* Allocate buffers for internal pipeline usage. */\n> +\t\tret = allocateBuffers(camera);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\tret = data->ipa_->start();\n> +\t\tif (ret) {\n> +\t\t\tLOG(RkISP1, Error)\n> +\t\t\t\t<< \"Failed to start IPA \" << camera->id();\n> +\t\t\tgoto err_allocate;\n> +\t\t}\n> +\n> +\t\tdata->frame_ = 0;\n> +\n> +\t\tret = param_->streamOn();\n> +\t\tif (ret) {\n> +\t\t\tLOG(RkISP1, Error)\n> +\t\t\t\t<< \"Failed to start parameters \" << camera->id();\n> +\t\t\tgoto err_ipa;\n> +\t\t}\n> +\n> +\t\tret = stat_->streamOn();\n> +\t\tif (ret) {\n> +\t\t\tLOG(RkISP1, Error)\n> +\t\t\t\t<< \"Failed to start statistics \" << camera->id();\n> +\t\t\tgoto err_param;\n> +\t\t}\n> +\n> +\t\tret = mainPath_.start();\n> +\t\tif (ret)\n> +\t\t\tgoto err_stat;\n> +\n> +\t\tret = selfPath_.start();\n> +\t\tif (ret)\n> +\t\t\tgoto err_main;\n> +\n> +\t\treturn 0;\n> +\terr_main:\n> \t\tmainPath_.stop();\n> -\t\tparam_->streamOff();\n> +\terr_stat:\n> \t\tstat_->streamOff();\n> +\terr_param:\n> +\t\tparam_->streamOff();\n> +\terr_ipa:\n> \t\tdata->ipa_->stop();\n> +\terr_allocate:\n> \t\tfreeBuffers(camera);\n> \t\treturn ret;\n> -\t}\n> +\t};\n> +\n> +\tret = startDevices(camera, data);\n> +\tif (ret)\n> +\t\treturn ret;\n>\n> \tactiveCamera_ = camera;\n>\n> --\n> 2.28.0\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 6D36AC3B5C\n\tfor ;\n\tFri, 25 Sep 2020 15:32:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C81E63041;\n\tFri, 25 Sep 2020 17:32:09 +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 33C8962FD8\n\tfor ;\n\tFri, 25 Sep 2020 17:32:08 +0200 (CEST)","from uno.localdomain (host-87-18-63-10.retail.telecomitalia.it\n\t[87.18.63.10]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 5CDE81C0005;\n\tFri, 25 Sep 2020 15:32:07 +0000 (UTC)"],"X-Originating-IP":"87.18.63.10","Date":"Fri, 25 Sep 2020 17:36:00 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200925153600.5bbotjnxzux6a6dr@uno.localdomain>","References":"<20200925014207.1455796-1-niklas.soderlund@ragnatech.se>\n\t<20200925014207.1455796-23-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200925014207.1455796-23-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v3 22/22] libcamera: pipeline: rkisp1:\n\tUse cleanup error paths for start()","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12787,"web_url":"https://patchwork.libcamera.org/comment/12787/","msgid":"<20200925165731.GI1757254@oden.dyn.berto.se>","date":"2020-09-25T16:57:31","subject":"Re: [libcamera-devel] [PATCH v3 22/22] libcamera: pipeline: rkisp1:\n\tUse cleanup error paths for start()","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nOn 2020-09-25 17:36:00 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Fri, Sep 25, 2020 at 03:42:07AM +0200, Niklas Söderlund wrote:\n> > Use a lambda function to make the error paths a bit cleaner in start(),\n> > no functional change.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 98 ++++++++++++------------\n> > 1 file changed, 51 insertions(+), 47 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index c6c2e3aa3dc6d0dc..34196af3057b14bb 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -827,58 +827,62 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> > \tRkISP1CameraData *data = cameraData(camera);\n> > \tint ret;\n> >\n> > -\t/* Allocate buffers for internal pipeline usage. */\n> > -\tret = allocateBuffers(camera);\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > -\n> > -\tret = data->ipa_->start();\n> > -\tif (ret) {\n> > -\t\tfreeBuffers(camera);\n> > -\t\tLOG(RkISP1, Error)\n> > -\t\t\t<< \"Failed to start IPA \" << camera->id();\n> > -\t\treturn ret;\n> > -\t}\n> > -\n> > -\tdata->frame_ = 0;\n> > -\n> > -\tret = param_->streamOn();\n> > -\tif (ret) {\n> > -\t\tdata->ipa_->stop();\n> > -\t\tfreeBuffers(camera);\n> > -\t\tLOG(RkISP1, Error)\n> > -\t\t\t<< \"Failed to start parameters \" << camera->id();\n> > -\t\treturn ret;\n> > -\t}\n> > -\n> > -\tret = stat_->streamOn();\n> > -\tif (ret) {\n> > -\t\tparam_->streamOff();\n> > -\t\tdata->ipa_->stop();\n> > -\t\tfreeBuffers(camera);\n> > -\t\tLOG(RkISP1, Error)\n> > -\t\t\t<< \"Failed to start statistics \" << camera->id();\n> > -\t\treturn ret;\n> > -\t}\n> > -\n> > -\tret = mainPath_.start();\n> > -\tif (ret) {\n> > -\t\tparam_->streamOff();\n> > -\t\tstat_->streamOff();\n> > -\t\tdata->ipa_->stop();\n> > -\t\tfreeBuffers(camera);\n> > -\t\treturn ret;\n> > -\t}\n> > -\n> > -\tret = selfPath_.start();\n> > -\tif (ret) {\n> > +\tauto startDevices = [this](Camera *camera, RkISP1CameraData *data) {\n> \n> Why a lamda function for a function that is only called for a single place as a\n> regulr function ?\n\nIt was suggested in review of an earlier version to make the code more \nreadable. But as the cover letter suggests this 22/22 should be thought \nof as a RFC. I found it rather neat to make the code more readable.\n\n> \n> Also, shouldn't re-working the existing code to use labels be better ?\n\nMaybe, it's such a larger function so I thought it neater to crate a \nseparate \"block\" for the code needing error handling. If this was C I \nwould have created a helper function just to get it out but creating a \nhelper class function for this seems a bit much so a lambda was \nsomewhere in the middle.\n\nMaybe I will break this out of this series so people don't spend all \nreview energy bikeshedding over this one ;-P\n\n> \n> > +\t\tint ret;\n> > +\n> > +\t\t/* Allocate buffers for internal pipeline usage. */\n> > +\t\tret = allocateBuffers(camera);\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > +\n> > +\t\tret = data->ipa_->start();\n> > +\t\tif (ret) {\n> > +\t\t\tLOG(RkISP1, Error)\n> > +\t\t\t\t<< \"Failed to start IPA \" << camera->id();\n> > +\t\t\tgoto err_allocate;\n> > +\t\t}\n> > +\n> > +\t\tdata->frame_ = 0;\n> > +\n> > +\t\tret = param_->streamOn();\n> > +\t\tif (ret) {\n> > +\t\t\tLOG(RkISP1, Error)\n> > +\t\t\t\t<< \"Failed to start parameters \" << camera->id();\n> > +\t\t\tgoto err_ipa;\n> > +\t\t}\n> > +\n> > +\t\tret = stat_->streamOn();\n> > +\t\tif (ret) {\n> > +\t\t\tLOG(RkISP1, Error)\n> > +\t\t\t\t<< \"Failed to start statistics \" << camera->id();\n> > +\t\t\tgoto err_param;\n> > +\t\t}\n> > +\n> > +\t\tret = mainPath_.start();\n> > +\t\tif (ret)\n> > +\t\t\tgoto err_stat;\n> > +\n> > +\t\tret = selfPath_.start();\n> > +\t\tif (ret)\n> > +\t\t\tgoto err_main;\n> > +\n> > +\t\treturn 0;\n> > +\terr_main:\n> > \t\tmainPath_.stop();\n> > -\t\tparam_->streamOff();\n> > +\terr_stat:\n> > \t\tstat_->streamOff();\n> > +\terr_param:\n> > +\t\tparam_->streamOff();\n> > +\terr_ipa:\n> > \t\tdata->ipa_->stop();\n> > +\terr_allocate:\n> > \t\tfreeBuffers(camera);\n> > \t\treturn ret;\n> > -\t}\n> > +\t};\n> > +\n> > +\tret = startDevices(camera, data);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> >\n> > \tactiveCamera_ = camera;\n> >\n> > --\n> > 2.28.0\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 EB7E9C3B5B\n\tfor ;\n\tFri, 25 Sep 2020 16:57:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 68B8263041;\n\tFri, 25 Sep 2020 18:57:34 +0200 (CEST)","from mail-lf1-x143.google.com (mail-lf1-x143.google.com\n\t[IPv6:2a00:1450:4864:20::143])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BADCD62FD8\n\tfor ;\n\tFri, 25 Sep 2020 18:57:32 +0200 (CEST)","by mail-lf1-x143.google.com with SMTP id 77so3596213lfj.0\n\tfor ;\n\tFri, 25 Sep 2020 09:57:32 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tm132sm2702947lfa.217.2020.09.25.09.57.31\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 25 Sep 2020 09:57:31 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"h/b5CYtH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=RUV2I0d+5mDDlj1U0jq1nwimdA4uKhKOnw5Xp6T37mg=;\n\tb=h/b5CYtHgyke0RdCyCDWX+tAf9a/X+Z4kvCaKamVTYcES0PEDApiRYh6wb03Vd2WYR\n\t3J+VADUBYbTphgI5pPSP8RUKfW1B0dgYkNIQPPJ9CJ1/dQRonDrqs+JsYp/DxaTFi/cE\n\tRVmCs0jarpDyXYcM83Vk9GmPVYmOaVg5NG4Bf6g22/ZBVEILHpJXXavQpNgFmzjJ8SN3\n\tJi8Ctp2M/YQRFPk7MkTqabEe+DklpOfwAhliBmVsGT5dV9UUh1iDvh8bz4haRIwx6Lwn\n\tZFSGkI3yLIkHooi9UlGYpzs0aY8VCN8unTcLzkve07o9N1/LDLOsEk+YeME4IM7mP0Yy\n\tgxfQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=RUV2I0d+5mDDlj1U0jq1nwimdA4uKhKOnw5Xp6T37mg=;\n\tb=HA5sLtMOhFquQOObl7oyWoNXb1nLkJ4N2gN8adPgCWXwCYS2Tk4Dq+334nQuEvQaK4\n\t3KkmHQgkfUiMVnNHWqRsM4Ux+nQl64zgoNYh3qI27EjLBjMCIUSN9G7G7rb+KGnjfZfr\n\tizcUTSEFBrRgtwqfTmZlxe+p2pKiSMBQ8ng5phK0OUjRWl1G5b+xrKv5Jo6ricMMUhNq\n\tMjnpsU6Ph/V7ttshhNiv9fXNjs7JmjaijE4jKxD9iCBCbN19T7vPd6xA96y1qk5Ei7ND\n\tc/L9YiqHYo1lknf0l5E7IDiGjpN++jVcxG/M4ozQs+RstVDfmxgbTxZWwtfFUAEKFua2\n\tmD7g==","X-Gm-Message-State":"AOAM531VAlsatS2O6BQ01bskeO5WHEZmcnC0PVRTi8sHgGpN2Nkqno4c\n\ta0Gvkjw0MOCu5biUwpe8ajqlX/dE/95VKg==","X-Google-Smtp-Source":"ABdhPJy6Wr+ugTaX7K23IkwdK3rV3O3fRBBWYPFMxmgz4XjoOMsS66E9WB/CoowcXvAKMSN2KhzOPA==","X-Received":"by 2002:a19:dd5:: with SMTP id 204mr1580146lfn.579.1601053052066;\n\tFri, 25 Sep 2020 09:57:32 -0700 (PDT)","Date":"Fri, 25 Sep 2020 18:57:31 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200925165731.GI1757254@oden.dyn.berto.se>","References":"<20200925014207.1455796-1-niklas.soderlund@ragnatech.se>\n\t<20200925014207.1455796-23-niklas.soderlund@ragnatech.se>\n\t<20200925153600.5bbotjnxzux6a6dr@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200925153600.5bbotjnxzux6a6dr@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 22/22] libcamera: pipeline: rkisp1:\n\tUse cleanup error paths for start()","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=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12797,"web_url":"https://patchwork.libcamera.org/comment/12797/","msgid":"<20200928090903.agknvh5lme5f6hki@uno.localdomain>","date":"2020-09-28T09:09:03","subject":"Re: [libcamera-devel] [PATCH v3 22/22] libcamera: pipeline: rkisp1:\n\tUse cleanup error paths for start()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Fri, Sep 25, 2020 at 06:57:31PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> On 2020-09-25 17:36:00 +0200, Jacopo Mondi wrote:\n> > Hi Niklas,\n> >\n> > On Fri, Sep 25, 2020 at 03:42:07AM +0200, Niklas Söderlund wrote:\n> > > Use a lambda function to make the error paths a bit cleaner in start(),\n> > > no functional change.\n> > >\n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 98 ++++++++++++------------\n> > > 1 file changed, 51 insertions(+), 47 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index c6c2e3aa3dc6d0dc..34196af3057b14bb 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -827,58 +827,62 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> > > \tRkISP1CameraData *data = cameraData(camera);\n> > > \tint ret;\n> > >\n> > > -\t/* Allocate buffers for internal pipeline usage. */\n> > > -\tret = allocateBuffers(camera);\n> > > -\tif (ret)\n> > > -\t\treturn ret;\n> > > -\n> > > -\tret = data->ipa_->start();\n> > > -\tif (ret) {\n> > > -\t\tfreeBuffers(camera);\n> > > -\t\tLOG(RkISP1, Error)\n> > > -\t\t\t<< \"Failed to start IPA \" << camera->id();\n> > > -\t\treturn ret;\n> > > -\t}\n> > > -\n> > > -\tdata->frame_ = 0;\n> > > -\n> > > -\tret = param_->streamOn();\n> > > -\tif (ret) {\n> > > -\t\tdata->ipa_->stop();\n> > > -\t\tfreeBuffers(camera);\n> > > -\t\tLOG(RkISP1, Error)\n> > > -\t\t\t<< \"Failed to start parameters \" << camera->id();\n> > > -\t\treturn ret;\n> > > -\t}\n> > > -\n> > > -\tret = stat_->streamOn();\n> > > -\tif (ret) {\n> > > -\t\tparam_->streamOff();\n> > > -\t\tdata->ipa_->stop();\n> > > -\t\tfreeBuffers(camera);\n> > > -\t\tLOG(RkISP1, Error)\n> > > -\t\t\t<< \"Failed to start statistics \" << camera->id();\n> > > -\t\treturn ret;\n> > > -\t}\n> > > -\n> > > -\tret = mainPath_.start();\n> > > -\tif (ret) {\n> > > -\t\tparam_->streamOff();\n> > > -\t\tstat_->streamOff();\n> > > -\t\tdata->ipa_->stop();\n> > > -\t\tfreeBuffers(camera);\n> > > -\t\treturn ret;\n> > > -\t}\n> > > -\n> > > -\tret = selfPath_.start();\n> > > -\tif (ret) {\n> > > +\tauto startDevices = [this](Camera *camera, RkISP1CameraData *data) {\n> >\n> > Why a lamda function for a function that is only called for a single place as a\n> > regulr function ?\n>\n> It was suggested in review of an earlier version to make the code more\n> readable. But as the cover letter suggests this 22/22 should be thought\n> of as a RFC. I found it rather neat to make the code more readable.\n>\n> >\n> > Also, shouldn't re-working the existing code to use labels be better ?\n>\n> Maybe, it's such a larger function so I thought it neater to crate a\n> separate \"block\" for the code needing error handling. If this was C I\n> would have created a helper function just to get it out but creating a\n> helper class function for this seems a bit much so a lambda was\n> somewhere in the middle.\n>\n> Maybe I will break this out of this series so people don't spend all\n> review energy bikeshedding over this one ;-P\n\nJust for sake of discussion:\n\nIt's mostly for the fact that it's called from a single point that\nmeans it's not necesasry to use lambda in my opinion (also, why do you\nneed to capture [this] ?)\n\nI would see a lambda fit if you were to iterate on an array of video\ndevices and perform the same action on all of them like (not compiled\nor tested):\n\n std::array videoDevices{\n param_, stat_, main_, self_\n };\n\n /* Allocate buffers for internal pipeline usage. */\n ret = allocateBuffers(camera);\n if (ret)\n return ret;\n\n int ret;\n std::for_each(std::begin(videoDevices),\n std::end(videoDevices),\n [&ret](const V4L2VideoDevice &vdev) {\n ret = vdev.streamOn();\n if (ret)\n goto stopDevices;\n }\n }\n\n return 0;\n\n stopDevices:\n freeBuffers(camera);\n std::for_each(std::begin(videoDevices),\n std::end(videoDevices),\n [](const V4L2VideoDevice &vdev) {\n vdev.streamOff();\n }\n }\n\n return ret;\n\nBut you have both V4L2VideoDevice to start with streamOn() and\nRkISP1Path to start with start() so I don't see it very neat unless\nyou create two different arrays or wrap the stat_ and param_\nV4L2VideoDevice * in a RkISP1Path.\n\nJust adding labels to the existing seems easier :)\n\n>\n> >\n> > > +\t\tint ret;\n> > > +\n> > > +\t\t/* Allocate buffers for internal pipeline usage. */\n> > > +\t\tret = allocateBuffers(camera);\n> > > +\t\tif (ret)\n> > > +\t\t\treturn ret;\n> > > +\n> > > +\t\tret = data->ipa_->start();\n> > > +\t\tif (ret) {\n> > > +\t\t\tLOG(RkISP1, Error)\n> > > +\t\t\t\t<< \"Failed to start IPA \" << camera->id();\n> > > +\t\t\tgoto err_allocate;\n> > > +\t\t}\n> > > +\n> > > +\t\tdata->frame_ = 0;\n> > > +\n> > > +\t\tret = param_->streamOn();\n> > > +\t\tif (ret) {\n> > > +\t\t\tLOG(RkISP1, Error)\n> > > +\t\t\t\t<< \"Failed to start parameters \" << camera->id();\n> > > +\t\t\tgoto err_ipa;\n> > > +\t\t}\n> > > +\n> > > +\t\tret = stat_->streamOn();\n> > > +\t\tif (ret) {\n> > > +\t\t\tLOG(RkISP1, Error)\n> > > +\t\t\t\t<< \"Failed to start statistics \" << camera->id();\n> > > +\t\t\tgoto err_param;\n> > > +\t\t}\n> > > +\n> > > +\t\tret = mainPath_.start();\n> > > +\t\tif (ret)\n> > > +\t\t\tgoto err_stat;\n> > > +\n> > > +\t\tret = selfPath_.start();\n> > > +\t\tif (ret)\n> > > +\t\t\tgoto err_main;\n> > > +\n> > > +\t\treturn 0;\n> > > +\terr_main:\n> > > \t\tmainPath_.stop();\n> > > -\t\tparam_->streamOff();\n> > > +\terr_stat:\n> > > \t\tstat_->streamOff();\n> > > +\terr_param:\n> > > +\t\tparam_->streamOff();\n> > > +\terr_ipa:\n> > > \t\tdata->ipa_->stop();\n> > > +\terr_allocate:\n> > > \t\tfreeBuffers(camera);\n> > > \t\treturn ret;\n> > > -\t}\n> > > +\t};\n> > > +\n> > > +\tret = startDevices(camera, data);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > >\n> > > \tactiveCamera_ = camera;\n> > >\n> > > --\n> > > 2.28.0\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","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 1F785C3B5B\n\tfor ;\n\tMon, 28 Sep 2020 09:05:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6D9D860BD7;\n\tMon, 28 Sep 2020 11:05:10 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0CEEB6035F\n\tfor ;\n\tMon, 28 Sep 2020 11:05:09 +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 relay11.mail.gandi.net (Postfix) with ESMTPSA id 6A161100015;\n\tMon, 28 Sep 2020 09:05:07 +0000 (UTC)"],"Date":"Mon, 28 Sep 2020 11:09:03 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200928090903.agknvh5lme5f6hki@uno.localdomain>","References":"<20200925014207.1455796-1-niklas.soderlund@ragnatech.se>\n\t<20200925014207.1455796-23-niklas.soderlund@ragnatech.se>\n\t<20200925153600.5bbotjnxzux6a6dr@uno.localdomain>\n\t<20200925165731.GI1757254@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200925165731.GI1757254@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v3 22/22] libcamera: pipeline: rkisp1:\n\tUse cleanup error paths for start()","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12828,"web_url":"https://patchwork.libcamera.org/comment/12828/","msgid":"<20200928215012.GB1052036@oden.dyn.berto.se>","date":"2020-09-28T21:50:12","subject":"Re: [libcamera-devel] [PATCH v3 22/22] libcamera: pipeline: rkisp1:\n\tUse cleanup error paths for start()","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nOn 2020-09-28 11:09:03 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Fri, Sep 25, 2020 at 06:57:31PM +0200, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> >\n> > On 2020-09-25 17:36:00 +0200, Jacopo Mondi wrote:\n> > > Hi Niklas,\n> > >\n> > > On Fri, Sep 25, 2020 at 03:42:07AM +0200, Niklas Söderlund wrote:\n> > > > Use a lambda function to make the error paths a bit cleaner in start(),\n> > > > no functional change.\n> > > >\n> > > > Signed-off-by: Niklas Söderlund \n> > > > ---\n> > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 98 ++++++++++++------------\n> > > > 1 file changed, 51 insertions(+), 47 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > index c6c2e3aa3dc6d0dc..34196af3057b14bb 100644\n> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > @@ -827,58 +827,62 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> > > > \tRkISP1CameraData *data = cameraData(camera);\n> > > > \tint ret;\n> > > >\n> > > > -\t/* Allocate buffers for internal pipeline usage. */\n> > > > -\tret = allocateBuffers(camera);\n> > > > -\tif (ret)\n> > > > -\t\treturn ret;\n> > > > -\n> > > > -\tret = data->ipa_->start();\n> > > > -\tif (ret) {\n> > > > -\t\tfreeBuffers(camera);\n> > > > -\t\tLOG(RkISP1, Error)\n> > > > -\t\t\t<< \"Failed to start IPA \" << camera->id();\n> > > > -\t\treturn ret;\n> > > > -\t}\n> > > > -\n> > > > -\tdata->frame_ = 0;\n> > > > -\n> > > > -\tret = param_->streamOn();\n> > > > -\tif (ret) {\n> > > > -\t\tdata->ipa_->stop();\n> > > > -\t\tfreeBuffers(camera);\n> > > > -\t\tLOG(RkISP1, Error)\n> > > > -\t\t\t<< \"Failed to start parameters \" << camera->id();\n> > > > -\t\treturn ret;\n> > > > -\t}\n> > > > -\n> > > > -\tret = stat_->streamOn();\n> > > > -\tif (ret) {\n> > > > -\t\tparam_->streamOff();\n> > > > -\t\tdata->ipa_->stop();\n> > > > -\t\tfreeBuffers(camera);\n> > > > -\t\tLOG(RkISP1, Error)\n> > > > -\t\t\t<< \"Failed to start statistics \" << camera->id();\n> > > > -\t\treturn ret;\n> > > > -\t}\n> > > > -\n> > > > -\tret = mainPath_.start();\n> > > > -\tif (ret) {\n> > > > -\t\tparam_->streamOff();\n> > > > -\t\tstat_->streamOff();\n> > > > -\t\tdata->ipa_->stop();\n> > > > -\t\tfreeBuffers(camera);\n> > > > -\t\treturn ret;\n> > > > -\t}\n> > > > -\n> > > > -\tret = selfPath_.start();\n> > > > -\tif (ret) {\n> > > > +\tauto startDevices = [this](Camera *camera, RkISP1CameraData *data) {\n> > >\n> > > Why a lamda function for a function that is only called for a single place as a\n> > > regulr function ?\n> >\n> > It was suggested in review of an earlier version to make the code more\n> > readable. But as the cover letter suggests this 22/22 should be thought\n> > of as a RFC. I found it rather neat to make the code more readable.\n> >\n> > >\n> > > Also, shouldn't re-working the existing code to use labels be better ?\n> >\n> > Maybe, it's such a larger function so I thought it neater to crate a\n> > separate \"block\" for the code needing error handling. If this was C I\n> > would have created a helper function just to get it out but creating a\n> > helper class function for this seems a bit much so a lambda was\n> > somewhere in the middle.\n> >\n> > Maybe I will break this out of this series so people don't spend all\n> > review energy bikeshedding over this one ;-P\n> \n> Just for sake of discussion:\n\n:-)\n\n> \n> It's mostly for the fact that it's called from a single point that\n> means it's not necesasry to use lambda in my opinion (also, why do you\n> need to capture [this] ?)\n\nI need [this] to allow the labmda to access the class member variables.\n\n> \n> I would see a lambda fit if you were to iterate on an array of video\n> devices and perform the same action on all of them like (not compiled\n> or tested):\n> \n> std::array videoDevices{\n> param_, stat_, main_, self_\n> };\n> \n> /* Allocate buffers for internal pipeline usage. */\n> ret = allocateBuffers(camera);\n> if (ret)\n> return ret;\n> \n> int ret;\n> std::for_each(std::begin(videoDevices),\n> std::end(videoDevices),\n> [&ret](const V4L2VideoDevice &vdev) {\n> ret = vdev.streamOn();\n> if (ret)\n> goto stopDevices;\n> }\n> }\n> \n> return 0;\n> \n> stopDevices:\n> freeBuffers(camera);\n> std::for_each(std::begin(videoDevices),\n> std::end(videoDevices),\n> [](const V4L2VideoDevice &vdev) {\n> vdev.streamOff();\n> }\n> }\n> \n> return ret;\n> \n> But you have both V4L2VideoDevice to start with streamOn() and\n> RkISP1Path to start with start() so I don't see it very neat unless\n> you create two different arrays or wrap the stat_ and param_\n> V4L2VideoDevice * in a RkISP1Path.\n> \n> Just adding labels to the existing seems easier :)\n\nWell one could do that but then we need to declare all variables before \nthe first goto. But as stated elsewhere this was a fun RFC but I will \ndrop it.\n\n> \n> >\n> > >\n> > > > +\t\tint ret;\n> > > > +\n> > > > +\t\t/* Allocate buffers for internal pipeline usage. */\n> > > > +\t\tret = allocateBuffers(camera);\n> > > > +\t\tif (ret)\n> > > > +\t\t\treturn ret;\n> > > > +\n> > > > +\t\tret = data->ipa_->start();\n> > > > +\t\tif (ret) {\n> > > > +\t\t\tLOG(RkISP1, Error)\n> > > > +\t\t\t\t<< \"Failed to start IPA \" << camera->id();\n> > > > +\t\t\tgoto err_allocate;\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tdata->frame_ = 0;\n> > > > +\n> > > > +\t\tret = param_->streamOn();\n> > > > +\t\tif (ret) {\n> > > > +\t\t\tLOG(RkISP1, Error)\n> > > > +\t\t\t\t<< \"Failed to start parameters \" << camera->id();\n> > > > +\t\t\tgoto err_ipa;\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tret = stat_->streamOn();\n> > > > +\t\tif (ret) {\n> > > > +\t\t\tLOG(RkISP1, Error)\n> > > > +\t\t\t\t<< \"Failed to start statistics \" << camera->id();\n> > > > +\t\t\tgoto err_param;\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tret = mainPath_.start();\n> > > > +\t\tif (ret)\n> > > > +\t\t\tgoto err_stat;\n> > > > +\n> > > > +\t\tret = selfPath_.start();\n> > > > +\t\tif (ret)\n> > > > +\t\t\tgoto err_main;\n> > > > +\n> > > > +\t\treturn 0;\n> > > > +\terr_main:\n> > > > \t\tmainPath_.stop();\n> > > > -\t\tparam_->streamOff();\n> > > > +\terr_stat:\n> > > > \t\tstat_->streamOff();\n> > > > +\terr_param:\n> > > > +\t\tparam_->streamOff();\n> > > > +\terr_ipa:\n> > > > \t\tdata->ipa_->stop();\n> > > > +\terr_allocate:\n> > > > \t\tfreeBuffers(camera);\n> > > > \t\treturn ret;\n> > > > -\t}\n> > > > +\t};\n> > > > +\n> > > > +\tret = startDevices(camera, data);\n> > > > +\tif (ret)\n> > > > +\t\treturn ret;\n> > > >\n> > > > \tactiveCamera_ = camera;\n> > > >\n> > > > --\n> > > > 2.28.0\n> > > >\n> > > > _______________________________________________\n> > > > libcamera-devel mailing list\n> > > > libcamera-devel@lists.libcamera.org\n> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund","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 E08A8C3B5B\n\tfor ;\n\tMon, 28 Sep 2020 21:50:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 716B760BF7;\n\tMon, 28 Sep 2020 23:50:15 +0200 (CEST)","from mail-lf1-x133.google.com (mail-lf1-x133.google.com\n\t[IPv6:2a00:1450:4864:20::133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 126A560366\n\tfor ;\n\tMon, 28 Sep 2020 23:50:14 +0200 (CEST)","by mail-lf1-x133.google.com with SMTP id z17so3037630lfi.12\n\tfor ;\n\tMon, 28 Sep 2020 14:50:14 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tm18sm193451ljc.81.2020.09.28.14.50.12\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 28 Sep 2020 14:50:12 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"omFT8tXW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=dJ07ieK+NGcJVVT8USvO4qRzp5lo0y9cv+H5JfQGKIA=;\n\tb=omFT8tXW0QHiJZTKi6HglPLDzJTtSlXwHBbkha+Rr++GBAeGq5e0AN8tnQOH+P7vyj\n\t3mHJbqvN79zF4YiU5DW8TkUZgwHPr2hqSYmvx0QnaeBgyF5F+JTOauO4f8JYHS2o0A/m\n\tO3VuNJSDB0YZVTYiNyt8bNSUiDgFszEj9G0etrpoLpUp6WF/0ao6h2MFo751GMV7QBUE\n\tHw+bse7nf9VHM52eigdSnHkjQV2h1vjcTCamIqd7Q2l1KRgAZPJPo9c5kwwXa1ngOOuD\n\tzz6blOOOvA/VI4Pgin29gZcDkC6jxHcQRI+rS4NNI/A49oTS8SNJH8LwZoDPUAUdvK1J\n\tcBUw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=dJ07ieK+NGcJVVT8USvO4qRzp5lo0y9cv+H5JfQGKIA=;\n\tb=Lszt8Gs/a2YUiXlqcy5RsXiY/UZ7DwV2guWro5ZuqxGQvEx9hS4zFrMawWDfEx/lmJ\n\tIsII3nUU+D9HMxW5WznxeTqA4JK/0oWlb47njwYFwF0kDEwsAUHzwCLK5R621eRjT28W\n\trlXGBBXsyX1czG2jJzZ/mcbTd7ZZ6kzqG+HhUYH8kxervHzTrn+QGSs8xtvplJytjOwf\n\tH6Vswu6QKl3v8JS3F6BwqCtxQZ1KequaC39kDmibZTAGyXNQCavSQ5yXP+N30cTKVtex\n\tREcprL6ZLeQIgMzCXZJbwzZMzQJS51j+bWTyRnNfKX7KmM+jP8uzeUyQO/MYeN0bUeGm\n\t5P2g==","X-Gm-Message-State":"AOAM5338TQgEe0KO6JDJxAHjjRuNeMYc2d8twTa5h8IKE1iozs76Up0X\n\t6isG59RxZH1xeHcrK7Xu04EidGxOcX/Tiw==","X-Google-Smtp-Source":"ABdhPJw9w21TSFGmy2wi8VwdEA14TIjv1IHNjRUc12u9Kb90acesPPGnG3ERDaRvtUFXlHDc2JT7BQ==","X-Received":"by 2002:a19:8906:: with SMTP id l6mr102614lfd.136.1601329813360; \n\tMon, 28 Sep 2020 14:50:13 -0700 (PDT)","Date":"Mon, 28 Sep 2020 23:50:12 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200928215012.GB1052036@oden.dyn.berto.se>","References":"<20200925014207.1455796-1-niklas.soderlund@ragnatech.se>\n\t<20200925014207.1455796-23-niklas.soderlund@ragnatech.se>\n\t<20200925153600.5bbotjnxzux6a6dr@uno.localdomain>\n\t<20200925165731.GI1757254@oden.dyn.berto.se>\n\t<20200928090903.agknvh5lme5f6hki@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200928090903.agknvh5lme5f6hki@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 22/22] libcamera: pipeline: rkisp1:\n\tUse cleanup error paths for start()","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=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12838,"web_url":"https://patchwork.libcamera.org/comment/12838/","msgid":"<20200928230135.GZ23539@pendragon.ideasonboard.com>","date":"2020-09-28T23:01:35","subject":"Re: [libcamera-devel] [PATCH v3 22/22] libcamera: pipeline: rkisp1:\n\tUse cleanup error paths for start()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Mon, Sep 28, 2020 at 11:50:12PM +0200, Niklas Söderlund wrote:\n> On 2020-09-28 11:09:03 +0200, Jacopo Mondi wrote:\n> > On Fri, Sep 25, 2020 at 06:57:31PM +0200, Niklas Söderlund wrote:\n> > > On 2020-09-25 17:36:00 +0200, Jacopo Mondi wrote:\n> > > > On Fri, Sep 25, 2020 at 03:42:07AM +0200, Niklas Söderlund wrote:\n> > > > > Use a lambda function to make the error paths a bit cleaner in start(),\n> > > > > no functional change.\n> > > > >\n> > > > > Signed-off-by: Niklas Söderlund \n> > > > > ---\n> > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 98 ++++++++++++------------\n> > > > > 1 file changed, 51 insertions(+), 47 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > index c6c2e3aa3dc6d0dc..34196af3057b14bb 100644\n> > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > @@ -827,58 +827,62 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> > > > > \tRkISP1CameraData *data = cameraData(camera);\n> > > > > \tint ret;\n> > > > >\n> > > > > -\t/* Allocate buffers for internal pipeline usage. */\n> > > > > -\tret = allocateBuffers(camera);\n> > > > > -\tif (ret)\n> > > > > -\t\treturn ret;\n> > > > > -\n> > > > > -\tret = data->ipa_->start();\n> > > > > -\tif (ret) {\n> > > > > -\t\tfreeBuffers(camera);\n> > > > > -\t\tLOG(RkISP1, Error)\n> > > > > -\t\t\t<< \"Failed to start IPA \" << camera->id();\n> > > > > -\t\treturn ret;\n> > > > > -\t}\n> > > > > -\n> > > > > -\tdata->frame_ = 0;\n> > > > > -\n> > > > > -\tret = param_->streamOn();\n> > > > > -\tif (ret) {\n> > > > > -\t\tdata->ipa_->stop();\n> > > > > -\t\tfreeBuffers(camera);\n> > > > > -\t\tLOG(RkISP1, Error)\n> > > > > -\t\t\t<< \"Failed to start parameters \" << camera->id();\n> > > > > -\t\treturn ret;\n> > > > > -\t}\n> > > > > -\n> > > > > -\tret = stat_->streamOn();\n> > > > > -\tif (ret) {\n> > > > > -\t\tparam_->streamOff();\n> > > > > -\t\tdata->ipa_->stop();\n> > > > > -\t\tfreeBuffers(camera);\n> > > > > -\t\tLOG(RkISP1, Error)\n> > > > > -\t\t\t<< \"Failed to start statistics \" << camera->id();\n> > > > > -\t\treturn ret;\n> > > > > -\t}\n> > > > > -\n> > > > > -\tret = mainPath_.start();\n> > > > > -\tif (ret) {\n> > > > > -\t\tparam_->streamOff();\n> > > > > -\t\tstat_->streamOff();\n> > > > > -\t\tdata->ipa_->stop();\n> > > > > -\t\tfreeBuffers(camera);\n> > > > > -\t\treturn ret;\n> > > > > -\t}\n> > > > > -\n> > > > > -\tret = selfPath_.start();\n> > > > > -\tif (ret) {\n> > > > > +\tauto startDevices = [this](Camera *camera, RkISP1CameraData *data) {\n> > > >\n> > > > Why a lamda function for a function that is only called for a single place as a\n> > > > regulr function ?\n> > >\n> > > It was suggested in review of an earlier version to make the code more\n> > > readable. But as the cover letter suggests this 22/22 should be thought\n> > > of as a RFC. I found it rather neat to make the code more readable.\n\nFor the record, I've suggested a cleanup mechanism, not a start\nmechanism :-)\n\n> > > > Also, shouldn't re-working the existing code to use labels be better ?\n> > >\n> > > Maybe, it's such a larger function so I thought it neater to crate a\n> > > separate \"block\" for the code needing error handling. If this was C I\n> > > would have created a helper function just to get it out but creating a\n> > > helper class function for this seems a bit much so a lambda was\n> > > somewhere in the middle.\n\nYou could still create a separate private function in the class.\n\n> > > Maybe I will break this out of this series so people don't spend all\n> > > review energy bikeshedding over this one ;-P\n> > \n> > Just for sake of discussion:\n> \n> :-)\n> \n> > It's mostly for the fact that it's called from a single point that\n> > means it's not necesasry to use lambda in my opinion (also, why do you\n> > need to capture [this] ?)\n> \n> I need [this] to allow the labmda to access the class member variables.\n>\n> > I would see a lambda fit if you were to iterate on an array of video\n> > devices and perform the same action on all of them like (not compiled\n> > or tested):\n> > \n> > std::array videoDevices{\n> > param_, stat_, main_, self_\n> > };\n> > \n> > /* Allocate buffers for internal pipeline usage. */\n> > ret = allocateBuffers(camera);\n> > if (ret)\n> > return ret;\n> > \n> > int ret;\n> > std::for_each(std::begin(videoDevices),\n> > std::end(videoDevices),\n> > [&ret](const V4L2VideoDevice &vdev) {\n> > ret = vdev.streamOn();\n> > if (ret)\n> > goto stopDevices;\n> > }\n> > }\n> > \n> > return 0;\n> > \n> > stopDevices:\n> > freeBuffers(camera);\n> > std::for_each(std::begin(videoDevices),\n> > std::end(videoDevices),\n> > [](const V4L2VideoDevice &vdev) {\n> > vdev.streamOff();\n> > }\n> > }\n> > \n> > return ret;\n> > \n> > But you have both V4L2VideoDevice to start with streamOn() and\n> > RkISP1Path to start with start() so I don't see it very neat unless\n> > you create two different arrays or wrap the stat_ and param_\n> > V4L2VideoDevice * in a RkISP1Path.\n> > \n> > Just adding labels to the existing seems easier :)\n> \n> Well one could do that but then we need to declare all variables before \n> the first goto. But as stated elsewhere this was a fun RFC but I will \n> drop it.\n\nIn its current form I agree with Jacopo, I'm not very fond of it. As a\ncleanup handler, however, I think it would be nice. Making the\nindividual cleanup functions idempotent (when they're not already) won't\nhave any noticeable impact on performances are they're not call in hot\npaths, and would make cleanup paths simpler in all pipeline handlers.\n\n> > > > > +\t\tint ret;\n> > > > > +\n> > > > > +\t\t/* Allocate buffers for internal pipeline usage. */\n> > > > > +\t\tret = allocateBuffers(camera);\n> > > > > +\t\tif (ret)\n> > > > > +\t\t\treturn ret;\n> > > > > +\n> > > > > +\t\tret = data->ipa_->start();\n> > > > > +\t\tif (ret) {\n> > > > > +\t\t\tLOG(RkISP1, Error)\n> > > > > +\t\t\t\t<< \"Failed to start IPA \" << camera->id();\n> > > > > +\t\t\tgoto err_allocate;\n> > > > > +\t\t}\n> > > > > +\n> > > > > +\t\tdata->frame_ = 0;\n> > > > > +\n> > > > > +\t\tret = param_->streamOn();\n> > > > > +\t\tif (ret) {\n> > > > > +\t\t\tLOG(RkISP1, Error)\n> > > > > +\t\t\t\t<< \"Failed to start parameters \" << camera->id();\n> > > > > +\t\t\tgoto err_ipa;\n> > > > > +\t\t}\n> > > > > +\n> > > > > +\t\tret = stat_->streamOn();\n> > > > > +\t\tif (ret) {\n> > > > > +\t\t\tLOG(RkISP1, Error)\n> > > > > +\t\t\t\t<< \"Failed to start statistics \" << camera->id();\n> > > > > +\t\t\tgoto err_param;\n> > > > > +\t\t}\n> > > > > +\n> > > > > +\t\tret = mainPath_.start();\n> > > > > +\t\tif (ret)\n> > > > > +\t\t\tgoto err_stat;\n> > > > > +\n> > > > > +\t\tret = selfPath_.start();\n> > > > > +\t\tif (ret)\n> > > > > +\t\t\tgoto err_main;\n> > > > > +\n> > > > > +\t\treturn 0;\n> > > > > +\terr_main:\n> > > > > \t\tmainPath_.stop();\n> > > > > -\t\tparam_->streamOff();\n> > > > > +\terr_stat:\n> > > > > \t\tstat_->streamOff();\n> > > > > +\terr_param:\n> > > > > +\t\tparam_->streamOff();\n> > > > > +\terr_ipa:\n> > > > > \t\tdata->ipa_->stop();\n> > > > > +\terr_allocate:\n> > > > > \t\tfreeBuffers(camera);\n> > > > > \t\treturn ret;\n> > > > > -\t}\n> > > > > +\t};\n> > > > > +\n> > > > > +\tret = startDevices(camera, data);\n> > > > > +\tif (ret)\n> > > > > +\t\treturn ret;\n> > > > >\n> > > > > \tactiveCamera_ = camera;\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 5A7CDC3B5C\n\tfor ;\n\tMon, 28 Sep 2020 23:02:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C028B603ED;\n\tTue, 29 Sep 2020 01:02:11 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2ED5D60394\n\tfor ;\n\tTue, 29 Sep 2020 01:02:10 +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 A3899A58;\n\tTue, 29 Sep 2020 01:02:09 +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=\"p/aPTGkf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601334129;\n\tbh=t1JCm06CXBFsIPd2ayEX+PRX9d1ZdNuJ1bt+t3P900s=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=p/aPTGkfuJ8yLI5Wh6q5+NVhgzMNacuTEVt5/UjB1V0E4yJsHXxvffMPxusxsfmbK\n\tVtc+6EHc39JREhQAi0LxiPjxtsscBEbcmklmTwe/yEC0P6BAjrfYSykfJXh4EheMAW\n\trkkIFtiid30z/0aIEWI8iwYL26zSB/D9EIf1G0NM=","Date":"Tue, 29 Sep 2020 02:01:35 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200928230135.GZ23539@pendragon.ideasonboard.com>","References":"<20200925014207.1455796-1-niklas.soderlund@ragnatech.se>\n\t<20200925014207.1455796-23-niklas.soderlund@ragnatech.se>\n\t<20200925153600.5bbotjnxzux6a6dr@uno.localdomain>\n\t<20200925165731.GI1757254@oden.dyn.berto.se>\n\t<20200928090903.agknvh5lme5f6hki@uno.localdomain>\n\t<20200928215012.GB1052036@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200928215012.GB1052036@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v3 22/22] libcamera: pipeline: rkisp1:\n\tUse cleanup error paths for start()","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]