[{"id":13398,"web_url":"https://patchwork.libcamera.org/comment/13398/","msgid":"<20201022055827.GA4816@pendragon.ideasonboard.com>","date":"2020-10-22T05:58:27","subject":"Re: [libcamera-devel] [PATCH v4 5/5] libcamera: pipeline:\n\traspberrypi: Implementation of digital zoom","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Mon, Oct 19, 2020 at 01:51:56PM +0100, David Plowman wrote:\n> During configure() we update the ScalerCropMaximum to the correct\n> value for this camera mode (which we also note internally) and work\n> out the minimum crop size allowed by the ISP.\n> \n> Whenever a new ScalerCrop request is received we check it's valid and\n> apply it to the ISP V4L2 device. We also forward it to the IPA so that\n> the IPA can return the values used in the image metadata.\n> \n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/ipa/raspberrypi.h | 1 +\n> src/ipa/raspberrypi/raspberrypi.cpp | 7 ++\n> .../pipeline/raspberrypi/raspberrypi.cpp | 70 +++++++++++++++----\n> 3 files changed, 63 insertions(+), 15 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index b3041591..bb4e1a8c 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -60,6 +60,7 @@ static const ControlInfoMap Controls = {\n> \t{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n> \t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> \t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> +\t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> };\n> \n> } /* namespace RPi */\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 48a72dd2..e777f0b4 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -699,6 +699,13 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\t\tbreak;\n> \t\t}\n> \n> +\t\tcase controls::SCALER_CROP: {\n> +\t\t\t/* Just copy the information back. */\n> +\t\t\tRectangle crop = ctrl.second.get();\n> +\t\t\tlibcameraMetadata_.set(controls::ScalerCrop, crop);\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> \t\tdefault:\n> \t\t\tLOG(IPARPI, Warning)\n> \t\t\t\t<< \"Ctrl \" << controls::controls.at(ctrl.first)->name()\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 26dbd257..43b860ae 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -193,6 +193,11 @@ public:\n> \tbool flipsAlterBayerOrder_;\n> \tBayerFormat::Order nativeBayerOrder_;\n> \n> +\t/* For handling digital zoom. */\n> +\tCameraSensorInfo sensorInfo_;\n> +\tRectangle lastIspCrop_;\n> +\tSize ispMinSize_;\n> +\n> \tunsigned int dropFrameCount_;\n> \n> private:\n> @@ -677,26 +682,30 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> \t\treturn ret;\n> \t}\n> \n> -\t/* Adjust aspect ratio by providing crops on the input image. */\n> -\tRectangle crop{ 0, 0, sensorFormat.size };\n> -\n> -\tint ar = maxSize.height * sensorFormat.size.width - maxSize.width * sensorFormat.size.height;\n> -\tif (ar > 0)\n> -\t\tcrop.width = maxSize.width * sensorFormat.size.height / maxSize.height;\n> -\telse if (ar < 0)\n> -\t\tcrop.height = maxSize.height * sensorFormat.size.width / maxSize.width;\n> +\t/* Figure out the smallest selection the ISP will allow. */\n> +\tRectangle testCrop(0, 0, 1, 1);\n> +\tdata->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &testCrop);\n> +\tdata->ispMinSize_ = testCrop.size();\n\nCan this vary based on the configuration, or could it be done at match()\ntime ?\n\n> \n> -\tcrop.width &= ~1;\n> -\tcrop.height &= ~1;\n> +\t/* Adjust aspect ratio by providing crops on the input image. */\n> +\tRectangle crop = sensorFormat.size.boundedToAspectRatio(maxSize).centeredTo(sensorFormat.size.center());\n\nThe line is a bit long, should it be wrapped ?\n\n\tRectangle crop = sensorFormat.size.boundedToAspectRatio(maxSize)\n\t\t\t\t\t .centeredTo(sensorFormat.size.center());\n\n> +\tdata->lastIspCrop_ = crop;\n> \n> -\tcrop.x = (sensorFormat.size.width - crop.width) >> 1;\n> -\tcrop.y = (sensorFormat.size.height - crop.height) >> 1;\n> \tdata->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> \n> \tret = data->configureIPA(config);\n> \tif (ret)\n> \t\tLOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> \n> +\t/*\n> +\t * Update the ScalerCropMaximum to the correct value for this camera mode.\n> +\t * For us, it's the same as the \"analogue crop\".\n> +\t *\n> +\t * \\todo Make this property the ScalerCrop maximum value when dynamic\n> +\t * controls are available and set it at validate() time\n> +\t */\n> +\tdata->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop);\n> +\n> \treturn ret;\n> }\n> \n> @@ -1147,8 +1156,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> \t\tipaConfig.data.push_back(static_cast(lsTable_.fd()));\n> \t}\n> \n> -\tCameraSensorInfo sensorInfo = {};\n> -\tint ret = sensor_->sensorInfo(&sensorInfo);\n> +\t/* We store the CameraSensorInfo for digital zoom calculations. */\n> +\tint ret = sensor_->sensorInfo(&sensorInfo_);\n> \tif (ret) {\n> \t\tLOG(RPI, Error) << \"Failed to retrieve camera sensor info\";\n> \t\treturn ret;\n> @@ -1157,7 +1166,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> \t/* Ready the IPA - it must know about the sensor resolution. */\n> \tIPAOperationData result;\n> \n> -\tipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> +\tipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,\n> \t\t\t&result);\n> \n> \tunsigned int resultIdx = 0;\n> @@ -1588,6 +1597,37 @@ void RPiCameraData::tryRunPipeline()\n> \t/* Take the first request from the queue and action the IPA. */\n> \tRequest *request = requestQueue_.front();\n> \n> +\tif (request->controls().contains(controls::ScalerCrop)) {\n> +\t\tRectangle crop = request->controls().get(controls::ScalerCrop);\n> +\n> +\t\tif (crop.width && crop.height) {\n> +\t\t\t/* First scale the crop from sensor native to camera mode pixels. */\n> +\t\t\tcrop.translateBy(-sensorInfo_.analogCrop.x, -sensorInfo_.analogCrop.y);\n> +\t\t\tcrop.rescaleTo(sensorInfo_.analogCrop.size(), sensorInfo_.outputSize);\n> +\n> +\t\t\t/*\n> +\t\t\t * The crop that we set must be:\n> +\t\t\t * 1. At least as big as ispMinSize_, once that's been\n> +\t\t\t * enlarged to the same aspect ratio.\n> +\t\t\t * 2. With the same mid-point, if possible.\n> +\t\t\t * 3. But it can't go outside the sensor area.\n> +\t\t\t */\n> +\t\t\tSize minSize = ispMinSize_.expandedToAspectRatio(crop.size());\n> +\t\t\tSize size = crop.size().expandedTo(minSize);\n> +\t\t\tcrop = size.centeredTo(crop.center()).clampedTo(Rectangle(sensorInfo_.outputSize));\n> +\n> +\t\t\tif (crop != lastIspCrop_)\n> +\t\t\t\tisp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> +\t\t\tlastIspCrop_ = crop;\n> +\t\t}\n> +\n> +\t\t/* We must scale the actual crop used back into native pixels. */\n> +\t\tcrop = lastIspCrop_.rescaledTo(sensorInfo_.outputSize, sensorInfo_.analogCrop.size());\n> +\t\tcrop.translateBy(sensorInfo_.analogCrop.x, sensorInfo_.analogCrop.y);\n\nIt takes a bit of time to process this, but I think it's quite fine,\nthere's a good amount of descriptive comments, and the function names\nhelp. Great job :-) I agree with the cover letter that it may be a bit\nnicer to write\n\n\tcrop.translateBy(-sensorInfo_.analogCrop.topLeft());\n\nand\n\n\tcrop.translateBy(sensorInfo_.analogCrop.topLeft());\n\n> +\n> +\t\trequest->controls().set(controls::ScalerCrop, crop);\n\nThis bothers me a little bit, as we have not considered the pipeline\nhandler to be allowed to modify the controls stored in the request. If I\nunderstand the code correctly, this is only used to pass the crop\nrectangle to the IPA. I see two possible options:\n\n- Pass the computed crop rectangle explicitly through IPAOperationData.\n It's not amazing as it requires serializing it explicitly in the data\n vector, but that would be temporary only, given Paul's work on the IPA\n interface that will allow custom parameters to be passed to IPA\n operations in a much better way.\n\n- Don't pass the crop rectangle to the IPA, and set the metadata here\n directly. The IPA currently doesn't use the crop rectangle, but this\n may change later. Still, for the time being, this is an option.\n\nI don't have a preference.\n\nReviewed-by: Laurent Pinchart \n\n> +\t}\n> +\n> \t/*\n> \t * Process all the user controls by the IPA. Once this is complete, we\n> \t * queue the ISP output buffer listed in the request to start the HW","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 ACE6BBDB13\n\tfor ;\n\tThu, 22 Oct 2020 05:59:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3CCBF6112D;\n\tThu, 22 Oct 2020 07:59:15 +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 A728D6034F\n\tfor ;\n\tThu, 22 Oct 2020 07:59:13 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1D0E753;\n\tThu, 22 Oct 2020 07:59:13 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"cHiRhnpz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603346353;\n\tbh=qmNM9Kcm2AMFVwPstOD1F54k0QsXGKdNZDDl7406ckk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cHiRhnpzYySqVuv6j871ivleP+t1Kb5U4hiEDazDZo/R30tSLVVjVf2txV0nNwFro\n\ta8yqztOZs1Lp4m187P4T7Jw/tFGJm/EGgNsBOdUozhazEyDW0DiPxIcxc80hTOroL5\n\tGD0tKdvS9XaHLzmJvh50Opq3sLX3woG5kDmb78x4=","Date":"Thu, 22 Oct 2020 08:58:27 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20201022055827.GA4816@pendragon.ideasonboard.com>","References":"<20201019125156.26751-1-david.plowman@raspberrypi.com>\n\t<20201019125156.26751-6-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201019125156.26751-6-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v4 5/5] libcamera: pipeline:\n\traspberrypi: Implementation of digital zoom","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]