[{"id":27981,"web_url":"https://patchwork.libcamera.org/comment/27981/","msgid":"<20231018192203.GE1512@pendragon.ideasonboard.com>","date":"2023-10-18T19:22:03","subject":"Re: [libcamera-devel] [PATCH v5 04/12] libcamera: properties: Make\n\t'Rotation' the mounting rotation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Fri, Sep 01, 2023 at 05:02:07PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> Specify in the documentation that properties::Rotation specifies the\n> mounting rotation of the camera module. This avoids confusion with the\n> image orientation which is instead expressed by\n> CameraConfiguration::orientation.\n> \n> For this reason, do not compensate the Rotation property when\n> initializing the CameraSensor class but report the value of\n> V4L2_CID_CAMERA_SENSOR_ROTATION or 0 if the control is not available.\n> \n> Signed-off-by: Jacopo Mondi \n> Reviewed-by: David Plowman \n> ---\n> src/libcamera/camera_sensor.cpp | 11 +----------\n> src/libcamera/property_ids.yaml | 8 ++++----\n> 2 files changed, 5 insertions(+), 14 deletions(-)\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 9a033459742f..3ba364c44a40 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -476,20 +476,11 @@ int CameraSensor::initProperties()\n> \t\t\trotationTransform_ = Transform::Identity;\n> \t\t}\n> \n> -\t\t/*\n> -\t\t * Adjust property::Rotation as validateTransform() compensates\n> -\t\t * for the mounting rotation. However, as a camera sensor can\n> -\t\t * only compensate rotations by applying H/VFlips, only rotation\n> -\t\t * of 180 degrees are automatically compensated. The other valid\n> -\t\t * rotations (Rot90 and Rot270) require transposition, which the\n> -\t\t * camera sensor cannot perform, so leave them untouched.\n> -\t\t */\n> -\t\tif (propertyValue == 180 && supportFlips_)\n> -\t\t\tpropertyValue = 0;\n> \t\tproperties_.set(properties::Rotation, propertyValue);\n> \t} else {\n> \t\tLOG(CameraSensor, Warning)\n> \t\t\t<< \"Rotation control not available, default to 0 degrees\";\n> +\t\tproperties_.set(properties::Rotation, propertyValue);\n\nThis doesn't seem right, did you mean\n\n\t\tproperties_.set(properties::Rotation, 0);\n\n?\n\nTo avoid further errors, I think the propertyValue variable should be\nmade local to the two 'if' branches where it gets used.\n\n> \t\trotationTransform_ = Transform::Identity;\n> \t}\n> \n> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> index 5bddafc29364..fb53081c1bd2 100644\n> --- a/src/libcamera/property_ids.yaml\n> +++ b/src/libcamera/property_ids.yaml\n> @@ -29,10 +29,10 @@ controls:\n> - Rotation:\n> type: int32_t\n> description: |\n> - The camera rotation is expressed as the angular difference in degrees\n> - between two reference systems, one relative to the camera module, and\n> - one defined on the external world scene to be captured when projected\n> - on the image sensor pixel array.\n> + The camera physical mounting rotation, expressed as the angular\n> + difference in degrees between two reference systems, one relative to the\n> + camera module, and one defined on the external world scene to be\n> + captured when projected on the image sensor pixel array.\n\nThe longer the sentence, the harder it is to read. Let's break it.\n\n The camera physical mounting rotation. It is expressed as the angular\n difference in degrees between two reference systems, one relative to the\n camera module, and one defined on the external world scene to be\n captured when projected on the image sensor pixel array.\n\nWith these small issues addressed,\n\nReviewed-by: Laurent Pinchart \n\n> \n> A camera sensor has a 2-dimensional reference system 'Rc' defined by\n> its pixel array read-out order. The origin is set to the first pixel","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 EA0C0C3272\n\tfor ;\n\tWed, 18 Oct 2023 19:21:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5CD3B61DD1;\n\tWed, 18 Oct 2023 21:21:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9EC8A61DD1\n\tfor ;\n\tWed, 18 Oct 2023 21:21:56 +0200 (CEST)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DAA44666;\n\tWed, 18 Oct 2023 21:21:48 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1697656919;\n\tbh=dZxkEuXlX9wPi/zQotDiBijPwFWTJW++LqLt+wQePfo=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=sYuPi9Q53nZ7hpSFf3Oj5ieMSlP73+rJB26cfBRXwGeO9Qr+oHWWKJpYGC1iNLqzQ\n\tbP8StV++bR7/Mn6jMla1NPXDuphpLswM0sx0Ffz+hhIeT/DF4AUmdswht/cLvVspw/\n\taSanrnh7hlJAYGxq3vAzbdkmMKWQF9vp7LHmSulAw8mXzPfFM248wXZhT6gOKVpGtT\n\tXYzyi5zy3W786SoWTt9E+yYn4lPzCK3kD+MgfLyFYcpM4OeDPAreqE3AMs+4PvPoxX\n\tNdOTU+7p3CjKM74PoNvSQu/F2v2WqQqCRehpxGw9EX1T+9oh0SOoLHi5ZJIe4Mnkco\n\tiPKD1TZFPVBCg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1697656909;\n\tbh=dZxkEuXlX9wPi/zQotDiBijPwFWTJW++LqLt+wQePfo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FNsUa/OS6sQy5KShhIwr0OFF0ASnAn7m5A3H7D0nM/DdGWkEfCnpK1r4quGif0RDC\n\tPm9EXLtjciiSlatJ4ND4JF02R61RJ19RKD+YuDUB/H3fqNJCkPL62CrCJFpyBaswGU\n\tfRS6K3RKgvJAY9gN+p6m4pJ2CTVqfWHDOqwtmurc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"FNsUa/OS\"; dkim-atps=neutral","Date":"Wed, 18 Oct 2023 22:22:03 +0300","To":"Jacopo Mondi ","Message-ID":"<20231018192203.GE1512@pendragon.ideasonboard.com>","References":"<20230901150215.11585-1-jacopo.mondi@ideasonboard.com>\n\t<20230901150215.11585-5-jacopo.mondi@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230901150215.11585-5-jacopo.mondi@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 04/12] libcamera: properties: Make\n\t'Rotation' the mounting rotation","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]