[libcamera-devel] [RFC 3/4] libcamera: Move to use CameraConfiguration::orientation
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Wed Jun 21 09:44:40 CEST 2023
Hi Umang
On Wed, Jun 21, 2023 at 12:20:45PM +0530, Umang Jain wrote:
> Hi Jacopo,
>
> thank you for the patch.
>
> On 6/20/23 7:59 PM, Jacopo Mondi via libcamera-devel wrote:
> > Replace the usage of CameraConfiguration::transform with the newly
> > introduced CameraConfiguration::orientation.
> >
> > Rework 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.
> >
> > Introduce in transform.cpp two functions to convert from Transform to
> > Orientation and vice-versa.
> >
> > Port all pipeline handlers to use the newly introduced function.
> >
> > This commit breaks existing applications as it removes the public
> > CameraConfiguration::transform in faviour of
> > CameraConfiguration::orientation.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > ---
> > include/libcamera/camera.h | 2 -
> > include/libcamera/internal/camera_sensor.h | 3 +-
> > include/libcamera/transform.h | 4 +
> > src/libcamera/camera.cpp | 15 +--
> > src/libcamera/camera_sensor.cpp | 95 +++++++++----------
> > 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 +-
> > src/libcamera/transform.cpp | 58 +++++++++++
> > 12 files changed, 129 insertions(+), 85 deletions(-)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 608774ce7768..bffac6bc8922 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -20,7 +20,6 @@
> > #include <libcamera/controls.h>
> > #include <libcamera/request.h>
> > #include <libcamera/stream.h>
> > -#include <libcamera/transform.h>
> > namespace libcamera {
> > @@ -76,7 +75,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..1d47a2b1a500 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -14,6 +14,7 @@
> > #include <libcamera/base/class.h>
> > #include <libcamera/base/log.h>
> > +#include <libcamera/camera.h>
> > #include <libcamera/control_ids.h>
> > #include <libcamera/controls.h>
> > #include <libcamera/geometry.h>
> > @@ -71,7 +72,7 @@ public:
> > CameraLens *focusLens() { return focusLens_.get(); }
> > - Transform validateTransform(Transform *transform) const;
> > + Transform computeTransform(CameraConfiguration::Orientation *orientation) const;
> > protected:
> > std::string logPrefix() const override;
> > diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h
> > index 2a6838c78369..573adf18715d 100644
> > --- a/include/libcamera/transform.h
> > +++ b/include/libcamera/transform.h
> > @@ -9,6 +9,8 @@
> > #include <string>
> > +#include <libcamera/camera.h>
> > +
> > namespace libcamera {
> > enum class Transform : int {
> > @@ -69,6 +71,8 @@ constexpr Transform operator~(Transform t)
> > }
> > Transform transformFromRotation(int angle, bool *success = nullptr);
> > +Transform transformFromOrientation(const CameraConfiguration::Orientation &orientation);
> > +CameraConfiguration::Orientation transformToOrientation(const Transform &transform);
> > const char *transformToString(Transform t);
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 5ca05719ebfc..bc1409b5c960 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -197,7 +197,7 @@ LOG_DECLARE_CATEGORY(Camera)
> > * \brief Create an empty camera configuration
> > */
> > CameraConfiguration::CameraConfiguration()
> > - : transform(Transform::Identity), orientation(rotate0), config_({})
> > + : orientation(rotate0), config_({})
> > {
> > }
> > @@ -428,19 +428,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 video streams produced by the camera
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index a15a6c89c5c8..5c9f30a62527 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -465,7 +465,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);
> > @@ -477,7 +477,7 @@ int CameraSensor::initProperties()
> > }
> > /*
> > - * Adjust property::Rotation as validateTransform() compensates
> > + * Adjust property::Rotation as computeTransform() compensates
> > * for the mounting rotation. However, as a camera sensor can
> > * only compensate rotations by applying H/VFlips, only rotation
> > * of 180 degrees are automatically compensated. The other valid
> > @@ -1033,69 +1033,66 @@ 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 what the current image orientation is 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(CameraConfiguration::Orientation *orientation) const
> > {
> > /*
> > - * Combine the requested transform to compensate the sensor mounting
> > - * rotation.
> > + * We cannot do any flips, we cannot change the native camera mounting
> > + * orientation.
> > */
> > - Transform combined = *transform * rotationTransform_;
> > + if (!supportFlips_) {
> > + *orientation = transformToOrientation(rotationTransform_);
> > + 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).
> > + * If the camera is mounted 90 or 270 degrees rotated, there is no
> > + * way we can correct it and there's no point in continuing as the
> > + * user request cannot be satisfied in full.
> > */
> > - 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;
> > + if (!!(rotationTransform_ & Transform::Transpose)) {
> > + *orientation = transformToOrientation(rotationTransform_);
> > + return Transform::Identity;
> > }
> > /*
> > - * 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 the user request contains a transform there's no way we can
>
> you mean "request contains a transpose..." not a transform right ?
Ups, right!
> > + * satisfy it, default it to Identity. We cannot return early as the
> > + * camera mounting rotation has to be corrected, and if we get here we
> > + * know we can do that (we adjusted property::Rotation already because
> > + * of this).
> > */
> > - 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 = transform * rotationTransform.)
> > - */
> > - *transform = -rotationTransform_;
> > - combined = Transform::Identity;
> > - }
> > + Transform request = transformFromOrientation(*orientation);
> > + if (!!(request & Transform::Transpose))
> > + request = Transform::Identity;
> > +
> > + *orientation = transformToOrientation(request);
> > - return combined;
> > + return request * rotationTransform_;
>
> Nice. If rotationTransform_ is 180, this will return Transform 180 - which
> will be applied to the sensor, so we get an upright image. The property is
> also adjusted already in that case, and orientation too will report rotate0.
>
> And if rotationTransform_ is identity (sensor is already upright), this will
> return Transform identity, so no transform to be applied.
>
> If request is Rot180 and rotationTransform is identity, It will return
> Rot180 which will be applied to sensor. But the property will still be
> reporting 0, which makes sense.
>
> Looks good to me overall... and API is much easier to grasp now.
Thanks!
> > }
> > std::string CameraSensor::logPrefix() const
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 355cb0cb76b8..ded41e011be2 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 91a3c60757e1..81ae84b13a62 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 df7482920e75..9d6d816f637a 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > @@ -254,9 +254,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;
> > @@ -1193,7 +1193,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 050285fd389e..cae67c90bd20 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -888,9 +888,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 277465b72164..e1f215f06db2 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 != CameraConfiguration::rotate0) {
> > + orientation = CameraConfiguration::rotate0;
> > status = Adjusted;
> > }
> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > index 204f5ad73f6d..33c165d0cee2 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 != CameraConfiguration::rotate0) {
> > + orientation = CameraConfiguration::rotate0;
> > status = Adjusted;
> > }
> > diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp
> > index 4668303d0676..930155b60ff4 100644
> > --- a/src/libcamera/transform.cpp
> > +++ b/src/libcamera/transform.cpp
> > @@ -299,6 +299,64 @@ Transform transformFromRotation(int angle, bool *success)
> > return Transform::Identity;
> > }
> > +/**
> > + * \brief Return the transform representing \a orientation
> > + * \param[in] orientation The orientation to convert
> > + * \return The transform corresponding to \a orientation
> > + */
> > +Transform transformFromOrientation(const CameraConfiguration::Orientation &orientation)
> > +{
> > + switch (orientation) {
> > + case CameraConfiguration::rotate0:
> > + return Transform::Identity;
> > + case CameraConfiguration::flipRotate0:
> > + return Transform::HFlip;
> > + case CameraConfiguration::rotate180:
> > + return Transform::Rot180;
> > + case CameraConfiguration::flipRotate180:
> > + return Transform::VFlip;
> > + case CameraConfiguration::flipRotate270:
> > + return Transform::Transpose;
> > + case CameraConfiguration::rotate90:
> > + return Transform::Rot90;
> > + case CameraConfiguration::flipRotate90:
> > + return Transform::Rot180Transpose;
> > + case CameraConfiguration::rotate270:
> > + return Transform::Rot270;
> > + }
> > +
> > + return Transform::Identity;
> > +}
> > +
> > +/**
> > + * \brief Return the CameraConfiguration::Orientation representing \a transform
> > + * \param[in] transform The transform to convert
> > + * \return The Orientation corresponding to \a transform
> > + */
> > +CameraConfiguration::Orientation transformToOrientation(const Transform &transform)
> > +{
> > + switch (transform) {
> > + case Transform::Identity:
> > + return CameraConfiguration::rotate0;
> > + case Transform::HFlip:
> > + return CameraConfiguration::flipRotate0;
> > + case Transform::VFlip:
> > + return CameraConfiguration::flipRotate180;
> > + case Transform::Rot180:
> > + return CameraConfiguration::rotate180;
> > + case Transform::Transpose:
> > + return CameraConfiguration::flipRotate270;
> > + case Transform::Rot270:
> > + return CameraConfiguration::rotate270;
> > + case Transform::Rot90:
> > + return CameraConfiguration::rotate90;
> > + case Transform::Rot180Transpose:
> > + return CameraConfiguration::flipRotate90;
> > + }
> > +
> > + return CameraConfiguration::rotate0;
> > +}
> > +
> > /**
> > * \brief Return a character string describing the transform
> > * \param[in] t The transform to be described.
>
More information about the libcamera-devel
mailing list