[{"id":14988,"web_url":"https://patchwork.libcamera.org/comment/14988/","msgid":"","date":"2021-02-04T21:18:25","subject":"Re: [libcamera-devel] [PATCH v5 1/6] pipeline: raspberrypi:\n\tRefactor stream configuration routine","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Fri, Jan 29, 2021 at 03:11:49PM +0000, Naushir Patuck wrote:\n> Refactor the high/low resolution stream format and output selection\n> routine. This change is in preparation of adding 1/4 resolution output\n> for fast colour denoise.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> Reviewed-by: Kieran Bingham \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 57 ++++++-------------\n> 1 file changed, 16 insertions(+), 41 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 524cc960dd37..d9895c779725 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -624,52 +624,27 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\tif (i == maxIndex) {\n> -\t\t\t/* ISP main output format. */\n> -\t\t\tV4L2VideoDevice *dev = data->isp_[Isp::Output0].dev();\n> -\t\t\tV4L2PixelFormat fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);\n> -\t\t\tformat.size = cfg.size;\n> -\t\t\tformat.fourcc = fourcc;\n> -\n> -\t\t\tret = dev->setFormat(&format);\n> -\t\t\tif (ret)\n> -\t\t\t\treturn -EINVAL;\n> -\n> -\t\t\tif (format.size != cfg.size || format.fourcc != fourcc) {\n> -\t\t\t\tLOG(RPI, Error)\n> -\t\t\t\t\t<< \"Failed to set format on ISP capture0 device: \"\n> -\t\t\t\t\t<< format.toString();\n> -\t\t\t\treturn -EINVAL;\n> -\t\t\t}\n> -\n> -\t\t\tcfg.setStream(&data->isp_[Isp::Output0]);\n> -\t\t\tdata->isp_[Isp::Output0].setExternal(true);\n> -\t\t}\n> +\t\t/* The largest resolution gets routed to the ISP Output 0 node. */\n> +\t\tRPi::Stream *stream = (i == maxIndex) ? &data->isp_[Isp::Output0]\n\nNo need for the outer parentheses.\n\n> +\t\t\t\t\t\t : &data->isp_[Isp::Output1];\n> \n> -\t\t/*\n> -\t\t * ISP second output format. This fallthrough means that if a\n> -\t\t * second output stream has not been configured, we simply use\n> -\t\t * the Output0 configuration.\n> -\t\t */\n> -\t\tV4L2VideoDevice *dev = data->isp_[Isp::Output1].dev();\n> -\t\tformat.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);\n> +\t\tV4L2PixelFormat fourcc = stream->dev()->toV4L2PixelFormat(cfg.pixelFormat);\n> \t\tformat.size = cfg.size;\n> +\t\tformat.fourcc = fourcc;\n> \n> -\t\tret = dev->setFormat(&format);\n> -\t\tif (ret) {\n> +\t\tret = stream->dev()->setFormat(&format);\n> +\t\tif (ret)\n> +\t\t\treturn -EINVAL;\n> +\n> +\t\tif (format.size != cfg.size || format.fourcc != fourcc) {\n> \t\t\tLOG(RPI, Error)\n> -\t\t\t\t<< \"Failed to set format on ISP capture1 device: \"\n> -\t\t\t\t<< format.toString();\n> -\t\t\treturn ret;\n> -\t\t}\n> -\t\t/*\n> -\t\t * If we have not yet provided a stream for this config, it\n> -\t\t * means this is to be routed from Output1.\n> -\t\t */\n> -\t\tif (!cfg.stream()) {\n> -\t\t\tcfg.setStream(&data->isp_[Isp::Output1]);\n> -\t\t\tdata->isp_[Isp::Output1].setExternal(true);\n> +\t\t\t\t<< \"Failed get requested format on \" << stream->name()\n\n\"get\" or \"set\" ?\n\nReviewed-by: Laurent Pinchart \n\nI can fix these small issues when applying.\n\n> +\t\t\t\t<< \", returned \" << format.toString();\n> +\t\t\treturn -EINVAL;\n> \t\t}\n> +\n> +\t\tcfg.setStream(stream);\n> +\t\tstream->setExternal(true);\n> \t}\n> \n> \t/* ISP statistics output format. */","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 E6A2FBD160\n\tfor ;\n\tThu, 4 Feb 2021 21:18:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 690AD61461;\n\tThu, 4 Feb 2021 22:18:50 +0100 (CET)","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 F15F061430\n\tfor ;\n\tThu, 4 Feb 2021 22:18:48 +0100 (CET)","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 61BF845D;\n\tThu, 4 Feb 2021 22:18:48 +0100 (CET)"],"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=\"NZEp50cs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612473528;\n\tbh=LsNTSmcH4gx8OfP3z7IJ+kXwcmyW14oN7kYvd78uuzE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NZEp50cs53+LqUGgiYx5+790IP1kSXlLivHc6L6xyAiaZ6gsLxCSAvBZhaaSy57Zs\n\tTMlB2gEvOZGQbqFGuSc9jZZqLJPR2xwxm06uH2osSHC9y+EN+MEBrGIJpd2DZIKhw9\n\t47pKTEFz2YTduuV6ZJMfy7uHZqwv3x3Vjv0MdP6U=","Date":"Thu, 4 Feb 2021 23:18:25 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210129151154.1051163-1-naush@raspberrypi.com>\n\t<20210129151154.1051163-2-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210129151154.1051163-2-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v5 1/6] pipeline: raspberrypi:\n\tRefactor stream configuration routine","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":14989,"web_url":"https://patchwork.libcamera.org/comment/14989/","msgid":"","date":"2021-02-04T21:18:54","subject":"Re: [libcamera-devel] [PATCH v5 1/6] pipeline: raspberrypi:\n\tRefactor stream configuration routine","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Feb 04, 2021 at 11:18:27PM +0200, Laurent Pinchart wrote:\n> Hi Naush,\n> \n> Thank you for the patch.\n> \n> On Fri, Jan 29, 2021 at 03:11:49PM +0000, Naushir Patuck wrote:\n> > Refactor the high/low resolution stream format and output selection\n> > routine. This change is in preparation of adding 1/4 resolution output\n> > for fast colour denoise.\n> > \n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > Reviewed-by: Kieran Bingham \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 57 ++++++-------------\n> > 1 file changed, 16 insertions(+), 41 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 524cc960dd37..d9895c779725 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -624,52 +624,27 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > \t\t\tcontinue;\n> > \t\t}\n> > \n> > -\t\tif (i == maxIndex) {\n> > -\t\t\t/* ISP main output format. */\n> > -\t\t\tV4L2VideoDevice *dev = data->isp_[Isp::Output0].dev();\n> > -\t\t\tV4L2PixelFormat fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);\n> > -\t\t\tformat.size = cfg.size;\n> > -\t\t\tformat.fourcc = fourcc;\n> > -\n> > -\t\t\tret = dev->setFormat(&format);\n> > -\t\t\tif (ret)\n> > -\t\t\t\treturn -EINVAL;\n> > -\n> > -\t\t\tif (format.size != cfg.size || format.fourcc != fourcc) {\n> > -\t\t\t\tLOG(RPI, Error)\n> > -\t\t\t\t\t<< \"Failed to set format on ISP capture0 device: \"\n> > -\t\t\t\t\t<< format.toString();\n> > -\t\t\t\treturn -EINVAL;\n> > -\t\t\t}\n> > -\n> > -\t\t\tcfg.setStream(&data->isp_[Isp::Output0]);\n> > -\t\t\tdata->isp_[Isp::Output0].setExternal(true);\n> > -\t\t}\n> > +\t\t/* The largest resolution gets routed to the ISP Output 0 node. */\n> > +\t\tRPi::Stream *stream = (i == maxIndex) ? &data->isp_[Isp::Output0]\n> \n> No need for the outer parentheses.\n> \n> > +\t\t\t\t\t\t : &data->isp_[Isp::Output1];\n> > \n> > -\t\t/*\n> > -\t\t * ISP second output format. This fallthrough means that if a\n> > -\t\t * second output stream has not been configured, we simply use\n> > -\t\t * the Output0 configuration.\n> > -\t\t */\n> > -\t\tV4L2VideoDevice *dev = data->isp_[Isp::Output1].dev();\n> > -\t\tformat.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);\n> > +\t\tV4L2PixelFormat fourcc = stream->dev()->toV4L2PixelFormat(cfg.pixelFormat);\n> > \t\tformat.size = cfg.size;\n> > +\t\tformat.fourcc = fourcc;\n> > \n> > -\t\tret = dev->setFormat(&format);\n> > -\t\tif (ret) {\n> > +\t\tret = stream->dev()->setFormat(&format);\n> > +\t\tif (ret)\n> > +\t\t\treturn -EINVAL;\n> > +\n> > +\t\tif (format.size != cfg.size || format.fourcc != fourcc) {\n> > \t\t\tLOG(RPI, Error)\n> > -\t\t\t\t<< \"Failed to set format on ISP capture1 device: \"\n> > -\t\t\t\t<< format.toString();\n> > -\t\t\treturn ret;\n> > -\t\t}\n> > -\t\t/*\n> > -\t\t * If we have not yet provided a stream for this config, it\n> > -\t\t * means this is to be routed from Output1.\n> > -\t\t */\n> > -\t\tif (!cfg.stream()) {\n> > -\t\t\tcfg.setStream(&data->isp_[Isp::Output1]);\n> > -\t\t\tdata->isp_[Isp::Output1].setExternal(true);\n> > +\t\t\t\t<< \"Failed get requested format on \" << stream->name()\n> \n> \"get\" or \"set\" ?\n\nAnd maybe \"Failed to set\" ?\n\n> Reviewed-by: Laurent Pinchart \n> \n> I can fix these small issues when applying.\n> \n> > +\t\t\t\t<< \", returned \" << format.toString();\n> > +\t\t\treturn -EINVAL;\n> > \t\t}\n> > +\n> > +\t\tcfg.setStream(stream);\n> > +\t\tstream->setExternal(true);\n> > \t}\n> > \n> > \t/* ISP statistics output format. */","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 077F0BD160\n\tfor ;\n\tThu, 4 Feb 2021 21:19:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C78C861461;\n\tThu, 4 Feb 2021 22:19:17 +0100 (CET)","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 0C5D961430\n\tfor ;\n\tThu, 4 Feb 2021 22:19:17 +0100 (CET)","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 8CFA745D;\n\tThu, 4 Feb 2021 22:19:16 +0100 (CET)"],"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=\"A+EqsG4j\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612473556;\n\tbh=yoH5YDP0OkRA3NcLjYe+vaQQUo/MKvaJnREoPo1NZcw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=A+EqsG4jqwsDwpI/BR+8pyOmXXWhBSAMEDPlpwqJQpuRCjCIBWSDtb6hGDHPrxJRf\n\tZAKHFXLlF586YzUFP9+UCLVuALVCIT3IMBoBJcMkfOGQla8xHeumXkT17S3DlhGQJd\n\tDpLiu3MehUfV6bghrQvlE18nZkDr3FEs2A/n0mv4=","Date":"Thu, 4 Feb 2021 23:18:54 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210129151154.1051163-1-naush@raspberrypi.com>\n\t<20210129151154.1051163-2-naush@raspberrypi.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 1/6] pipeline: raspberrypi:\n\tRefactor stream configuration routine","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\" "}}]