[{"id":27986,"web_url":"https://patchwork.libcamera.org/comment/27986/","msgid":"<20231018212254.GL1512@pendragon.ideasonboard.com>","date":"2023-10-18T21:22:54","subject":"Re: [libcamera-devel] [PATCH v5 10/12] libcamera: Use\n\tCameraConfiguration::orientation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Fri, Sep 01, 2023 at 05:02:13PM +0200, Jacopo Mondi via libcamera-devel wrote:\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 <jacopo.mondi@ideasonboard.com>\n> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>\n> ---\n>  include/libcamera/camera.h                    |  2 -\n>  include/libcamera/internal/camera_sensor.h    |  4 +-\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>  10 files changed, 66 insertions(+), 85 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 <libcamera/orientation.h>\n>  #include <libcamera/request.h>\n>  #include <libcamera/stream.h>\n> -#include <libcamera/transform.h>\n>  \n>  namespace libcamera {\n>  \n> @@ -67,7 +66,6 @@ public:\n>  \tbool empty() const;\n>  \tstd::size_t size() const;\n>  \n> -\tTransform transform;\n>  \tOrientation 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>  \tCameraLens *focusLens() { return focusLens_.get(); }\n>  \n> -\tTransform validateTransform(Transform *transform) const;\n> +\tTransform computeTransform(Orientation *orientation) const;\n>  \n>  protected:\n>  \tstd::string logPrefix() const override;\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> -\t: transform(Transform::Identity), orientation(Orientation::rotate0),\n> -\t  config_({})\n> +\t: orientation(Orientation::rotate0), config_({})\n>  {\n>  }\n>  \n> @@ -392,19 +391,6 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF\n>  \treturn 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 <math.h>\n>  #include <string.h>\n>  \n> +#include <libcamera/orientation.h>\n>  #include <libcamera/property_ids.h>\n>  \n>  #include <libcamera/base/utils.h>\n> @@ -465,7 +466,7 @@ int CameraSensor::initProperties()\n>  \n>  \t\t/*\n>  \t\t * Cache the Transform associated with the camera mounting\n> -\t\t * rotation for later use in validateTransform().\n> +\t\t * rotation for later use in computeTransform().\n>  \t\t */\n>  \t\tbool success;\n>  \t\trotationTransform_ = 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> +\tOrientation mountingOrientation = transformToOrientation(rotationTransform_);\n\nLet's replace rotationTransform_ with orientation_, as the value is used\nhere only. You will then be able to make the transformToOrientation\nfunction static in transform.cpp, or (my preference I think) fold it in\nits single caller in that file.\n\nThis can be done with a patch on top.\n\n> +\n>  \t/*\n> -\t * Combine the requested transform to compensate the sensor mounting\n> -\t * rotation.\n> +\t * We cannot do any flips: we cannot change the native camera mounting\n> +\t * orientation.\n\n\t * If we cannot do any flips, we cannot change the native camera\n\t * mounting orientation.\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n>  \t */\n> -\tTransform combined = rotationTransform_ * *transform;\n> +\tif (!supportFlips_) {\n> +\t\t*orientation = mountingOrientation;\n> +\t\treturn Transform::Identity;\n> +\t}\n>  \n>  \t/*\n> -\t * We combine the platform and user transform, but must \"adjust away\"\n> -\t * any combined result that includes a transposition, as we can't do\n> -\t * those. 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 * Now compute the required transform to obtain 'orientation' starting\n> +\t * from the mounting rotation.\n> +\t *\n> +\t * As a note:\n> +\t * \torientation / mountingOrientation = transform\n> +\t * \tmountingOrientation * transform = orientation\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\t*transform ^= Transform::Transpose;\n> -\t\tcombined &= ~Transform::Transpose;\n> -\t}\n> +\tTransform transform = *orientation / mountingOrientation;\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 * If transform contains any Transpose we cannot do it, so adjust\n> +\t * 'orientation' to report the image native orientation and return Identity.\n>  \t */\n> -\tif (!supportFlips_ && !!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 is the inverse of the rotation. (Recall that\n> -\t\t * combined = rotationTransform * transform.)\n> -\t\t */\n> -\t\t*transform = -rotationTransform_;\n> -\t\tcombined = Transform::Identity;\n> +\tif (!!(transform & Transform::Transpose)) {\n> +\t\t*orientation = mountingOrientation;\n> +\t\treturn Transform::Identity;\n>  \t}\n>  \n> -\treturn combined;\n> +\treturn 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>  \t * rotation and store the final combined transform that configure() will\n>  \t * need to apply to the sensor to save us working it out again.\n>  \t */\n> -\tTransform requestedTransform = transform;\n> -\tcombinedTransform_ = data_->cio2_.sensor()->validateTransform(&transform);\n> -\tif (transform != requestedTransform)\n> +\tOrientation requestedOrientation = orientation;\n> +\tcombinedTransform_ = data_->cio2_.sensor()->computeTransform(&orientation);\n> +\tif (orientation != requestedOrientation)\n>  \t\tstatus = Adjusted;\n>  \n>  \t/* 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>  \t\tstatus = Adjusted;\n>  \t}\n>  \n> -\tTransform requestedTransform = transform;\n> -\tTransform combined = sensor->validateTransform(&transform);\n> -\tif (transform != requestedTransform)\n> +\tOrientation requestedOrientation = orientation;\n> +\tcombinedTransform_ = data_->sensor_->computeTransform(&orientation);\n> +\tif (orientation != requestedOrientation)\n>  \t\tstatus = Adjusted;\n>  \n>  \t/*\n> @@ -595,8 +595,6 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>  \tif (sensorFormat_.size.isNull())\n>  \t\tsensorFormat_.size = sensor->resolution();\n>  \n> -\tcombinedTransform_ = combined;\n> -\n>  \treturn 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 51fa1bbf9aa9..7827c5a3a990 100644\n> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> @@ -187,9 +187,9 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n>  \t * rotation and store the final combined transform that configure() will\n>  \t * need to apply to the sensor to save us working it out again.\n>  \t */\n> -\tTransform requestedTransform = transform;\n> -\tcombinedTransform_ = data_->sensor_->validateTransform(&transform);\n> -\tif (transform != requestedTransform)\n> +\tOrientation requestedOrientation = orientation;\n> +\tcombinedTransform_ = data_->sensor_->computeTransform(&orientation);\n> +\tif (orientation != requestedOrientation)\n>  \t\tstatus = Adjusted;\n>  \n>  \tstd::vector<CameraData::StreamParams> rawStreams, outStreams;\n> @@ -1184,7 +1184,8 @@ int CameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::Config\n>  \t}\n>  \n>  \t/* Always send the user transform to the IPA. */\n> -\tparams.transform = static_cast<unsigned int>(config->transform);\n> +\tparams.transform =\n> +\t\tstatic_cast<unsigned int>(transformFromOrientation(config->orientation));\n>  \n>  \t/* Ready the IPA - it must know about the sensor resolution. */\n>  \tret = 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>  \tif (config_.empty())\n>  \t\treturn Invalid;\n>  \n> -\tTransform requestedTransform = transform;\n> -\tcombinedTransform_ = sensor->validateTransform(&transform);\n> -\tif (transform != requestedTransform)\n> +\tOrientation requestedOrientation = orientation;\n> +\tcombinedTransform_ = sensor->computeTransform(&orientation);\n> +\tif (orientation != requestedOrientation)\n>  \t\tstatus = Adjusted;\n>  \n>  \t/* 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>  \tif (config_.empty())\n>  \t\treturn Invalid;\n>  \n> -\tif (transform != Transform::Identity) {\n> -\t\ttransform = Transform::Identity;\n> +\tif (orientation != Orientation::rotate0) {\n> +\t\torientation = Orientation::rotate0;\n>  \t\tstatus = Adjusted;\n>  \t}\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>  \tif (config_.empty())\n>  \t\treturn Invalid;\n>  \n> -\tif (transform != Transform::Identity) {\n> -\t\ttransform = Transform::Identity;\n> +\tif (orientation != Orientation::rotate0) {\n> +\t\torientation = Orientation::rotate0;\n>  \t\tstatus = Adjusted;\n>  \t}\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 0D012BDCBD\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 18 Oct 2023 21:22:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4B9A36297F;\n\tWed, 18 Oct 2023 23:22: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 C819D61DD1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 18 Oct 2023 23:22:47 +0200 (CEST)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 01E0A666;\n\tWed, 18 Oct 2023 23:22:39 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1697664169;\n\tbh=+k9Ik8S3tmSpzddOyVK+wbdwwdVlCj4vn8ZG5WA6S6c=;\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=0IV3twiECtZf3mCPbaF0AO0abNcnTpzjFGo9A0c8ZHIAJXWDPQnvJ6uWLlpRRg4//\n\t8Q9SgTWXYw30JyI+OT1zfTkqxDnDH9pi/V2Iaun02JxxSecNSTpTjjEQdYxnUTVu2X\n\tNgIn84ExY1Tth3jep/h0I8SSJebZZHCiX69QgO87zwdQb14FvrQw0E2iN7xyIgDD1Q\n\tqSeBUzuwTP8IZb9xMkbeWmbyojUXnRqEGfs6OXLi5wodSOwepcUEGpZninD02hiow9\n\tK/ElXjaNVCCtziRsF+SsF4JKQOS1bylAoLeE9pPO4q/nlU+iWzyUFo+m12Z8sYmPZ2\n\tZG9ISZez+DUxw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1697664160;\n\tbh=+k9Ik8S3tmSpzddOyVK+wbdwwdVlCj4vn8ZG5WA6S6c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HZoBSxjZsvtN9S355rXsTd+aBo5e+VrOELtR+b8xz6vhjKJJberTgZ2i+DOwAr16W\n\t+dZxPmjBuyylEs8+DzNIxRUkBhW7Gf5pv3/Iz4HIvAJuiNWR32OUGUgkA0t2O3uJCH\n\toDlaSjDfmhkdupslM+CzqLaPLfekt0CV8E20rRno="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"HZoBSxjZ\"; dkim-atps=neutral","Date":"Thu, 19 Oct 2023 00:22:54 +0300","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Message-ID":"<20231018212254.GL1512@pendragon.ideasonboard.com>","References":"<20230901150215.11585-1-jacopo.mondi@ideasonboard.com>\n\t<20230901150215.11585-11-jacopo.mondi@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230901150215.11585-11-jacopo.mondi@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 10/12] libcamera: Use\n\tCameraConfiguration::orientation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]