[{"id":32303,"web_url":"https://patchwork.libcamera.org/comment/32303/","msgid":"","date":"2024-11-20T10:59:24","subject":"Re: [PATCH 2/7] libcamera: rkisp1: Keep aspect ratio on imx8mp","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Wed, Nov 20, 2024 at 09:57:41AM +0100, Stefan Klug wrote:\n> In the current code, the input stage of the image resizer is used to\n> apply a crop to keep the aspect ratio in cases where the requested\n> output aspect ratio differs from the one of the selected sensor mode. On\n> the imx8mp the resizer hardware is not capable of cropping (for\n> reference see also rkisp1-resizer.c:rkisp1_rsz_set_sink_crop() in the\n> linux kernel v6.10).\n> \n> Therefore apply the necessary cropping on the output of the ISP (on the\n> image stabilization block). The cropping code on the image resizer\n> doesn't need modifications as the requested crop gets ignored by the\n> kernel.\n> \n> Signed-off-by: Stefan Klug \n\nReviewed-by: Paul Elder \n\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 +++++++++++++++\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 6 ++++++\n> 2 files changed, 21 insertions(+)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 5ffcfbce56be..9d36554cec6e 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -811,6 +811,21 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \tif (!isRaw_)\n> \t\tformat.code = MEDIA_BUS_FMT_YUYV8_2X8;\n> \n> +\t/*\n> +\t * On devices without DUAL_CROP (like the imx8mp) cropping needs to be\n> +\t * done on the ISP/IS output.\n> +\t */\n> +\tif (media_->hwRevision() == RKISP1_V_IMX8MP) {\n> +\t\tconst auto &cfg = config->at(0);\n> +\t\tSize ispCrop = format.size.boundedToAspectRatio(cfg.size)\n> +\t\t\t\t .alignedUpTo(2, 2);\n> +\t\trect = ispCrop.centeredTo(Rectangle(format.size).center());\n> +\t\tif (ispCrop != format.size)\n> +\t\t\tLOG(RkISP1, Info) << \"ISP output needs to be cropped to \"\n> +\t\t\t\t\t << rect;\n> +\t\tformat.size = ispCrop;\n> +\t}\n> +\n> \tLOG(RkISP1, Debug)\n> \t\t<< \"Configuring ISP output pad with \" << format\n> \t\t<< \" crop \" << rect;\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> index 236d05af7a2f..0651de464907 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> @@ -417,6 +417,12 @@ int RkISP1Path::configure(const StreamConfiguration &config,\n> \t/*\n> \t * Crop on the resizer input to maintain FOV before downscaling.\n> \t *\n> +\t * Note that this does not apply on the imx8mp, where the cropping needs\n> +\t * to be done on the ImageStabilizer output and therefore is configured\n> +\t * before this stage. For simplicity we still set the crop. This gets\n> +\t * ignored by the kernel driver because the hardware is missing the\n> +\t * capability.\n> +\t *\n> \t * \\todo The alignment to a multiple of 2 pixels is required but may\n> \t * change the aspect ratio very slightly. A more advanced algorithm to\n> \t * compute the resizer input crop rectangle is needed, and it should\n> -- \n> 2.43.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 02FEBC0F2A\n\tfor ;\n\tWed, 20 Nov 2024 10:59:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 240F065F49;\n\tWed, 20 Nov 2024 11:59:33 +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 8ACE9658B6\n\tfor ;\n\tWed, 20 Nov 2024 11:59:31 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:6f8:18b9:c92f:628d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8201F55A;\n\tWed, 20 Nov 2024 11:59:12 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"EixGi3ze\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732100353;\n\tbh=q1+G1w7jQuy3iEfZgbYlITdhQpT6t8nPqQ9QnGcFXOw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EixGi3zejgech4sT+b980xxRI5/RwTl1OIQ0+HY1fbYLkeq4Ku0pen6G+25aVnCeh\n\t+Dj4TmCZNNCmdZab0S+JdtOv6hNIEy28jExTwWAycZYgP9x/RX3rgY/yDpCA1sCYhc\n\tAbGpu8LRh2HLo51w1b2qr4EuIyKXK6QaxAqBdPjc=","Date":"Wed, 20 Nov 2024 19:59:24 +0900","From":"Paul Elder ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/7] libcamera: rkisp1: Keep aspect ratio on imx8mp","Message-ID":"","References":"<20241120085753.1993436-1-stefan.klug@ideasonboard.com>\n\t<20241120085753.1993436-3-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20241120085753.1993436-3-stefan.klug@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32328,"web_url":"https://patchwork.libcamera.org/comment/32328/","msgid":"<7j47fa3klssey43kt2mc7jjqnneup4u73djtq7w3mv7fhiwts6@ojpnmll5brmn>","date":"2024-11-21T09:20:14","subject":"Re: [PATCH 2/7] libcamera: rkisp1: Keep aspect ratio on imx8mp","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Wed, Nov 20, 2024 at 09:57:41AM +0100, Stefan Klug wrote:\n> In the current code, the input stage of the image resizer is used to\n> apply a crop to keep the aspect ratio in cases where the requested\n> output aspect ratio differs from the one of the selected sensor mode. On\n> the imx8mp the resizer hardware is not capable of cropping (for\n> reference see also rkisp1-resizer.c:rkisp1_rsz_set_sink_crop() in the\n> linux kernel v6.10).\n>\n> Therefore apply the necessary cropping on the output of the ISP (on the\n> image stabilization block). The cropping code on the image resizer\n> doesn't need modifications as the requested crop gets ignored by the\n> kernel.\n>\n> Signed-off-by: Stefan Klug \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 +++++++++++++++\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 6 ++++++\n> 2 files changed, 21 insertions(+)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 5ffcfbce56be..9d36554cec6e 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -811,6 +811,21 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \tif (!isRaw_)\n> \t\tformat.code = MEDIA_BUS_FMT_YUYV8_2X8;\n>\n> +\t/*\n> +\t * On devices without DUAL_CROP (like the imx8mp) cropping needs to be\n> +\t * done on the ISP/IS output.\n> +\t */\n> +\tif (media_->hwRevision() == RKISP1_V_IMX8MP) {\n\nI wish we had a better way to identify the single ISP features instead\nof relying on the whole model..\n\n> +\t\tconst auto &cfg = config->at(0);\n\nThis will only work on (!DUAL_CROP && single_stream) which is the\nimx8mp case.\n\nI would add a comment to clarify we can assume config->at(0) because\nof this\n\n> +\t\tSize ispCrop = format.size.boundedToAspectRatio(cfg.size)\n> +\t\t\t\t .alignedUpTo(2, 2);\n\nPlease align\n\n\t\tSize ispCrop = format.size.boundedToAspectRatio(cfg.size)\n\t\t\t\t .alignedUpTo(2, 2);\n\n> +\t\trect = ispCrop.centeredTo(Rectangle(format.size).center());\n> +\t\tif (ispCrop != format.size)\n> +\t\t\tLOG(RkISP1, Info) << \"ISP output needs to be cropped to \"\n> +\t\t\t\t\t << rect;\n> +\t\tformat.size = ispCrop;\n> +\t}\n> +\n> \tLOG(RkISP1, Debug)\n> \t\t<< \"Configuring ISP output pad with \" << format\n> \t\t<< \" crop \" << rect;\n\nUnrelated, but we have\n\n\tLOG(RkISP1, Debug)\n\t\t<< \"ISP input pad configured with \" << format\n\t\t<< \" crop \" << rect;\n\n\n\tLOG(RkISP1, Debug)\n\t\t<< \"Configuring ISP output pad with \" << format\n\t\t<< \" crop \" << rect;\n\nIt would be nice to align the two messages.\n\nAlso, unrelated, but do you know why we were already setting both crop\nand format on the source pad with the same sizes ? Setting a format source\npad propagates its sizes to the crop rectangle. Is this a better safe\nthen sorry ?\n\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> index 236d05af7a2f..0651de464907 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> @@ -417,6 +417,12 @@ int RkISP1Path::configure(const StreamConfiguration &config,\n> \t/*\n> \t * Crop on the resizer input to maintain FOV before downscaling.\n> \t *\n> +\t * Note that this does not apply on the imx8mp, where the cropping needs\n\nI would say, as you did already\n\n\t * Note that this does not apply to devices without DUAL_CROP\n * support (like imx8mp) , where the cropping needs\n\n> +\t * to be done on the ImageStabilizer output and therefore is configured\n\nWhat about re-stating that the IS block is configured through the ISP\nsource pad crop rectangle ?\n\n\t * to be done on the ImageStabilizer block on the ISP source pad and\n * therefore is configured\n\n> +\t * before this stage. For simplicity we still set the crop. This gets\n> +\t * ignored by the kernel driver because the hardware is missing the\n> +\t * capability.\n> +\t *\n\nnits apart\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> \t * \\todo The alignment to a multiple of 2 pixels is required but may\n> \t * change the aspect ratio very slightly. A more advanced algorithm to\n> \t * compute the resizer input crop rectangle is needed, and it should\n\nthe driver aligns to 2 anyway, so this todo is kind of moot maybe\n> --\n> 2.43.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 EE5BEC326C\n\tfor ;\n\tThu, 21 Nov 2024 09:20:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2E8B265FB9;\n\tThu, 21 Nov 2024 10:20:19 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 16BB365FB0\n\tfor ;\n\tThu, 21 Nov 2024 10:20:18 +0100 (CET)","from ideasonboard.com (93-46-82-201.ip106.fastwebnet.it\n\t[93.46.82.201])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5C2EA670;\n\tThu, 21 Nov 2024 10:19:59 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"qHXRTsqw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732180799;\n\tbh=up7dECJger/Y6K1rC3dC5iD7AS5gMCv4n7Bw4MkUU5A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qHXRTsqw0KIEoGFBC7v8Lz1/x5Wp/tVnzRJiUvNilBEptkIEi8WlHdRHHl/fMmW/h\n\tKlQK3rQs/LHXlSt4Np6pg8r2/VlAA6WU+9CNIYIeqB7aHz7cz7MCcMWa9guDMKPI5n\n\tqzMGTMPGfsn0gWFqhdNRnZIZpe9C6bQChKlHSIiw=","Date":"Thu, 21 Nov 2024 10:20:14 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/7] libcamera: rkisp1: Keep aspect ratio on imx8mp","Message-ID":"<7j47fa3klssey43kt2mc7jjqnneup4u73djtq7w3mv7fhiwts6@ojpnmll5brmn>","References":"<20241120085753.1993436-1-stefan.klug@ideasonboard.com>\n\t<20241120085753.1993436-3-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241120085753.1993436-3-stefan.klug@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32335,"web_url":"https://patchwork.libcamera.org/comment/32335/","msgid":"","date":"2024-11-21T14:03:27","subject":"Re: [PATCH 2/7] libcamera: rkisp1: Keep aspect ratio on imx8mp","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the review. \n\nOn Thu, Nov 21, 2024 at 10:20:14AM +0100, Jacopo Mondi wrote:\n> Hi Stefan\n> \n> On Wed, Nov 20, 2024 at 09:57:41AM +0100, Stefan Klug wrote:\n> > In the current code, the input stage of the image resizer is used to\n> > apply a crop to keep the aspect ratio in cases where the requested\n> > output aspect ratio differs from the one of the selected sensor mode. On\n> > the imx8mp the resizer hardware is not capable of cropping (for\n> > reference see also rkisp1-resizer.c:rkisp1_rsz_set_sink_crop() in the\n> > linux kernel v6.10).\n> >\n> > Therefore apply the necessary cropping on the output of the ISP (on the\n> > image stabilization block). The cropping code on the image resizer\n> > doesn't need modifications as the requested crop gets ignored by the\n> > kernel.\n> >\n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 +++++++++++++++\n> > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 6 ++++++\n> > 2 files changed, 21 insertions(+)\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 5ffcfbce56be..9d36554cec6e 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -811,6 +811,21 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > \tif (!isRaw_)\n> > \t\tformat.code = MEDIA_BUS_FMT_YUYV8_2X8;\n> >\n> > +\t/*\n> > +\t * On devices without DUAL_CROP (like the imx8mp) cropping needs to be\n> > +\t * done on the ISP/IS output.\n> > +\t */\n> > +\tif (media_->hwRevision() == RKISP1_V_IMX8MP) {\n> \n> I wish we had a better way to identify the single ISP features instead\n> of relying on the whole model..\n\nmee too :-)\n\n> \n> > +\t\tconst auto &cfg = config->at(0);\n> \n> This will only work on (!DUAL_CROP && single_stream) which is the\n> imx8mp case.\n> \n> I would add a comment to clarify we can assume config->at(0) because\n> of this\n> \n> > +\t\tSize ispCrop = format.size.boundedToAspectRatio(cfg.size)\n> > +\t\t\t\t .alignedUpTo(2, 2);\n> \n> Please align\n\nThanks for spotting. Will do.\n\n> \n> \t\tSize ispCrop = format.size.boundedToAspectRatio(cfg.size)\n> \t\t\t\t .alignedUpTo(2, 2);\n> \n> > +\t\trect = ispCrop.centeredTo(Rectangle(format.size).center());\n> > +\t\tif (ispCrop != format.size)\n> > +\t\t\tLOG(RkISP1, Info) << \"ISP output needs to be cropped to \"\n> > +\t\t\t\t\t << rect;\n> > +\t\tformat.size = ispCrop;\n> > +\t}\n> > +\n> > \tLOG(RkISP1, Debug)\n> > \t\t<< \"Configuring ISP output pad with \" << format\n> > \t\t<< \" crop \" << rect;\n> \n> Unrelated, but we have\n> \n> \tLOG(RkISP1, Debug)\n> \t\t<< \"ISP input pad configured with \" << format\n> \t\t<< \" crop \" << rect;\n> \n> \n> \tLOG(RkISP1, Debug)\n> \t\t<< \"Configuring ISP output pad with \" << format\n> \t\t<< \" crop \" << rect;\n> \n> It would be nice to align the two messages.\n\nI don't really get what you mean here. On the output pad there are two\nmessages:\n\nConfiguring ISP output pad with... and\nISP output pad configured with ...\n\nWhich seems reasonable, if we expect the kernel to adjust the TGT_CROP.\nOr do you mean that we should add the\n\nConfiguring ISP input pad with... \n\nmessage?\n\n> \n> Also, unrelated, but do you know why we were already setting both crop\n> and format on the source pad with the same sizes ? Setting a format source\n> pad propagates its sizes to the crop rectangle. Is this a better safe\n> then sorry ?\n\nIt wasn't me :-). No I really don't know. Either there was a driver\nwhich didn't do that properly or it is really a better safe than sorry\ncase.\n\n> \n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > index 236d05af7a2f..0651de464907 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > @@ -417,6 +417,12 @@ int RkISP1Path::configure(const StreamConfiguration &config,\n> > \t/*\n> > \t * Crop on the resizer input to maintain FOV before downscaling.\n> > \t *\n> > +\t * Note that this does not apply on the imx8mp, where the cropping needs\n> \n> I would say, as you did already\n\nMaybe a bit over verbose.\n\n> \n> \t * Note that this does not apply to devices without DUAL_CROP\n> * support (like imx8mp) , where the cropping needs\n> \n> > +\t * to be done on the ImageStabilizer output and therefore is configured\n> \n> What about re-stating that the IS block is configured through the ISP\n> source pad crop rectangle ?\n\nYes I can improve that comment.\n\n> \n> \t * to be done on the ImageStabilizer block on the ISP source pad and\n> * therefore is configured\n> \n> > +\t * before this stage. For simplicity we still set the crop. This gets\n> > +\t * ignored by the kernel driver because the hardware is missing the\n> > +\t * capability.\n> > +\t *\n> \n> nits apart\n> Reviewed-by: Jacopo Mondi \n> \n\nThank you!\n\nCheers,\nStefan\n\n> Thanks\n> j\n> \n> > \t * \\todo The alignment to a multiple of 2 pixels is required but may\n> > \t * change the aspect ratio very slightly. A more advanced algorithm to\n> > \t * compute the resizer input crop rectangle is needed, and it should\n> \n> the driver aligns to 2 anyway, so this todo is kind of moot maybe\n> > --\n> > 2.43.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 D4C9AC326C\n\tfor ;\n\tThu, 21 Nov 2024 14:03:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1579065FCB;\n\tThu, 21 Nov 2024 15:03:32 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6225965FC6\n\tfor ;\n\tThu, 21 Nov 2024 15:03:30 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:37ce:8a43:16b4:e4e9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6BFB4219;\n\tThu, 21 Nov 2024 15:03:11 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"mZ+XIpYr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732197791;\n\tbh=JEBngrgAHQiK2qYZy3LITvRS35NA0JPAWjEj/DD9ScM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mZ+XIpYrhChCOEJyeZccZDsti4ln+UknxymSzXrj6GnxaiW9JlAdgXSN3e9INbonE\n\tJ4qqXSfPQUaWdKenk3bKL0gN7+JWwzcVYeQ9v7FyP+HzvfTqTMtXz7HAkd+5mVyMy8\n\tHSCmeXzK8FE7dYwyKTR1xo/Ho8x1lPV8HcUA0w0Y=","Date":"Thu, 21 Nov 2024 15:03:27 +0100","From":"Stefan Klug ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/7] libcamera: rkisp1: Keep aspect ratio on imx8mp","Message-ID":"","References":"<20241120085753.1993436-1-stefan.klug@ideasonboard.com>\n\t<20241120085753.1993436-3-stefan.klug@ideasonboard.com>\n\t<7j47fa3klssey43kt2mc7jjqnneup4u73djtq7w3mv7fhiwts6@ojpnmll5brmn>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<7j47fa3klssey43kt2mc7jjqnneup4u73djtq7w3mv7fhiwts6@ojpnmll5brmn>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32350,"web_url":"https://patchwork.libcamera.org/comment/32350/","msgid":"","date":"2024-11-25T09:25:50","subject":"Re: [PATCH 2/7] libcamera: rkisp1: Keep aspect ratio on imx8mp","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Thu, Nov 21, 2024 at 03:03:27PM +0100, Stefan Klug wrote:\n> Hi Jacopo,\n>\n> Thank you for the review.\n>\n> On Thu, Nov 21, 2024 at 10:20:14AM +0100, Jacopo Mondi wrote:\n> > Hi Stefan\n> >\n> > On Wed, Nov 20, 2024 at 09:57:41AM +0100, Stefan Klug wrote:\n> > > In the current code, the input stage of the image resizer is used to\n> > > apply a crop to keep the aspect ratio in cases where the requested\n> > > output aspect ratio differs from the one of the selected sensor mode. On\n> > > the imx8mp the resizer hardware is not capable of cropping (for\n> > > reference see also rkisp1-resizer.c:rkisp1_rsz_set_sink_crop() in the\n> > > linux kernel v6.10).\n> > >\n> > > Therefore apply the necessary cropping on the output of the ISP (on the\n> > > image stabilization block). The cropping code on the image resizer\n> > > doesn't need modifications as the requested crop gets ignored by the\n> > > kernel.\n> > >\n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 +++++++++++++++\n> > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 6 ++++++\n> > > 2 files changed, 21 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 5ffcfbce56be..9d36554cec6e 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -811,6 +811,21 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > > \tif (!isRaw_)\n> > > \t\tformat.code = MEDIA_BUS_FMT_YUYV8_2X8;\n> > >\n> > > +\t/*\n> > > +\t * On devices without DUAL_CROP (like the imx8mp) cropping needs to be\n> > > +\t * done on the ISP/IS output.\n> > > +\t */\n> > > +\tif (media_->hwRevision() == RKISP1_V_IMX8MP) {\n> >\n> > I wish we had a better way to identify the single ISP features instead\n> > of relying on the whole model..\n>\n> mee too :-)\n>\n> >\n> > > +\t\tconst auto &cfg = config->at(0);\n> >\n> > This will only work on (!DUAL_CROP && single_stream) which is the\n> > imx8mp case.\n> >\n> > I would add a comment to clarify we can assume config->at(0) because\n> > of this\n> >\n> > > +\t\tSize ispCrop = format.size.boundedToAspectRatio(cfg.size)\n> > > +\t\t\t\t .alignedUpTo(2, 2);\n> >\n> > Please align\n>\n> Thanks for spotting. Will do.\n>\n> >\n> > \t\tSize ispCrop = format.size.boundedToAspectRatio(cfg.size)\n> > \t\t\t\t .alignedUpTo(2, 2);\n> >\n> > > +\t\trect = ispCrop.centeredTo(Rectangle(format.size).center());\n> > > +\t\tif (ispCrop != format.size)\n> > > +\t\t\tLOG(RkISP1, Info) << \"ISP output needs to be cropped to \"\n> > > +\t\t\t\t\t << rect;\n> > > +\t\tformat.size = ispCrop;\n> > > +\t}\n> > > +\n> > > \tLOG(RkISP1, Debug)\n> > > \t\t<< \"Configuring ISP output pad with \" << format\n> > > \t\t<< \" crop \" << rect;\n> >\n> > Unrelated, but we have\n> >\n> > \tLOG(RkISP1, Debug)\n> > \t\t<< \"ISP input pad configured with \" << format\n> > \t\t<< \" crop \" << rect;\n> >\n> >\n> > \tLOG(RkISP1, Debug)\n> > \t\t<< \"Configuring ISP output pad with \" << format\n> > \t\t<< \" crop \" << rect;\n> >\n> > It would be nice to align the two messages.\n>\n> I don't really get what you mean here. On the output pad there are two\n> messages:\n>\n> Configuring ISP output pad with... and\n> ISP output pad configured with ...\n\nSorry, I didn't notice the second one, I thought we only had\n\nConfiguring ISP input pad with... and\nISP output pad configured with ...\n\nwhich I was suggesting to align to the same style.\n\nPlease ignore this comment :)\n\n>\n> Which seems reasonable, if we expect the kernel to adjust the TGT_CROP.\n> Or do you mean that we should add the\n>\n> Configuring ISP input pad with...\n>\n> message?\n>\n> >\n> > Also, unrelated, but do you know why we were already setting both crop\n> > and format on the source pad with the same sizes ? Setting a format source\n> > pad propagates its sizes to the crop rectangle. Is this a better safe\n> > then sorry ?\n>\n> It wasn't me :-). No I really don't know. Either there was a driver\n> which didn't do that properly or it is really a better safe than sorry\n> case.\n>\n> >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > index 236d05af7a2f..0651de464907 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > @@ -417,6 +417,12 @@ int RkISP1Path::configure(const StreamConfiguration &config,\n> > > \t/*\n> > > \t * Crop on the resizer input to maintain FOV before downscaling.\n> > > \t *\n> > > +\t * Note that this does not apply on the imx8mp, where the cropping needs\n> >\n> > I would say, as you did already\n>\n> Maybe a bit over verbose.\n>\n> >\n> > \t * Note that this does not apply to devices without DUAL_CROP\n> > * support (like imx8mp) , where the cropping needs\n> >\n> > > +\t * to be done on the ImageStabilizer output and therefore is configured\n> >\n> > What about re-stating that the IS block is configured through the ISP\n> > source pad crop rectangle ?\n>\n> Yes I can improve that comment.\n>\n> >\n> > \t * to be done on the ImageStabilizer block on the ISP source pad and\n> > * therefore is configured\n> >\n> > > +\t * before this stage. For simplicity we still set the crop. This gets\n> > > +\t * ignored by the kernel driver because the hardware is missing the\n> > > +\t * capability.\n> > > +\t *\n> >\n> > nits apart\n> > Reviewed-by: Jacopo Mondi \n> >\n>\n> Thank you!\n>\n> Cheers,\n> Stefan\n>\n> > Thanks\n> > j\n> >\n> > > \t * \\todo The alignment to a multiple of 2 pixels is required but may\n> > > \t * change the aspect ratio very slightly. A more advanced algorithm to\n> > > \t * compute the resizer input crop rectangle is needed, and it should\n> >\n> > the driver aligns to 2 anyway, so this todo is kind of moot maybe\n> > > --\n> > > 2.43.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 3409DBD160\n\tfor ;\n\tMon, 25 Nov 2024 09:25:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 86B8A6600E;\n\tMon, 25 Nov 2024 10:25:56 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6129A66008\n\tfor ;\n\tMon, 25 Nov 2024 10:25:54 +0100 (CET)","from ideasonboard.com (mob-5-90-136-47.net.vodafone.it\n\t[5.90.136.47])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7B32A4AD;\n\tMon, 25 Nov 2024 10:25:32 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"bXgiA4Bs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732526732;\n\tbh=7DDhuci2rELuIpIeThAM0OYZ4bCFcPi5oBxzkZotP/w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bXgiA4Bs6AkYWWofmzpIkydjIv6EK8szDbj1bYSMmi1S4S4seXYg3eRoNVY29SgMv\n\tgiL+h9WdK0vBhz1wDILj3BvIUlFZLF0gQSsLm6HNd54FdpGgm3bu4vQ2KZ61nCn0Fr\n\tO+T8HjVY48zDz7CxpZHaNF2ikD6WpSXoTl8oGyEo=","Date":"Mon, 25 Nov 2024 10:25:50 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/7] libcamera: rkisp1: Keep aspect ratio on imx8mp","Message-ID":"","References":"<20241120085753.1993436-1-stefan.klug@ideasonboard.com>\n\t<20241120085753.1993436-3-stefan.klug@ideasonboard.com>\n\t<7j47fa3klssey43kt2mc7jjqnneup4u73djtq7w3mv7fhiwts6@ojpnmll5brmn>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]