[{"id":1760,"web_url":"https://patchwork.libcamera.org/comment/1760/","msgid":"<20190604121328.GB7812@pendragon.ideasonboard.com>","date":"2019-06-04T12:13:28","subject":"Re: [libcamera-devel] [PATCH v2 10/10] libcamera: pipeline: vimc:\n\tadd dummy IPA","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Mon, Jun 03, 2019 at 07:16:37PM -0400, Paul Elder wrote:\n> Make the vimc pipeline handler get the dummy IPA, to show how an IPA can\n> be acquired by a pipeline handler.\n> \n> Signed-off-by: Paul Elder \n> ---\n> Changes in v2:\n> - save IPA to pipeline data\n> - no IPA is fatal error\n> \n> src/libcamera/pipeline/vimc.cpp | 11 +++++++++++\n> 1 file changed, 11 insertions(+)\n> \n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 78feeb8..f000c21 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -10,10 +10,12 @@\n> \n> #include \n> #include \n> +#include \n\nsort() :-)\n\n> #include \n> #include \n> \n> #include \"device_enumerator.h\"\n> +#include \"ipa_manager.h\"\n> #include \"log.h\"\n> #include \"media_device.h\"\n> #include \"pipeline_handler.h\"\n> @@ -252,6 +254,15 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n> \tif (!media)\n> \t\treturn false;\n> \n> +\tIPAManager *ipaManager = IPAManager::instance();\n> +\tipa_ = ipaManager->createIPA(this);\n\nYou could also write this\n\n\tipa_ = IPAManager::instance()->createIPA(this);\n\n> +\tif (ipa_ == nullptr) {\n> +\t\tLOG(VIMC, Error) << \"no matching IPA found\";\n> +\t\treturn false;\n\nAs the IPA isn't required yet, should we make this a non-fatal error and\ndowngrade the message to a warning ? My review of v1 wasn't very clear,\nI meant to ask if it should really be an error, or just a warning. What\ndo you think ?\n\n> +\t} else {\n> +\t\tipa_->init();\n\nI'm tempted to move the init() call to createIPA(), what do you think ?\n\n> +\t}\n> +\n> \tstd::unique_ptr data = utils::make_unique(this);\n> \n> \t/* Locate and open the capture video node. */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CEEA463162\n\tfor ;\n\tTue, 4 Jun 2019 14:13:41 +0200 (CEST)","from pendragon.ideasonboard.com (unknown\n\t[IPv6:2a02:2788:668:163:5bb7:9f6c:564c:d55e])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7D9745D;\n\tTue, 4 Jun 2019 14:13:41 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1559650421;\n\tbh=LeHxIFHcdqH5nIK28eGImyMiPo7nQuZ2zh0YY80R1GQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=D7sV0Exl7naqKZoKj/c9kNKGgrAJNIIjXuBw/70LG9l1s5O4Vy2nk+C0VpGmfCK5d\n\tkW20SMKwC1J8IHVfRU84Ayy/Y92zEk+9z2YCzsj5e77CstO6FdIQreLJ/pov10knzE\n\tW7KYe4Wc4Y/2QAWwqKx3rURIXSzqc9erI0NDHw1E=","Date":"Tue, 4 Jun 2019 15:13:28 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190604121328.GB7812@pendragon.ideasonboard.com>","References":"<20190603231637.28554-1-paul.elder@ideasonboard.com>\n\t<20190603231637.28554-11-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190603231637.28554-11-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 10/10] libcamera: pipeline: vimc:\n\tadd dummy IPA","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":"Tue, 04 Jun 2019 12:13:42 -0000"}},{"id":1762,"web_url":"https://patchwork.libcamera.org/comment/1762/","msgid":"<20190604155343.GM32191@localhost.localdomain>","date":"2019-06-04T15:53:43","subject":"Re: [libcamera-devel] [PATCH v2 10/10] libcamera: pipeline: vimc:\n\tadd dummy IPA","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Tue, Jun 04, 2019 at 03:13:28PM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n\nThank you for the review.\n\n> On Mon, Jun 03, 2019 at 07:16:37PM -0400, Paul Elder wrote:\n> > Make the vimc pipeline handler get the dummy IPA, to show how an IPA can\n> > be acquired by a pipeline handler.\n> > \n> > Signed-off-by: Paul Elder \n> > ---\n> > Changes in v2:\n> > - save IPA to pipeline data\n> > - no IPA is fatal error\n> > \n> > src/libcamera/pipeline/vimc.cpp | 11 +++++++++++\n> > 1 file changed, 11 insertions(+)\n> > \n> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index 78feeb8..f000c21 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -10,10 +10,12 @@\n> > \n> > #include \n> > #include \n> > +#include \n> \n> sort() :-)\n> \n> > #include \n> > #include \n> > \n> > #include \"device_enumerator.h\"\n> > +#include \"ipa_manager.h\"\n> > #include \"log.h\"\n> > #include \"media_device.h\"\n> > #include \"pipeline_handler.h\"\n> > @@ -252,6 +254,15 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n> > \tif (!media)\n> > \t\treturn false;\n> > \n> > +\tIPAManager *ipaManager = IPAManager::instance();\n> > +\tipa_ = ipaManager->createIPA(this);\n> \n> You could also write this\n> \n> \tipa_ = IPAManager::instance()->createIPA(this);\n> \n> > +\tif (ipa_ == nullptr) {\n> > +\t\tLOG(VIMC, Error) << \"no matching IPA found\";\n> > +\t\treturn false;\n> \n> As the IPA isn't required yet, should we make this a non-fatal error and\n> downgrade the message to a warning ? My review of v1 wasn't very clear,\n> I meant to ask if it should really be an error, or just a warning. What\n> do you think ?\n\nI don't think it should be a fatal error, since it isn't required.\n\n> > +\t} else {\n> > +\t\tipa_->init();\n> \n> I'm tempted to move the init() call to createIPA(), what do you think ?\n\nI'm not sure. I can't think of much reason for or against.\n\nI suppose, createIPA() is meant to return the IPAInterface as-is\ncreated by the factory, with no other calls to it, so init() should not\nbe called from createIPA().\n\n> > +\t}\n> > +\n> > \tstd::unique_ptr data = utils::make_unique(this);\n> > \n> > \t/* Locate and open the capture video node. */\n> \n\nThanks,\n\nPaul","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 97A75638BE\n\tfor ;\n\tTue, 4 Jun 2019 17:53:51 +0200 (CEST)","from localhost.localdomain (unknown [96.44.9.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B60DA2D1;\n\tTue, 4 Jun 2019 17:53:50 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1559663631;\n\tbh=MAFWZYn0RLlwKZzTEchGg/gTjOT/IbFDY66ktvTjUEI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=T3R/SJWGDssczL1Kb/L2/kvY/tZcHOTcnd0WWUd7BRTdvzoFp0+JXZW6hUJxWP09U\n\tvJ3D0pa7/O8hSGNrWdP6WBvlBKU/gH3J/8AV44CR7RTX/QbjlXcrNuGl0MDD7Eag3t\n\tMQFwRtE3lFcGqYFro/TC/2f+tmqZB6HRkLC17wxc=","Date":"Tue, 4 Jun 2019 11:53:43 -0400","From":"Paul Elder ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190604155343.GM32191@localhost.localdomain>","References":"<20190603231637.28554-1-paul.elder@ideasonboard.com>\n\t<20190603231637.28554-11-paul.elder@ideasonboard.com>\n\t<20190604121328.GB7812@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20190604121328.GB7812@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 10/10] libcamera: pipeline: vimc:\n\tadd dummy IPA","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":"Tue, 04 Jun 2019 15:53:51 -0000"}},{"id":1765,"web_url":"https://patchwork.libcamera.org/comment/1765/","msgid":"<20190604164735.GB21733@pendragon.ideasonboard.com>","date":"2019-06-04T16:47:35","subject":"Re: [libcamera-devel] [PATCH v2 10/10] libcamera: pipeline: vimc:\n\tadd dummy IPA","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nOn Tue, Jun 04, 2019 at 11:53:43AM -0400, Paul Elder wrote:\n> On Tue, Jun 04, 2019 at 03:13:28PM +0300, Laurent Pinchart wrote:\n> > On Mon, Jun 03, 2019 at 07:16:37PM -0400, Paul Elder wrote:\n> >> Make the vimc pipeline handler get the dummy IPA, to show how an IPA can\n> >> be acquired by a pipeline handler.\n> >> \n> >> Signed-off-by: Paul Elder \n> >> ---\n> >> Changes in v2:\n> >> - save IPA to pipeline data\n> >> - no IPA is fatal error\n> >> \n> >> src/libcamera/pipeline/vimc.cpp | 11 +++++++++++\n> >> 1 file changed, 11 insertions(+)\n> >> \n> >> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> >> index 78feeb8..f000c21 100644\n> >> --- a/src/libcamera/pipeline/vimc.cpp\n> >> +++ b/src/libcamera/pipeline/vimc.cpp\n> >> @@ -10,10 +10,12 @@\n> >> \n> >> #include \n> >> #include \n> >> +#include \n> > \n> > sort() :-)\n> > \n> >> #include \n> >> #include \n> >> \n> >> #include \"device_enumerator.h\"\n> >> +#include \"ipa_manager.h\"\n> >> #include \"log.h\"\n> >> #include \"media_device.h\"\n> >> #include \"pipeline_handler.h\"\n> >> @@ -252,6 +254,15 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n> >> \tif (!media)\n> >> \t\treturn false;\n> >> \n> >> +\tIPAManager *ipaManager = IPAManager::instance();\n> >> +\tipa_ = ipaManager->createIPA(this);\n> > \n> > You could also write this\n> > \n> > \tipa_ = IPAManager::instance()->createIPA(this);\n> > \n> >> +\tif (ipa_ == nullptr) {\n> >> +\t\tLOG(VIMC, Error) << \"no matching IPA found\";\n> >> +\t\treturn false;\n> > \n> > As the IPA isn't required yet, should we make this a non-fatal error and\n> > downgrade the message to a warning ? My review of v1 wasn't very clear,\n> > I meant to ask if it should really be an error, or just a warning. What\n> > do you think ?\n> \n> I don't think it should be a fatal error, since it isn't required.\n\nI agree.\n\n> >> +\t} else {\n> >> +\t\tipa_->init();\n> > \n> > I'm tempted to move the init() call to createIPA(), what do you think ?\n> \n> I'm not sure. I can't think of much reason for or against.\n> \n> I suppose, createIPA() is meant to return the IPAInterface as-is\n> created by the factory, with no other calls to it, so init() should not\n> be called from createIPA().\n\nIt makes sense. The init() method may later receive parameters specific\nto the pipeline handler. They could be passed to createIPA(), but we can\ndecide about that later.\n\n> >> +\t}\n> >> +\n> >> \tstd::unique_ptr data = utils::make_unique(this);\n> >> \n> >> \t/* Locate and open the capture video node. */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 42BEA60C2A\n\tfor ;\n\tTue, 4 Jun 2019 18:47:49 +0200 (CEST)","from pendragon.ideasonboard.com (unknown\n\t[IPv6:2a02:2788:668:163:5bb7:9f6c:564c:d55e])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CD64B2D1;\n\tTue, 4 Jun 2019 18:47:48 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1559666868;\n\tbh=bwXbogwpHTvMYx7Yf7CAIe+3IaQOy4+BNYFESgUTTq4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XM1fgEQhzAnS6/uSM5MYjCwCd111vGgpHHAhwGPtyaG0WZSjV4AA9NzzC7rMdvDcw\n\tjn+d6+WUbeTUS7vcvhuoUPVh6GXBPG0D7/zR/+7jvSo6Jtjvn4mVy9ZXEH6BFyWXVw\n\tlxawmz4qDjuG3AiViO3HATe3jEkKW6GgGTIKdIWk=","Date":"Tue, 4 Jun 2019 19:47:35 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190604164735.GB21733@pendragon.ideasonboard.com>","References":"<20190603231637.28554-1-paul.elder@ideasonboard.com>\n\t<20190603231637.28554-11-paul.elder@ideasonboard.com>\n\t<20190604121328.GB7812@pendragon.ideasonboard.com>\n\t<20190604155343.GM32191@localhost.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190604155343.GM32191@localhost.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 10/10] libcamera: pipeline: vimc:\n\tadd dummy IPA","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":"Tue, 04 Jun 2019 16:47:49 -0000"}},{"id":1767,"web_url":"https://patchwork.libcamera.org/comment/1767/","msgid":"<20190604170050.wgbq5nxiyt4rxt56@uno.localdomain>","date":"2019-06-04T17:00:50","subject":"Re: [libcamera-devel] [PATCH v2 10/10] libcamera: pipeline: vimc:\n\tadd dummy IPA","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello,\n sorry to jump in on a non-design minor issue, but I've just noticed while\nreading the patch that:\n\nOn Tue, Jun 04, 2019 at 11:53:43AM -0400, Paul Elder wrote:\n> Hi Laurent,\n>\n> On Tue, Jun 04, 2019 at 03:13:28PM +0300, Laurent Pinchart wrote:\n> > Hi Paul,\n> >\n> > Thank you for the patch.\n>\n> Thank you for the review.\n>\n> > On Mon, Jun 03, 2019 at 07:16:37PM -0400, Paul Elder wrote:\n> > > Make the vimc pipeline handler get the dummy IPA, to show how an IPA can\n> > > be acquired by a pipeline handler.\n> > >\n> > > Signed-off-by: Paul Elder \n> > > ---\n> > > Changes in v2:\n> > > - save IPA to pipeline data\n> > > - no IPA is fatal error\n> > >\n> > > src/libcamera/pipeline/vimc.cpp | 11 +++++++++++\n> > > 1 file changed, 11 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > > index 78feeb8..f000c21 100644\n> > > --- a/src/libcamera/pipeline/vimc.cpp\n> > > +++ b/src/libcamera/pipeline/vimc.cpp\n> > > @@ -10,10 +10,12 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> >\n> > sort() :-)\n> >\n> > > #include \n> > > #include \n> > >\n> > > #include \"device_enumerator.h\"\n> > > +#include \"ipa_manager.h\"\n> > > #include \"log.h\"\n> > > #include \"media_device.h\"\n> > > #include \"pipeline_handler.h\"\n> > > @@ -252,6 +254,15 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n> > > \tif (!media)\n> > > \t\treturn false;\n> > >\n> > > +\tIPAManager *ipaManager = IPAManager::instance();\n> > > +\tipa_ = ipaManager->createIPA(this);\n> >\n> > You could also write this\n> >\n> > \tipa_ = IPAManager::instance()->createIPA(this);\n> >\n> > > +\tif (ipa_ == nullptr) {\n> > > +\t\tLOG(VIMC, Error) << \"no matching IPA found\";\n> > > +\t\treturn false;\n> >\n> > As the IPA isn't required yet, should we make this a non-fatal error and\n> > downgrade the message to a warning ? My review of v1 wasn't very clear,\n> > I meant to ask if it should really be an error, or just a warning. What\n> > do you think ?\n>\n> I don't think it should be a fatal error, since it isn't required.\n>\n> > > +\t} else {\n> > > +\t\tipa_->init();\n\nIf you keep init() as a separate call, and fail if (!ipa), you should\ndrop the else here.\n\n\n\n> >\n> > I'm tempted to move the init() call to createIPA(), what do you think ?\n>\n> I'm not sure. I can't think of much reason for or against.\n>\n> I suppose, createIPA() is meant to return the IPAInterface as-is\n> created by the factory, with no other calls to it, so init() should not\n> be called from createIPA().\n>\n> > > +\t}\n> > > +\n> > > \tstd::unique_ptr data = utils::make_unique(this);\n> > >\n> > > \t/* Locate and open the capture video node. */\n> >\n>\n> Thanks,\n>\n> Paul\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 relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B045060C2A\n\tfor ;\n\tTue, 4 Jun 2019 18:59:39 +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 relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 10F831BF207;\n\tTue, 4 Jun 2019 16:59:38 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 4 Jun 2019 19:00:50 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190604170050.wgbq5nxiyt4rxt56@uno.localdomain>","References":"<20190603231637.28554-1-paul.elder@ideasonboard.com>\n\t<20190603231637.28554-11-paul.elder@ideasonboard.com>\n\t<20190604121328.GB7812@pendragon.ideasonboard.com>\n\t<20190604155343.GM32191@localhost.localdomain>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"ehgicfxzgici5627\"","Content-Disposition":"inline","In-Reply-To":"<20190604155343.GM32191@localhost.localdomain>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 10/10] libcamera: pipeline: vimc:\n\tadd dummy IPA","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":"Tue, 04 Jun 2019 16:59:39 -0000"}}]