[libcamera-devel] [PATCH v5 10/12] libcamera: Use CameraConfiguration::orientation
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Oct 18 23:22:54 CEST 2023
Hi Jacopo,
Thank you for the patch.
On Fri, Sep 01, 2023 at 05:02:13PM +0200, Jacopo Mondi via libcamera-devel wrote:
> Replace the usage of CameraConfiguration::transform with the newly
> introduced CameraConfiguration::orientation.
>
> Rework and rename the CameraSensor::validateTransform(transform) to
> CameraSensor::computeTransform(orientation), that given the desired
> image orientation computes the Transform that pipeline handlers should
> apply to the sensor to obtain it.
>
> Port all pipeline handlers to use the newly introduced function.
>
> This commit breaks existing applications as it removes the public
> CameraConfiguration::transform in favour of
> CameraConfiguration::orientation.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> ---
> include/libcamera/camera.h | 2 -
> include/libcamera/internal/camera_sensor.h | 4 +-
> src/libcamera/camera.cpp | 16 +---
> src/libcamera/camera_sensor.cpp | 92 +++++++++----------
> src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +-
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +-
> .../pipeline/rpi/common/pipeline_base.cpp | 9 +-
> src/libcamera/pipeline/simple/simple.cpp | 6 +-
> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +-
> src/libcamera/pipeline/vimc/vimc.cpp | 4 +-
> 10 files changed, 66 insertions(+), 85 deletions(-)
>
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 6e9342773962..98c72ba148ed 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -22,7 +22,6 @@
> #include <libcamera/orientation.h>
> #include <libcamera/request.h>
> #include <libcamera/stream.h>
> -#include <libcamera/transform.h>
>
> namespace libcamera {
>
> @@ -67,7 +66,6 @@ public:
> bool empty() const;
> std::size_t size() const;
>
> - Transform transform;
> Orientation orientation;
>
> protected:
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 02b4b4d25e6d..6c5952a8fa1a 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -32,6 +32,8 @@ class MediaEntity;
>
> struct CameraSensorProperties;
>
> +enum class Orientation;
> +
> class CameraSensor : protected Loggable
> {
> public:
> @@ -71,7 +73,7 @@ public:
>
> CameraLens *focusLens() { return focusLens_.get(); }
>
> - Transform validateTransform(Transform *transform) const;
> + Transform computeTransform(Orientation *orientation) const;
>
> protected:
> std::string logPrefix() const override;
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index d4ad4a553752..a9ea821a78c2 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -160,8 +160,7 @@ LOG_DECLARE_CATEGORY(Camera)
> * \brief Create an empty camera configuration
> */
> CameraConfiguration::CameraConfiguration()
> - : transform(Transform::Identity), orientation(Orientation::rotate0),
> - config_({})
> + : orientation(Orientation::rotate0), config_({})
> {
> }
>
> @@ -392,19 +391,6 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
> return status;
> }
>
> -/**
> - * \var CameraConfiguration::transform
> - * \brief User-specified transform to be applied to the image
> - *
> - * The transform is a user-specified 2D plane transform that will be applied
> - * to the camera images by the processing pipeline before being handed to
> - * the application.
> - *
> - * The usual 2D plane transforms are allowed here (horizontal/vertical
> - * flips, multiple of 90-degree rotations etc.), but the validate() function
> - * may adjust this field at its discretion if the selection is not supported.
> - */
> -
> /**
> * \var CameraConfiguration::orientation
> * \brief The desired orientation of the images produced by the camera
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 038d8b959072..3f06ceae5659 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -15,6 +15,7 @@
> #include <math.h>
> #include <string.h>
>
> +#include <libcamera/orientation.h>
> #include <libcamera/property_ids.h>
>
> #include <libcamera/base/utils.h>
> @@ -465,7 +466,7 @@ int CameraSensor::initProperties()
>
> /*
> * Cache the Transform associated with the camera mounting
> - * rotation for later use in validateTransform().
> + * rotation for later use in computeTransform().
> */
> bool success;
> rotationTransform_ = transformFromRotation(propertyValue, &success);
> @@ -1024,69 +1025,64 @@ void CameraSensor::updateControlInfo()
> */
>
> /**
> - * \brief Validate a transform request against the sensor capabilities
> - * \param[inout] transform The requested transformation, updated to match
> - * the sensor capabilities
> + * \brief Compute the Transform that gives the requested \a orientation
> + * \param[inout] orientation The desired image orientation
> *
> - * The input \a transform is the transform that the caller wants, and it is
> - * adjusted according to the capabilities of the sensor to represent the
> - * "nearest" transform that can actually be delivered.
> + * This function computes the Transform that the pipeline handler should apply
> + * to the CameraSensor to obtain the requested \a orientation.
> *
> - * The returned Transform is the transform applied to the sensor in order to
> - * produce the input \a transform, It is also validated against the sensor's
> - * ability to perform horizontal and vertical flips.
> + * The intended caller of this function is the validate() implementation of
> + * pipeline handlers, that pass in the application requested
> + * CameraConfiguration::orientation and obtain a Transform to apply to the
> + * camera sensor, likely at configure() time.
> *
> - * For example, if the requested \a transform is Transform::Identity and the
> - * sensor rotation is 180 degrees, the output transform will be
> - * Transform::Rot180 to correct the images so that they appear to have
> - * Transform::Identity, but only if the sensor can apply horizontal and vertical
> - * flips.
> + * If the requested \a orientation cannot be obtained, the \a orientation
> + * parameter is adjusted to report the current image orientation and
> + * Transform::Identity is returned.
> *
> - * \return A Transform instance that represents which transformation has been
> - * applied to the camera sensor
> + * If the requested \a orientation can be obtained, the function computes a
> + * Transform and does not adjust \a orientation.
> + *
> + * Pipeline handlers are expected to verify if \a orientation has been
> + * adjusted by this function and set the CameraConfiguration::status to
> + * Adjusted accordingly.
> + *
> + * \return A Transform instance that applied to the CameraSensor produces images
> + * with \a orientation
> */
> -Transform CameraSensor::validateTransform(Transform *transform) const
> +Transform CameraSensor::computeTransform(Orientation *orientation) const
> {
> + Orientation mountingOrientation = transformToOrientation(rotationTransform_);
Let's replace rotationTransform_ with orientation_, as the value is used
here only. You will then be able to make the transformToOrientation
function static in transform.cpp, or (my preference I think) fold it in
its single caller in that file.
This can be done with a patch on top.
> +
> /*
> - * Combine the requested transform to compensate the sensor mounting
> - * rotation.
> + * We cannot do any flips: we cannot change the native camera mounting
> + * orientation.
* If we cannot do any flips, we cannot change the native camera
* mounting orientation.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> */
> - Transform combined = rotationTransform_ * *transform;
> + if (!supportFlips_) {
> + *orientation = mountingOrientation;
> + return Transform::Identity;
> + }
>
> /*
> - * We combine the platform and user transform, but must "adjust away"
> - * any combined result that includes a transposition, as we can't do
> - * those. In this case, flipping only the transpose bit is helpful to
> - * applications - they either get the transform they requested, or have
> - * to do a simple transpose themselves (they don't have to worry about
> - * the other possible cases).
> + * Now compute the required transform to obtain 'orientation' starting
> + * from the mounting rotation.
> + *
> + * As a note:
> + * orientation / mountingOrientation = transform
> + * mountingOrientation * transform = orientation
> */
> - if (!!(combined & Transform::Transpose)) {
> - /*
> - * Flipping the transpose bit in "transform" flips it in the
> - * combined result too (as it's the last thing that happens),
> - * which is of course clearing it.
> - */
> - *transform ^= Transform::Transpose;
> - combined &= ~Transform::Transpose;
> - }
> + Transform transform = *orientation / mountingOrientation;
>
> /*
> - * We also check if the sensor doesn't do h/vflips at all, in which
> - * case we clear them, and the application will have to do everything.
> + * If transform contains any Transpose we cannot do it, so adjust
> + * 'orientation' to report the image native orientation and return Identity.
> */
> - if (!supportFlips_ && !!combined) {
> - /*
> - * If the sensor can do no transforms, then combined must be
> - * changed to the identity. The only user transform that gives
> - * rise to this is the inverse of the rotation. (Recall that
> - * combined = rotationTransform * transform.)
> - */
> - *transform = -rotationTransform_;
> - combined = Transform::Identity;
> + if (!!(transform & Transform::Transpose)) {
> + *orientation = mountingOrientation;
> + return Transform::Identity;
> }
>
> - return combined;
> + return transform;
> }
>
> std::string CameraSensor::logPrefix() const
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index a81c817a1255..fa4bd0bb73e2 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -187,9 +187,9 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> * rotation and store the final combined transform that configure() will
> * need to apply to the sensor to save us working it out again.
> */
> - Transform requestedTransform = transform;
> - combinedTransform_ = data_->cio2_.sensor()->validateTransform(&transform);
> - if (transform != requestedTransform)
> + Orientation requestedOrientation = orientation;
> + combinedTransform_ = data_->cio2_.sensor()->computeTransform(&orientation);
> + if (orientation != requestedOrientation)
> status = Adjusted;
>
> /* Cap the number of entries to the available streams. */
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 6efa79f29f66..586b46d64630 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -481,9 +481,9 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> status = Adjusted;
> }
>
> - Transform requestedTransform = transform;
> - Transform combined = sensor->validateTransform(&transform);
> - if (transform != requestedTransform)
> + Orientation requestedOrientation = orientation;
> + combinedTransform_ = data_->sensor_->computeTransform(&orientation);
> + if (orientation != requestedOrientation)
> status = Adjusted;
>
> /*
> @@ -595,8 +595,6 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> if (sensorFormat_.size.isNull())
> sensorFormat_.size = sensor->resolution();
>
> - combinedTransform_ = combined;
> -
> return status;
> }
>
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index 51fa1bbf9aa9..7827c5a3a990 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -187,9 +187,9 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> * rotation and store the final combined transform that configure() will
> * need to apply to the sensor to save us working it out again.
> */
> - Transform requestedTransform = transform;
> - combinedTransform_ = data_->sensor_->validateTransform(&transform);
> - if (transform != requestedTransform)
> + Orientation requestedOrientation = orientation;
> + combinedTransform_ = data_->sensor_->computeTransform(&orientation);
> + if (orientation != requestedOrientation)
> status = Adjusted;
>
> std::vector<CameraData::StreamParams> rawStreams, outStreams;
> @@ -1184,7 +1184,8 @@ int CameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::Config
> }
>
> /* Always send the user transform to the IPA. */
> - params.transform = static_cast<unsigned int>(config->transform);
> + params.transform =
> + static_cast<unsigned int>(transformFromOrientation(config->orientation));
>
> /* Ready the IPA - it must know about the sensor resolution. */
> ret = ipa_->configure(sensorInfo_, params, result);
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 05ba76bce630..911051b28498 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -889,9 +889,9 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
> if (config_.empty())
> return Invalid;
>
> - Transform requestedTransform = transform;
> - combinedTransform_ = sensor->validateTransform(&transform);
> - if (transform != requestedTransform)
> + Orientation requestedOrientation = orientation;
> + combinedTransform_ = sensor->computeTransform(&orientation);
> + if (orientation != requestedOrientation)
> status = Adjusted;
>
> /* Cap the number of entries to the available streams. */
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 38f48a5d9269..4bdb7e5f9b87 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -111,8 +111,8 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()
> if (config_.empty())
> return Invalid;
>
> - if (transform != Transform::Identity) {
> - transform = Transform::Identity;
> + if (orientation != Orientation::rotate0) {
> + orientation = Orientation::rotate0;
> status = Adjusted;
> }
>
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index 00e6f4c6a51f..bc705f01284a 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -128,8 +128,8 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()
> if (config_.empty())
> return Invalid;
>
> - if (transform != Transform::Identity) {
> - transform = Transform::Identity;
> + if (orientation != Orientation::rotate0) {
> + orientation = Orientation::rotate0;
> status = Adjusted;
> }
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list