[{"id":27584,"web_url":"https://patchwork.libcamera.org/comment/27584/","msgid":"","date":"2023-07-18T12:02:17","subject":"Re: [libcamera-devel] [PATCH v3 09/10] libcamera: Move to use\n\tCameraConfiguration::orientation","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for the update.\n\nOn Tue, 18 Jul 2023 at 11:52, Jacopo Mondi\n wrote:\n>\n> Replace the usage of CameraConfiguration::transform with the newly\n> introduced CameraConfiguration::orientation.\n>\n> Rework and rename the CameraSensor::validateTransform(transform) to\n> CameraSensor::computeTransform(orientation), that given the desired\n> image orientation computes the Transform that pipeline handlers should\n> apply to the sensor to obtain it.\n>\n> Port all pipeline handlers to use the newly introduced function.\n>\n> This commit breaks existing applications as it removes the public\n> CameraConfiguration::transform in favour of\n> CameraConfiguration::orientation.\n>\n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/camera.h | 2 -\n> include/libcamera/internal/camera_sensor.h | 4 +-\n> include/libcamera/transform.h | 7 --\n> src/libcamera/camera.cpp | 16 +---\n> src/libcamera/camera_sensor.cpp | 92 +++++++++----------\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +-\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +-\n> .../pipeline/rpi/common/pipeline_base.cpp | 9 +-\n> src/libcamera/pipeline/simple/simple.cpp | 6 +-\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +-\n> src/libcamera/pipeline/vimc/vimc.cpp | 4 +-\n> 11 files changed, 66 insertions(+), 92 deletions(-)\n>\n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 6e9342773962..98c72ba148ed 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -22,7 +22,6 @@\n> #include \n> #include \n> #include \n> -#include \n>\n> namespace libcamera {\n>\n> @@ -67,7 +66,6 @@ public:\n> bool empty() const;\n> std::size_t size() const;\n>\n> - Transform transform;\n> Orientation orientation;\n>\n> protected:\n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index 02b4b4d25e6d..6c5952a8fa1a 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -32,6 +32,8 @@ class MediaEntity;\n>\n> struct CameraSensorProperties;\n>\n> +enum class Orientation;\n> +\n> class CameraSensor : protected Loggable\n> {\n> public:\n> @@ -71,7 +73,7 @@ public:\n>\n> CameraLens *focusLens() { return focusLens_.get(); }\n>\n> - Transform validateTransform(Transform *transform) const;\n> + Transform computeTransform(Orientation *orientation) const;\n>\n> protected:\n> std::string logPrefix() const override;\n> diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h\n> index 833b50d46fd0..847a1f94713c 100644\n> --- a/include/libcamera/transform.h\n> +++ b/include/libcamera/transform.h\n> @@ -16,20 +16,13 @@ enum class Orientation;\n> enum class Transform : int {\n> Identity = 0,\n> Rot0 = Identity,\n> -\n> HFlip = 1,\n> -\n> VFlip = 2,\n> -\n> HVFlip = HFlip | VFlip,\n> Rot180 = HVFlip,\n> -\n> Transpose = 4,\n> -\n> Rot270 = HFlip | Transpose,\n> -\n> Rot90 = VFlip | Transpose,\n> -\n> Rot180Transpose = HFlip | VFlip | Transpose\n> };\n>\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index d4ad4a553752..a9ea821a78c2 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -160,8 +160,7 @@ LOG_DECLARE_CATEGORY(Camera)\n> * \\brief Create an empty camera configuration\n> */\n> CameraConfiguration::CameraConfiguration()\n> - : transform(Transform::Identity), orientation(Orientation::rotate0),\n> - config_({})\n> + : orientation(Orientation::rotate0), config_({})\n> {\n> }\n>\n> @@ -392,19 +391,6 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF\n> return status;\n> }\n>\n> -/**\n> - * \\var CameraConfiguration::transform\n> - * \\brief User-specified transform to be applied to the image\n> - *\n> - * The transform is a user-specified 2D plane transform that will be applied\n> - * to the camera images by the processing pipeline before being handed to\n> - * the application.\n> - *\n> - * The usual 2D plane transforms are allowed here (horizontal/vertical\n> - * flips, multiple of 90-degree rotations etc.), but the validate() function\n> - * may adjust this field at its discretion if the selection is not supported.\n> - */\n> -\n> /**\n> * \\var CameraConfiguration::orientation\n> * \\brief The desired orientation of the images produced by the camera\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 038d8b959072..3f06ceae5659 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -15,6 +15,7 @@\n> #include \n> #include \n>\n> +#include \n> #include \n>\n> #include \n> @@ -465,7 +466,7 @@ int CameraSensor::initProperties()\n>\n> /*\n> * Cache the Transform associated with the camera mounting\n> - * rotation for later use in validateTransform().\n> + * rotation for later use in computeTransform().\n> */\n> bool success;\n> rotationTransform_ = transformFromRotation(propertyValue, &success);\n> @@ -1024,69 +1025,64 @@ void CameraSensor::updateControlInfo()\n> */\n>\n> /**\n> - * \\brief Validate a transform request against the sensor capabilities\n> - * \\param[inout] transform The requested transformation, updated to match\n> - * the sensor capabilities\n> + * \\brief Compute the Transform that gives the requested \\a orientation\n> + * \\param[inout] orientation The desired image orientation\n> *\n> - * The input \\a transform is the transform that the caller wants, and it is\n> - * adjusted according to the capabilities of the sensor to represent the\n> - * \"nearest\" transform that can actually be delivered.\n> + * This function computes the Transform that the pipeline handler should apply\n> + * to the CameraSensor to obtain the requested \\a orientation.\n> *\n> - * The returned Transform is the transform applied to the sensor in order to\n> - * produce the input \\a transform, It is also validated against the sensor's\n> - * ability to perform horizontal and vertical flips.\n> + * The intended caller of this function is the validate() implementation of\n> + * pipeline handlers, that pass in the application requested\n> + * CameraConfiguration::orientation and obtain a Transform to apply to the\n> + * camera sensor, likely at configure() time.\n> *\n> - * For example, if the requested \\a transform is Transform::Identity and the\n> - * sensor rotation is 180 degrees, the output transform will be\n> - * Transform::Rot180 to correct the images so that they appear to have\n> - * Transform::Identity, but only if the sensor can apply horizontal and vertical\n> - * flips.\n> + * If the requested \\a orientation cannot be obtained, the \\a orientation\n> + * parameter is adjusted to report the current image orientation and\n> + * Transform::Identity is returned.\n> *\n> - * \\return A Transform instance that represents which transformation has been\n> - * applied to the camera sensor\n> + * If the requested \\a orientation can be obtained, the function computes a\n> + * Transform and does not adjust \\a orientation.\n> + *\n> + * Pipeline handlers are expected to verify if \\a orientation has been\n> + * adjusted by this function and set the CameraConfiguration::status to\n> + * Adjusted accordingly.\n> + *\n> + * \\return A Transform instance that applied to the CameraSensor produces images\n> + * with \\a orientation\n> */\n> -Transform CameraSensor::validateTransform(Transform *transform) const\n> +Transform CameraSensor::computeTransform(Orientation *orientation) const\n> {\n> + Orientation mountingOrientation = transformToOrientation(rotationTransform_);\n\nI suppose I wonder slightly whether we should simply cache the\nrotation here as an orientation in the first place, and not a\ntransform, because I think it probably is in reality an orientation.\nBut this is only a teeny semantic quibble, I'm not really fussed!\n\nReviewed-by: David Plowman \n\nThanks\nDavid\n\n> +\n> /*\n> - * Combine the requested transform to compensate the sensor mounting\n> - * rotation.\n> + * We cannot do any flips: we cannot change the native camera mounting\n> + * orientation.\n> */\n> - Transform combined = rotationTransform_ * *transform;\n> + if (!supportFlips_) {\n> + *orientation = mountingOrientation;\n> + return Transform::Identity;\n> + }\n>\n> /*\n> - * We combine the platform and user transform, but must \"adjust away\"\n> - * any combined result that includes a transposition, as we can't do\n> - * those. In this case, flipping only the transpose bit is helpful to\n> - * applications - they either get the transform they requested, or have\n> - * to do a simple transpose themselves (they don't have to worry about\n> - * the other possible cases).\n> + * Now compute the required transform to obtain 'orientation' starting\n> + * from the mounting rotation.\n> + *\n> + * As a note:\n> + * orientation / mountingOrientation = transform\n> + * mountingOrientation * transform = orientation\n> */\n> - if (!!(combined & Transform::Transpose)) {\n> - /*\n> - * Flipping the transpose bit in \"transform\" flips it in the\n> - * combined result too (as it's the last thing that happens),\n> - * which is of course clearing it.\n> - */\n> - *transform ^= Transform::Transpose;\n> - combined &= ~Transform::Transpose;\n> - }\n> + Transform transform = *orientation / mountingOrientation;\n>\n> /*\n> - * We also check if the sensor doesn't do h/vflips at all, in which\n> - * case we clear them, and the application will have to do everything.\n> + * If transform contains any Transpose we cannot do it, so adjust\n> + * 'orientation' to report the image native orientation and return Identity.\n> */\n> - if (!supportFlips_ && !!combined) {\n> - /*\n> - * If the sensor can do no transforms, then combined must be\n> - * changed to the identity. The only user transform that gives\n> - * rise to this is the inverse of the rotation. (Recall that\n> - * combined = rotationTransform * transform.)\n> - */\n> - *transform = -rotationTransform_;\n> - combined = Transform::Identity;\n> + if (!!(transform & Transform::Transpose)) {\n> + *orientation = mountingOrientation;\n> + return Transform::Identity;\n> }\n>\n> - return combined;\n> + return transform;\n> }\n>\n> std::string CameraSensor::logPrefix() const\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index a81c817a1255..fa4bd0bb73e2 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -187,9 +187,9 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> * rotation and store the final combined transform that configure() will\n> * need to apply to the sensor to save us working it out again.\n> */\n> - Transform requestedTransform = transform;\n> - combinedTransform_ = data_->cio2_.sensor()->validateTransform(&transform);\n> - if (transform != requestedTransform)\n> + Orientation requestedOrientation = orientation;\n> + combinedTransform_ = data_->cio2_.sensor()->computeTransform(&orientation);\n> + if (orientation != requestedOrientation)\n> status = Adjusted;\n>\n> /* Cap the number of entries to the available streams. */\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 6efa79f29f66..586b46d64630 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -481,9 +481,9 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> status = Adjusted;\n> }\n>\n> - Transform requestedTransform = transform;\n> - Transform combined = sensor->validateTransform(&transform);\n> - if (transform != requestedTransform)\n> + Orientation requestedOrientation = orientation;\n> + combinedTransform_ = data_->sensor_->computeTransform(&orientation);\n> + if (orientation != requestedOrientation)\n> status = Adjusted;\n>\n> /*\n> @@ -595,8 +595,6 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> if (sensorFormat_.size.isNull())\n> sensorFormat_.size = sensor->resolution();\n>\n> - combinedTransform_ = combined;\n> -\n> return status;\n> }\n>\n> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> index 179a5b81a516..5d541d71defe 100644\n> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> @@ -254,9 +254,9 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> * rotation and store the final combined transform that configure() will\n> * need to apply to the sensor to save us working it out again.\n> */\n> - Transform requestedTransform = transform;\n> - combinedTransform_ = data_->sensor_->validateTransform(&transform);\n> - if (transform != requestedTransform)\n> + Orientation requestedOrientation = orientation;\n> + combinedTransform_ = data_->sensor_->computeTransform(&orientation);\n> + if (orientation != requestedOrientation)\n> status = Adjusted;\n>\n> std::vector rawStreams, outStreams;\n> @@ -1193,7 +1193,8 @@ int CameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::Config\n> }\n>\n> /* Always send the user transform to the IPA. */\n> - params.transform = static_cast(config->transform);\n> + params.transform =\n> + static_cast(transformFromOrientation(config->orientation));\n>\n> /* Ready the IPA - it must know about the sensor resolution. */\n> ret = ipa_->configure(sensorInfo_, params, result);\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 05ba76bce630..911051b28498 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -889,9 +889,9 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> if (config_.empty())\n> return Invalid;\n>\n> - Transform requestedTransform = transform;\n> - combinedTransform_ = sensor->validateTransform(&transform);\n> - if (transform != requestedTransform)\n> + Orientation requestedOrientation = orientation;\n> + combinedTransform_ = sensor->computeTransform(&orientation);\n> + if (orientation != requestedOrientation)\n> status = Adjusted;\n>\n> /* Cap the number of entries to the available streams. */\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 38f48a5d9269..4bdb7e5f9b87 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -111,8 +111,8 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()\n> if (config_.empty())\n> return Invalid;\n>\n> - if (transform != Transform::Identity) {\n> - transform = Transform::Identity;\n> + if (orientation != Orientation::rotate0) {\n> + orientation = Orientation::rotate0;\n> status = Adjusted;\n> }\n>\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index 00e6f4c6a51f..bc705f01284a 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -128,8 +128,8 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()\n> if (config_.empty())\n> return Invalid;\n>\n> - if (transform != Transform::Identity) {\n> - transform = Transform::Identity;\n> + if (orientation != Orientation::rotate0) {\n> + orientation = Orientation::rotate0;\n> status = Adjusted;\n> }\n>\n> --\n> 2.40.1\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 54806BDC71\n\tfor ;\n\tTue, 18 Jul 2023 12:02:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BD70D628C1;\n\tTue, 18 Jul 2023 14:02:30 +0200 (CEST)","from mail-qt1-x831.google.com (mail-qt1-x831.google.com\n\t[IPv6:2607:f8b0:4864:20::831])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 22B87628BC\n\tfor ;\n\tTue, 18 Jul 2023 14:02:29 +0200 (CEST)","by mail-qt1-x831.google.com with SMTP id\n\td75a77b69052e-40292285362so40060451cf.3\n\tfor ;\n\tTue, 18 Jul 2023 05:02:29 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689681750;\n\tbh=ccwinX9icbnIodYe7ic1AZwIZz+AZtq08VzfCKsWCuA=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=evVIijG6xhP1KKazQCtqPkNa6/I2/hk56hoadeH/9g4/QzgGjDmR9HrRTFoJ8PqN/\n\tVg8U2Y35QtUIX5V+5BBHVWtO8Vm0JC/pj5ZFBRTnQ7IjktL+wSmooYnFUWQiWQQyyR\n\twADRCS/b2FRxpvxQKjMNR+zOyJg84X8sw/t2yu98Sx7eI2GUZs0u2stbFk7M7G7JUH\n\tqnT75C0A7kaO3GAsL8toToSe2DI9o96+8n+fL45DUmL6Z0GWCwYmwaZBOd8keLnYUd\n\tQCsyCa8Kid8UnFMPJU/xWDHSOizZheZddyXwkulF3tz7YcogVpAE+pgZ3hT52sN/Ot\n\tnl7f86zESUQyg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1689681748; x=1692273748;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=FgUDBmU/bIxAVf8fEXYmdN5pwQeQZT2euRlFt+uTBMw=;\n\tb=EWNOFtDo3jkBHEMgiPDQfF9yTFIVHm7n5EBNtpP7Bdpb6tFa+CSGKJfA7tK4J8/mfL\n\tYEgBplZLMB8XFpDXLbFhI+oWqhx7yY9kDVs1VsAOWW9IaRTZghZL06lrXL54JaYGopIK\n\tUYzQF93aO+7VzBRoWkZhQdT9u4+TpWqmMETKIYuq4/iREsOdCv3TPCqH//ufhx2jxJ6w\n\tQFwJq/ztG552gkMdbzfjFGiUqk588xEZDdccP4343TIWs8J4ZpINh7YAOI8dXKDXR6X4\n\tc0DGC6G4nKtFllOmu8lIPd/LlRElNN1ITsqssRg9IDooRWhY1wmFfjpklvfDgt/8nksc\n\tnHPQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"EWNOFtDo\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1689681748; x=1692273748;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=FgUDBmU/bIxAVf8fEXYmdN5pwQeQZT2euRlFt+uTBMw=;\n\tb=B4S0bvFpaijWuX150tsOuV498H5IuUP1qDXX5X8a7hWVinLAXGEqnnO8UL6ko7qrN9\n\tosx7r6bXQKWBXmhiqnG/mcj53t+QR/460/MriJBRKHamKNPPjGVRRbljK6+s7sNePi/w\n\tzbjF2rHkOTH+8p2sg7jvR4+gyAk43FV+a1RlboEm54UOLocYhF+09Q9mU3q4M+I1wAtR\n\tbR3t2ITt9EE1/eICHcN2tegUX71gMj6R/6jOG4SxmuHzc+eX/7jDplBE03flYWaUKLCk\n\tQWZDDu8mduJSFgTQWtolL7Z8/ope1RJixRaA49XtYATWMbfWU82EsX9zH0RjtxLGXHCU\n\tPQ5w==","X-Gm-Message-State":"ABy/qLY7YLwJsJL5SM7ebJ1mV3QvUYjeotpW/BFTX+97CC6oWtjred5v\n\t/ifH+0CBuyi8zmXVOr9KcZ8rFooyBZtii/WjYf0hlg==","X-Google-Smtp-Source":"APBJJlEVCJB2/czXP6YV0lr+ueCtBK6n59L8T+qVtlK+FWBzSv8ziEhjc1xZ0yWk9e/nz3eI2wfJnp1d3s2Lj3vUwJ8=","X-Received":"by 2002:a0c:f292:0:b0:623:6b1a:c5f1 with SMTP id\n\tk18-20020a0cf292000000b006236b1ac5f1mr14377832qvl.4.1689681747657;\n\tTue, 18 Jul 2023 05:02:27 -0700 (PDT)","MIME-Version":"1.0","References":"<20230718105210.83558-1-jacopo.mondi@ideasonboard.com>\n\t<20230718105210.83558-10-jacopo.mondi@ideasonboard.com>","In-Reply-To":"<20230718105210.83558-10-jacopo.mondi@ideasonboard.com>","Date":"Tue, 18 Jul 2023 13:02:17 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 09/10] libcamera: Move to use\n\tCameraConfiguration::orientation","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":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27586,"web_url":"https://patchwork.libcamera.org/comment/27586/","msgid":"","date":"2023-07-18T12:21:54","subject":"Re: [libcamera-devel] [PATCH v3 09/10] libcamera: Move to use\n\tCameraConfiguration::orientation","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David\n\nOn Tue, Jul 18, 2023 at 01:02:17PM +0100, David Plowman wrote:\n> Hi Jacopo\n>\n> Thanks for the update.\n>\n> On Tue, 18 Jul 2023 at 11:52, Jacopo Mondi\n> wrote:\n> >\n> > Replace the usage of CameraConfiguration::transform with the newly\n> > introduced CameraConfiguration::orientation.\n> >\n> > Rework and rename the CameraSensor::validateTransform(transform) to\n> > CameraSensor::computeTransform(orientation), that given the desired\n> > image orientation computes the Transform that pipeline handlers should\n> > apply to the sensor to obtain it.\n> >\n> > Port all pipeline handlers to use the newly introduced function.\n> >\n> > This commit breaks existing applications as it removes the public\n> > CameraConfiguration::transform in favour of\n> > CameraConfiguration::orientation.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/camera.h | 2 -\n> > include/libcamera/internal/camera_sensor.h | 4 +-\n> > include/libcamera/transform.h | 7 --\n> > src/libcamera/camera.cpp | 16 +---\n> > src/libcamera/camera_sensor.cpp | 92 +++++++++----------\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +-\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +-\n> > .../pipeline/rpi/common/pipeline_base.cpp | 9 +-\n> > src/libcamera/pipeline/simple/simple.cpp | 6 +-\n> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +-\n> > src/libcamera/pipeline/vimc/vimc.cpp | 4 +-\n> > 11 files changed, 66 insertions(+), 92 deletions(-)\n> >\n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index 6e9342773962..98c72ba148ed 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -22,7 +22,6 @@\n> > #include \n> > #include \n> > #include \n> > -#include \n> >\n> > namespace libcamera {\n> >\n> > @@ -67,7 +66,6 @@ public:\n> > bool empty() const;\n> > std::size_t size() const;\n> >\n> > - Transform transform;\n> > Orientation orientation;\n> >\n> > protected:\n> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > index 02b4b4d25e6d..6c5952a8fa1a 100644\n> > --- a/include/libcamera/internal/camera_sensor.h\n> > +++ b/include/libcamera/internal/camera_sensor.h\n> > @@ -32,6 +32,8 @@ class MediaEntity;\n> >\n> > struct CameraSensorProperties;\n> >\n> > +enum class Orientation;\n> > +\n> > class CameraSensor : protected Loggable\n> > {\n> > public:\n> > @@ -71,7 +73,7 @@ public:\n> >\n> > CameraLens *focusLens() { return focusLens_.get(); }\n> >\n> > - Transform validateTransform(Transform *transform) const;\n> > + Transform computeTransform(Orientation *orientation) const;\n> >\n> > protected:\n> > std::string logPrefix() const override;\n> > diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h\n> > index 833b50d46fd0..847a1f94713c 100644\n> > --- a/include/libcamera/transform.h\n> > +++ b/include/libcamera/transform.h\n> > @@ -16,20 +16,13 @@ enum class Orientation;\n> > enum class Transform : int {\n> > Identity = 0,\n> > Rot0 = Identity,\n> > -\n> > HFlip = 1,\n> > -\n> > VFlip = 2,\n> > -\n> > HVFlip = HFlip | VFlip,\n> > Rot180 = HVFlip,\n> > -\n> > Transpose = 4,\n> > -\n> > Rot270 = HFlip | Transpose,\n> > -\n> > Rot90 = VFlip | Transpose,\n> > -\n> > Rot180Transpose = HFlip | VFlip | Transpose\n> > };\n> >\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index d4ad4a553752..a9ea821a78c2 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -160,8 +160,7 @@ LOG_DECLARE_CATEGORY(Camera)\n> > * \\brief Create an empty camera configuration\n> > */\n> > CameraConfiguration::CameraConfiguration()\n> > - : transform(Transform::Identity), orientation(Orientation::rotate0),\n> > - config_({})\n> > + : orientation(Orientation::rotate0), config_({})\n> > {\n> > }\n> >\n> > @@ -392,19 +391,6 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF\n> > return status;\n> > }\n> >\n> > -/**\n> > - * \\var CameraConfiguration::transform\n> > - * \\brief User-specified transform to be applied to the image\n> > - *\n> > - * The transform is a user-specified 2D plane transform that will be applied\n> > - * to the camera images by the processing pipeline before being handed to\n> > - * the application.\n> > - *\n> > - * The usual 2D plane transforms are allowed here (horizontal/vertical\n> > - * flips, multiple of 90-degree rotations etc.), but the validate() function\n> > - * may adjust this field at its discretion if the selection is not supported.\n> > - */\n> > -\n> > /**\n> > * \\var CameraConfiguration::orientation\n> > * \\brief The desired orientation of the images produced by the camera\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index 038d8b959072..3f06ceae5659 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -15,6 +15,7 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -465,7 +466,7 @@ int CameraSensor::initProperties()\n> >\n> > /*\n> > * Cache the Transform associated with the camera mounting\n> > - * rotation for later use in validateTransform().\n> > + * rotation for later use in computeTransform().\n> > */\n> > bool success;\n> > rotationTransform_ = transformFromRotation(propertyValue, &success);\n> > @@ -1024,69 +1025,64 @@ void CameraSensor::updateControlInfo()\n> > */\n> >\n> > /**\n> > - * \\brief Validate a transform request against the sensor capabilities\n> > - * \\param[inout] transform The requested transformation, updated to match\n> > - * the sensor capabilities\n> > + * \\brief Compute the Transform that gives the requested \\a orientation\n> > + * \\param[inout] orientation The desired image orientation\n> > *\n> > - * The input \\a transform is the transform that the caller wants, and it is\n> > - * adjusted according to the capabilities of the sensor to represent the\n> > - * \"nearest\" transform that can actually be delivered.\n> > + * This function computes the Transform that the pipeline handler should apply\n> > + * to the CameraSensor to obtain the requested \\a orientation.\n> > *\n> > - * The returned Transform is the transform applied to the sensor in order to\n> > - * produce the input \\a transform, It is also validated against the sensor's\n> > - * ability to perform horizontal and vertical flips.\n> > + * The intended caller of this function is the validate() implementation of\n> > + * pipeline handlers, that pass in the application requested\n> > + * CameraConfiguration::orientation and obtain a Transform to apply to the\n> > + * camera sensor, likely at configure() time.\n> > *\n> > - * For example, if the requested \\a transform is Transform::Identity and the\n> > - * sensor rotation is 180 degrees, the output transform will be\n> > - * Transform::Rot180 to correct the images so that they appear to have\n> > - * Transform::Identity, but only if the sensor can apply horizontal and vertical\n> > - * flips.\n> > + * If the requested \\a orientation cannot be obtained, the \\a orientation\n> > + * parameter is adjusted to report the current image orientation and\n> > + * Transform::Identity is returned.\n> > *\n> > - * \\return A Transform instance that represents which transformation has been\n> > - * applied to the camera sensor\n> > + * If the requested \\a orientation can be obtained, the function computes a\n> > + * Transform and does not adjust \\a orientation.\n> > + *\n> > + * Pipeline handlers are expected to verify if \\a orientation has been\n> > + * adjusted by this function and set the CameraConfiguration::status to\n> > + * Adjusted accordingly.\n> > + *\n> > + * \\return A Transform instance that applied to the CameraSensor produces images\n> > + * with \\a orientation\n> > */\n> > -Transform CameraSensor::validateTransform(Transform *transform) const\n> > +Transform CameraSensor::computeTransform(Orientation *orientation) const\n> > {\n> > + Orientation mountingOrientation = transformToOrientation(rotationTransform_);\n>\n> I suppose I wonder slightly whether we should simply cache the\n> rotation here as an orientation in the first place, and not a\n> transform, because I think it probably is in reality an orientation.\n> But this is only a teeny semantic quibble, I'm not really fussed!\n\nGood point, let's see if I have to resend so that I can do this and\nfix your other comment on the commit message of 06/10\n\nThanks\n j\n\n>\n> Reviewed-by: David Plowman \n>\n> Thanks\n> David\n>\n> > +\n> > /*\n> > - * Combine the requested transform to compensate the sensor mounting\n> > - * rotation.\n> > + * We cannot do any flips: we cannot change the native camera mounting\n> > + * orientation.\n> > */\n> > - Transform combined = rotationTransform_ * *transform;\n> > + if (!supportFlips_) {\n> > + *orientation = mountingOrientation;\n> > + return Transform::Identity;\n> > + }\n> >\n> > /*\n> > - * We combine the platform and user transform, but must \"adjust away\"\n> > - * any combined result that includes a transposition, as we can't do\n> > - * those. In this case, flipping only the transpose bit is helpful to\n> > - * applications - they either get the transform they requested, or have\n> > - * to do a simple transpose themselves (they don't have to worry about\n> > - * the other possible cases).\n> > + * Now compute the required transform to obtain 'orientation' starting\n> > + * from the mounting rotation.\n> > + *\n> > + * As a note:\n> > + * orientation / mountingOrientation = transform\n> > + * mountingOrientation * transform = orientation\n> > */\n> > - if (!!(combined & Transform::Transpose)) {\n> > - /*\n> > - * Flipping the transpose bit in \"transform\" flips it in the\n> > - * combined result too (as it's the last thing that happens),\n> > - * which is of course clearing it.\n> > - */\n> > - *transform ^= Transform::Transpose;\n> > - combined &= ~Transform::Transpose;\n> > - }\n> > + Transform transform = *orientation / mountingOrientation;\n> >\n> > /*\n> > - * We also check if the sensor doesn't do h/vflips at all, in which\n> > - * case we clear them, and the application will have to do everything.\n> > + * If transform contains any Transpose we cannot do it, so adjust\n> > + * 'orientation' to report the image native orientation and return Identity.\n> > */\n> > - if (!supportFlips_ && !!combined) {\n> > - /*\n> > - * If the sensor can do no transforms, then combined must be\n> > - * changed to the identity. The only user transform that gives\n> > - * rise to this is the inverse of the rotation. (Recall that\n> > - * combined = rotationTransform * transform.)\n> > - */\n> > - *transform = -rotationTransform_;\n> > - combined = Transform::Identity;\n> > + if (!!(transform & Transform::Transpose)) {\n> > + *orientation = mountingOrientation;\n> > + return Transform::Identity;\n> > }\n> >\n> > - return combined;\n> > + return transform;\n> > }\n> >\n> > std::string CameraSensor::logPrefix() const\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index a81c817a1255..fa4bd0bb73e2 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -187,9 +187,9 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > * rotation and store the final combined transform that configure() will\n> > * need to apply to the sensor to save us working it out again.\n> > */\n> > - Transform requestedTransform = transform;\n> > - combinedTransform_ = data_->cio2_.sensor()->validateTransform(&transform);\n> > - if (transform != requestedTransform)\n> > + Orientation requestedOrientation = orientation;\n> > + combinedTransform_ = data_->cio2_.sensor()->computeTransform(&orientation);\n> > + if (orientation != requestedOrientation)\n> > status = Adjusted;\n> >\n> > /* Cap the number of entries to the available streams. */\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 6efa79f29f66..586b46d64630 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -481,9 +481,9 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > status = Adjusted;\n> > }\n> >\n> > - Transform requestedTransform = transform;\n> > - Transform combined = sensor->validateTransform(&transform);\n> > - if (transform != requestedTransform)\n> > + Orientation requestedOrientation = orientation;\n> > + combinedTransform_ = data_->sensor_->computeTransform(&orientation);\n> > + if (orientation != requestedOrientation)\n> > status = Adjusted;\n> >\n> > /*\n> > @@ -595,8 +595,6 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > if (sensorFormat_.size.isNull())\n> > sensorFormat_.size = sensor->resolution();\n> >\n> > - combinedTransform_ = combined;\n> > -\n> > return status;\n> > }\n> >\n> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > index 179a5b81a516..5d541d71defe 100644\n> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > @@ -254,9 +254,9 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > * rotation and store the final combined transform that configure() will\n> > * need to apply to the sensor to save us working it out again.\n> > */\n> > - Transform requestedTransform = transform;\n> > - combinedTransform_ = data_->sensor_->validateTransform(&transform);\n> > - if (transform != requestedTransform)\n> > + Orientation requestedOrientation = orientation;\n> > + combinedTransform_ = data_->sensor_->computeTransform(&orientation);\n> > + if (orientation != requestedOrientation)\n> > status = Adjusted;\n> >\n> > std::vector rawStreams, outStreams;\n> > @@ -1193,7 +1193,8 @@ int CameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::Config\n> > }\n> >\n> > /* Always send the user transform to the IPA. */\n> > - params.transform = static_cast(config->transform);\n> > + params.transform =\n> > + static_cast(transformFromOrientation(config->orientation));\n> >\n> > /* Ready the IPA - it must know about the sensor resolution. */\n> > ret = ipa_->configure(sensorInfo_, params, result);\n> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > index 05ba76bce630..911051b28498 100644\n> > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -889,9 +889,9 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> > if (config_.empty())\n> > return Invalid;\n> >\n> > - Transform requestedTransform = transform;\n> > - combinedTransform_ = sensor->validateTransform(&transform);\n> > - if (transform != requestedTransform)\n> > + Orientation requestedOrientation = orientation;\n> > + combinedTransform_ = sensor->computeTransform(&orientation);\n> > + if (orientation != requestedOrientation)\n> > status = Adjusted;\n> >\n> > /* Cap the number of entries to the available streams. */\n> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > index 38f48a5d9269..4bdb7e5f9b87 100644\n> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > @@ -111,8 +111,8 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()\n> > if (config_.empty())\n> > return Invalid;\n> >\n> > - if (transform != Transform::Identity) {\n> > - transform = Transform::Identity;\n> > + if (orientation != Orientation::rotate0) {\n> > + orientation = Orientation::rotate0;\n> > status = Adjusted;\n> > }\n> >\n> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > index 00e6f4c6a51f..bc705f01284a 100644\n> > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > @@ -128,8 +128,8 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()\n> > if (config_.empty())\n> > return Invalid;\n> >\n> > - if (transform != Transform::Identity) {\n> > - transform = Transform::Identity;\n> > + if (orientation != Orientation::rotate0) {\n> > + orientation = Orientation::rotate0;\n> > status = Adjusted;\n> > }\n> >\n> > --\n> > 2.40.1\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 C7EE6BDB1C\n\tfor ;\n\tTue, 18 Jul 2023 12:22:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8207961E2B;\n\tTue, 18 Jul 2023 14:22:00 +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 0A52561E2B\n\tfor ;\n\tTue, 18 Jul 2023 14:21:59 +0200 (CEST)","from ideasonboard.com (mob-5-90-54-150.net.vodafone.it\n\t[5.90.54.150])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4EDD2838;\n\tTue, 18 Jul 2023 14:21:05 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689682920;\n\tbh=u7iO8nVAMvE259nx8D88yYIfBXH/uxLBpp+q1JtBnZs=;\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=OIRm50NVTFKEksofjdTcGT12vP83W6i5QSd5lE3A9xtRNuzT10e6dFpj8gRTjwtBN\n\tNlf9GGhFdqsMl5WK15B2PijRqsr/16bSpupGppkrjPDFgAcDH3qEtfUMaH/QuxpDS7\n\tj8q5miAfFVekVyR3eZMQb+GqdX7j6dmuAwOKpT+OvrcLBmljGE/vkBEoLDm/S3WdWf\n\t2P74DGSyNqmVq+F9ooj44PFjsTnIsc7vIA54q+5cF/lJZ1fzGQ8lqdHY5qSOCkN3mu\n\tjPo8JjH/GEDOVH9C+aO0dbpvPycZ0zGZJsNsdfXDVaO5Dlp9X6MnqV1ic9bzImEbwb\n\tPyHuT6EG8zubg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1689682865;\n\tbh=u7iO8nVAMvE259nx8D88yYIfBXH/uxLBpp+q1JtBnZs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wge1bW1PFWxTx6zmwftylD4nSZhuNXy0cFXjdECWHwJes97PUtOo2u/NK/tlb3gHd\n\tZvw8sxj+SudLtrmv0oy/KtZlyfXVou9Y1oBMfvcAZxAvYltr5syTczVrCwDEokHTJZ\n\tyJ7OTvZ5he6Rv6aS8NDqeJ95NIc1Mu8ejzAMt6g8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"wge1bW1P\"; dkim-atps=neutral","Date":"Tue, 18 Jul 2023 14:21:54 +0200","To":"David Plowman ","Message-ID":"","References":"<20230718105210.83558-1-jacopo.mondi@ideasonboard.com>\n\t<20230718105210.83558-10-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] [PATCH v3 09/10] libcamera: Move to use\n\tCameraConfiguration::orientation","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\" "}}]