[{"id":14473,"web_url":"https://patchwork.libcamera.org/comment/14473/","msgid":"<20210107173324.tfep2xl6c3ypq5jg@uno.localdomain>","date":"2021-01-07T17:33:24","subject":"Re: [libcamera-devel] [PATCH] libcamera: ipu3: Add rotation to ipu3\n\tpipeline","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Fabian\n\nOn Wed, Jan 06, 2021 at 02:35:13PM +0100, Fabian Wüthrich wrote:\n> Use the same transformation logic as in the raspberry pipeline to\n> implement rotations in the ipu3 pipeline.\n>\n> Tested on a Surface Book 2 with an experimental driver for OV5693.\n>\n> Signed-off-by: Fabian Wüthrich \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 76 +++++++++++++++++++++++++++-\n> 1 file changed, 74 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index f1151733..76d75a74 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -14,6 +14,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n>\n> @@ -62,6 +63,12 @@ public:\n> \tStream outStream_;\n> \tStream vfStream_;\n> \tStream rawStream_;\n> +\n> +\t/*\n> +\t * Manage horizontal and vertical flips supported (or not) by the\n> +\t * sensor.\n> +\t */\n> +\tbool supportsFlips_;\n> };\n>\n> class IPU3CameraConfiguration : public CameraConfiguration\n> @@ -74,6 +81,9 @@ public:\n> \tconst StreamConfiguration &cio2Format() const { return cio2Configuration_; }\n> \tconst ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; }\n>\n> +\t/* Cache the combinedTransform_ that will be applied to the sensor */\n> +\tTransform combinedTransform_;\n> +\n> private:\n> \t/*\n> \t * The IPU3CameraData instance is guaranteed to be valid as long as the\n> @@ -143,11 +153,54 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \tif (config_.empty())\n> \t\treturn Invalid;\n>\n> -\tif (transform != Transform::Identity) {\n> -\t\ttransform = Transform::Identity;\n> +\tint32_t rotation = data_->cio2_.sensor()->properties().get(properties::Rotation);\n> +\tbool success;\n> +\tTransform rotationTransform = transformFromRotation(rotation, &success);\n\nCan you cache this in CameraData ?\nAt registerCamera() time we initialize the properties so we can cache\nthe above 'rotationTransform' just after\n\n\t\t/* Initialize the camera properties. */\n\t\tdata->properties_ = cio2->sensor()->properties();\n\nInstead of re-calculating it every validation.\n\n> +\tif (!success)\n> +\t\tLOG(IPU3, Warning) << \"Invalid rotation of \" << rotation << \" degrees - ignoring\";\n> +\tTransform combined = transform * rotationTransform;\n> +\n> +\t/*\n> +\t * We combine the platform and user transform, but must \"adjust away\"\n> +\t * any combined result that includes a transform, as we can't do those.\n> +\t * In this case, flipping only the transpose bit is helpful to\n> +\t * applications - they either get the transform they requested, or have\n> +\t * to do a simple transpose themselves (they don't have to worry about\n> +\t * the other possible cases).\n> +\t */\n\nThe 'rotationTransform' obtained by inspecting the sensor rotation\ndoes not contain any Transpose, looking at\nTransform::transformFromRotation().\n\nCan you address the transpose directly in the\nCameraConfiguration::transpose field before multiplying it by\nrotationTransform ?\n\n> +\tif (!!(combined & Transform::Transpose)) {\n> +\t\t/*\n> +\t\t * Flipping the transpose bit in \"transform\" flips it in the\n> +\t\t * combined result too (as it's the last thing that happens),\n> +\t\t * which is of course clearing it.\n> +\t\t */\n> +\t\ttransform ^= Transform::Transpose;\n> +\t\tcombined &= ~Transform::Transpose;\n> \t\tstatus = Adjusted;\n> \t}\n>\n> +\t/*\n> +\t * We also check if the sensor doesn't do h/vflips at all, in which\n> +\t * case we clear them, ad the application will have to do everything.\n\ns/ad/and\n\n> +\t */\n> +\tif (!data_->supportsFlips_ && !!combined) {\n> +\t\t/*\n> +\t\t * If the sensor can do no transforms, then combined must be\n> +\t\t * changed to the identity. The only user transform that gives\n> +\t\t * rise to this the inverse of the rotation. (Recall that\n\ns/this the/this is the/\n\n> +\t\t * combined = transform * rotationTransform.)\n> +\t\t */\n> +\t\ttransform = -rotationTransform;\n> +\t\tcombined = Transform::Identity;\n> +\t\tstatus = Adjusted;\n> +\t}\n> +\n> +\t/*\n> +\t * Store the final combined transform that configure() will need to\n> +\t * apply to the sensor to save us working it out again.\n> +\t */\n> +\tcombinedTransform_ = combined;\n> +\n> \t/* Cap the number of entries to the available streams. */\n> \tif (config_.size() > IPU3_MAX_STREAMS) {\n> \t\tconfig_.resize(IPU3_MAX_STREAMS);\n> @@ -540,6 +593,19 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \t\treturn ret;\n> \t}\n>\n> +\t/*\n> +\t * Configure the H/V flip controls based on the combination of\n> +\t * the sensor and user transform.\n> +\t */\n> +\tif (data->supportsFlips_) {\n> +\t\tctrls = cio2->sensor()->controls();\n\nI would not re-use ctrls, or rather we should probably:\n\n--- a/include/libcamera/controls.h\n+++ b/include/libcamera/controls.h\n@@ -352,7 +352,7 @@ private:\n public:\n ControlList();\n ControlList(const ControlIdMap &idmap, ControlValidator *validator = nullptr);\n- ControlList(const ControlInfoMap &infoMap, ControlValidator *validator = nullptr);\n+ explicit ControlList(const ControlInfoMap &infoMap, ControlValidator *validator = nullptr);\n\nto avoid this assignment. But maybe it's just me\n\nThanks\n j\n\n> +\t\tctrls.set(V4L2_CID_HFLIP,\n> +\t\t\t static_cast(!!(config->combinedTransform_ & Transform::HFlip)));\n> +\t\tctrls.set(V4L2_CID_VFLIP,\n> +\t\t\t static_cast(!!(config->combinedTransform_ & Transform::VFlip)));\n> +\t\tcio2->sensor()->setControls(&ctrls);\n> +\t}\n> +\n> \treturn 0;\n> }\n>\n> @@ -779,6 +845,12 @@ int PipelineHandlerIPU3::registerCameras()\n> \t\t/* Initialze the camera controls. */\n> \t\tdata->controlInfo_ = IPU3Controls;\n>\n> +\t\tControlList ctrls = cio2->sensor()->getControls({ V4L2_CID_HFLIP });\n> +\t\tif (!ctrls.empty()) {\n> +\t\t\t/* We assume it will support vflips too... */\n> +\t\t\tdata->supportsFlips_ = true;\n> +\t\t}\n> +\n> \t\t/**\n> \t\t * \\todo Dynamically assign ImgU and output devices to each\n> \t\t * stream and camera; as of now, limit support to two cameras\n> --\n> 2.30.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 2AB84C0F1A\n\tfor ;\n\tThu, 7 Jan 2021 17:33:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A41B8635B0;\n\tThu, 7 Jan 2021 18:33:11 +0100 (CET)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D2C8962013\n\tfor ;\n\tThu, 7 Jan 2021 18:33:09 +0100 (CET)","from uno.localdomain (unknown [93.56.78.48])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 0C081100009;\n\tThu, 7 Jan 2021 17:33:08 +0000 (UTC)"],"Date":"Thu, 7 Jan 2021 18:33:24 +0100","From":"Jacopo Mondi ","To":"Fabian =?utf-8?b?V8O8dGhyaWNo?= ","Message-ID":"<20210107173324.tfep2xl6c3ypq5jg@uno.localdomain>","References":"<20210106133513.34118-1-me@fabwu.ch>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210106133513.34118-1-me@fabwu.ch>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: ipu3: Add rotation to ipu3\n\tpipeline","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14525,"web_url":"https://patchwork.libcamera.org/comment/14525/","msgid":"<7ee206e0-0215-2267-5471-a5977a555ff2@fabwu.ch>","date":"2021-01-11T15:21:49","subject":"Re: [libcamera-devel] [PATCH] libcamera: ipu3: Add rotation to ipu3\n\tpipeline","submitter":{"id":77,"url":"https://patchwork.libcamera.org/api/people/77/","name":"Fabian Wüthrich","email":"me@fabwu.ch"},"content":"Hi Jacopo\n\nThanks for your review. \n\nOn 07.01.21 18:33, Jacopo Mondi wrote:\n> Hi Fabian\n> \n> On Wed, Jan 06, 2021 at 02:35:13PM +0100, Fabian Wüthrich wrote:\n>> Use the same transformation logic as in the raspberry pipeline to\n>> implement rotations in the ipu3 pipeline.\n>>\n>> Tested on a Surface Book 2 with an experimental driver for OV5693.\n>>\n>> Signed-off-by: Fabian Wüthrich \n>> ---\n>> src/libcamera/pipeline/ipu3/ipu3.cpp | 76 +++++++++++++++++++++++++++-\n>> 1 file changed, 74 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> index f1151733..76d75a74 100644\n>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> @@ -14,6 +14,7 @@\n>> #include \n>> #include \n>> #include \n>> +#include \n>> #include \n>> #include \n>>\n>> @@ -62,6 +63,12 @@ public:\n>> \tStream outStream_;\n>> \tStream vfStream_;\n>> \tStream rawStream_;\n>> +\n>> +\t/*\n>> +\t * Manage horizontal and vertical flips supported (or not) by the\n>> +\t * sensor.\n>> +\t */\n>> +\tbool supportsFlips_;\n>> };\n>>\n>> class IPU3CameraConfiguration : public CameraConfiguration\n>> @@ -74,6 +81,9 @@ public:\n>> \tconst StreamConfiguration &cio2Format() const { return cio2Configuration_; }\n>> \tconst ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; }\n>>\n>> +\t/* Cache the combinedTransform_ that will be applied to the sensor */\n>> +\tTransform combinedTransform_;\n>> +\n>> private:\n>> \t/*\n>> \t * The IPU3CameraData instance is guaranteed to be valid as long as the\n>> @@ -143,11 +153,54 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n>> \tif (config_.empty())\n>> \t\treturn Invalid;\n>>\n>> -\tif (transform != Transform::Identity) {\n>> -\t\ttransform = Transform::Identity;\n>> +\tint32_t rotation = data_->cio2_.sensor()->properties().get(properties::Rotation);\n>> +\tbool success;\n>> +\tTransform rotationTransform = transformFromRotation(rotation, &success);\n> \n> Can you cache this in CameraData ?\n> At registerCamera() time we initialize the properties so we can cache\n> the above 'rotationTransform' just after\n> \n> \t\t/* Initialize the camera properties. */\n> \t\tdata->properties_ = cio2->sensor()->properties();\n> \n> Instead of re-calculating it every validation.\n> \n\nDone\n\n>> +\tif (!success)\n>> +\t\tLOG(IPU3, Warning) << \"Invalid rotation of \" << rotation << \" degrees - ignoring\";\n>> +\tTransform combined = transform * rotationTransform;\n>> +\n>> +\t/*\n>> +\t * We combine the platform and user transform, but must \"adjust away\"\n>> +\t * any combined result that includes a transform, as we can't do those.\n>> +\t * In this case, flipping only the transpose bit is helpful to\n>> +\t * applications - they either get the transform they requested, or have\n>> +\t * to do a simple transpose themselves (they don't have to worry about\n>> +\t * the other possible cases).\n>> +\t */\n> \n> The 'rotationTransform' obtained by inspecting the sensor rotation\n> does not contain any Transpose, looking at\n> Transform::transformFromRotation().\n> \n> Can you address the transpose directly in the\n> CameraConfiguration::transpose field before multiplying it by\n> rotationTransform ?\n> \n\nI copied the code from the raspberry pipeline (see \nsrc/libcamera/pipeline/raspberrypi/raspberrypi.cpp line 287 ff.) and\nassumed that this is the correct way to handle transpositions.\n\nShould I just flip the transpose bit of CameraConfiguration::transpose\nand then combine it with the sensor rotation?\n\nAnd if so should I send a patch for the raspberry pipeline as well?\n\n>> +\tif (!!(combined & Transform::Transpose)) {\n>> +\t\t/*\n>> +\t\t * Flipping the transpose bit in \"transform\" flips it in the\n>> +\t\t * combined result too (as it's the last thing that happens),\n>> +\t\t * which is of course clearing it.\n>> +\t\t */\n>> +\t\ttransform ^= Transform::Transpose;\n>> +\t\tcombined &= ~Transform::Transpose;\n>> \t\tstatus = Adjusted;\n>> \t}\n>>\n>> +\t/*\n>> +\t * We also check if the sensor doesn't do h/vflips at all, in which\n>> +\t * case we clear them, ad the application will have to do everything.\n> \n> s/ad/and\n> \n\nDone\n\n>> +\t */\n>> +\tif (!data_->supportsFlips_ && !!combined) {\n>> +\t\t/*\n>> +\t\t * If the sensor can do no transforms, then combined must be\n>> +\t\t * changed to the identity. The only user transform that gives\n>> +\t\t * rise to this the inverse of the rotation. (Recall that\n> \n> s/this the/this is the/\n> \n\nDone\n\n>> +\t\t * combined = transform * rotationTransform.)\n>> +\t\t */\n>> +\t\ttransform = -rotationTransform;\n>> +\t\tcombined = Transform::Identity;\n>> +\t\tstatus = Adjusted;\n>> +\t}\n>> +\n>> +\t/*\n>> +\t * Store the final combined transform that configure() will need to\n>> +\t * apply to the sensor to save us working it out again.\n>> +\t */\n>> +\tcombinedTransform_ = combined;\n>> +\n>> \t/* Cap the number of entries to the available streams. */\n>> \tif (config_.size() > IPU3_MAX_STREAMS) {\n>> \t\tconfig_.resize(IPU3_MAX_STREAMS);\n>> @@ -540,6 +593,19 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n>> \t\treturn ret;\n>> \t}\n>>\n>> +\t/*\n>> +\t * Configure the H/V flip controls based on the combination of\n>> +\t * the sensor and user transform.\n>> +\t */\n>> +\tif (data->supportsFlips_) {\n>> +\t\tctrls = cio2->sensor()->controls();\n> \n> I would not re-use ctrls, or rather we should probably:\n> \n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -352,7 +352,7 @@ private:\n> public:\n> ControlList();\n> ControlList(const ControlIdMap &idmap, ControlValidator *validator = nullptr);\n> - ControlList(const ControlInfoMap &infoMap, ControlValidator *validator = nullptr);\n> + explicit ControlList(const ControlInfoMap &infoMap, ControlValidator *validator = nullptr);\n> \n> to avoid this assignment. But maybe it's just me\n> \n\nI created a sensor_ctrls variable to prevent the re-use.\n\n> Thanks\n> j\n> \n>> +\t\tctrls.set(V4L2_CID_HFLIP,\n>> +\t\t\t static_cast(!!(config->combinedTransform_ & Transform::HFlip)));\n>> +\t\tctrls.set(V4L2_CID_VFLIP,\n>> +\t\t\t static_cast(!!(config->combinedTransform_ & Transform::VFlip)));\n>> +\t\tcio2->sensor()->setControls(&ctrls);\n>> +\t}\n>> +\n>> \treturn 0;\n>> }\n>>\n>> @@ -779,6 +845,12 @@ int PipelineHandlerIPU3::registerCameras()\n>> \t\t/* Initialze the camera controls. */\n>> \t\tdata->controlInfo_ = IPU3Controls;\n>>\n>> +\t\tControlList ctrls = cio2->sensor()->getControls({ V4L2_CID_HFLIP });\n>> +\t\tif (!ctrls.empty()) {\n>> +\t\t\t/* We assume it will support vflips too... */\n>> +\t\t\tdata->supportsFlips_ = true;\n>> +\t\t}\n>> +\n>> \t\t/**\n>> \t\t * \\todo Dynamically assign ImgU and output devices to each\n>> \t\t * stream and camera; as of now, limit support to two cameras\n>> --\n>> 2.30.0\n>>\n>> _______________________________________________\n>> libcamera-devel mailing list\n>> libcamera-devel@lists.libcamera.org\n>> https://lists.libcamera.org/listinfo/libcamera-devel","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 8BF5DBD808\n\tfor ;\n\tMon, 11 Jan 2021 15:21:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 03123680B5;\n\tMon, 11 Jan 2021 16:21:52 +0100 (CET)","from gusto4.metanet.ch (gusto4.metanet.ch [80.74.154.158])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7E6CB6010E\n\tfor ;\n\tMon, 11 Jan 2021 16:21:50 +0100 (CET)","from [IPv6:2001:1715:9d9c:4e40:c8ee:888d:e6cb:532f] (localhost\n\t[127.0.0.1]) by gusto4.metanet.ch (Postfix) with ESMTPSA id\n\t09ED01011ECA; Mon, 11 Jan 2021 16:21:50 +0100 (CET)"],"Authentication-Results":["lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=fabwu.ch header.i=@fabwu.ch header.b=\"D1wvk+wH\";\n\tdkim-atps=neutral","gusto.metanet.ch;\n\tspf=pass (sender IP is 2001:1715:9d9c:4e40:c8ee:888d:e6cb:532f)\n\tsmtp.mailfrom=me@fabwu.ch\n\tsmtp.helo=[IPv6:2001:1715:9d9c:4e40:c8ee:888d:e6cb:532f]"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=fabwu.ch; s=default; \n\tt=1610378510; bh=eCvzcYe+I2xsPITgpnToIzOZ1Sgenj9BLAVlpmj1Sdo=;\n\th=Subject:To:From;\n\tb=D1wvk+wHyPK4awVRDpNLR99Gi/XAHhlTlBMSPSHGr8wVVCFCBa+lFqyrqc0Ef6yIT\n\tUphQxtf0jsobAxpLW4bs4O/rE5xUs3kNtd3inZMOmod/bsxCMa3t0PLfYFo8wTZUkm\n\tDMkm3nU8CD2k9W+rM06OZFXNae5ReWEaxo74MA1k=","Received-SPF":"pass (gusto.metanet.ch: connection is authenticated)","To":"Jacopo Mondi ","References":"<20210106133513.34118-1-me@fabwu.ch>\n\t<20210107173324.tfep2xl6c3ypq5jg@uno.localdomain>","From":"=?utf-8?q?Fabian_W=C3=BCthrich?= ","Message-ID":"<7ee206e0-0215-2267-5471-a5977a555ff2@fabwu.ch>","Date":"Mon, 11 Jan 2021 16:21:49 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.6.0","MIME-Version":"1.0","In-Reply-To":"<20210107173324.tfep2xl6c3ypq5jg@uno.localdomain>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH] libcamera: ipu3: Add rotation to ipu3\n\tpipeline","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14526,"web_url":"https://patchwork.libcamera.org/comment/14526/","msgid":"<20210111154527.6df3bbzhgwcwsdax@uno.localdomain>","date":"2021-01-11T15:45:27","subject":"Re: [libcamera-devel] [PATCH] libcamera: ipu3: Add rotation to ipu3\n\tpipeline","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Fabian\n\nOn Mon, Jan 11, 2021 at 04:21:49PM +0100, Fabian Wüthrich wrote:\n> Hi Jacopo\n>\n> Thanks for your review.\n>\n> On 07.01.21 18:33, Jacopo Mondi wrote:\n> > Hi Fabian\n> >\n> > On Wed, Jan 06, 2021 at 02:35:13PM +0100, Fabian Wüthrich wrote:\n> >> Use the same transformation logic as in the raspberry pipeline to\n> >> implement rotations in the ipu3 pipeline.\n> >>\n> >> Tested on a Surface Book 2 with an experimental driver for OV5693.\n> >>\n> >> Signed-off-by: Fabian Wüthrich \n> >> ---\n> >> src/libcamera/pipeline/ipu3/ipu3.cpp | 76 +++++++++++++++++++++++++++-\n> >> 1 file changed, 74 insertions(+), 2 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> index f1151733..76d75a74 100644\n> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> @@ -14,6 +14,7 @@\n> >> #include \n> >> #include \n> >> #include \n> >> +#include \n> >> #include \n> >> #include \n> >>\n> >> @@ -62,6 +63,12 @@ public:\n> >> \tStream outStream_;\n> >> \tStream vfStream_;\n> >> \tStream rawStream_;\n> >> +\n> >> +\t/*\n> >> +\t * Manage horizontal and vertical flips supported (or not) by the\n> >> +\t * sensor.\n> >> +\t */\n> >> +\tbool supportsFlips_;\n> >> };\n> >>\n> >> class IPU3CameraConfiguration : public CameraConfiguration\n> >> @@ -74,6 +81,9 @@ public:\n> >> \tconst StreamConfiguration &cio2Format() const { return cio2Configuration_; }\n> >> \tconst ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; }\n> >>\n> >> +\t/* Cache the combinedTransform_ that will be applied to the sensor */\n> >> +\tTransform combinedTransform_;\n> >> +\n> >> private:\n> >> \t/*\n> >> \t * The IPU3CameraData instance is guaranteed to be valid as long as the\n> >> @@ -143,11 +153,54 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> >> \tif (config_.empty())\n> >> \t\treturn Invalid;\n> >>\n> >> -\tif (transform != Transform::Identity) {\n> >> -\t\ttransform = Transform::Identity;\n> >> +\tint32_t rotation = data_->cio2_.sensor()->properties().get(properties::Rotation);\n> >> +\tbool success;\n> >> +\tTransform rotationTransform = transformFromRotation(rotation, &success);\n> >\n> > Can you cache this in CameraData ?\n> > At registerCamera() time we initialize the properties so we can cache\n> > the above 'rotationTransform' just after\n> >\n> > \t\t/* Initialize the camera properties. */\n> > \t\tdata->properties_ = cio2->sensor()->properties();\n> >\n> > Instead of re-calculating it every validation.\n> >\n>\n> Done\n>\n> >> +\tif (!success)\n> >> +\t\tLOG(IPU3, Warning) << \"Invalid rotation of \" << rotation << \" degrees - ignoring\";\n> >> +\tTransform combined = transform * rotationTransform;\n> >> +\n> >> +\t/*\n> >> +\t * We combine the platform and user transform, but must \"adjust away\"\n> >> +\t * any combined result that includes a transform, as we can't do those.\n> >> +\t * In this case, flipping only the transpose bit is helpful to\n> >> +\t * applications - they either get the transform they requested, or have\n> >> +\t * to do a simple transpose themselves (they don't have to worry about\n> >> +\t * the other possible cases).\n> >> +\t */\n> >\n> > The 'rotationTransform' obtained by inspecting the sensor rotation\n> > does not contain any Transpose, looking at\n> > Transform::transformFromRotation().\n> >\n> > Can you address the transpose directly in the\n> > CameraConfiguration::transpose field before multiplying it by\n> > rotationTransform ?\n> >\n>\n> I copied the code from the raspberry pipeline (see\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp line 287 ff.) and\n> assumed that this is the correct way to handle transpositions.\n>\n> Should I just flip the transpose bit of CameraConfiguration::transpose\n> and then combine it with the sensor rotation?\n>\n> And if so should I send a patch for the raspberry pipeline as well?\n>\n\nOk, didn't know it came from there. I think it's a minor\noptimization, and changing it in two places might delay the acceptance\nof this patch (and require you more work). If the two implementations are\nalgigned, they can be changed in one go later.\n\nWith the other comments addressed\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> >> +\tif (!!(combined & Transform::Transpose)) {\n> >> +\t\t/*\n> >> +\t\t * Flipping the transpose bit in \"transform\" flips it in the\n> >> +\t\t * combined result too (as it's the last thing that happens),\n> >> +\t\t * which is of course clearing it.\n> >> +\t\t */\n> >> +\t\ttransform ^= Transform::Transpose;\n> >> +\t\tcombined &= ~Transform::Transpose;\n> >> \t\tstatus = Adjusted;\n> >> \t}\n> >>\n> >> +\t/*\n> >> +\t * We also check if the sensor doesn't do h/vflips at all, in which\n> >> +\t * case we clear them, ad the application will have to do everything.\n> >\n> > s/ad/and\n> >\n>\n> Done\n>\n> >> +\t */\n> >> +\tif (!data_->supportsFlips_ && !!combined) {\n> >> +\t\t/*\n> >> +\t\t * If the sensor can do no transforms, then combined must be\n> >> +\t\t * changed to the identity. The only user transform that gives\n> >> +\t\t * rise to this the inverse of the rotation. (Recall that\n> >\n> > s/this the/this is the/\n> >\n>\n> Done\n>\n> >> +\t\t * combined = transform * rotationTransform.)\n> >> +\t\t */\n> >> +\t\ttransform = -rotationTransform;\n> >> +\t\tcombined = Transform::Identity;\n> >> +\t\tstatus = Adjusted;\n> >> +\t}\n> >> +\n> >> +\t/*\n> >> +\t * Store the final combined transform that configure() will need to\n> >> +\t * apply to the sensor to save us working it out again.\n> >> +\t */\n> >> +\tcombinedTransform_ = combined;\n> >> +\n> >> \t/* Cap the number of entries to the available streams. */\n> >> \tif (config_.size() > IPU3_MAX_STREAMS) {\n> >> \t\tconfig_.resize(IPU3_MAX_STREAMS);\n> >> @@ -540,6 +593,19 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> >> \t\treturn ret;\n> >> \t}\n> >>\n> >> +\t/*\n> >> +\t * Configure the H/V flip controls based on the combination of\n> >> +\t * the sensor and user transform.\n> >> +\t */\n> >> +\tif (data->supportsFlips_) {\n> >> +\t\tctrls = cio2->sensor()->controls();\n> >\n> > I would not re-use ctrls, or rather we should probably:\n> >\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -352,7 +352,7 @@ private:\n> > public:\n> > ControlList();\n> > ControlList(const ControlIdMap &idmap, ControlValidator *validator = nullptr);\n> > - ControlList(const ControlInfoMap &infoMap, ControlValidator *validator = nullptr);\n> > + explicit ControlList(const ControlInfoMap &infoMap, ControlValidator *validator = nullptr);\n> >\n> > to avoid this assignment. But maybe it's just me\n> >\n>\n> I created a sensor_ctrls variable to prevent the re-use.\n>\n> > Thanks\n> > j\n> >\n> >> +\t\tctrls.set(V4L2_CID_HFLIP,\n> >> +\t\t\t static_cast(!!(config->combinedTransform_ & Transform::HFlip)));\n> >> +\t\tctrls.set(V4L2_CID_VFLIP,\n> >> +\t\t\t static_cast(!!(config->combinedTransform_ & Transform::VFlip)));\n> >> +\t\tcio2->sensor()->setControls(&ctrls);\n> >> +\t}\n> >> +\n> >> \treturn 0;\n> >> }\n> >>\n> >> @@ -779,6 +845,12 @@ int PipelineHandlerIPU3::registerCameras()\n> >> \t\t/* Initialze the camera controls. */\n> >> \t\tdata->controlInfo_ = IPU3Controls;\n> >>\n> >> +\t\tControlList ctrls = cio2->sensor()->getControls({ V4L2_CID_HFLIP });\n> >> +\t\tif (!ctrls.empty()) {\n> >> +\t\t\t/* We assume it will support vflips too... */\n> >> +\t\t\tdata->supportsFlips_ = true;\n> >> +\t\t}\n> >> +\n> >> \t\t/**\n> >> \t\t * \\todo Dynamically assign ImgU and output devices to each\n> >> \t\t * stream and camera; as of now, limit support to two cameras\n> >> --\n> >> 2.30.0\n> >>\n> >> _______________________________________________\n> >> libcamera-devel mailing list\n> >> libcamera-devel@lists.libcamera.org\n> >> https://lists.libcamera.org/listinfo/libcamera-devel","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 6E711BD80C\n\tfor ;\n\tMon, 11 Jan 2021 15:45:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C65E680B7;\n\tMon, 11 Jan 2021 16:45:12 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 49E246010E\n\tfor ;\n\tMon, 11 Jan 2021 16:45:11 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id AC0A740008;\n\tMon, 11 Jan 2021 15:45:10 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 11 Jan 2021 16:45:27 +0100","From":"Jacopo Mondi ","To":"Fabian =?utf-8?b?V8O8dGhyaWNo?= ","Message-ID":"<20210111154527.6df3bbzhgwcwsdax@uno.localdomain>","References":"<20210106133513.34118-1-me@fabwu.ch>\n\t<20210107173324.tfep2xl6c3ypq5jg@uno.localdomain>\n\t<7ee206e0-0215-2267-5471-a5977a555ff2@fabwu.ch>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<7ee206e0-0215-2267-5471-a5977a555ff2@fabwu.ch>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: ipu3: Add rotation to ipu3\n\tpipeline","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]