[{"id":13516,"web_url":"https://patchwork.libcamera.org/comment/13516/","msgid":"<20201027124646.GD3967@pendragon.ideasonboard.com>","date":"2020-10-27T12:46:46","subject":"Re: [libcamera-devel] [PATCH v7 5/6] 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 Tue, Oct 27, 2020 at 11:07:38AM +0000, David Plowman wrote:\n> During configure() we update the ScalerCropMaximum to the correct\n> value for this camera mode and work out the minimum crop size allowed\n> 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. When the IPA returns its metadata to\n> us we add the ScalerCrop information, rescaled to sensor native\n> pixels.\n> \n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/ipa/raspberrypi.h | 1 +\n> src/ipa/raspberrypi/raspberrypi.cpp | 5 +\n> .../pipeline/raspberrypi/raspberrypi.cpp | 93 +++++++++++++++----\n> 3 files changed, 82 insertions(+), 17 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index 4ca1528a..2b55dbfc 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -62,6 +62,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 1c255aa2..f338ff8b 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -699,6 +699,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\t\tbreak;\n> \t\t}\n> \n> +\t\tcase controls::SCALER_CROP: {\n> +\t\t\t/* We do nothing with this, but should avoid the warning below. */\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 763451a8..a48013d8 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -135,7 +135,7 @@ public:\n> \tRPiCameraData(PipelineHandler *pipe)\n> \t\t: CameraData(pipe), sensor_(nullptr), state_(State::Stopped),\n> \t\t supportsFlips_(false), flipsAlterBayerOrder_(false),\n> -\t\t dropFrameCount_(0), ispOutputCount_(0)\n> +\t\t updateScalerCrop_(true), dropFrameCount_(0), ispOutputCount_(0)\n> \t{\n> \t}\n> \n> @@ -193,6 +193,13 @@ public:\n> \tbool flipsAlterBayerOrder_;\n> \tBayerFormat::Order nativeBayerOrder_;\n> \n> +\t/* For handling digital zoom. */\n> +\tCameraSensorInfo sensorInfo_;\n> +\tRectangle ispCrop_; /* crop in ISP (camera mode) pixels */\n> +\tRectangle scalerCrop_; /* crop in sensor native pixels */\n> +\tbool updateScalerCrop_;\n> +\tSize ispMinCropSize_;\n> +\n> \tunsigned int dropFrameCount_;\n> \n> private:\n> @@ -677,26 +684,31 @@ 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->ispMinCropSize_ = testCrop.size();\n> \n> -\tcrop.width &= ~1;\n> -\tcrop.height &= ~1;\n> +\t/* Adjust aspect ratio by providing crops on the input image. */\n> +\tSize size = sensorFormat.size.boundedToAspectRatio(maxSize);\n> +\tRectangle crop = size.centeredTo(Rectangle(sensorFormat.size).center());\n> +\tdata->ispCrop_ = 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> @@ -1154,8 +1166,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> @@ -1164,7 +1176,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> @@ -1243,8 +1255,26 @@ void RPiCameraData::queueFrameAction([[maybe_unused]] unsigned int frame,\n> \t\tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> \n> \t\thandleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> +\n> \t\t/* Fill the Request metadata buffer with what the IPA has provided */\n> -\t\trequestQueue_.front()->metadata() = std::move(action.controls[0]);\n> +\t\tRequest *request = requestQueue_.front();\n> +\t\trequest->metadata() = std::move(action.controls[0]);\n> +\n> +\t\t/*\n> +\t\t * Also update the ScalerCrop in the metadata with what we actually\n> +\t\t * used. But we must first rescale that from ISP (camera mode) pixels\n> +\t\t * back into sensor native pixels.\n> +\t\t *\n> +\t\t * Sending this information on every frame may be helpful.\n> +\t\t */\n> +\t\tif (updateScalerCrop_) {\n> +\t\t\tupdateScalerCrop_ = false;\n> +\t\t\tscalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> +\t\t\t\t\t\t\tsensorInfo_.outputSize);\n> +\t\t\tscalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> +\t\t}\n> +\t\trequest->metadata().set(controls::ScalerCrop, scalerCrop_);\n> +\n> \t\tstate_ = State::IpaComplete;\n> \t\tbreak;\n> \t}\n> @@ -1595,6 +1625,35 @@ 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 nativeCrop = request->controls().get(controls::ScalerCrop);\n> +\n> +\t\tif (!nativeCrop.width || !nativeCrop.height)\n> +\t\t\tnativeCrop = { 0, 0, 1, 1 };\n> +\n> +\t\t/* Create a version of the crop scaled to ISP (camera mode) pixels. */\n> +\t\tRectangle ispCrop = nativeCrop.translatedBy(-sensorInfo_.analogCrop.topLeft());\n> +\t\tispCrop.scaleBy(sensorInfo_.outputSize, sensorInfo_.analogCrop.size());\n> +\n> +\t\t/*\n> +\t\t * The crop that we set must be:\n> +\t\t * 1. At least as big as ispMinCropSize_, once that's been\n> +\t\t * enlarged to the same aspect ratio.\n> +\t\t * 2. With the same mid-point, if possible.\n> +\t\t * 3. But it can't go outside the sensor area.\n> +\t\t */\n> +\t\tSize minSize = ispMinCropSize_.expandedToAspectRatio(nativeCrop.size());\n\nClever :-)\n\nReviewed-by: Laurent Pinchart \n\nand pushed.\n\n> +\t\tSize size = ispCrop.size().expandedTo(minSize);\n> +\t\tispCrop = size.centeredTo(ispCrop.center()).enclosedIn(Rectangle(sensorInfo_.outputSize));\n> +\n> +\t\tif (ispCrop != ispCrop_) {\n> +\t\t\tisp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &ispCrop);\n> +\t\t\tispCrop_ = ispCrop;\n> +\t\t\t/* queueFrameAction will have to update its scalerCrop_ */\n> +\t\t\tupdateScalerCrop_ = true;\n> +\t\t}\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 553F6C3B5C\n\tfor ;\n\tTue, 27 Oct 2020 12:47:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B6CFB62109;\n\tTue, 27 Oct 2020 13:47:35 +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 C3A6C62034\n\tfor ;\n\tTue, 27 Oct 2020 13:47:34 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 31290BBE;\n\tTue, 27 Oct 2020 13:47:34 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"gAtndpRV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603802854;\n\tbh=WQ6u9QvrhH7t2Nbuwr2yROP1hOgrMYxk7i1i4xXjDb0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gAtndpRVSXkX6dUewkFzwtjb7zi9NS5ET7GlPAUDSMFRMmO9n1DnD+mAk0bC5Oa5j\n\tw0/L9kIQercpgIPIQ84cYicgas6DYNfXr1Xglq+kRroBfKpb8hr4mWvxmHQFxstGVI\n\tui8ZthecLzdbGzjoBT2NCeYsscRBUodONhPMq5Fo=","Date":"Tue, 27 Oct 2020 14:46:46 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20201027124646.GD3967@pendragon.ideasonboard.com>","References":"<20201026171908.21463-6-david.plowman@raspberrypi.com>\n\t<20201027110738.15751-1-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201027110738.15751-1-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v7 5/6] 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\" "}}]