[{"id":31652,"web_url":"https://patchwork.libcamera.org/comment/31652/","msgid":"<172848686832.532453.10615828971267421523@ping.linuxembedded.co.uk>","date":"2024-10-09T15:14:28","subject":"Re: [PATCH v4 13/13] 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-07-09 15:39:13)\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> Reviewed-by: Daniel Scally \n> Signed-off-by: Jacopo Mondi \n> Signed-off-by: Daniel Scally \n> ---\n> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 133 +++++++++++++++++++\n> 1 file changed, 133 insertions(+)\n> \n> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> index f5ca2ca4..916b1d30 100644\n> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> @@ -85,6 +85,8 @@ public:\n> int pixfmtToMbusCode(const PixelFormat &pixFmt) const;\n> const 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> @@ -264,6 +266,27 @@ const PixelFormat &MaliC55CameraData::bestRawFormat() const\n> return invalidPixFmt;\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\nI'm weary that 0,0 might not be accurate.\nI assume 640x480 is the minimum size from the ISP, but we might also\nhave a minimum x,y if the PixelArrayActiveAreas doesn't start at 0,0...\n\nhttps://libcamera.org/api-html/namespacelibcamera_1_1controls.html#a854f9e84af827f57ada03fcc12090c56\n```\n◆ ScalerCrop\nlibcamera::controls::ScalerCrop\n\nSets the image portion that will be scaled to form the whole of the\nfinal output image.\n\nThe (x,y) location of this rectangle is relative to the\nPixelArrayActiveAreas that is being used. The units remain native sensor\npixels, even if the sensor is being used in a binning or skipping mode.\n\nThis control is only present when the pipeline supports scaling. Its\nmaximum valid value is given by the properties::ScalerCropMaximum\nproperty, and the two can be used to implement digital zoom.\n```\n\n\n$ cam -c1 -p\n\nUsing camera /base/soc@0/bus@30800000/i2c@30a30000/sensor@1a as cam0\nProperty: SystemDevices = [ 20743, 20744 ]\nProperty: PixelArrayActiveAreas = [ (108, 40)/5472x3648 ]\nProperty: PixelArraySize = 5472x3648\n\n\nSo for this sensor (IMX283) - the minimum ScalerCrop surely must be\n\t{ 108, 40, 640, 480 }; ?\n\n\nOhhh - wait. No it's \"relative\" to the PixelArrayActiveAreas? So 0,0\nscaler crop is correct!\n\n\nI think we've got some work (in core and all pipeline handlers) to make\nsure we clear up exactly what pixels are being delivered to\napplications when modes and crops are adjusted...\n\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> @@ -544,6 +567,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> @@ -871,6 +896,8 @@ int PipelineHandlerMaliC55::configure(Camera *camera,\n> pipe->stream = stream;\n> }\n> \n> + data->updateControls();\n> +\n> return 0;\n> }\n> \n> @@ -918,6 +945,102 @@ 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> + 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> + 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> + * different.\n> + */\n> + for (MaliC55Pipe &pipe : pipes_) {\n> + if (!pipe.stream)\n> + continue;\n> +\n> + /* Create a copy to avoid setSelection() to modify ispCrop. */\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\nSo we're lacking 'stream' metadata ... so we can't do this per stream -\nbut I think we should definitely be reporting whatever &pipeCrop gets\nset to (as returned from the kernel) as metadata for what actually got\nset.\n\nWith that if it's possible, or just handled on top later if it's not\npossible right now:\n\n\nReviewed-by: Kieran Bingham \n\n--\nKieran\n\n\n> + }\n> +}\n> +\n> int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request)\n> {\n> int ret;\n> @@ -930,6 +1053,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> @@ -1005,6 +1137,7 @@ bool PipelineHandlerMaliC55::registerSensorCamera(MediaLink *ispLink)\n> return false;\n> \n> data->properties_ = data->sensor_->properties();\n> + data->updateControls();\n> \n> registerMaliCamera(std::move(data), sensor->name());\n> }\n> -- \n> 2.34.1\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 46554C32E0\n\tfor ;\n\tWed, 9 Oct 2024 15:14:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0CB3063538;\n\tWed, 9 Oct 2024 17:14:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 81921618C5\n\tfor ;\n\tWed, 9 Oct 2024 17:14:31 +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 E79C9594;\n\tWed, 9 Oct 2024 17:12:53 +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=\"wiLZst9w\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728486774;\n\tbh=tVNj41480ZeCrVSiLLmI3q9RkmfHM1QuPKs72jukhms=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=wiLZst9wGCjWCB0ioQGRxvUV9azIOHdR2KSsW7PCvjOK/fIT7n1aNesBof7kTOoFJ\n\tvntFCYa4zMzOyPlOg8g/ftsFO3SdUA4Z5SRH5bp9zMUIQv6KeW08du4ZspxGxX0UdP\n\tlg7TUv8M4Qxm2rOIp1G3u7cLUrgYo9cb4MqCfklo=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240709143913.3276983-14-dan.scally@ideasonboard.com>","References":"<20240709143913.3276983-1-dan.scally@ideasonboard.com>\n\t<20240709143913.3276983-14-dan.scally@ideasonboard.com>","Subject":"Re: [PATCH v4 13/13] mali-c55: implement support for ScalerCrop","From":"Kieran Bingham ","Cc":"Jacopo Mondi ,\n\tDaniel Scally ","To":"Daniel Scally ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 09 Oct 2024 16:14:28 +0100","Message-ID":"<172848686832.532453.10615828971267421523@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":31653,"web_url":"https://patchwork.libcamera.org/comment/31653/","msgid":"<172848699989.532453.6893207178058282895@ping.linuxembedded.co.uk>","date":"2024-10-09T15:16:39","subject":"Re: [PATCH v4 13/13] 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":"In $SUBJECT to be consistent with the other commits in the series,\nplease add \"libcamera: \" as a prefix.\n\n--\nKieran","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 D181CC32E0\n\tfor ;\n\tWed, 9 Oct 2024 15:16:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4FF3765369;\n\tWed, 9 Oct 2024 17:16:45 +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 818DB618C5\n\tfor ;\n\tWed, 9 Oct 2024 17:16:43 +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 C28B282A;\n\tWed, 9 Oct 2024 17:15:05 +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=\"Xk/kZbzr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728486905;\n\tbh=a6hn8rhQEgCgx8CoFCywQGNdHuD9GgalmxNz+x08o1Y=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Xk/kZbzrnfEkEwBZKnQ0o5SNw2/MtSvKtNPI3A93HyucD1o0yerrWEF/hWLdw4+CG\n\t0RUMEOQOZaGlGuIeYE6/2wrW61Bymd9JfoljPLTmhCXe/AKZE9c3OYCHNIsdBEOYkR\n\t8i7xNI3k7uLB89xzcSrBplYcXjlTbktorN328EtY=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240709143913.3276983-14-dan.scally@ideasonboard.com>","References":"<20240709143913.3276983-1-dan.scally@ideasonboard.com>\n\t<20240709143913.3276983-14-dan.scally@ideasonboard.com>","Subject":"Re: [PATCH v4 13/13] mali-c55: implement support for ScalerCrop","From":"Kieran Bingham ","Cc":"Jacopo Mondi ,\n\tDaniel Scally ","To":"Daniel Scally ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 09 Oct 2024 16:16:39 +0100","Message-ID":"<172848699989.532453.6893207178058282895@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\" "}}]