[{"id":2521,"web_url":"https://patchwork.libcamera.org/comment/2521/","msgid":"<20190828160759.3c4pjh2mquogk4dw@uno.localdomain>","date":"2019-08-28T16:07:59","subject":"Re: [libcamera-devel] [PATCH 04/13] libcamera: pipeline: Add\n\tinitialization hook for CameraData","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas\n\nOn Wed, Aug 28, 2019 at 03:17:01AM +0200, Niklas Söderlund wrote:\n> Add a hook so CameraData can be initialized before a camera is exposed\n> to an application but after all resources and possibly an IPA have been\n> found.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/include/pipeline_handler.h | 2 ++\n> src/libcamera/pipeline_handler.cpp | 12 ++++++++++++\n> 2 files changed, 14 insertions(+)\n>\n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index 91d40ef40a465c4e..45f9457879c64363 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -43,6 +43,8 @@ public:\n> \t}\n> \tvirtual ~CameraData() {}\n>\n> +\tvirtual int initCameraData() { return 0; };\n> +\n\nAs this is CameraData::initCameraData() I think it could be just named\ninit().\n\nHowever, as I see it in use, and as you state in the commit message,\nthis seems to be related to later initialization, specifically when\nfor the IPA initialization. Do you see other uses for this? Or should\nthis be made an initIPA() operation ?\n\n\n> \tCamera *camera_;\n> \tPipelineHandler *pipe_;\n> \tstd::list queuedRequests_;\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 766fd496306ece9c..6b3ee0ed8f39676a 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -66,6 +66,12 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> * is needed for the camera both parameters should be set to 0.\n> */\n>\n> +/**\n> + * \\fn CameraData::initCameraData()\n> + * \\brief Hook to initialize the camera data\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n> /**\n> * \\var CameraData::camera_\n> * \\brief The camera related to this CameraData instance\n> @@ -462,6 +468,12 @@ void PipelineHandler::registerCamera(std::shared_ptr camera,\n> \t}\n>\n> \tdata->camera_ = camera.get();\n> +\tif (data->initCameraData()) {\n> +\t\tLOG(Pipeline, Warning) << \"Skipping \" << camera->name()\n> +\t\t\t\t << \" initialization camera data failed\";\n> +\t\treturn;\n> +\t}\n> +\n\nShouldn't the error be reported up and fail the whole cameras\nregistration?\n\nThanks\n j\n> \tcameraData_[camera.get()] = std::move(data);\n> \tcameras_.push_back(camera);\n> \tmanager_->addCamera(std::move(camera));\n> --\n> 2.22.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":"","Received":["from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A500C60BF6\n\tfor ;\n\tWed, 28 Aug 2019 18:06:27 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 1E131FF809;\n\tWed, 28 Aug 2019 16:06:26 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 28 Aug 2019 18:07:59 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190828160759.3c4pjh2mquogk4dw@uno.localdomain>","References":"<20190828011710.32128-1-niklas.soderlund@ragnatech.se>\n\t<20190828011710.32128-5-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"6rbd5ddovtupq5zp\"","Content-Disposition":"inline","In-Reply-To":"<20190828011710.32128-5-niklas.soderlund@ragnatech.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 04/13] libcamera: pipeline: Add\n\tinitialization hook for CameraData","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 28 Aug 2019 16:06:28 -0000"}},{"id":2555,"web_url":"https://patchwork.libcamera.org/comment/2555/","msgid":"<20190829191156.GC8479@bigcity.dyn.berto.se>","date":"2019-08-29T19:11:56","subject":"Re: [libcamera-devel] [PATCH 04/13] libcamera: pipeline: Add\n\tinitialization hook for CameraData","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your feedback.\n\nOn 2019-08-28 18:07:59 +0200, Jacopo Mondi wrote:\n> Hi Niklas\n> \n> On Wed, Aug 28, 2019 at 03:17:01AM +0200, Niklas Söderlund wrote:\n> > Add a hook so CameraData can be initialized before a camera is exposed\n> > to an application but after all resources and possibly an IPA have been\n> > found.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/include/pipeline_handler.h | 2 ++\n> > src/libcamera/pipeline_handler.cpp | 12 ++++++++++++\n> > 2 files changed, 14 insertions(+)\n> >\n> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> > index 91d40ef40a465c4e..45f9457879c64363 100644\n> > --- a/src/libcamera/include/pipeline_handler.h\n> > +++ b/src/libcamera/include/pipeline_handler.h\n> > @@ -43,6 +43,8 @@ public:\n> > \t}\n> > \tvirtual ~CameraData() {}\n> >\n> > +\tvirtual int initCameraData() { return 0; };\n> > +\n> \n> As this is CameraData::initCameraData() I think it could be just named\n> init().\n> \n> However, as I see it in use, and as you state in the commit message,\n> this seems to be related to later initialization, specifically when\n> for the IPA initialization. Do you see other uses for this? Or should\n> this be made an initIPA() operation ?\n\nI see no other use for it and the only call site for this should be in \npipeline_handler.cpp as the intention is to initialize the camera data \nafter all resources (including IPA) have been found.\n\nI do agree with you that initCameraData() was a bad name and will rename \nit to init().\n\n> \n> \n> > \tCamera *camera_;\n> > \tPipelineHandler *pipe_;\n> > \tstd::list queuedRequests_;\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index 766fd496306ece9c..6b3ee0ed8f39676a 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -66,6 +66,12 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> > * is needed for the camera both parameters should be set to 0.\n> > */\n> >\n> > +/**\n> > + * \\fn CameraData::initCameraData()\n> > + * \\brief Hook to initialize the camera data\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +\n> > /**\n> > * \\var CameraData::camera_\n> > * \\brief The camera related to this CameraData instance\n> > @@ -462,6 +468,12 @@ void PipelineHandler::registerCamera(std::shared_ptr camera,\n> > \t}\n> >\n> > \tdata->camera_ = camera.get();\n> > +\tif (data->initCameraData()) {\n> > +\t\tLOG(Pipeline, Warning) << \"Skipping \" << camera->name()\n> > +\t\t\t\t << \" initialization camera data failed\";\n> > +\t\treturn;\n> > +\t}\n> > +\n> \n> Shouldn't the error be reported up and fail the whole cameras\n> registration?\n\nTHe camera is not registered by the system if this fails, as \nCameraManager::addCamera() is never called.\n\nWe could propagate the error to an upper layer (pipeline handler \nmatch()), but what would it do with it? Fail the whole pipeline or print \nan error message and try to work with the (potential) other cameras \nhandled by the same pipeline?\n\nI think the second option is the most sane as it will make the error \nknown while still providing as much functionality as possible for the \nuser. I'm open to discuss this tho if we think the whole pipeline should \nfail instead.\n\n> \n> Thanks\n> j\n> > \tcameraData_[camera.get()] = std::move(data);\n> > \tcameras_.push_back(camera);\n> > \tmanager_->addCamera(std::move(camera));\n> > --\n> > 2.22.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":"","Received":["from mail-lf1-x135.google.com (mail-lf1-x135.google.com\n\t[IPv6:2a00:1450:4864:20::135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7ECE060BE5\n\tfor ;\n\tThu, 29 Aug 2019 21:11:58 +0200 (CEST)","by mail-lf1-x135.google.com with SMTP id v16so3360393lfg.11\n\tfor ;\n\tThu, 29 Aug 2019 12:11:58 -0700 (PDT)","from localhost (h-177-236.A463.priv.bahnhof.se. [217.31.177.236])\n\tby smtp.gmail.com with ESMTPSA id\n\tj3sm518375lfp.34.2019.08.29.12.11.57\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 29 Aug 2019 12:11:57 -0700 (PDT)"],"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\t:user-agent; bh=8483WQfwjIA44fcinryWxf0pO3sgHW44wu/zrZiQWvo=;\n\tb=IncPCpzGihRxjEQQi7VVTbuNeb3oM417WsP/SApGh6C2AvuwpaoJXADv9MXe3XmiZY\n\tZickcvmRm4IMAWTKs3sR2RTkiXw/B0b77xUgu16sEqyC9qzRuUH/bDrig2Nl2a4KcwFL\n\tUZniM2xp2oCty6wO0VgVsVWvqIXAunEF8wcAZ+auxXZi/HOdoopCs98UROEoN90qvCVI\n\tokiYBCJIUHUdrXgLkA4iM4N1r84WiF0Ebq8YuSoY47psWbAbJVr66k8f3ZcyLUZulJpg\n\tLxRnezC25/rFloUJ7s5LX9qd5JnuZGCmSTUeffagmQY2ZDSWHfF7Bsdlohbi7b6qXQAy\n\tegUA==","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:user-agent;\n\tbh=8483WQfwjIA44fcinryWxf0pO3sgHW44wu/zrZiQWvo=;\n\tb=RkoBkLO2QSDi0HZdcI63w0xghUq0DRnmXOb8ctHL1qV56zPQRhsYfVRk/zywTT4xRb\n\tsiSqzY5I2Ey1VTXjBwcZ/KcxlNiQq96dAMQdWukhJovAedSNz9x5LbDAqanUk2Gu/hYt\n\tovtWv9EuWsKAKtsw4MP9WmJlv2ghSaHcXrOGihb1SMwuiZk+coPVjDDTZSARt9IenCu9\n\tos2yI/M4cUf0q/hpZ93WZy7sEIanEg38kPQNCJ+uKU0+kETmZorUxNks28WSEMtK7p+a\n\t7q3PInx1tA3oNtIyGnB8y+wDldQ74WHTRs4ybVkg0k++Nsq7cvk0cBNULnjX2LNOf1Ig\n\tHGCw==","X-Gm-Message-State":"APjAAAV+hAjsJ4f+YRfkQebioQcd2qzjPTS3RJ4yxZZl1mUT9g/Oi2Ex\n\tl2zHXfC8IQ0as/85gO8baUfWxQ==","X-Google-Smtp-Source":"APXvYqycl4x1ha5Eu6CdmxpxOTsGZ+YB3xN1yE8iUJSj+YiWNP8wPxWUW3BR0xdfVPiXoUNtkn4g3g==","X-Received":"by 2002:a19:c14a:: with SMTP id r71mr7044373lff.55.1567105917938;\n\tThu, 29 Aug 2019 12:11:57 -0700 (PDT)","Date":"Thu, 29 Aug 2019 21:11:56 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190829191156.GC8479@bigcity.dyn.berto.se>","References":"<20190828011710.32128-1-niklas.soderlund@ragnatech.se>\n\t<20190828011710.32128-5-niklas.soderlund@ragnatech.se>\n\t<20190828160759.3c4pjh2mquogk4dw@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190828160759.3c4pjh2mquogk4dw@uno.localdomain>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH 04/13] libcamera: pipeline: Add\n\tinitialization hook for CameraData","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 29 Aug 2019 19:11:58 -0000"}}]