[{"id":29934,"web_url":"https://patchwork.libcamera.org/comment/29934/","msgid":"<171830719141.2248009.16384466080783047609@ping.linuxembedded.co.uk>","date":"2024-06-13T19:33:11","subject":"Re: [PATCH 3/7] mali-c55: implement support for ScalerCrop","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Daniel Scally (2024-06-13 16:59:45)\n> From: Jacopo Mondi \n> \n> Implement support for the ScalerCrop control that allows to apply a\n> digital zoom to the captured streams.\n> \n> Initialize the camera controls at camera registration time and update\n> them at configure time as the sensor's analogue crop size might change\n> depending on the desired Camera configuration.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 138 ++++++++++++++++++-\n> 1 file changed, 137 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> index c4f1afbc..2c34f3e9 100644\n> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> @@ -100,6 +100,8 @@ public:\n> \n> PixelFormat bestRawFormat() const;\n> \n> + void updateControls();\n> +\n> PixelFormat adjustRawFormat(const PixelFormat &pixFmt) const;\n> Size adjustRawSizes(const PixelFormat &pixFmt, const Size &rawSize) const;\n> \n> @@ -245,6 +247,27 @@ PixelFormat MaliC55CameraData::bestRawFormat() const\n> return rawFormat;\n> }\n> \n> +void MaliC55CameraData::updateControls()\n> +{\n> + if (!sensor_)\n> + return;\n> +\n> + IPACameraSensorInfo sensorInfo;\n> + int ret = sensor_->sensorInfo(&sensorInfo);\n> + if (ret) {\n> + LOG(MaliC55, Error) << \"Failed to retrieve sensor info\";\n> + return;\n> + }\n> +\n> + ControlInfoMap::Map controls;\n> + Rectangle ispMinCrop{ 0, 0, 640, 480 };\n\nThis looks like it should be a constant somewhere outside of here?\nPerhaps class constant or just a constant at the top of the file.\n\n> + controls[&controls::ScalerCrop] =\n> + ControlInfo(ispMinCrop, sensorInfo.analogCrop,\n> + sensorInfo.analogCrop);\n> +\n> + controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);\n> +}\n> +\n> /*\n> * Make sure the provided raw pixel format is supported and adjust it to\n> * one of the supported ones if it's not.\n> @@ -515,6 +538,8 @@ private:\n> const StreamConfiguration &config,\n> V4L2SubdeviceFormat &subdevFormat);\n> \n> + void applyScalerCrop(Camera *camera, const ControlList &controls);\n> +\n> void registerMaliCamera(std::unique_ptr data,\n> const std::string &name);\n> bool registerTPGCamera(MediaLink *link);\n> @@ -828,6 +853,8 @@ int PipelineHandlerMaliC55::configure(Camera *camera,\n> pipe->stream = stream;\n> }\n> \n> + data->updateControls();\n> +\n> return 0;\n> }\n> \n> @@ -875,6 +902,104 @@ void PipelineHandlerMaliC55::stopDevice([[maybe_unused]] Camera *camera)\n> }\n> }\n> \n> +void PipelineHandlerMaliC55::applyScalerCrop(Camera *camera,\n> + const ControlList &controls)\n> +{\n> + MaliC55CameraData *data = cameraData(camera);\n> +\n> + const auto &scalerCrop = controls.get(controls::ScalerCrop);\n> + if (!scalerCrop)\n> + return;\n> +\n> + if (!data->sensor_) {\n> + LOG(MaliC55, Error) << \"ScalerCrop not supported for TPG\";\n> + return;\n> + }\n> +\n> + Rectangle nativeCrop = *scalerCrop;\n> +\n> + IPACameraSensorInfo sensorInfo;\n> + int ret = data->sensor_->sensorInfo(&sensorInfo);\n> + if (ret) {\n> + LOG(MaliC55, Error) << \"Failed to retrieve sensor info\";\n> + return;\n> + }\n> +\n> + /*\n> + * The ScalerCrop rectangle re-scaling in the ISP crop rectangle\n> + * comes straight from the RPi pipeline handler.\n> + *\n> + * Create a version of the crop rectangle aligned to the analogue crop\n> + * rectangle top-left coordinates and scaled in the [analogue crop to\n> + * output frame] ratio to take into account binning/skipping on the\n> + * sensor.\n> + */\n> + Rectangle ispCrop = nativeCrop.translatedBy(-sensorInfo.analogCrop\n> + .topLeft());\n> + ispCrop.scaleBy(sensorInfo.outputSize, sensorInfo.analogCrop.size());\n> +\n> + /*\n> + * The crop rectangle should be:\n> + * 1. At least as big as ispMinCropSize_, once that's been\n> + * enlarged to the same aspect ratio.\n> + * 2. With the same mid-point, if possible.\n> + * 3. But it can't go outside the sensor area.\n> + */\n> + Rectangle ispMinCrop{ 0, 0, 640, 480 };\n\nEspecially if it's used multiple times.\n\n\n> + Size minSize = ispMinCrop.size().expandedToAspectRatio(nativeCrop.size());\n> + Size size = ispCrop.size().expandedTo(minSize);\n> + ispCrop = size.centeredTo(ispCrop.center())\n> + .enclosedIn(Rectangle(sensorInfo.outputSize));\n> +\n> + /*\n> + * As the resizer can't upscale, the crop rectangle has to be larger\n> + * than the larger stream output size.\n> + */\n> + Size maxYuvSize;\n> + for (MaliC55Pipe &pipe : pipes_) {\n> +\n\nProbably delete that blank line.\n\n> + if (!pipe.stream)\n> + continue;\n> +\n> + const StreamConfiguration &config = pipe.stream->configuration();\n> + if (isFormatRaw(config.pixelFormat)) {\n> + LOG(MaliC55, Debug) << \"Cannot crop with a RAW stream\";\n> + return;\n> + }\n> +\n> + Size streamSize = config.size;\n> + if (streamSize.width > maxYuvSize.width)\n> + maxYuvSize.width = streamSize.width;\n> + if (streamSize.height > maxYuvSize.height)\n> + maxYuvSize.height = streamSize.height;\n> + }\n> +\n> + ispCrop.size().expandTo(maxYuvSize);\n> +\n> + /*\n> + * Now apply the scaler crop to each enabled output. This overrides the\n> + * crop configuration performed at configure() time and can cause\n> + * square pixels if the crop rectangle and scaler output FOV ratio are\n\nCan cause 'non square' pixels perhaps?\n\n> + * different.\n> + */\n> + for (MaliC55Pipe &pipe : pipes_) {\n> +\n> + if (!pipe.stream)\n> + continue;\n> +\n> + /* Create a copy to avoid setSelection() to modify ispCrop. */\n\n'to prevent setSelection() from modifying ispCrop'\n\n> + Rectangle pipeCrop = ispCrop;\n> + ret = pipe.resizer->setSelection(0, V4L2_SEL_TGT_CROP, &pipeCrop);\n> + if (ret) {\n> + LOG(MaliC55, Error)\n> + << \"Failed to apply crop to \"\n> + << (pipe.stream == &data->frStream_ ?\n> + \"FR\" : \"DS\") << \" pipe\";\n> + return;\n> + }\n> + }\n> +}\n> +\n> int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request)\n> {\n> int ret;\n> @@ -887,6 +1012,15 @@ int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request)\n> return ret;\n> }\n> \n> + /*\n> + * Some controls need to be applied immediately, as in example,\n> + * the ScalerCrop one.\n> + *\n> + * \\todo Move it buffer queue time (likely after the IPA has filled in\n> + * the parameters buffer) once we have plumbed the IPA loop in.\n> + */\n> + applyScalerCrop(camera, request->controls());\n> +\n> return 0;\n> }\n> \n> @@ -965,7 +1099,9 @@ bool PipelineHandlerMaliC55::registerSensorCamera(MediaLink *ispLink)\n> if (data->init())\n> return false;\n> \n> - /* \\todo: Init properties and controls. */\n> + /* \\todo: Init properties. */\n> +\n> + data->updateControls();\n> \n> registerMaliCamera(std::move(data), sensor->name());\n> }\n> -- \n> 2.30.2\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 C9D3DC3237\n\tfor ;\n\tThu, 13 Jun 2024 19:33:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BEDDA65490;\n\tThu, 13 Jun 2024 21:33:16 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DD1016548D\n\tfor ;\n\tThu, 13 Jun 2024 21:33:14 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 73CE74CF;\n\tThu, 13 Jun 2024 21:33:00 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"l3RorUzB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718307180;\n\tbh=tkMEANguBAbZK3FWXfRPcqWDQmCBP+pFbjxkIVoZ8vU=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=l3RorUzBXWaV0cP/JMyOeFL/KONRYFYVJIoarsmkOVTQ+EhszdtRl408raHjLlzIu\n\tYBxcpM9/WN3zlbuWogAx+vblTRIqxiJrpp1Szd9ZgjgjwmaVnoR0lRGqAQuIMbM8Io\n\tD1JBrbMJRnLJ/tGB+xlNQIGPGR4+gxTxP/7eHBCA=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240613155949.1041061-4-dan.scally@ideasonboard.com>","References":"<20240613155949.1041061-1-dan.scally@ideasonboard.com>\n\t<20240613155949.1041061-4-dan.scally@ideasonboard.com>","Subject":"Re: [PATCH 3/7] mali-c55: implement support for ScalerCrop","From":"Kieran Bingham ","Cc":"Jacopo Mondi ","To":"Daniel Scally ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 13 Jun 2024 20:33:11 +0100","Message-ID":"<171830719141.2248009.16384466080783047609@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":29965,"web_url":"https://patchwork.libcamera.org/comment/29965/","msgid":"<20240617051133.GF28126@pendragon.ideasonboard.com>","date":"2024-06-17T05:11:33","subject":"Re: [PATCH 3/7] mali-c55: implement support for ScalerCrop","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Jun 13, 2024 at 08:33:11PM +0100, Kieran Bingham wrote:\n> Quoting Daniel Scally (2024-06-13 16:59:45)\n> > From: Jacopo Mondi \n> > \n> > Implement support for the ScalerCrop control that allows to apply a\n> > digital zoom to the captured streams.\n> > \n> > Initialize the camera controls at camera registration time and update\n> > them at configure time as the sensor's analogue crop size might change\n> > depending on the desired Camera configuration.\n> > \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/mali-c55/mali-c55.cpp | 138 ++++++++++++++++++-\n> > 1 file changed, 137 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> > index c4f1afbc..2c34f3e9 100644\n> > --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> > @@ -100,6 +100,8 @@ public:\n> > \n> > PixelFormat bestRawFormat() const;\n> > \n> > + void updateControls();\n> > +\n> > PixelFormat adjustRawFormat(const PixelFormat &pixFmt) const;\n> > Size adjustRawSizes(const PixelFormat &pixFmt, const Size &rawSize) const;\n> > \n> > @@ -245,6 +247,27 @@ PixelFormat MaliC55CameraData::bestRawFormat() const\n> > return rawFormat;\n> > }\n> > \n> > +void MaliC55CameraData::updateControls()\n> > +{\n> > + if (!sensor_)\n> > + return;\n> > +\n> > + IPACameraSensorInfo sensorInfo;\n> > + int ret = sensor_->sensorInfo(&sensorInfo);\n> > + if (ret) {\n> > + LOG(MaliC55, Error) << \"Failed to retrieve sensor info\";\n> > + return;\n> > + }\n> > +\n> > + ControlInfoMap::Map controls;\n> > + Rectangle ispMinCrop{ 0, 0, 640, 480 };\n> \n> This looks like it should be a constant somewhere outside of here?\n> Perhaps class constant or just a constant at the top of the file.\n> \n> > + controls[&controls::ScalerCrop] =\n> > + ControlInfo(ispMinCrop, sensorInfo.analogCrop,\n> > + sensorInfo.analogCrop);\n> > +\n> > + controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);\n> > +}\n> > +\n> > /*\n> > * Make sure the provided raw pixel format is supported and adjust it to\n> > * one of the supported ones if it's not.\n> > @@ -515,6 +538,8 @@ private:\n> > const StreamConfiguration &config,\n> > V4L2SubdeviceFormat &subdevFormat);\n> > \n> > + void applyScalerCrop(Camera *camera, const ControlList &controls);\n> > +\n> > void registerMaliCamera(std::unique_ptr data,\n> > const std::string &name);\n> > bool registerTPGCamera(MediaLink *link);\n> > @@ -828,6 +853,8 @@ int PipelineHandlerMaliC55::configure(Camera *camera,\n> > pipe->stream = stream;\n> > }\n> > \n> > + data->updateControls();\n> > +\n> > return 0;\n> > }\n> > \n> > @@ -875,6 +902,104 @@ void PipelineHandlerMaliC55::stopDevice([[maybe_unused]] Camera *camera)\n> > }\n> > }\n> > \n> > +void PipelineHandlerMaliC55::applyScalerCrop(Camera *camera,\n> > + const ControlList &controls)\n> > +{\n> > + MaliC55CameraData *data = cameraData(camera);\n> > +\n> > + const auto &scalerCrop = controls.get(controls::ScalerCrop);\n> > + if (!scalerCrop)\n> > + return;\n> > +\n> > + if (!data->sensor_) {\n> > + LOG(MaliC55, Error) << \"ScalerCrop not supported for TPG\";\n> > + return;\n> > + }\n> > +\n> > + Rectangle nativeCrop = *scalerCrop;\n> > +\n> > + IPACameraSensorInfo sensorInfo;\n> > + int ret = data->sensor_->sensorInfo(&sensorInfo);\n\nGathering the sensor info is a bit expensive, it should be cached.\n\n> > + if (ret) {\n> > + LOG(MaliC55, Error) << \"Failed to retrieve sensor info\";\n> > + return;\n> > + }\n> > +\n> > + /*\n> > + * The ScalerCrop rectangle re-scaling in the ISP crop rectangle\n> > + * comes straight from the RPi pipeline handler.\n\nDoes this mean we should have a helper somewhere ? And does this comment\nbelong here, or to the commit message ?\n\n> > + *\n> > + * Create a version of the crop rectangle aligned to the analogue crop\n> > + * rectangle top-left coordinates and scaled in the [analogue crop to\n> > + * output frame] ratio to take into account binning/skipping on the\n> > + * sensor.\n> > + */\n> > + Rectangle ispCrop = nativeCrop.translatedBy(-sensorInfo.analogCrop\n> > + .topLeft());\n> > + ispCrop.scaleBy(sensorInfo.outputSize, sensorInfo.analogCrop.size());\n> > +\n> > + /*\n> > + * The crop rectangle should be:\n> > + * 1. At least as big as ispMinCropSize_, once that's been\n> > + * enlarged to the same aspect ratio.\n\nDoes this mean the hardware doesn' support sensor resolutions smaller\nthan 640x480 ?\n\n> > + * 2. With the same mid-point, if possible.\n\nWhy so ? Isn't the user in control of that ?\n\n> > + * 3. But it can't go outside the sensor area.\n> > + */\n> > + Rectangle ispMinCrop{ 0, 0, 640, 480 };\n> \n> Especially if it's used multiple times.\n> \n> > + Size minSize = ispMinCrop.size().expandedToAspectRatio(nativeCrop.size());\n> > + Size size = ispCrop.size().expandedTo(minSize);\n> > + ispCrop = size.centeredTo(ispCrop.center())\n> > + .enclosedIn(Rectangle(sensorInfo.outputSize));\n> > +\n> > + /*\n> > + * As the resizer can't upscale, the crop rectangle has to be larger\n> > + * than the larger stream output size.\n> > + */\n> > + Size maxYuvSize;\n> > + for (MaliC55Pipe &pipe : pipes_) {\n> > +\n> \n> Probably delete that blank line.\n> \n> > + if (!pipe.stream)\n> > + continue;\n> > +\n> > + const StreamConfiguration &config = pipe.stream->configuration();\n> > + if (isFormatRaw(config.pixelFormat)) {\n> > + LOG(MaliC55, Debug) << \"Cannot crop with a RAW stream\";\n> > + return;\n> > + }\n> > +\n> > + Size streamSize = config.size;\n> > + if (streamSize.width > maxYuvSize.width)\n> > + maxYuvSize.width = streamSize.width;\n> > + if (streamSize.height > maxYuvSize.height)\n> > + maxYuvSize.height = streamSize.height;\n> > + }\n> > +\n> > + ispCrop.size().expandTo(maxYuvSize);\n> > +\n> > + /*\n> > + * Now apply the scaler crop to each enabled output. This overrides the\n> > + * crop configuration performed at configure() time and can cause\n> > + * square pixels if the crop rectangle and scaler output FOV ratio are\n> \n> Can cause 'non square' pixels perhaps?\n> \n> > + * different.\n> > + */\n> > + for (MaliC55Pipe &pipe : pipes_) {\n> > +\n> > + if (!pipe.stream)\n> > + continue;\n> > +\n> > + /* Create a copy to avoid setSelection() to modify ispCrop. */\n> \n> 'to prevent setSelection() from modifying ispCrop'\n> \n> > + Rectangle pipeCrop = ispCrop;\n> > + ret = pipe.resizer->setSelection(0, V4L2_SEL_TGT_CROP, &pipeCrop);\n> > + if (ret) {\n> > + LOG(MaliC55, Error)\n> > + << \"Failed to apply crop to \"\n> > + << (pipe.stream == &data->frStream_ ?\n> > + \"FR\" : \"DS\") << \" pipe\";\n> > + return;\n> > + }\n> > + }\n\nThe effective ScalerCrop value should be reported in metadata.\n\n> > +}\n> > +\n> > int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request)\n> > {\n> > int ret;\n> > @@ -887,6 +1012,15 @@ int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request)\n> > return ret;\n> > }\n> > \n> > + /*\n> > + * Some controls need to be applied immediately, as in example,\n> > + * the ScalerCrop one.\n> > + *\n> > + * \\todo Move it buffer queue time (likely after the IPA has filled in\n> > + * the parameters buffer) once we have plumbed the IPA loop in.\n> > + */\n> > + applyScalerCrop(camera, request->controls());\n> > +\n> > return 0;\n> > }\n> > \n> > @@ -965,7 +1099,9 @@ bool PipelineHandlerMaliC55::registerSensorCamera(MediaLink *ispLink)\n> > if (data->init())\n> > return false;\n> > \n> > - /* \\todo: Init properties and controls. */\n> > + /* \\todo: Init properties. */\n> > +\n> > + data->updateControls();\n> > \n> > registerMaliCamera(std::move(data), sensor->name());\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 E0816BD87C\n\tfor ;\n\tMon, 17 Jun 2024 05:11:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B2D0F6548D;\n\tMon, 17 Jun 2024 07:11:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 21242619F8\n\tfor ;\n\tMon, 17 Jun 2024 07:11:54 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 28C1D9C1;\n\tMon, 17 Jun 2024 07:11:37 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Nmucu3Ui\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718601097;\n\tbh=VSSVNm8PhB3MkYlr50MG7/SIpRE+G+yY+VQSPDRxHpQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Nmucu3UioD0rtONkgMC93IPpZxH5qI1mPupPiYl5OpH4jz+/TrNbXPYPtztEHjnMv\n\tVBgoXMlGrSBOYf+Kq9FUyz4rNQ4VfQR8g9aZhnI/5jLuBu0KpeVe1l/hv6XKA/m29w\n\tCanDSqBJBXB535ZM+e/ZrSC70rx0vJYXpzvP8x1Q=","Date":"Mon, 17 Jun 2024 08:11:33 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Daniel Scally ,\n\tlibcamera-devel@lists.libcamera.org,\n\tJacopo Mondi ","Subject":"Re: [PATCH 3/7] mali-c55: implement support for ScalerCrop","Message-ID":"<20240617051133.GF28126@pendragon.ideasonboard.com>","References":"<20240613155949.1041061-1-dan.scally@ideasonboard.com>\n\t<20240613155949.1041061-4-dan.scally@ideasonboard.com>\n\t<171830719141.2248009.16384466080783047609@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<171830719141.2248009.16384466080783047609@ping.linuxembedded.co.uk>","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":30054,"web_url":"https://patchwork.libcamera.org/comment/30054/","msgid":"","date":"2024-06-24T15:01:42","subject":"Re: [PATCH 3/7] mali-c55: implement support for ScalerCrop","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\n\nOn Mon, Jun 17, 2024 at 08:11:33AM GMT, Laurent Pinchart wrote:\n> On Thu, Jun 13, 2024 at 08:33:11PM +0100, Kieran Bingham wrote:\n> > Quoting Daniel Scally (2024-06-13 16:59:45)\n> > > From: Jacopo Mondi \n> > >\n> > > Implement support for the ScalerCrop control that allows to apply a\n> > > digital zoom to the captured streams.\n> > >\n> > > Initialize the camera controls at camera registration time and update\n> > > them at configure time as the sensor's analogue crop size might change\n> > > depending on the desired Camera configuration.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/pipeline/mali-c55/mali-c55.cpp | 138 ++++++++++++++++++-\n> > > 1 file changed, 137 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> > > index c4f1afbc..2c34f3e9 100644\n> > > --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> > > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> > > @@ -100,6 +100,8 @@ public:\n> > >\n> > > PixelFormat bestRawFormat() const;\n> > >\n> > > + void updateControls();\n> > > +\n> > > PixelFormat adjustRawFormat(const PixelFormat &pixFmt) const;\n> > > Size adjustRawSizes(const PixelFormat &pixFmt, const Size &rawSize) const;\n> > >\n> > > @@ -245,6 +247,27 @@ PixelFormat MaliC55CameraData::bestRawFormat() const\n> > > return rawFormat;\n> > > }\n> > >\n> > > +void MaliC55CameraData::updateControls()\n> > > +{\n> > > + if (!sensor_)\n> > > + return;\n> > > +\n> > > + IPACameraSensorInfo sensorInfo;\n> > > + int ret = sensor_->sensorInfo(&sensorInfo);\n> > > + if (ret) {\n> > > + LOG(MaliC55, Error) << \"Failed to retrieve sensor info\";\n> > > + return;\n> > > + }\n> > > +\n> > > + ControlInfoMap::Map controls;\n> > > + Rectangle ispMinCrop{ 0, 0, 640, 480 };\n> >\n> > This looks like it should be a constant somewhere outside of here?\n> > Perhaps class constant or just a constant at the top of the file.\n> >\n\nack\n\n> > > + controls[&controls::ScalerCrop] =\n> > > + ControlInfo(ispMinCrop, sensorInfo.analogCrop,\n> > > + sensorInfo.analogCrop);\n> > > +\n> > > + controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);\n> > > +}\n> > > +\n> > > /*\n> > > * Make sure the provided raw pixel format is supported and adjust it to\n> > > * one of the supported ones if it's not.\n> > > @@ -515,6 +538,8 @@ private:\n> > > const StreamConfiguration &config,\n> > > V4L2SubdeviceFormat &subdevFormat);\n> > >\n> > > + void applyScalerCrop(Camera *camera, const ControlList &controls);\n> > > +\n> > > void registerMaliCamera(std::unique_ptr data,\n> > > const std::string &name);\n> > > bool registerTPGCamera(MediaLink *link);\n> > > @@ -828,6 +853,8 @@ int PipelineHandlerMaliC55::configure(Camera *camera,\n> > > pipe->stream = stream;\n> > > }\n> > >\n> > > + data->updateControls();\n> > > +\n> > > return 0;\n> > > }\n> > >\n> > > @@ -875,6 +902,104 @@ void PipelineHandlerMaliC55::stopDevice([[maybe_unused]] Camera *camera)\n> > > }\n> > > }\n> > >\n> > > +void PipelineHandlerMaliC55::applyScalerCrop(Camera *camera,\n> > > + const ControlList &controls)\n> > > +{\n> > > + MaliC55CameraData *data = cameraData(camera);\n> > > +\n> > > + const auto &scalerCrop = controls.get(controls::ScalerCrop);\n> > > + if (!scalerCrop)\n> > > + return;\n> > > +\n> > > + if (!data->sensor_) {\n> > > + LOG(MaliC55, Error) << \"ScalerCrop not supported for TPG\";\n> > > + return;\n> > > + }\n> > > +\n> > > + Rectangle nativeCrop = *scalerCrop;\n> > > +\n> > > + IPACameraSensorInfo sensorInfo;\n> > > + int ret = data->sensor_->sensorInfo(&sensorInfo);\n>\n> Gathering the sensor info is a bit expensive, it should be cached.\n>\n\nI can cache it at configure() time, even if the pixel rate and\nblanking values might be outdated as they can change during streaming\ntime.\n\nIn general, sensorInfo describes the current sensor configuration,\nit's not desirable to cache it. However here I actually only need the\nsensor's output size and the analog crop which shouldn't change during\nstreaming. Should I cache the two ? It seems a bit of an hack to have\nthem cached in CameraData.\n\n> > > + if (ret) {\n> > > + LOG(MaliC55, Error) << \"Failed to retrieve sensor info\";\n> > > + return;\n> > > + }\n> > > +\n> > > + /*\n> > > + * The ScalerCrop rectangle re-scaling in the ISP crop rectangle\n> > > + * comes straight from the RPi pipeline handler.\n>\n> Does this mean we should have a helper somewhere ? And does this comment\n\nI'm not sure where should I put it. This combines properties of the\nCameraSensor class with ISP's ones (the min crop size) and with the\nvalue of an application provided control. Could go in CameraSensor but a\nCameraSensor::rescaleScalerCrop() feels a bit out of place ?\n\n> belong here, or to the commit message ?\n>\n\nYes\n\n> > > + *\n> > > + * Create a version of the crop rectangle aligned to the analogue crop\n> > > + * rectangle top-left coordinates and scaled in the [analogue crop to\n> > > + * output frame] ratio to take into account binning/skipping on the\n> > > + * sensor.\n> > > + */\n> > > + Rectangle ispCrop = nativeCrop.translatedBy(-sensorInfo.analogCrop\n> > > + .topLeft());\n> > > + ispCrop.scaleBy(sensorInfo.outputSize, sensorInfo.analogCrop.size());\n> > > +\n> > > + /*\n> > > + * The crop rectangle should be:\n> > > + * 1. At least as big as ispMinCropSize_, once that's been\n> > > + * enlarged to the same aspect ratio.\n>\n> Does this mean the hardware doesn' support sensor resolutions smaller\n> than 640x480 ?\n>\n\nYes, the TRM reports:\n\nMinimum supported resolution: 640 (w) x 480 (h)\n\nNow I wonder if is this the minimum supported sensor input resolution or the\nISP minimum output size ?\n\nIf 640x480 represents the minimum allowed input size, then it\nshouldn't be here. We crop on the resizer's input, not on the ISP\ninput, so (unless there's a minimum resizer's crop limit) we shouldn't\ncare about this here, but rather filter out all sensor modes smaller\nthan 640x480 during validate().\n\nIf 640x480 represnts the minimum output resolution, then we should fix\n\n constexpr Size kMaliC55MinSize = { 128, 128 };\n\nDan do you have any idea here ?\n\n> > > + * 2. With the same mid-point, if possible.\n>\n> Why so ? Isn't the user in control of that ?\n\nIt is, if you look at\n\n Size minSize = ispMinCrop.size().expandedToAspectRatio(nativeCrop.size());\n Size size = ispCrop.size().expandedTo(minSize);\n ispCrop = size.centeredTo(ispCrop.center())\n .enclosedIn(Rectangle(sensorInfo.outputSize));\n\n'size' is the ScalerCrop size expanded to the minimum accepted ISP crop\nsize, and we're centering it inside 'ispCrop.center()' and\n'ispCrop' already has a top-left corner set by the user relatively to\nthe native pixel array size (and re-scaled in the analogueCrop-to-outputSize\nratio).\n\nSo, if ispCrop.size() > minSize then\n\n Size size = ispCrop.size().expandedTo(minSize);\n\nwill give you ispCrop.size() and the rectangle resulting from\ncentering 'size' inside 'ispCrop' will be 'ispCrop' again.\n\nIf ispCrop.size() < minSize then ispCrop will be expanded to minSize\nand the resulting Rectangle will be centered over ispCrop.\n\nDoes this match your understanding ?\n\nHowever, there is one thing missing in this patch:\n\n * Create a version of the crop rectangle aligned to the analogue crop\n * rectangle top-left coordinates\n */\n Rectangle ispCrop = nativeCrop.translatedBy(-sensorInfo.analogCrop\n .topLeft());\n\nAssumes the ScalerCrop is contained in the analogueCrop rectangle,\notherwise the here above 'translatedBy' will give you a negative\ntop-left corner.\n\nThis is actually correct if we assume controls::ScalerCropMaximum\nis set to be the AnalogueCrop (so all ScalerCrop will be contained in\nAnalogueCrop). RPi does set ScalerCropMaximum, this patch doesn't.\n\nI presume however we should actually make sure the actual value of\nScalerCrop is at least contained in the AnalogueCrop instead of\nassuming the application does the right thing. In case ScalerCrop >\nAnalogueCrop should we adjust it and return its value without applying\nany crop ? An helper in CameraSensor could actually help addressing\nthis for all pipelines\n\n>\n> > > + * 3. But it can't go outside the sensor area.\n> > > + */\n> > > + Rectangle ispMinCrop{ 0, 0, 640, 480 };\n> >\n> > Especially if it's used multiple times.\n> >\n> > > + Size minSize = ispMinCrop.size().expandedToAspectRatio(nativeCrop.size());\n> > > + Size size = ispCrop.size().expandedTo(minSize);\n> > > + ispCrop = size.centeredTo(ispCrop.center())\n> > > + .enclosedIn(Rectangle(sensorInfo.outputSize));\n> > > +\n> > > + /*\n> > > + * As the resizer can't upscale, the crop rectangle has to be larger\n> > > + * than the larger stream output size.\n> > > + */\n> > > + Size maxYuvSize;\n> > > + for (MaliC55Pipe &pipe : pipes_) {\n> > > +\n> >\n> > Probably delete that blank line.\n> >\n\nI like it! :)\n\n> > > + if (!pipe.stream)\n> > > + continue;\n> > > +\n> > > + const StreamConfiguration &config = pipe.stream->configuration();\n> > > + if (isFormatRaw(config.pixelFormat)) {\n> > > + LOG(MaliC55, Debug) << \"Cannot crop with a RAW stream\";\n> > > + return;\n> > > + }\n> > > +\n> > > + Size streamSize = config.size;\n> > > + if (streamSize.width > maxYuvSize.width)\n> > > + maxYuvSize.width = streamSize.width;\n> > > + if (streamSize.height > maxYuvSize.height)\n> > > + maxYuvSize.height = streamSize.height;\n> > > + }\n> > > +\n> > > + ispCrop.size().expandTo(maxYuvSize);\n> > > +\n> > > + /*\n> > > + * Now apply the scaler crop to each enabled output. This overrides the\n> > > + * crop configuration performed at configure() time and can cause\n> > > + * square pixels if the crop rectangle and scaler output FOV ratio are\n> >\n> > Can cause 'non square' pixels perhaps?\n> >\n\nAh! I always said \"square pixels\" to indicate this situation,\nbut in my mind \"square pixels\" were good. So I thought it was an\nexpression in English I didn't fully understand :) I'll fix it.\n\n> > > + * different.\n> > > + */\n> > > + for (MaliC55Pipe &pipe : pipes_) {\n> > > +\n> > > + if (!pipe.stream)\n> > > + continue;\n> > > +\n> > > + /* Create a copy to avoid setSelection() to modify ispCrop. */\n> >\n> > 'to prevent setSelection() from modifying ispCrop'\n> >\n> > > + Rectangle pipeCrop = ispCrop;\n> > > + ret = pipe.resizer->setSelection(0, V4L2_SEL_TGT_CROP, &pipeCrop);\n> > > + if (ret) {\n> > > + LOG(MaliC55, Error)\n> > > + << \"Failed to apply crop to \"\n> > > + << (pipe.stream == &data->frStream_ ?\n> > > + \"FR\" : \"DS\") << \" pipe\";\n> > > + return;\n> > > + }\n> > > + }\n>\n> The effective ScalerCrop value should be reported in metadata.\n>\n\ntrue that\n\n> > > +}\n> > > +\n> > > int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request)\n> > > {\n> > > int ret;\n> > > @@ -887,6 +1012,15 @@ int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request)\n> > > return ret;\n> > > }\n> > >\n> > > + /*\n> > > + * Some controls need to be applied immediately, as in example,\n> > > + * the ScalerCrop one.\n> > > + *\n> > > + * \\todo Move it buffer queue time (likely after the IPA has filled in\n> > > + * the parameters buffer) once we have plumbed the IPA loop in.\n> > > + */\n> > > + applyScalerCrop(camera, request->controls());\n> > > +\n> > > return 0;\n> > > }\n> > >\n> > > @@ -965,7 +1099,9 @@ bool PipelineHandlerMaliC55::registerSensorCamera(MediaLink *ispLink)\n> > > if (data->init())\n> > > return false;\n> > >\n> > > - /* \\todo: Init properties and controls. */\n> > > + /* \\todo: Init properties. */\n> > > +\n> > > + data->updateControls();\n> > >\n> > > registerMaliCamera(std::move(data), sensor->name());\n> > > }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 2132BBD87C\n\tfor ;\n\tMon, 24 Jun 2024 15:01:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 11A33654A9;\n\tMon, 24 Jun 2024 17:01:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7BD74654A1\n\tfor ;\n\tMon, 24 Jun 2024 17:01:45 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 58AA07E0;\n\tMon, 24 Jun 2024 17:01:23 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"pCG1mcXW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719241283;\n\tbh=H+UTZPjYHGp64DghINipvopDuWh2kcpffVbm6b0vMqo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pCG1mcXW+4LHxa7v9rnaBHCzDBw+i87dGnlCvpfRClqedsuGRCvltpLnqzFHbBUZ1\n\tBSyq7zb5IkDnNmNysjrXflf8bE9t6Okcd7JAMkB4MtrADwkrAj+0ePEf0hqiXd+oX1\n\tSabWyOJvYObCCN4f2e9elOvtnYGQEweIlM3uBeII=","Date":"Mon, 24 Jun 2024 17:01:42 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Kieran Bingham , \n\tDaniel Scally ,\n\tlibcamera-devel@lists.libcamera.org, \n\tJacopo Mondi ","Subject":"Re: [PATCH 3/7] mali-c55: implement support for ScalerCrop","Message-ID":"","References":"<20240613155949.1041061-1-dan.scally@ideasonboard.com>\n\t<20240613155949.1041061-4-dan.scally@ideasonboard.com>\n\t<171830719141.2248009.16384466080783047609@ping.linuxembedded.co.uk>\n\t<20240617051133.GF28126@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240617051133.GF28126@pendragon.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":30055,"web_url":"https://patchwork.libcamera.org/comment/30055/","msgid":"<7f118c9b-8720-4fdc-847f-07537ce858f9@ideasonboard.com>","date":"2024-06-24T15:08:17","subject":"Re: [PATCH 3/7] mali-c55: implement support for ScalerCrop","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hello both\n\nOn 24/06/2024 16:01, Jacopo Mondi wrote:\n> Hi Laurent\n>\n> On Mon, Jun 17, 2024 at 08:11:33AM GMT, Laurent Pinchart wrote:\n>> On Thu, Jun 13, 2024 at 08:33:11PM +0100, Kieran Bingham wrote:\n>>> Quoting Daniel Scally (2024-06-13 16:59:45)\n>>>> From: Jacopo Mondi \n>>>>\n>>>> Implement support for the ScalerCrop control that allows to apply a\n>>>> digital zoom to the captured streams.\n>>>>\n>>>> Initialize the camera controls at camera registration time and update\n>>>> them at configure time as the sensor's analogue crop size might change\n>>>> depending on the desired Camera configuration.\n>>>>\n>>>> Signed-off-by: Jacopo Mondi \n>>>> ---\n>>>> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 138 ++++++++++++++++++-\n>>>> 1 file changed, 137 insertions(+), 1 deletion(-)\n>>>>\n>>>> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n>>>> index c4f1afbc..2c34f3e9 100644\n>>>> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n>>>> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n>>>> @@ -100,6 +100,8 @@ public:\n>>>>\n>>>> PixelFormat bestRawFormat() const;\n>>>>\n>>>> + void updateControls();\n>>>> +\n>>>> PixelFormat adjustRawFormat(const PixelFormat &pixFmt) const;\n>>>> Size adjustRawSizes(const PixelFormat &pixFmt, const Size &rawSize) const;\n>>>>\n>>>> @@ -245,6 +247,27 @@ PixelFormat MaliC55CameraData::bestRawFormat() const\n>>>> return rawFormat;\n>>>> }\n>>>>\n>>>> +void MaliC55CameraData::updateControls()\n>>>> +{\n>>>> + if (!sensor_)\n>>>> + return;\n>>>> +\n>>>> + IPACameraSensorInfo sensorInfo;\n>>>> + int ret = sensor_->sensorInfo(&sensorInfo);\n>>>> + if (ret) {\n>>>> + LOG(MaliC55, Error) << \"Failed to retrieve sensor info\";\n>>>> + return;\n>>>> + }\n>>>> +\n>>>> + ControlInfoMap::Map controls;\n>>>> + Rectangle ispMinCrop{ 0, 0, 640, 480 };\n>>> This looks like it should be a constant somewhere outside of here?\n>>> Perhaps class constant or just a constant at the top of the file.\n>>>\n> ack\n>\n>>>> + controls[&controls::ScalerCrop] =\n>>>> + ControlInfo(ispMinCrop, sensorInfo.analogCrop,\n>>>> + sensorInfo.analogCrop);\n>>>> +\n>>>> + controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);\n>>>> +}\n>>>> +\n>>>> /*\n>>>> * Make sure the provided raw pixel format is supported and adjust it to\n>>>> * one of the supported ones if it's not.\n>>>> @@ -515,6 +538,8 @@ private:\n>>>> const StreamConfiguration &config,\n>>>> V4L2SubdeviceFormat &subdevFormat);\n>>>>\n>>>> + void applyScalerCrop(Camera *camera, const ControlList &controls);\n>>>> +\n>>>> void registerMaliCamera(std::unique_ptr data,\n>>>> const std::string &name);\n>>>> bool registerTPGCamera(MediaLink *link);\n>>>> @@ -828,6 +853,8 @@ int PipelineHandlerMaliC55::configure(Camera *camera,\n>>>> pipe->stream = stream;\n>>>> }\n>>>>\n>>>> + data->updateControls();\n>>>> +\n>>>> return 0;\n>>>> }\n>>>>\n>>>> @@ -875,6 +902,104 @@ void PipelineHandlerMaliC55::stopDevice([[maybe_unused]] Camera *camera)\n>>>> }\n>>>> }\n>>>>\n>>>> +void PipelineHandlerMaliC55::applyScalerCrop(Camera *camera,\n>>>> + const ControlList &controls)\n>>>> +{\n>>>> + MaliC55CameraData *data = cameraData(camera);\n>>>> +\n>>>> + const auto &scalerCrop = controls.get(controls::ScalerCrop);\n>>>> + if (!scalerCrop)\n>>>> + return;\n>>>> +\n>>>> + if (!data->sensor_) {\n>>>> + LOG(MaliC55, Error) << \"ScalerCrop not supported for TPG\";\n>>>> + return;\n>>>> + }\n>>>> +\n>>>> + Rectangle nativeCrop = *scalerCrop;\n>>>> +\n>>>> + IPACameraSensorInfo sensorInfo;\n>>>> + int ret = data->sensor_->sensorInfo(&sensorInfo);\n>> Gathering the sensor info is a bit expensive, it should be cached.\n>>\n> I can cache it at configure() time, even if the pixel rate and\n> blanking values might be outdated as they can change during streaming\n> time.\n>\n> In general, sensorInfo describes the current sensor configuration,\n> it's not desirable to cache it. However here I actually only need the\n> sensor's output size and the analog crop which shouldn't change during\n> streaming. Should I cache the two ? It seems a bit of an hack to have\n> them cached in CameraData.\n>\n>>>> + if (ret) {\n>>>> + LOG(MaliC55, Error) << \"Failed to retrieve sensor info\";\n>>>> + return;\n>>>> + }\n>>>> +\n>>>> + /*\n>>>> + * The ScalerCrop rectangle re-scaling in the ISP crop rectangle\n>>>> + * comes straight from the RPi pipeline handler.\n>> Does this mean we should have a helper somewhere ? And does this comment\n> I'm not sure where should I put it. This combines properties of the\n> CameraSensor class with ISP's ones (the min crop size) and with the\n> value of an application provided control. Could go in CameraSensor but a\n> CameraSensor::rescaleScalerCrop() feels a bit out of place ?\n>\n>> belong here, or to the commit message ?\n>>\n> Yes\n>\n>>>> + *\n>>>> + * Create a version of the crop rectangle aligned to the analogue crop\n>>>> + * rectangle top-left coordinates and scaled in the [analogue crop to\n>>>> + * output frame] ratio to take into account binning/skipping on the\n>>>> + * sensor.\n>>>> + */\n>>>> + Rectangle ispCrop = nativeCrop.translatedBy(-sensorInfo.analogCrop\n>>>> + .topLeft());\n>>>> + ispCrop.scaleBy(sensorInfo.outputSize, sensorInfo.analogCrop.size());\n>>>> +\n>>>> + /*\n>>>> + * The crop rectangle should be:\n>>>> + * 1. At least as big as ispMinCropSize_, once that's been\n>>>> + * enlarged to the same aspect ratio.\n>> Does this mean the hardware doesn' support sensor resolutions smaller\n>> than 640x480 ?\n>>\n> Yes, the TRM reports:\n>\n> Minimum supported resolution: 640 (w) x 480 (h)\n>\n> Now I wonder if is this the minimum supported sensor input resolution or the\n> ISP minimum output size ?\n>\n> If 640x480 represents the minimum allowed input size, then it\n> shouldn't be here. We crop on the resizer's input, not on the ISP\n> input, so (unless there's a minimum resizer's crop limit) we shouldn't\n> care about this here, but rather filter out all sensor modes smaller\n> than 640x480 during validate().\n>\n> If 640x480 represnts the minimum output resolution, then we should fix\n>\n> constexpr Size kMaliC55MinSize = { 128, 128 };\n>\n> Dan do you have any idea here ?\n\n\nI think it's both minimum supported input and output; I discussed with Arm smaller input framesizes \n(in the context of the TPG at the time) and the conclusion from the discussion was not to support \nsmaller than 640x480.\n\n>\n>>>> + * 2. With the same mid-point, if possible.\n>> Why so ? Isn't the user in control of that ?\n> It is, if you look at\n>\n> Size minSize = ispMinCrop.size().expandedToAspectRatio(nativeCrop.size());\n> Size size = ispCrop.size().expandedTo(minSize);\n> ispCrop = size.centeredTo(ispCrop.center())\n> .enclosedIn(Rectangle(sensorInfo.outputSize));\n>\n> 'size' is the ScalerCrop size expanded to the minimum accepted ISP crop\n> size, and we're centering it inside 'ispCrop.center()' and\n> 'ispCrop' already has a top-left corner set by the user relatively to\n> the native pixel array size (and re-scaled in the analogueCrop-to-outputSize\n> ratio).\n>\n> So, if ispCrop.size() > minSize then\n>\n> Size size = ispCrop.size().expandedTo(minSize);\n>\n> will give you ispCrop.size() and the rectangle resulting from\n> centering 'size' inside 'ispCrop' will be 'ispCrop' again.\n>\n> If ispCrop.size() < minSize then ispCrop will be expanded to minSize\n> and the resulting Rectangle will be centered over ispCrop.\n>\n> Does this match your understanding ?\n>\n> However, there is one thing missing in this patch:\n>\n> * Create a version of the crop rectangle aligned to the analogue crop\n> * rectangle top-left coordinates\n> */\n> Rectangle ispCrop = nativeCrop.translatedBy(-sensorInfo.analogCrop\n> .topLeft());\n>\n> Assumes the ScalerCrop is contained in the analogueCrop rectangle,\n> otherwise the here above 'translatedBy' will give you a negative\n> top-left corner.\n>\n> This is actually correct if we assume controls::ScalerCropMaximum\n> is set to be the AnalogueCrop (so all ScalerCrop will be contained in\n> AnalogueCrop). RPi does set ScalerCropMaximum, this patch doesn't.\n>\n> I presume however we should actually make sure the actual value of\n> ScalerCrop is at least contained in the AnalogueCrop instead of\n> assuming the application does the right thing. In case ScalerCrop >\n> AnalogueCrop should we adjust it and return its value without applying\n> any crop ? An helper in CameraSensor could actually help addressing\n> this for all pipelines\n>\n>>>> + * 3. But it can't go outside the sensor area.\n>>>> + */\n>>>> + Rectangle ispMinCrop{ 0, 0, 640, 480 };\n>>> Especially if it's used multiple times.\n>>>\n>>>> + Size minSize = ispMinCrop.size().expandedToAspectRatio(nativeCrop.size());\n>>>> + Size size = ispCrop.size().expandedTo(minSize);\n>>>> + ispCrop = size.centeredTo(ispCrop.center())\n>>>> + .enclosedIn(Rectangle(sensorInfo.outputSize));\n>>>> +\n>>>> + /*\n>>>> + * As the resizer can't upscale, the crop rectangle has to be larger\n>>>> + * than the larger stream output size.\n>>>> + */\n>>>> + Size maxYuvSize;\n>>>> + for (MaliC55Pipe &pipe : pipes_) {\n>>>> +\n>>> Probably delete that blank line.\n>>>\n> I like it! :)\n>\n>>>> + if (!pipe.stream)\n>>>> + continue;\n>>>> +\n>>>> + const StreamConfiguration &config = pipe.stream->configuration();\n>>>> + if (isFormatRaw(config.pixelFormat)) {\n>>>> + LOG(MaliC55, Debug) << \"Cannot crop with a RAW stream\";\n>>>> + return;\n>>>> + }\n>>>> +\n>>>> + Size streamSize = config.size;\n>>>> + if (streamSize.width > maxYuvSize.width)\n>>>> + maxYuvSize.width = streamSize.width;\n>>>> + if (streamSize.height > maxYuvSize.height)\n>>>> + maxYuvSize.height = streamSize.height;\n>>>> + }\n>>>> +\n>>>> + ispCrop.size().expandTo(maxYuvSize);\n>>>> +\n>>>> + /*\n>>>> + * Now apply the scaler crop to each enabled output. This overrides the\n>>>> + * crop configuration performed at configure() time and can cause\n>>>> + * square pixels if the crop rectangle and scaler output FOV ratio are\n>>> Can cause 'non square' pixels perhaps?\n>>>\n> Ah! I always said \"square pixels\" to indicate this situation,\n> but in my mind \"square pixels\" were good. So I thought it was an\n> expression in English I didn't fully understand :) I'll fix it.\n>\n>>>> + * different.\n>>>> + */\n>>>> + for (MaliC55Pipe &pipe : pipes_) {\n>>>> +\n>>>> + if (!pipe.stream)\n>>>> + continue;\n>>>> +\n>>>> + /* Create a copy to avoid setSelection() to modify ispCrop. */\n>>> 'to prevent setSelection() from modifying ispCrop'\n>>>\n>>>> + Rectangle pipeCrop = ispCrop;\n>>>> + ret = pipe.resizer->setSelection(0, V4L2_SEL_TGT_CROP, &pipeCrop);\n>>>> + if (ret) {\n>>>> + LOG(MaliC55, Error)\n>>>> + << \"Failed to apply crop to \"\n>>>> + << (pipe.stream == &data->frStream_ ?\n>>>> + \"FR\" : \"DS\") << \" pipe\";\n>>>> + return;\n>>>> + }\n>>>> + }\n>> The effective ScalerCrop value should be reported in metadata.\n>>\n> true that\n>\n>>>> +}\n>>>> +\n>>>> int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request)\n>>>> {\n>>>> int ret;\n>>>> @@ -887,6 +1012,15 @@ int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request)\n>>>> return ret;\n>>>> }\n>>>>\n>>>> + /*\n>>>> + * Some controls need to be applied immediately, as in example,\n>>>> + * the ScalerCrop one.\n>>>> + *\n>>>> + * \\todo Move it buffer queue time (likely after the IPA has filled in\n>>>> + * the parameters buffer) once we have plumbed the IPA loop in.\n>>>> + */\n>>>> + applyScalerCrop(camera, request->controls());\n>>>> +\n>>>> return 0;\n>>>> }\n>>>>\n>>>> @@ -965,7 +1099,9 @@ bool PipelineHandlerMaliC55::registerSensorCamera(MediaLink *ispLink)\n>>>> if (data->init())\n>>>> return false;\n>>>>\n>>>> - /* \\todo: Init properties and controls. */\n>>>> + /* \\todo: Init properties. */\n>>>> +\n>>>> + data->updateControls();\n>>>>\n>>>> registerMaliCamera(std::move(data), sensor->name());\n>>>> }\n>> --\n>> Regards,\n>>\n>> Laurent Pinchart","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 7E92FBDB1D\n\tfor ;\n\tMon, 24 Jun 2024 15:08:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8AEC0654A9;\n\tMon, 24 Jun 2024 17:08:22 +0200 (CEST)","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 99E8C654A1\n\tfor ;\n\tMon, 24 Jun 2024 17:08:20 +0200 (CEST)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 38DA17E0;\n\tMon, 24 Jun 2024 17:07:58 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ntVUqDUl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719241678;\n\tbh=xdu5dD/dqI3/qiiUlFE1Y+Iu3zyKDdXFou3Xsli00QI=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=ntVUqDUlFod3wYg/mGi/otNkLMNwGpBq6LCAlX7otl5uvxWc7bnhI0zHQT3sQ3g8V\n\t3lIJMIpGwYNtuZhMRKQL2kCqmOPNyFj11dXePyn9HKinMg2LM2phiNd3vKxJErdJ+f\n\tRXG5Z0llqeZgbLeh3BtQppc9m1ahRCvbf9XdIfXo=","Message-ID":"<7f118c9b-8720-4fdc-847f-07537ce858f9@ideasonboard.com>","Date":"Mon, 24 Jun 2024 16:08:17 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 3/7] mali-c55: implement support for ScalerCrop","To":"Jacopo Mondi ,\n\tLaurent Pinchart ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20240613155949.1041061-1-dan.scally@ideasonboard.com>\n\t<20240613155949.1041061-4-dan.scally@ideasonboard.com>\n\t<171830719141.2248009.16384466080783047609@ping.linuxembedded.co.uk>\n\t<20240617051133.GF28126@pendragon.ideasonboard.com>\n\t","Content-Language":"en-US","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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\" "}}]