[{"id":32306,"web_url":"https://patchwork.libcamera.org/comment/32306/","msgid":"","date":"2024-11-20T11:49:40","subject":"Re: [PATCH 6/7] pipeline: rkisp1: Fix ScalerCrop to be in sensor\n\tcoordinates","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:45AM +0100, Stefan Klug wrote:\n> ScalerCrop is specified as beeing in sensor coordinates. The current\n\ns/beeing/being/\n\n> dewarper implementation on the imx8mp handles ScalerCrop in dewarper\n> coordinates. This leads to unexpected results and a unusable ScalerCrop\n\ns/ a / an /\n\n> control in camshark. Fix that by transforming back and forth between\n> sensor coordinates and dewarper coordinates.\n> \n> Signed-off-by: Stefan Klug \n\nLooks good.\n\nReviewed-by: Paul Elder \n\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 49 +++++++++++++++++++-----\n> 1 file changed, 40 insertions(+), 9 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 4a226d9b809f..c2ce38b1c253 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -204,6 +204,7 @@ private:\n> \tRkISP1SelfPath selfPath_;\n> \n> \tstd::unique_ptr dewarper_;\n> +\tRectangle dewarperSensorCrop_;\n> \tbool useDewarper_;\n> \n> \tstd::optional activeCrop_;\n> @@ -862,6 +863,14 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \t\t\t\toutputCfgs.push_back(const_cast(cfg));\n> \t\t\t\tret = dewarper_->configure(cfg, outputCfgs);\n> \t\t\t\tuseDewarper_ = ret ? false : true;\n> +\n> +\t\t\t\t/*\n> +\t\t\t\t * Calculate the crop rectangle of the data\n> +\t\t\t\t * flowing into the dewarper in sensor\n> +\t\t\t\t * coordinates.\n> +\t\t\t\t */\n> +\t\t\t\tdewarperSensorCrop_ = outputCrop.mappedBetween(inputCrop,\n> +\t\t\t\t\t\t\t\t\t ipaConfig.sensorInfo.analogCrop);\n> \t\t\t}\n> \t\t} else if (hasSelfPath_) {\n> \t\t\tret = selfPath_.configure(cfg, format);\n> @@ -1224,10 +1233,21 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)\n> \t\tstd::pair cropLimits =\n> \t\t\tdewarper_->inputCropBounds(&data->mainPathStream_);\n> \n> -\t\tcontrols[&controls::ScalerCrop] = ControlInfo(cropLimits.first,\n> -\t\t\t\t\t\t\t cropLimits.second,\n> -\t\t\t\t\t\t\t cropLimits.second);\n> -\t\tactiveCrop_ = cropLimits.second;\n> +\t\t/*\n> +\t\t * ScalerCrop is specified to be in Sensor coordinates.\n> +\t\t * So we need to transform the limits to sensor coordinates.\n> +\t\t * We can safely assume that the maximum crop limit contains the\n> +\t\t * full fov of the dewarper.\n> +\t\t */\n> +\t\tRectangle min = cropLimits.first.mappedBetween(cropLimits.second,\n> +\t\t\t\t\t\t\t dewarperSensorCrop_);\n> +\t\tRectangle max = cropLimits.second.mappedBetween(cropLimits.second,\n> +\t\t\t\t\t\t\t\tdewarperSensorCrop_);\n> +\n> +\t\tcontrols[&controls::ScalerCrop] = ControlInfo(min,\n> +\t\t\t\t\t\t\t max,\n> +\t\t\t\t\t\t\t max);\n> +\t\tactiveCrop_ = max;\n> \t}\n> \n> \t/* Add the IPA registered controls to list of camera controls. */\n> @@ -1478,22 +1498,33 @@ void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer)\n> \t/* Handle scaler crop control. */\n> \tconst auto &crop = request->controls().get(controls::ScalerCrop);\n> \tif (crop) {\n> -\t\tRectangle appliedRect = crop.value();\n> +\t\tRectangle rect = crop.value();\n> +\n> +\t\t/*\n> +\t\t * ScalerCrop is specified to be in Sensor coordinates.\n> +\t\t * So we need to transform it into dewarper coordinates.\n> +\t\t * We can safely assume that the maximum crop limit contains the\n> +\t\t * full fov of the dewarper.\n> +\t\t */\n> +\t\tstd::pair cropLimits =\n> +\t\t\tdewarper_->inputCropBounds(&data->mainPathStream_);\n> \n> +\t\trect = rect.mappedBetween(dewarperSensorCrop_, cropLimits.second);\n> \t\tint ret = dewarper_->setInputCrop(&data->mainPathStream_,\n> -\t\t\t\t\t\t &appliedRect);\n> -\t\tif (!ret && appliedRect != crop.value()) {\n> +\t\t\t\t\t\t &rect);\n> +\t\trect = rect.mappedBetween(cropLimits.second, dewarperSensorCrop_);\n> +\t\tif (!ret && rect != crop.value()) {\n> \t\t\t/*\n> \t\t\t * If the rectangle is changed by setInputCrop on the\n> \t\t\t * dewarper, log a debug message and cache the actual\n> \t\t\t * applied rectangle for metadata reporting.\n> \t\t\t */\n> \t\t\tLOG(RkISP1, Debug)\n> -\t\t\t\t<< \"Applied rectangle \" << appliedRect.toString()\n> +\t\t\t\t<< \"Applied rectangle \" << rect.toString()\n> \t\t\t\t<< \" differs from requested \" << crop.value().toString();\n> \t\t}\n> \n> -\t\tactiveCrop_ = appliedRect;\n> +\t\tactiveCrop_ = rect;\n> \t}\n> \n> \t/*\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 67288C32F5\n\tfor ;\n\tWed, 20 Nov 2024 11:49:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2108965F4A;\n\tWed, 20 Nov 2024 12:49:49 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 646D365898\n\tfor ;\n\tWed, 20 Nov 2024 12:49:47 +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 61718675;\n\tWed, 20 Nov 2024 12:49:28 +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=\"W/eHMU1m\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732103369;\n\tbh=65KcZOo02x2s6nvl4uxGjTatbkvuSzxSdopCDqcSruI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=W/eHMU1mACKNf4sxTPaE3ndWxOlPCkK9X/Coz22pr2V8FJYsZDJMho2b56K36bjnp\n\t1SpVBelM0i+trqTzo7TvvmBLCVq1M1qWZXsapSr9by+Mfe128Vo7ySkLiqbS4dGpfS\n\t7SBRkc2Qx/Ylxa9YNf4nWrWgSplwvWeNoHz4eP+U=","Date":"Wed, 20 Nov 2024 20:49:40 +0900","From":"Paul Elder ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 6/7] pipeline: rkisp1: Fix ScalerCrop to be in sensor\n\tcoordinates","Message-ID":"","References":"<20241120085753.1993436-1-stefan.klug@ideasonboard.com>\n\t<20241120085753.1993436-7-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20241120085753.1993436-7-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":32338,"web_url":"https://patchwork.libcamera.org/comment/32338/","msgid":"<5wfmawhyxeecaaltkugjo6j5hgpbd7c3psu3tgpswerth2w252@rqlvxrfngitw>","date":"2024-11-21T15:32:33","subject":"Re: [PATCH 6/7] pipeline: rkisp1: Fix ScalerCrop to be in sensor\n\tcoordinates","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:45AM +0100, Stefan Klug wrote:\n> ScalerCrop is specified as beeing in sensor coordinates. The current\n> dewarper implementation on the imx8mp handles ScalerCrop in dewarper\n> coordinates. This leads to unexpected results and a unusable ScalerCrop\n> control in camshark. Fix that by transforming back and forth between\n> sensor coordinates and dewarper coordinates.\n>\n> Signed-off-by: Stefan Klug \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 49 +++++++++++++++++++-----\n> 1 file changed, 40 insertions(+), 9 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 4a226d9b809f..c2ce38b1c253 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -204,6 +204,7 @@ private:\n> \tRkISP1SelfPath selfPath_;\n>\n> \tstd::unique_ptr dewarper_;\n> +\tRectangle dewarperSensorCrop_;\n> \tbool useDewarper_;\n>\n> \tstd::optional activeCrop_;\n> @@ -862,6 +863,14 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \t\t\t\toutputCfgs.push_back(const_cast(cfg));\n> \t\t\t\tret = dewarper_->configure(cfg, outputCfgs);\n> \t\t\t\tuseDewarper_ = ret ? false : true;\n> +\n> +\t\t\t\t/*\n> +\t\t\t\t * Calculate the crop rectangle of the data\n> +\t\t\t\t * flowing into the dewarper in sensor\n> +\t\t\t\t * coordinates.\n> +\t\t\t\t */\n> +\t\t\t\tdewarperSensorCrop_ = outputCrop.mappedBetween(inputCrop,\n> +\t\t\t\t\t\t\t\t\t ipaConfig.sensorInfo.analogCrop);\n\nScalerCrop is expressed in sensor native coordinates, not in respect\nto the current analogCrop ?\n\n> \t\t\t}\n> \t\t} else if (hasSelfPath_) {\n> \t\t\tret = selfPath_.configure(cfg, format);\n> @@ -1224,10 +1233,21 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)\n> \t\tstd::pair cropLimits =\n> \t\t\tdewarper_->inputCropBounds(&data->mainPathStream_);\n\nLooking at the implementation of V4L2M2MConverter::inputCropBound() it\nseems to me that inputCropBounds_ is only initialized after a\nconfigure(). As updateControls() is called by\nPipelineHandlerRkISP1::createCamera(), how are these fields\ninitialized ?\n\nAlso dewarperSensorCrop_ needs to be initialized to some default ?\n\n>\n> -\t\tcontrols[&controls::ScalerCrop] = ControlInfo(cropLimits.first,\n> -\t\t\t\t\t\t\t cropLimits.second,\n> -\t\t\t\t\t\t\t cropLimits.second);\n> -\t\tactiveCrop_ = cropLimits.second;\n> +\t\t/*\n> +\t\t * ScalerCrop is specified to be in Sensor coordinates.\n> +\t\t * So we need to transform the limits to sensor coordinates.\n> +\t\t * We can safely assume that the maximum crop limit contains the\n> +\t\t * full fov of the dewarper.\n> +\t\t */\n> +\t\tRectangle min = cropLimits.first.mappedBetween(cropLimits.second,\n\nWhat is the reference system of the dewarper crop bound rectangle ?\nShouldn't it be the size of the format configured in its sink pad ?\nOr is it what is returned in cropLimits.second already ? (It seems so,\nas it is the result of TGT_CROP_BOUNDS)\n\n> +\t\t\t\t\t\t\t dewarperSensorCrop_);\n> +\t\tRectangle max = cropLimits.second.mappedBetween(cropLimits.second,\n> +\t\t\t\t\t\t\t\tdewarperSensorCrop_);\n> +\n> +\t\tcontrols[&controls::ScalerCrop] = ControlInfo(min,\n> +\t\t\t\t\t\t\t max,\n> +\t\t\t\t\t\t\t max);\n> +\t\tactiveCrop_ = max;\n> \t}\n>\n> \t/* Add the IPA registered controls to list of camera controls. */\n> @@ -1478,22 +1498,33 @@ void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer)\n> \t/* Handle scaler crop control. */\n> \tconst auto &crop = request->controls().get(controls::ScalerCrop);\n> \tif (crop) {\n> -\t\tRectangle appliedRect = crop.value();\n> +\t\tRectangle rect = crop.value();\n> +\n> +\t\t/*\n> +\t\t * ScalerCrop is specified to be in Sensor coordinates.\n> +\t\t * So we need to transform it into dewarper coordinates.\n> +\t\t * We can safely assume that the maximum crop limit contains the\n> +\t\t * full fov of the dewarper.\n> +\t\t */\n> +\t\tstd::pair cropLimits =\n> +\t\t\tdewarper_->inputCropBounds(&data->mainPathStream_);\n>\n> +\t\trect = rect.mappedBetween(dewarperSensorCrop_, cropLimits.second);\n> \t\tint ret = dewarper_->setInputCrop(&data->mainPathStream_,\n> -\t\t\t\t\t\t &appliedRect);\n> -\t\tif (!ret && appliedRect != crop.value()) {\n> +\t\t\t\t\t\t &rect);\n> +\t\trect = rect.mappedBetween(cropLimits.second, dewarperSensorCrop_);\n> +\t\tif (!ret && rect != crop.value()) {\n> \t\t\t/*\n> \t\t\t * If the rectangle is changed by setInputCrop on the\n> \t\t\t * dewarper, log a debug message and cache the actual\n> \t\t\t * applied rectangle for metadata reporting.\n> \t\t\t */\n> \t\t\tLOG(RkISP1, Debug)\n> -\t\t\t\t<< \"Applied rectangle \" << appliedRect.toString()\n> +\t\t\t\t<< \"Applied rectangle \" << rect.toString()\n> \t\t\t\t<< \" differs from requested \" << crop.value().toString();\n> \t\t}\n>\n> -\t\tactiveCrop_ = appliedRect;\n> +\t\tactiveCrop_ = rect;\n\nI guess the only real question is what is the system's state before\nconfigure().\n\nThanks\n j\n\n> \t}\n>\n> \t/*\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 DFECAC32F9\n\tfor ;\n\tThu, 21 Nov 2024 15:32:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 119CB65FD3;\n\tThu, 21 Nov 2024 16:32:38 +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 73E2165FCD\n\tfor ;\n\tThu, 21 Nov 2024 16:32:36 +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 7F467670;\n\tThu, 21 Nov 2024 16:32:17 +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=\"kov+l38F\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732203137;\n\tbh=dM1wlYIprvUf/kcyI6B+7mjppH7xZqIFDlxhxx8jUqw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kov+l38FXtKvWsqdA0MH2N+nQd8h4vcqJDH+WbH75w+TPUNdkwHf+iGH6covxhbWi\n\tc7aGZBYxrYFh/Fhseyl+9kVNHjq8hegmLvJQ+AIDjXZMxLytcPW+VB4swMUruDHwKN\n\tcGKZTE/oJx1+rKjnJj4ZENhfDYAMRLlbCtpCjKFc=","Date":"Thu, 21 Nov 2024 16:32:33 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 6/7] pipeline: rkisp1: Fix ScalerCrop to be in sensor\n\tcoordinates","Message-ID":"<5wfmawhyxeecaaltkugjo6j5hgpbd7c3psu3tgpswerth2w252@rqlvxrfngitw>","References":"<20241120085753.1993436-1-stefan.klug@ideasonboard.com>\n\t<20241120085753.1993436-7-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241120085753.1993436-7-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\" "}}]