[{"id":11181,"web_url":"https://patchwork.libcamera.org/comment/11181/","msgid":"<20200704215103.GH10773@pendragon.ideasonboard.com>","date":"2020-07-04T21:51:03","subject":"Re: [libcamera-devel] [PATCH v3 18/22] libcamera: rkisp1: Fill\n\tstride and frameSize at config validation","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 Sat, Jul 04, 2020 at 10:31:36PM +0900, Paul Elder wrote:\n> Fill the stride and frameSize fields of the StreamConfiguration at\n> configuration validation time instead of at camera configuration time.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> New in v3\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 3c3f3f3..3ac7b3c 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -535,6 +535,10 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \n> \tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n> \n> +\tconst PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n> +\tcfg.stride = info.stride(cfg.size.width, 0);\n> +\tcfg.frameSize = info.frameSize(cfg.size);\n\nSame comment as for patches 16/22 and 17/22.\n\nThe alternative is to use V4L2VideoDevice::tryFormat() in here. Jacopo,\nany opinion on that ?\n\n> +\n> \treturn status;\n> }\n> \n> @@ -683,7 +687,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \t\treturn ret;\n> \n> \tcfg.setStream(&data->stream_);\n> -\tcfg.stride = outputFormat.planes[0].bpl;\n> \n> \treturn 0;\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 35AB8BD790\n\tfor ;\n\tSat, 4 Jul 2020 21:51:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 01A5C60C50;\n\tSat, 4 Jul 2020 23:51:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C9FF7609C7\n\tfor ;\n\tSat, 4 Jul 2020 23:51:07 +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 65D68296;\n\tSat, 4 Jul 2020 23:51:07 +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=\"NVOnn8aP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593899467;\n\tbh=ybm1zUX2LltWWlIlwx5UWHOpsS3mJIC54V2zOgCPWB4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NVOnn8aPd+KMZHIS2q6zm3uiNIMbjV+ZCKhA7Xlk7QaLv9ONIxqzSbMLotwI8U++5\n\t/5o3HmIZ/VQfegLhJ4Og58LeEM/D7VhVMsz55C86GCGlX6IcsjAUgKDd2xnF0pAcMB\n\txfI+F/sQSJI+wws3PEg93YZ/J6MWLYmfXWueRnbU=","Date":"Sun, 5 Jul 2020 00:51:03 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"<20200704215103.GH10773@pendragon.ideasonboard.com>","References":"<20200704133140.1738660-1-paul.elder@ideasonboard.com>\n\t<20200704133140.1738660-19-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200704133140.1738660-19-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 18/22] libcamera: rkisp1: Fill\n\tstride and frameSize at config validation","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":11223,"web_url":"https://patchwork.libcamera.org/comment/11223/","msgid":"<20200706151654.ymllb3pgnlk4ykmw@uno.localdomain>","date":"2020-07-06T15:16:54","subject":"Re: [libcamera-devel] [PATCH v3 18/22] libcamera: rkisp1: Fill\n\tstride and frameSize at config validation","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Sun, Jul 05, 2020 at 12:51:03AM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n>\n> Thank you for the patch.\n>\n> On Sat, Jul 04, 2020 at 10:31:36PM +0900, Paul Elder wrote:\n> > Fill the stride and frameSize fields of the StreamConfiguration at\n> > configuration validation time instead of at camera configuration time.\n> >\n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > New in v3\n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 ++++-\n> > 1 file changed, 4 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 3c3f3f3..3ac7b3c 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -535,6 +535,10 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >\n> > \tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n> >\n> > +\tconst PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n> > +\tcfg.stride = info.stride(cfg.size.width, 0);\n> > +\tcfg.frameSize = info.frameSize(cfg.size);\n>\n> Same comment as for patches 16/22 and 17/22.\n>\n> The alternative is to use V4L2VideoDevice::tryFormat() in here. Jacopo,\n> any opinion on that ?\n>\n\nI'm noticing that calculating the stride statically is becoming\ncumbersome...\n\nTrue that we have a few pipeline handlers where we have to reverse\nthose information from drivers, future pipeline handlers will be\ninstrumented from the beginning with those information in place, so it\nwill be less a concern.\n\nThe alternative, as you said, is to ask the video device, as that's\nindeed the less error-prone solution, as the only way to get it wrong\nis a driver bug :)\n\nRecently I've been working to remove the assignment of streams to\nvideodevices from the IPU3 pipeline handler, not because I see that as\nsomething frowned upon from an API perspective or for performances\nreason, but mostly because there's no infrastructure to keep that\nassignment in place between validate() and configure() and if every\npipeline handler has to implement its own method to do that, then I\nwould consider this approach \"wrong\" as it could easily become a\nmaintainance burden I would not like to encourage.\n\nI'm not sure I want to go there, but from the very beginning we assume\nstreams gets assigned to StreamConfigurations only after\nCamera::configure() has been called. I can't find in the documentation\nany strong rationale for this, and we have absolutely no protection in\nplace at the moment to prevent application from taking a pointer from\na StreamConfiguration before it gets assigned (documentation apart).\n\nFrom a V4L2 point of view, I think there's nothing preventing us from\ncalling try fmt at validation time. The device is open and there's no\nother requirements afaict.\n\nLong story short: going to the video device would be best, otherwise\nthe same information shall be duplicated in multiple places. Even\nif I agree we should try to move more knowledge of the platform to\nuserspace, the device requirements in terms of padding and alignement\nseems to me among the less sensible information to record outside of\nthe kernel. At the same time, I would not like to see custom vector of\nstreams filled in at validate() and retrieved at configure() time.\n\nWould calling StreamConfiguration::setStream() during validation be\nthat nasty in your opinion?\n\nThanks\n j\n\n> > +\n> > \treturn status;\n> > }\n> >\n> > @@ -683,7 +687,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > \t\treturn ret;\n> >\n> > \tcfg.setStream(&data->stream_);\n> > -\tcfg.stride = outputFormat.planes[0].bpl;\n> >\n> > \treturn 0;\n> > }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 358F6BD792\n\tfor ;\n\tMon, 6 Jul 2020 15:13:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B6D9760E01;\n\tMon, 6 Jul 2020 17:13:22 +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 4F1E0603AA\n\tfor ;\n\tMon, 6 Jul 2020 17:13:22 +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 relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 3949F1C0012;\n\tMon, 6 Jul 2020 15:13:20 +0000 (UTC)"],"X-Originating-IP":"79.34.235.173","Date":"Mon, 6 Jul 2020 17:16:54 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20200706151654.ymllb3pgnlk4ykmw@uno.localdomain>","References":"<20200704133140.1738660-1-paul.elder@ideasonboard.com>\n\t<20200704133140.1738660-19-paul.elder@ideasonboard.com>\n\t<20200704215103.GH10773@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200704215103.GH10773@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 18/22] libcamera: rkisp1: Fill\n\tstride and frameSize at config validation","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":11224,"web_url":"https://patchwork.libcamera.org/comment/11224/","msgid":"<20200706162325.GD19803@pendragon.ideasonboard.com>","date":"2020-07-06T16:23:25","subject":"Re: [libcamera-devel] [PATCH v3 18/22] libcamera: rkisp1: Fill\n\tstride and frameSize at config validation","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 05:16:54PM +0200, Jacopo Mondi wrote:\n> On Sun, Jul 05, 2020 at 12:51:03AM +0300, Laurent Pinchart wrote:\n> > On Sat, Jul 04, 2020 at 10:31:36PM +0900, Paul Elder wrote:\n> > > Fill the stride and frameSize fields of the StreamConfiguration at\n> > > configuration validation time instead of at camera configuration time.\n> > >\n> > > Signed-off-by: Paul Elder \n> > >\n> > > ---\n> > > New in v3\n> > > ---\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 ++++-\n> > > 1 file changed, 4 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 3c3f3f3..3ac7b3c 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -535,6 +535,10 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > >\n> > > \tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n> > >\n> > > +\tconst PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n> > > +\tcfg.stride = info.stride(cfg.size.width, 0);\n> > > +\tcfg.frameSize = info.frameSize(cfg.size);\n> >\n> > Same comment as for patches 16/22 and 17/22.\n> >\n> > The alternative is to use V4L2VideoDevice::tryFormat() in here. Jacopo,\n> > any opinion on that ?\n> \n> I'm noticing that calculating the stride statically is becoming\n> cumbersome...\n> \n> True that we have a few pipeline handlers where we have to reverse\n> those information from drivers, future pipeline handlers will be\n> instrumented from the beginning with those information in place, so it\n> will be less a concern.\n> \n> The alternative, as you said, is to ask the video device, as that's\n> indeed the less error-prone solution, as the only way to get it wrong\n> is a driver bug :)\n> \n> Recently I've been working to remove the assignment of streams to\n> videodevices from the IPU3 pipeline handler, not because I see that as\n> something frowned upon from an API perspective or for performances\n> reason, but mostly because there's no infrastructure to keep that\n> assignment in place between validate() and configure() and if every\n> pipeline handler has to implement its own method to do that, then I\n> would consider this approach \"wrong\" as it could easily become a\n> maintainance burden I would not like to encourage.\n> \n> I'm not sure I want to go there, but from the very beginning we assume\n> streams gets assigned to StreamConfigurations only after\n> Camera::configure() has been called. I can't find in the documentation\n> any strong rationale for this, and we have absolutely no protection in\n> place at the moment to prevent application from taking a pointer from\n> a StreamConfiguration before it gets assigned (documentation apart).\n> \n> From a V4L2 point of view, I think there's nothing preventing us from\n> calling try fmt at validation time. The device is open and there's no\n> other requirements afaict.\n> \n> Long story short: going to the video device would be best, otherwise\n> the same information shall be duplicated in multiple places. Even\n> if I agree we should try to move more knowledge of the platform to\n> userspace, the device requirements in terms of padding and alignement\n> seems to me among the less sensible information to record outside of\n> the kernel. At the same time, I would not like to see custom vector of\n> streams filled in at validate() and retrieved at configure() time.\n> \n> Would calling StreamConfiguration::setStream() during validation be\n> that nasty in your opinion?\n\nI don't mind, and I'm working on removing setStream() anyway :-)\n\n> > > +\n> > > \treturn status;\n> > > }\n> > >\n> > > @@ -683,7 +687,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > > \t\treturn ret;\n> > >\n> > > \tcfg.setStream(&data->stream_);\n> > > -\tcfg.stride = outputFormat.planes[0].bpl;\n> > >\n> > > \treturn 0;\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 9B287BD792\n\tfor ;\n\tMon, 6 Jul 2020 16:23:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 16AAF60E14;\n\tMon, 6 Jul 2020 18:23:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 57824603AB\n\tfor ;\n\tMon, 6 Jul 2020 18:23:30 +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 AA0DBA16;\n\tMon, 6 Jul 2020 18:23:29 +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=\"ktU4VCH0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1594052609;\n\tbh=MXP9Zr3A3DB7Ka4cdjK5fP89+iLtPEYBstiSmNtxmxI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ktU4VCH0wV5TNHmV8kd/qFSKFWh+A/W9rtSF1a/GstTJfd0Izrz/3fXBEeB8byeEs\n\tzNZaQpi0+qlejMEG4XayYjWlnSkkWrpx0iYxohuRXpcM6wiKv3rXisZGaB3LKJj5O+\n\tOSAEvI+m2Qef3Ti0CLAlN++z1mLhEsE2W9XdZzm0=","Date":"Mon, 6 Jul 2020 19:23:25 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200706162325.GD19803@pendragon.ideasonboard.com>","References":"<20200704133140.1738660-1-paul.elder@ideasonboard.com>\n\t<20200704133140.1738660-19-paul.elder@ideasonboard.com>\n\t<20200704215103.GH10773@pendragon.ideasonboard.com>\n\t<20200706151654.ymllb3pgnlk4ykmw@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200706151654.ymllb3pgnlk4ykmw@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 18/22] libcamera: rkisp1: Fill\n\tstride and frameSize at config validation","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\" "}}]