[{"id":27395,"web_url":"https://patchwork.libcamera.org/comment/27395/","msgid":"","date":"2023-06-21T05:21:33","subject":"Re: [libcamera-devel] [RFC 1/4] libcamera: camera_sensor: Cache\n\trotationTransform_","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn 6/20/23 7:59 PM, Jacopo Mondi via libcamera-devel wrote:\n> The rotationTransform_ depends on a V4L2 control whose value does not\n> change for the whole lifetime of the camera.\n>\n> Instead of re-calculating it everytime the camera is configured, cache\n> it at properties initialization time.\n>\n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/camera_sensor.h | 1 +\n> src/libcamera/camera_sensor.cpp | 54 +++++++++++++---------\n> 2 files changed, 32 insertions(+), 23 deletions(-)\n>\n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index 02c77ab037da..02b4b4d25e6d 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -107,6 +107,7 @@ private:\n> \tRectangle activeArea_;\n> \tconst BayerFormat *bayerFormat_;\n> \tbool supportFlips_;\n> +\tTransform rotationTransform_;\n> \n> \tControlList properties_;\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index f3a5aa37149f..a15a6c89c5c8 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -461,18 +461,36 @@ int CameraSensor::initProperties()\n> \n> \tconst auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> \tif (rotationControl != controls.end()) {\n> +\t\tpropertyValue = rotationControl->second.def().get();\n> +\n> \t\t/*\n> -\t\t * validateTransform() compensates for the mounting rotation.\n> -\t\t * However, as a camera sensor can only compensate rotations\n> -\t\t * by applying H/VFlips, only rotation of 180 degrees are\n> -\t\t * automatically compensated. The other valid rotations (Rot90\n> -\t\t * and Rot270) require transposition, which the camera sensor\n> -\t\t * cannot perform, so leave them untouched.\n> +\t\t * Cache the Transform associated with the camera mounting\n> +\t\t * rotation for later use in validateTransform().\n> +\t\t */\n> +\t\tbool success;\n> +\t\trotationTransform_ = transformFromRotation(propertyValue, &success);\n\nProbably a good time to convert to std::optional<> here? Or it can be \ndone on top. Either way I don't mind, as long as it happens ;-)\n\nOther than that, the code looks fine to me.\n\nReviewed-by: Umang Jain \n\n> +\t\tif (!success) {\n> +\t\t\tLOG(CameraSensor, Warning)\n> +\t\t\t\t<< \"Invalid rotation of \" << propertyValue\n> +\t\t\t\t<< \" degrees - ignoring\";\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\tpropertyValue = rotationControl->second.def().get();\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 degreees\";\n> +\t\trotationTransform_ = Transform::Identity;\n> \t}\n> \n> \tproperties_.set(properties::PixelArraySize, pixelArraySize_);\n> @@ -1038,21 +1056,11 @@ void CameraSensor::updateControlInfo()\n> */\n> Transform CameraSensor::validateTransform(Transform *transform) const\n> {\n> -\t/* Adjust the requested transform to compensate the sensor mounting rotation. */\n> -\tconst ControlInfoMap &controls = subdev_->controls();\n> -\tint rotation = 0;\n> -\n> -\tconst auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> -\tif (rotationControl != controls.end())\n> -\t\trotation = rotationControl->second.def().get();\n> -\n> -\tbool success;\n> -\tTransform rotationTransform = transformFromRotation(rotation, &success);\n> -\tif (!success)\n> -\t\tLOG(CameraSensor, Warning) << \"Invalid rotation of \" << rotation\n> -\t\t\t\t\t << \" degrees - ignoring\";\n> -\n> -\tTransform combined = *transform * rotationTransform;\n> +\t/*\n> +\t * Combine the requested transform to compensate the sensor mounting\n> +\t * rotation.\n> +\t */\n> +\tTransform combined = *transform * rotationTransform_;\n> \n> \t/*\n> \t * We combine the platform and user transform, but must \"adjust away\"\n> @@ -1083,7 +1091,7 @@ Transform CameraSensor::validateTransform(Transform *transform) const\n> \t\t * rise to this is the inverse of the rotation. (Recall that\n> \t\t * combined = transform * rotationTransform.)\n> \t\t */\n> -\t\t*transform = -rotationTransform;\n> +\t\t*transform = -rotationTransform_;\n> \t\tcombined = Transform::Identity;\n> \t}\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 1CE5EBD78E\n\tfor ;\n\tWed, 21 Jun 2023 05:21:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 54FC3628BD;\n\tWed, 21 Jun 2023 07:21:39 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 92EAB61E3C\n\tfor ;\n\tWed, 21 Jun 2023 07:21:37 +0200 (CEST)","from [192.168.1.108] (unknown [103.86.18.208])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D4EA7556;\n\tWed, 21 Jun 2023 07:21:01 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687324899;\n\tbh=91vR/Y/q7G09Rod/fP+ttVgOoWbF4a9Bcg/VpTv2MD4=;\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:\n\tFrom;\n\tb=NFLLB/2c5eF2zixDc30i3B2llekJXOBZvAPJC8B8St3zlSt0CUWk16CrXFLO9VjE5\n\tPaVsi0Ig8xu1+WP+rJt9U+z/5GB1bS447Oqh11TcoQmNXnPGZq2UZ8Egw2U+oY429K\n\tqjFWZ7tKjIbVua9E2H7hEXhfurN1dfuw9ZhZdYlww3VR0IdFU5ruZF5VKQFSJtXgkA\n\towxPTRgLXLcN8F9PMJI5x5AfzhYbjQKmLMPQ7mVGNcp7cPuVuBFMIfzdzZpQMVN8Dr\n\tGnYm7W5JHzEcfBn4OUSrA3wUeY4jeJf/CVSOikKQ67f8NPKGsmm2jMQ3j6R2H7vkmX\n\tdA3UkWFzJlNGw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1687324862;\n\tbh=91vR/Y/q7G09Rod/fP+ttVgOoWbF4a9Bcg/VpTv2MD4=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=dt3E3dyYKMgGUyHIrXL1Sw0ldtYXZ/crlU0a4PpltY70IUq97rkh7RK2vaqIHC83V\n\tQ1xp8+CRizqIIqS7esIrW1pv8tkOPhs5A188m9mhLbStpxInQSx4pr8U5tw6fOBK+d\n\tKAFLloJcKcAcOnhPnoV2kxgiSkpRv/pAQ94tiqPQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"dt3E3dyY\"; dkim-atps=neutral","Message-ID":"","Date":"Wed, 21 Jun 2023 10:51:33 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.7.1","Content-Language":"en-US","To":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20230620142904.72600-1-jacopo.mondi@ideasonboard.com>\n\t<20230620142904.72600-2-jacopo.mondi@ideasonboard.com>","In-Reply-To":"<20230620142904.72600-2-jacopo.mondi@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [RFC 1/4] libcamera: camera_sensor: Cache\n\trotationTransform_","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27398,"web_url":"https://patchwork.libcamera.org/comment/27398/","msgid":"","date":"2023-06-21T07:40:11","subject":"Re: [libcamera-devel] [RFC 1/4] libcamera: camera_sensor: Cache\n\trotationTransform_","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Umang\n\nOn Wed, Jun 21, 2023 at 10:51:33AM +0530, Umang Jain wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On 6/20/23 7:59 PM, Jacopo Mondi via libcamera-devel wrote:\n> > The rotationTransform_ depends on a V4L2 control whose value does not\n> > change for the whole lifetime of the camera.\n> >\n> > Instead of re-calculating it everytime the camera is configured, cache\n> > it at properties initialization time.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/internal/camera_sensor.h | 1 +\n> > src/libcamera/camera_sensor.cpp | 54 +++++++++++++---------\n> > 2 files changed, 32 insertions(+), 23 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > index 02c77ab037da..02b4b4d25e6d 100644\n> > --- a/include/libcamera/internal/camera_sensor.h\n> > +++ b/include/libcamera/internal/camera_sensor.h\n> > @@ -107,6 +107,7 @@ private:\n> > \tRectangle activeArea_;\n> > \tconst BayerFormat *bayerFormat_;\n> > \tbool supportFlips_;\n> > +\tTransform rotationTransform_;\n> > \tControlList properties_;\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index f3a5aa37149f..a15a6c89c5c8 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -461,18 +461,36 @@ int CameraSensor::initProperties()\n> > \tconst auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> > \tif (rotationControl != controls.end()) {\n> > +\t\tpropertyValue = rotationControl->second.def().get();\n> > +\n> > \t\t/*\n> > -\t\t * validateTransform() compensates for the mounting rotation.\n> > -\t\t * However, as a camera sensor can only compensate rotations\n> > -\t\t * by applying H/VFlips, only rotation of 180 degrees are\n> > -\t\t * automatically compensated. The other valid rotations (Rot90\n> > -\t\t * and Rot270) require transposition, which the camera sensor\n> > -\t\t * cannot perform, so leave them untouched.\n> > +\t\t * Cache the Transform associated with the camera mounting\n> > +\t\t * rotation for later use in validateTransform().\n> > +\t\t */\n> > +\t\tbool success;\n> > +\t\trotationTransform_ = transformFromRotation(propertyValue, &success);\n>\n> Probably a good time to convert to std::optional<> here? Or it can be done\n> on top. Either way I don't mind, as long as it happens ;-)\n\nWould would it give an std::optional<> here ?\n\nIn case \"nothing\" has to be done, transformFromRotation() returns\nIdentity and pipeline handlers can safely pass it to\nCameraSensor::setFormat(). True we can make the optional \"transform\"\nargument of CameraSensor::setFormat() and std::optional<> too...\n\n>\n> Other than that, the code looks fine to me.\n>\n> Reviewed-by: Umang Jain \n>\n> > +\t\tif (!success) {\n> > +\t\t\tLOG(CameraSensor, Warning)\n> > +\t\t\t\t<< \"Invalid rotation of \" << propertyValue\n> > +\t\t\t\t<< \" degrees - ignoring\";\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\tpropertyValue = rotationControl->second.def().get();\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 degreees\";\n> > +\t\trotationTransform_ = Transform::Identity;\n> > \t}\n> > \tproperties_.set(properties::PixelArraySize, pixelArraySize_);\n> > @@ -1038,21 +1056,11 @@ void CameraSensor::updateControlInfo()\n> > */\n> > Transform CameraSensor::validateTransform(Transform *transform) const\n> > {\n> > -\t/* Adjust the requested transform to compensate the sensor mounting rotation. */\n> > -\tconst ControlInfoMap &controls = subdev_->controls();\n> > -\tint rotation = 0;\n> > -\n> > -\tconst auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> > -\tif (rotationControl != controls.end())\n> > -\t\trotation = rotationControl->second.def().get();\n> > -\n> > -\tbool success;\n> > -\tTransform rotationTransform = transformFromRotation(rotation, &success);\n> > -\tif (!success)\n> > -\t\tLOG(CameraSensor, Warning) << \"Invalid rotation of \" << rotation\n> > -\t\t\t\t\t << \" degrees - ignoring\";\n> > -\n> > -\tTransform combined = *transform * rotationTransform;\n> > +\t/*\n> > +\t * Combine the requested transform to compensate the sensor mounting\n> > +\t * rotation.\n> > +\t */\n> > +\tTransform combined = *transform * rotationTransform_;\n> > \t/*\n> > \t * We combine the platform and user transform, but must \"adjust away\"\n> > @@ -1083,7 +1091,7 @@ Transform CameraSensor::validateTransform(Transform *transform) const\n> > \t\t * rise to this is the inverse of the rotation. (Recall that\n> > \t\t * combined = transform * rotationTransform.)\n> > \t\t */\n> > -\t\t*transform = -rotationTransform;\n> > +\t\t*transform = -rotationTransform_;\n> > \t\tcombined = Transform::Identity;\n> > \t}\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 8842CC3237\n\tfor ;\n\tWed, 21 Jun 2023 07:40:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E0A56628C2;\n\tWed, 21 Jun 2023 09:40:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0A37E61E41\n\tfor ;\n\tWed, 21 Jun 2023 09:40:14 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:1cf0:b3bc:c785:4625])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9AB59BC;\n\tWed, 21 Jun 2023 09:39:38 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687333214;\n\tbh=6E+ZH3L5wsNuBAoTFOSWIfQcPf1iaIkbvD/OKiIeOzQ=;\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=LREzyCfXk9DHpI+7gCPk8R1i7QRk0O9FJZ6dyPJvyGCf1ie8b4auc8CHBt3WFrNe5\n\tlXHYnIi+osz/Q2al+X7rKUvCx1PIgDN9R6q8AS7KxNSdRLDcLF5fxsrrjTaVxVZeL9\n\tcS/RWFSR5/2E14vfZwgEzexVYdb/KxYVi49xLqN3UtYEwBMn6BTCqzM/P4R3jddS9Z\n\tD4VSv0LxJ+ENmUKuKFu46RdXUnMyfT4QB/x48NfnOzXDy9Aq+0g58esYvJqe5NWBuv\n\t74RtZgxK6444ErjSulmHubQDvwet5Rf0GF3h51CaFhwG0kjZ098SG9M8ndtnoNUHFl\n\tOyQobQyoKshzg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1687333178;\n\tbh=6E+ZH3L5wsNuBAoTFOSWIfQcPf1iaIkbvD/OKiIeOzQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CSv57CCrNaOPn6xVNjA/MzRH1ZaLN7f7EvpsglpC0eNIzakPJOWExxeBZoaYMGRUf\n\t2roplcS0fgbr98AChTEQZ7lCTZfW3gi+Ta0uLjswI3YbWfDDR6/KxolxL9Re8UqWKu\n\to53fVyp3K+zn8U19N2L7DbOQA5STsf4BZH3JWlrE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"CSv57CCr\"; dkim-atps=neutral","Date":"Wed, 21 Jun 2023 09:40:11 +0200","To":"Umang Jain ","Message-ID":"","References":"<20230620142904.72600-1-jacopo.mondi@ideasonboard.com>\n\t<20230620142904.72600-2-jacopo.mondi@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC 1/4] libcamera: camera_sensor: Cache\n\trotationTransform_","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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27412,"web_url":"https://patchwork.libcamera.org/comment/27412/","msgid":"<20230622085420.GA7719@pendragon.ideasonboard.com>","date":"2023-06-22T08:54:20","subject":"Re: [libcamera-devel] [RFC 1/4] libcamera: camera_sensor: Cache\n\trotationTransform_","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 Tue, Jun 20, 2023 at 04:29:01PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> The rotationTransform_ depends on a V4L2 control whose value does not\n> change for the whole lifetime of the camera.\n> \n> Instead of re-calculating it everytime the camera is configured, cache\n> it at properties initialization time.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/camera_sensor.h | 1 +\n> src/libcamera/camera_sensor.cpp | 54 +++++++++++++---------\n> 2 files changed, 32 insertions(+), 23 deletions(-)\n> \n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index 02c77ab037da..02b4b4d25e6d 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -107,6 +107,7 @@ private:\n> \tRectangle activeArea_;\n> \tconst BayerFormat *bayerFormat_;\n> \tbool supportFlips_;\n> +\tTransform rotationTransform_;\n> \n> \tControlList properties_;\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index f3a5aa37149f..a15a6c89c5c8 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -461,18 +461,36 @@ int CameraSensor::initProperties()\n> \n> \tconst auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> \tif (rotationControl != controls.end()) {\n> +\t\tpropertyValue = rotationControl->second.def().get();\n> +\n> \t\t/*\n> -\t\t * validateTransform() compensates for the mounting rotation.\n> -\t\t * However, as a camera sensor can only compensate rotations\n> -\t\t * by applying H/VFlips, only rotation of 180 degrees are\n> -\t\t * automatically compensated. The other valid rotations (Rot90\n> -\t\t * and Rot270) require transposition, which the camera sensor\n> -\t\t * cannot perform, so leave them untouched.\n> +\t\t * Cache the Transform associated with the camera mounting\n> +\t\t * rotation for later use in validateTransform().\n> +\t\t */\n> +\t\tbool success;\n> +\t\trotationTransform_ = transformFromRotation(propertyValue, &success);\n> +\t\tif (!success) {\n> +\t\t\tLOG(CameraSensor, Warning)\n> +\t\t\t\t<< \"Invalid rotation of \" << propertyValue\n> +\t\t\t\t<< \" degrees - ignoring\";\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\tpropertyValue = rotationControl->second.def().get();\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 degreees\";\n\nI think \"degrees\" has enough e's without having to add an extra one :-)\nWith this fixed,\n\nReviewed-by: Laurent Pinchart \n\n> +\t\trotationTransform_ = Transform::Identity;\n> \t}\n> \n> \tproperties_.set(properties::PixelArraySize, pixelArraySize_);\n> @@ -1038,21 +1056,11 @@ void CameraSensor::updateControlInfo()\n> */\n> Transform CameraSensor::validateTransform(Transform *transform) const\n> {\n> -\t/* Adjust the requested transform to compensate the sensor mounting rotation. */\n> -\tconst ControlInfoMap &controls = subdev_->controls();\n> -\tint rotation = 0;\n> -\n> -\tconst auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> -\tif (rotationControl != controls.end())\n> -\t\trotation = rotationControl->second.def().get();\n> -\n> -\tbool success;\n> -\tTransform rotationTransform = transformFromRotation(rotation, &success);\n> -\tif (!success)\n> -\t\tLOG(CameraSensor, Warning) << \"Invalid rotation of \" << rotation\n> -\t\t\t\t\t << \" degrees - ignoring\";\n> -\n> -\tTransform combined = *transform * rotationTransform;\n> +\t/*\n> +\t * Combine the requested transform to compensate the sensor mounting\n> +\t * rotation.\n> +\t */\n> +\tTransform combined = *transform * rotationTransform_;\n> \n> \t/*\n> \t * We combine the platform and user transform, but must \"adjust away\"\n> @@ -1083,7 +1091,7 @@ Transform CameraSensor::validateTransform(Transform *transform) const\n> \t\t * rise to this is the inverse of the rotation. (Recall that\n> \t\t * combined = transform * rotationTransform.)\n> \t\t */\n> -\t\t*transform = -rotationTransform;\n> +\t\t*transform = -rotationTransform_;\n> \t\tcombined = Transform::Identity;\n> \t}\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 2728ABD87C\n\tfor ;\n\tThu, 22 Jun 2023 08:54:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5AD87628C0;\n\tThu, 22 Jun 2023 10:54:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4A48861E3D\n\tfor ;\n\tThu, 22 Jun 2023 10:54:22 +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 3582C891;\n\tThu, 22 Jun 2023 10:53:46 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687424064;\n\tbh=Vlm8b2xeV1+RROffD/jeCReVZArN3E1U7QmSFtpvBlE=;\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=ci/H89WC50VY03y/iH9AN7ENItcGyrhyL9F6iq4TTXIDycTrj0KD4X2L8QQ4pkeuQ\n\tZgYUnSYgLDRCVIgxRrZ3MUx4v5MmXcTr5XXEUsrS0Jra79crvupDk47wA0yWxaIqfb\n\tB16nzJnw0xtl0XAOkVombNYRJ+TFDXnbA3F0Ub9oZCuYPlz1nw4taoLIZyTkHb/BTg\n\tCyhLY/K3r8TmGjxf6IfnUrBK+7Z9IzV1lVVbf4a1dgbSArgsKaJmdQpGmpJ+CnU/9b\n\tAhx8EUbA0sfuqHV1rYtD90sc5YgtPBIY2IbEpag1sV5XUjJNm0Z02/IOQD72r480dq\n\tS7gsVzjCvAT4A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1687424026;\n\tbh=Vlm8b2xeV1+RROffD/jeCReVZArN3E1U7QmSFtpvBlE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jQKhHGPdauJ4Ag9GzSdbaDnVUsM3PWwRZy1pMkLfv847u7y8eLe/LjV9Dermm2INJ\n\taE2gwsJNBpQL7Df7kLSKXNj6yBrO1IqA2RoDwiMLbVYOk0msm6Wj0g+AvqURTR7gCk\n\thyY++JIYxl1OHLmWEMJ0zJO5FopyaQG858x7vMs4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jQKhHGPd\"; dkim-atps=neutral","Date":"Thu, 22 Jun 2023 11:54:20 +0300","To":"Jacopo Mondi ","Message-ID":"<20230622085420.GA7719@pendragon.ideasonboard.com>","References":"<20230620142904.72600-1-jacopo.mondi@ideasonboard.com>\n\t<20230620142904.72600-2-jacopo.mondi@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230620142904.72600-2-jacopo.mondi@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC 1/4] libcamera: camera_sensor: Cache\n\trotationTransform_","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\" "}}]