| Message ID | 20230620142904.72600-4-jacopo.mondi@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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 ? > + * 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. > } > > 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.
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@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. >
Hi Jacobo, On 20.06.23 16:29, Jacopo Mondi 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. I'm pondering how make this breaking change less painful for apps - especially Pipewire. Some distros like Fedora have a frozen libcamera version but update PW, so it would be nice if there was a macro to check for the new type at build time. Another issue is the question when to remove transform.h from the public headers (missing in this version) and the CameraConfiguration. Doing so right now would require users to update straight away - acceptable, given that libcamera does not have a stable API, but keeping it around for a release or two with a build-time deprecation warning maybe wouldn't hurt either? > > Signed-off-by: Jacopo Mondi <jacopo.mondi@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 > + * 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_; > } > > 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.
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 + * 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_; } 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.
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@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(-)