[{"id":12613,"web_url":"https://patchwork.libcamera.org/comment/12613/","msgid":"<697cde8f-220b-b9e6-a76d-25fdc701cf24@ideasonboard.com>","date":"2020-09-21T09:47:38","subject":"Re: [libcamera-devel] [PATCH v8 6/8] libcamera: raspberrypi: Set\n\tcamera flips correctly from user transform","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi David,\n\nOn 07/09/2020 08:16, David Plowman wrote:\n> The Raspberry Pi pipeline handler allows all transforms except those\n> involving a transpose. The user transform is combined with any\n> inherent rotation of the camera, and the camera's H and V flip bits\n> are set accordingly.\n> \n> Note that the validate() method has to work out what the final Bayer\n> order of any raw streams will be, before configure() actually applies\n> the transform to the sensor. We make a note of the \"native\"\n> (untransformed) Bayer order when the system starts, so that we can\n> deduce transformed Bayer orders more easily.\n> \n> Signed-off-by: David Plowman \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 155 ++++++++++++++++--\n> 1 file changed, 143 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 1087257..cc9af2c 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -23,6 +23,7 @@\n> \n> #include \n> \n> +#include \"libcamera/internal/bayer_format.h\"\n> #include \"libcamera/internal/camera_sensor.h\"\n> #include \"libcamera/internal/device_enumerator.h\"\n> #include \"libcamera/internal/ipa_manager.h\"\n> @@ -287,6 +288,7 @@ class RPiCameraData : public CameraData\n> 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 dropFrame_(false), ispOutputCount_(0)\n> \t{\n> \t}\n> @@ -294,7 +296,7 @@ public:\n> \tvoid frameStarted(uint32_t sequence);\n> \n> \tint loadIPA();\n> -\tint configureIPA();\n> +\tint configureIPA(const CameraConfiguration *config);\n> \n> \tvoid queueFrameAction(unsigned int frame, const IPAOperationData &action);\n> \n> @@ -335,6 +337,15 @@ public:\n> \tstd::queue embeddedQueue_;\n> \tstd::deque requestQueue_;\n> \n> +\t/*\n> +\t * Manage horizontal and vertical flips supported (or not) by the\n> +\t * sensor. Also store the \"native\" Bayer order (that is, with no\n> +\t * transforms applied).\n> +\t */\n> +\tbool supportsFlips_;\n> +\tbool flipsAlterBayerOrder_;\n> +\tBayerFormat::Order nativeBayerOrder_;\n> +\n> private:\n> \tvoid checkRequestCompleted();\n> \tvoid tryRunPipeline();\n> @@ -352,6 +363,9 @@ public:\n> \n> \tStatus validate() override;\n> \n> +\t/* Cache the combinedTransform_ that will be applied to the sensor */\n> +\tTransform combinedTransform_;\n> +\n> private:\n> \tconst RPiCameraData *data_;\n> };\n> @@ -400,11 +414,61 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> \tif (config_.empty())\n> \t\treturn Invalid;\n> \n> -\tif (transform != Transform::Identity) {\n> -\t\ttransform = Transform::Identity;\n> +\t/*\n> +\t * What if the platform has a non-90 degree rotation? We can't even\n> +\t * \"adjust\" the configuration and carry on. Alternatively, raising an\n> +\t * error means the platform can never run. Let's just print a warning\n> +\t * and continue regardless; the rotation is effectively set to zero.\n> +\t */\n> +\tint32_t rotation = data_->sensor_->properties().get(properties::Rotation);\n> +\tbool success;\n> +\tTransform rotationTransform = transformFromRotation(rotation, &success);\n> +\tif (!success)\n> +\t\tLOG(RPI, Warning) << \"Invalid rotation of \" << rotation\n> +\t\t\t\t << \" degrees - ignoring\";\n> +\tTransform combined = transform * rotationTransform;\n\nI think I recall that the rotation can never be anything other than a\nmultiple of 90, so this shouldn't happen, but given the return values I\nthink it's fine to validate it all the same.\n\nPerhaps our rotation control should return an enum of supported values\nrather than a freeform integer to convey this. But that's all separate.\n\n\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> +\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, and the application will have to do everything.\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> +\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> \tunsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0;\n> \tstd::pair outSize[2];\n> \tSize maxSize;\n> @@ -420,7 +484,23 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> \t\t\tif (ret)\n> \t\t\t\treturn Invalid;\n> \n> -\t\t\tPixelFormat sensorPixFormat = sensorFormat.fourcc.toPixelFormat();\n> +\t\t\t/*\n> +\t\t\t * Some sensors change their Bayer order when they are\n> +\t\t\t * h-flipped or v-flipped, according to the transform.\n> +\t\t\t * If this one does, we must advertise the transformed\n> +\t\t\t * Bayer order in the raw stream. Note how we must\n> +\t\t\t * fetch the \"native\" (i.e. untransformed) Bayer order,\n> +\t\t\t * because the sensor may currently be flipped!\n> +\t\t\t */\n> +\t\t\tV4L2PixelFormat fourcc = sensorFormat.fourcc;\n> +\t\t\tif (data_->flipsAlterBayerOrder_) {\n> +\t\t\t\tBayerFormat bayer(fourcc);\n> +\t\t\t\tbayer.order = data_->nativeBayerOrder_;\n> +\t\t\t\tbayer = bayer.transform(combined);\n> +\t\t\t\tfourcc = bayer.toV4L2PixelFormat();\n> +\t\t\t}\n> +\n> +\t\t\tPixelFormat sensorPixFormat = fourcc.toPixelFormat();\n> \t\t\tif (cfg.size != sensorFormat.size ||\n> \t\t\t cfg.pixelFormat != sensorPixFormat) {\n> \t\t\t\tcfg.size = sensorFormat.size;\n> @@ -756,7 +836,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\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();\n> +\tret = data->configureIPA(config);\n> \tif (ret)\n> \t\tLOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> \n> @@ -967,6 +1047,47 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> \t/* Initialize the camera properties. */\n> \tdata->properties_ = data->sensor_->properties();\n> \n> +\t/*\n> +\t * We cache three things about the sensor in relation to transforms\n> +\t * (meaning horizontal and vertical flips).\n> +\t *\n> +\t * Firstly, does it support them?\n> +\t * Secondly, if you use them does it affect the Bayer ordering?\n> +\t * Thirdly, what is the \"native\" Bayer order, when no transforms are\n> +\t * applied?\n> +\t *\n> +\t * As part of answering the final question, we reset the camera to\n> +\t * no transform at all.\n\nI'm still a bit weary of this happening, in case launching a second\ncamera could affect an existing stream - but now I think about it I\ndon't think that can happen as the media-device will be locked - so it\ncan't occur.\n\nAnyway, if it becomes a problem we can revisit it later. I can't see an\nalternative yet, (except having the system camera daemon that does all\nthis once instead of per-launch, which is something we might have to\nconsider in the future anyway).\n\nReviewed-by: Kieran Bingham \n\n> +\t */\n> +\n> +\tV4L2VideoDevice *dev = data->unicam_[Unicam::Image].dev();\n> +\tconst struct v4l2_query_ext_ctrl *hflipCtrl = dev->controlInfo(V4L2_CID_HFLIP);\n> +\tif (hflipCtrl) {\n> +\t\t/* We assume it will support vflips too... */\n> +\t\tdata->supportsFlips_ = true;\n> +\t\tdata->flipsAlterBayerOrder_ = hflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT;\n> +\n> +\t\tControlList ctrls(dev->controls());\n> +\t\tctrls.set(V4L2_CID_HFLIP, 0);\n> +\t\tctrls.set(V4L2_CID_VFLIP, 0);\n> +\t\tdev->setControls(&ctrls);\n> +\t}\n> +\n> +\t/* Look for a valid Bayer format. */\n> +\tBayerFormat bayerFormat;\n> +\tfor (const auto &iter : dev->formats()) {\n> +\t\tV4L2PixelFormat v4l2Format = iter.first;\n> +\t\tbayerFormat = BayerFormat(v4l2Format);\n> +\t\tif (bayerFormat.isValid())\n> +\t\t\tbreak;\n> +\t}\n> +\n> +\tif (!bayerFormat.isValid()) {\n> +\t\tLOG(RPI, Error) << \"No Bayer format found\";\n> +\t\treturn false;\n> +\t}\n> +\tdata->nativeBayerOrder_ = bayerFormat.order;\n> +\n> \t/*\n> \t * List the available output streams.\n> \t * Currently cannot do Unicam streams!\n> @@ -1114,8 +1235,12 @@ int RPiCameraData::loadIPA()\n> \treturn ipa_->init(settings);\n> }\n> \n> -int RPiCameraData::configureIPA()\n> +int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> {\n> +\t/* We know config must be an RPiCameraConfiguration. */\n> +\tconst RPiCameraConfiguration *rpiConfig =\n> +\t\tstatic_cast(config);\n> +\n> \tstd::map streamConfig;\n> \tstd::map entityControls;\n> \tIPAOperationData ipaConfig = {};\n> @@ -1172,12 +1297,18 @@ int RPiCameraData::configureIPA()\n> \t\t\tsensorMetadata_ = result.data[2];\n> \t\t}\n> \n> -\t\t/* Configure the H/V flip controls based on the sensor rotation. */\n> -\t\tControlList ctrls(unicam_[Unicam::Image].dev()->controls());\n> -\t\tint32_t rotation = sensor_->properties().get(properties::Rotation);\n> -\t\tctrls.set(V4L2_CID_HFLIP, static_cast(!!rotation));\n> -\t\tctrls.set(V4L2_CID_VFLIP, static_cast(!!rotation));\n> -\t\tunicam_[Unicam::Image].dev()->setControls(&ctrls);\n> +\t\t/*\n> +\t\t * Configure the H/V flip controls based on the combination of\n> +\t\t * the sensor and user transform.\n> +\t\t */\n> +\t\tif (supportsFlips_) {\n> +\t\t\tControlList ctrls(unicam_[Unicam::Image].dev()->controls());\n> +\t\t\tctrls.set(V4L2_CID_HFLIP,\n> +\t\t\t\t static_cast(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));\n> +\t\t\tctrls.set(V4L2_CID_VFLIP,\n> +\t\t\t\t static_cast(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));\n> +\t\t\tunicam_[Unicam::Image].dev()->setControls(&ctrls);\n> +\t\t}\n> \t}\n> \n> \tif (result.operation & RPI_IPA_CONFIG_SENSOR) {\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 43A6ABF01C\n\tfor ;\n\tMon, 21 Sep 2020 09:47:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AC1A362FD1;\n\tMon, 21 Sep 2020 11:47:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B450B60367\n\tfor ;\n\tMon, 21 Sep 2020 11:47:47 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8E00327B;\n\tMon, 21 Sep 2020 11:47:40 +0200 (CEST)"],"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=\"j21lZ52R\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600681662;\n\tbh=4w5Ed+dG/NWUeEIhl4j7Tln5rCV6OyofEizdDphayss=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=j21lZ52RPidlhummy9i4XorH3HEgiA+wAQ1nUbWZ8jFaqBDXhKMI11mHugU+UQ4ZW\n\tq3PXEjADSQLKQinofyhZqHWGlHWeju+R9bfoPT9LtarJJl6r9IwskJSL7efWNZhuiq\n\tdPPO458AZAINZmIzE6RoMICJFz4ki5tjM+B4r5ak=","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20200907071604.8355-1-david.plowman@raspberrypi.com>\n\t<20200907071604.8355-7-david.plowman@raspberrypi.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<697cde8f-220b-b9e6-a76d-25fdc701cf24@ideasonboard.com>","Date":"Mon, 21 Sep 2020 10:47:38 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20200907071604.8355-7-david.plowman@raspberrypi.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v8 6/8] libcamera: raspberrypi: Set\n\tcamera flips correctly from user transform","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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12799,"web_url":"https://patchwork.libcamera.org/comment/12799/","msgid":"<20200928103526.GA23539@pendragon.ideasonboard.com>","date":"2020-09-28T10:35:26","subject":"Re: [libcamera-devel] [PATCH v8 6/8] libcamera: raspberrypi: Set\n\tcamera flips correctly from user transform","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Mon, Sep 21, 2020 at 10:47:38AM +0100, Kieran Bingham wrote:\n> On 07/09/2020 08:16, David Plowman wrote:\n> > The Raspberry Pi pipeline handler allows all transforms except those\n> > involving a transpose. The user transform is combined with any\n> > inherent rotation of the camera, and the camera's H and V flip bits\n> > are set accordingly.\n> > \n> > Note that the validate() method has to work out what the final Bayer\n> > order of any raw streams will be, before configure() actually applies\n> > the transform to the sensor. We make a note of the \"native\"\n> > (untransformed) Bayer order when the system starts, so that we can\n> > deduce transformed Bayer orders more easily.\n> > \n> > Signed-off-by: David Plowman \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 155 ++++++++++++++++--\n> > 1 file changed, 143 insertions(+), 12 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 1087257..cc9af2c 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -23,6 +23,7 @@\n> > \n> > #include \n> > \n> > +#include \"libcamera/internal/bayer_format.h\"\n> > #include \"libcamera/internal/camera_sensor.h\"\n> > #include \"libcamera/internal/device_enumerator.h\"\n> > #include \"libcamera/internal/ipa_manager.h\"\n> > @@ -287,6 +288,7 @@ class RPiCameraData : public CameraData\n> > 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 dropFrame_(false), ispOutputCount_(0)\n> > \t{\n> > \t}\n> > @@ -294,7 +296,7 @@ public:\n> > \tvoid frameStarted(uint32_t sequence);\n> > \n> > \tint loadIPA();\n> > -\tint configureIPA();\n> > +\tint configureIPA(const CameraConfiguration *config);\n> > \n> > \tvoid queueFrameAction(unsigned int frame, const IPAOperationData &action);\n> > \n> > @@ -335,6 +337,15 @@ public:\n> > \tstd::queue embeddedQueue_;\n> > \tstd::deque requestQueue_;\n> > \n> > +\t/*\n> > +\t * Manage horizontal and vertical flips supported (or not) by the\n> > +\t * sensor. Also store the \"native\" Bayer order (that is, with no\n> > +\t * transforms applied).\n> > +\t */\n> > +\tbool supportsFlips_;\n> > +\tbool flipsAlterBayerOrder_;\n> > +\tBayerFormat::Order nativeBayerOrder_;\n> > +\n> > private:\n> > \tvoid checkRequestCompleted();\n> > \tvoid tryRunPipeline();\n> > @@ -352,6 +363,9 @@ public:\n> > \n> > \tStatus validate() override;\n> > \n> > +\t/* Cache the combinedTransform_ that will be applied to the sensor */\n> > +\tTransform combinedTransform_;\n> > +\n> > private:\n> > \tconst RPiCameraData *data_;\n> > };\n> > @@ -400,11 +414,61 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > \tif (config_.empty())\n> > \t\treturn Invalid;\n> > \n> > -\tif (transform != Transform::Identity) {\n> > -\t\ttransform = Transform::Identity;\n> > +\t/*\n> > +\t * What if the platform has a non-90 degree rotation? We can't even\n> > +\t * \"adjust\" the configuration and carry on. Alternatively, raising an\n> > +\t * error means the platform can never run. Let's just print a warning\n> > +\t * and continue regardless; the rotation is effectively set to zero.\n> > +\t */\n> > +\tint32_t rotation = data_->sensor_->properties().get(properties::Rotation);\n> > +\tbool success;\n> > +\tTransform rotationTransform = transformFromRotation(rotation, &success);\n> > +\tif (!success)\n> > +\t\tLOG(RPI, Warning) << \"Invalid rotation of \" << rotation\n> > +\t\t\t\t << \" degrees - ignoring\";\n> > +\tTransform combined = transform * rotationTransform;\n> \n> I think I recall that the rotation can never be anything other than a\n> multiple of 90, so this shouldn't happen, but given the return values I\n> think it's fine to validate it all the same.\n> \n> Perhaps our rotation control should return an enum of supported values\n> rather than a freeform integer to convey this. But that's all separate.\n> \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> > +\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, and the application will have to do everything.\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> > +\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> > \tunsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0;\n> > \tstd::pair outSize[2];\n> > \tSize maxSize;\n> > @@ -420,7 +484,23 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > \t\t\tif (ret)\n> > \t\t\t\treturn Invalid;\n> > \n> > -\t\t\tPixelFormat sensorPixFormat = sensorFormat.fourcc.toPixelFormat();\n> > +\t\t\t/*\n> > +\t\t\t * Some sensors change their Bayer order when they are\n> > +\t\t\t * h-flipped or v-flipped, according to the transform.\n> > +\t\t\t * If this one does, we must advertise the transformed\n> > +\t\t\t * Bayer order in the raw stream. Note how we must\n> > +\t\t\t * fetch the \"native\" (i.e. untransformed) Bayer order,\n> > +\t\t\t * because the sensor may currently be flipped!\n> > +\t\t\t */\n> > +\t\t\tV4L2PixelFormat fourcc = sensorFormat.fourcc;\n> > +\t\t\tif (data_->flipsAlterBayerOrder_) {\n> > +\t\t\t\tBayerFormat bayer(fourcc);\n> > +\t\t\t\tbayer.order = data_->nativeBayerOrder_;\n> > +\t\t\t\tbayer = bayer.transform(combined);\n> > +\t\t\t\tfourcc = bayer.toV4L2PixelFormat();\n> > +\t\t\t}\n> > +\n> > +\t\t\tPixelFormat sensorPixFormat = fourcc.toPixelFormat();\n> > \t\t\tif (cfg.size != sensorFormat.size ||\n> > \t\t\t cfg.pixelFormat != sensorPixFormat) {\n> > \t\t\t\tcfg.size = sensorFormat.size;\n> > @@ -756,7 +836,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\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();\n> > +\tret = data->configureIPA(config);\n> > \tif (ret)\n> > \t\tLOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> > \n> > @@ -967,6 +1047,47 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > \t/* Initialize the camera properties. */\n> > \tdata->properties_ = data->sensor_->properties();\n> > \n> > +\t/*\n> > +\t * We cache three things about the sensor in relation to transforms\n> > +\t * (meaning horizontal and vertical flips).\n> > +\t *\n> > +\t * Firstly, does it support them?\n> > +\t * Secondly, if you use them does it affect the Bayer ordering?\n> > +\t * Thirdly, what is the \"native\" Bayer order, when no transforms are\n> > +\t * applied?\n> > +\t *\n> > +\t * As part of answering the final question, we reset the camera to\n> > +\t * no transform at all.\n> \n> I'm still a bit weary of this happening, in case launching a second\n> camera could affect an existing stream - but now I think about it I\n> don't think that can happen as the media-device will be locked - so it\n> can't occur.\n\nLocking the media device is only done in Camera::acquire(). We could\nthus in theory have two PipelineHandlerRPi::match() racing each other\nfrom different processes, or a PipelineHandlerRPi::match() call running\nwith a camera acquired by a different process. There's not much else we\ncan do though, if this becomes an issue, we will need the kernel to\nreport the information we need in a way that doesn't require modifying\nthe configuration of the device.\n\nReviewed-by: Laurent Pinchart \n\n> Anyway, if it becomes a problem we can revisit it later. I can't see an\n> alternative yet, (except having the system camera daemon that does all\n> this once instead of per-launch, which is something we might have to\n> consider in the future anyway).\n> \n> Reviewed-by: Kieran Bingham \n> \n> > +\t */\n> > +\n> > +\tV4L2VideoDevice *dev = data->unicam_[Unicam::Image].dev();\n> > +\tconst struct v4l2_query_ext_ctrl *hflipCtrl = dev->controlInfo(V4L2_CID_HFLIP);\n> > +\tif (hflipCtrl) {\n> > +\t\t/* We assume it will support vflips too... */\n> > +\t\tdata->supportsFlips_ = true;\n> > +\t\tdata->flipsAlterBayerOrder_ = hflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT;\n> > +\n> > +\t\tControlList ctrls(dev->controls());\n> > +\t\tctrls.set(V4L2_CID_HFLIP, 0);\n> > +\t\tctrls.set(V4L2_CID_VFLIP, 0);\n> > +\t\tdev->setControls(&ctrls);\n> > +\t}\n> > +\n> > +\t/* Look for a valid Bayer format. */\n> > +\tBayerFormat bayerFormat;\n> > +\tfor (const auto &iter : dev->formats()) {\n> > +\t\tV4L2PixelFormat v4l2Format = iter.first;\n> > +\t\tbayerFormat = BayerFormat(v4l2Format);\n> > +\t\tif (bayerFormat.isValid())\n> > +\t\t\tbreak;\n> > +\t}\n> > +\n> > +\tif (!bayerFormat.isValid()) {\n> > +\t\tLOG(RPI, Error) << \"No Bayer format found\";\n> > +\t\treturn false;\n> > +\t}\n> > +\tdata->nativeBayerOrder_ = bayerFormat.order;\n> > +\n> > \t/*\n> > \t * List the available output streams.\n> > \t * Currently cannot do Unicam streams!\n> > @@ -1114,8 +1235,12 @@ int RPiCameraData::loadIPA()\n> > \treturn ipa_->init(settings);\n> > }\n> > \n> > -int RPiCameraData::configureIPA()\n> > +int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > {\n> > +\t/* We know config must be an RPiCameraConfiguration. */\n> > +\tconst RPiCameraConfiguration *rpiConfig =\n> > +\t\tstatic_cast(config);\n> > +\n> > \tstd::map streamConfig;\n> > \tstd::map entityControls;\n> > \tIPAOperationData ipaConfig = {};\n> > @@ -1172,12 +1297,18 @@ int RPiCameraData::configureIPA()\n> > \t\t\tsensorMetadata_ = result.data[2];\n> > \t\t}\n> > \n> > -\t\t/* Configure the H/V flip controls based on the sensor rotation. */\n> > -\t\tControlList ctrls(unicam_[Unicam::Image].dev()->controls());\n> > -\t\tint32_t rotation = sensor_->properties().get(properties::Rotation);\n> > -\t\tctrls.set(V4L2_CID_HFLIP, static_cast(!!rotation));\n> > -\t\tctrls.set(V4L2_CID_VFLIP, static_cast(!!rotation));\n> > -\t\tunicam_[Unicam::Image].dev()->setControls(&ctrls);\n> > +\t\t/*\n> > +\t\t * Configure the H/V flip controls based on the combination of\n> > +\t\t * the sensor and user transform.\n> > +\t\t */\n> > +\t\tif (supportsFlips_) {\n> > +\t\t\tControlList ctrls(unicam_[Unicam::Image].dev()->controls());\n> > +\t\t\tctrls.set(V4L2_CID_HFLIP,\n> > +\t\t\t\t static_cast(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));\n> > +\t\t\tctrls.set(V4L2_CID_VFLIP,\n> > +\t\t\t\t static_cast(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));\n> > +\t\t\tunicam_[Unicam::Image].dev()->setControls(&ctrls);\n> > +\t\t}\n> > \t}\n> > \n> > \tif (result.operation & RPI_IPA_CONFIG_SENSOR) {","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 3A4E7C3B5B\n\tfor ;\n\tMon, 28 Sep 2020 10:36:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A63A560BD7;\n\tMon, 28 Sep 2020 12:36:06 +0200 (CEST)","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 BB32360364\n\tfor ;\n\tMon, 28 Sep 2020 12:36:04 +0200 (CEST)","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 41E4A54E;\n\tMon, 28 Sep 2020 12:36:01 +0200 (CEST)"],"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=\"giqaAaIc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601289361;\n\tbh=BUI6XG8RFfBCQ0GSMjoFfxK+L+b+OVgQVmUgU7cweRc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=giqaAaIcsBIzue7oifddoakECIJYslsKal+zGxUNyxSJiXRCqBydEQW3efx99/AtL\n\txUenD/pbDEWvIWSmPkEO3U0XFcb8f5LKWBey5PEXekQSbXRINKihFZ8vxlMbwx2WQ1\n\tyWEMWG9TOycUalylEah6YRKswTyZPHvCFAfOJqz8=","Date":"Mon, 28 Sep 2020 13:35:26 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"<20200928103526.GA23539@pendragon.ideasonboard.com>","References":"<20200907071604.8355-1-david.plowman@raspberrypi.com>\n\t<20200907071604.8355-7-david.plowman@raspberrypi.com>\n\t<697cde8f-220b-b9e6-a76d-25fdc701cf24@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<697cde8f-220b-b9e6-a76d-25fdc701cf24@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v8 6/8] libcamera: raspberrypi: Set\n\tcamera flips correctly from user transform","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\" "}}]