| Message ID | 20230714141549.11085-9-jacopo.mondi@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo Thanks for the patch. On Fri, 14 Jul 2023 at 15:16, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Replace the usage of CameraConfiguration::transform with the newly > introduced CameraConfiguration::orientation. > > Rework and rename the CameraSensor::validateTransform(transform) to > CameraSensor::computeTransform(orientation), that given the desired > image orientation computes the Transform that pipeline handlers should > apply to the sensor to obtain it. > > Port all pipeline handlers to use the newly introduced function. > > This commit breaks existing applications as it removes the public > CameraConfiguration::transform in favour of > CameraConfiguration::orientation. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > include/libcamera/camera.h | 1 - > include/libcamera/internal/camera_sensor.h | 3 +- > src/libcamera/camera.cpp | 15 +-- > src/libcamera/camera_sensor.cpp | 92 +++++++++---------- > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +- > .../pipeline/rpi/common/pipeline_base.cpp | 9 +- > src/libcamera/pipeline/simple/simple.cpp | 6 +- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +- > src/libcamera/pipeline/vimc/vimc.cpp | 4 +- > 10 files changed, 65 insertions(+), 83 deletions(-) > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > index 55359d53f2ab..d937ffc24f54 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h > @@ -80,7 +80,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/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index af842e70dcb0..3d52d1a0019a 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -224,7 +224,7 @@ LOG_DECLARE_CATEGORY(Camera) > * \brief Create an empty camera configuration > */ > CameraConfiguration::CameraConfiguration() > - : transform(Transform::Identity), orientation(rotate0), config_({}) > + : orientation(rotate0), config_({}) > { > } > > @@ -476,19 +476,6 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF > return status; > } > > -/** > - * \var CameraConfiguration::transform > - * \brief User-specified transform to be applied to the image > - * > - * The transform is a user-specified 2D plane transform that will be applied > - * to the camera images by the processing pipeline before being handed to > - * the application. > - * > - * The usual 2D plane transforms are allowed here (horizontal/vertical > - * flips, multiple of 90-degree rotations etc.), but the validate() function > - * may adjust this field at its discretion if the selection is not supported. > - */ > - > /** > * \var CameraConfiguration::orientation > * \brief The desired orientation of the images produced by the camera > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index 3ba364c44a40..ac81ea3889f7 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); > @@ -1024,69 +1024,65 @@ void CameraSensor::updateControlInfo() > */ > > /** > - * \brief Validate a transform request against the sensor capabilities > - * \param[inout] transform The requested transformation, updated to match > - * the sensor capabilities > + * \brief Compute the Transform that gives the requested \a orientation > + * \param[inout] orientation The desired image orientation > * > - * The input \a transform is the transform that the caller wants, and it is > - * adjusted according to the capabilities of the sensor to represent the > - * "nearest" transform that can actually be delivered. > + * This function computes the Transform that the pipeline handler should apply > + * to the CameraSensor to obtain the requested \a orientation. > * > - * The returned Transform is the transform applied to the sensor in order to > - * produce the input \a transform, It is also validated against the sensor's > - * ability to perform horizontal and vertical flips. > + * The intended caller of this function is the validate() implementation of > + * pipeline handlers, that pass in the application requested > + * CameraConfiguration::orientation and obtain a Transform to apply to the > + * camera sensor, likely at configure() time. > * > - * For example, if the requested \a transform is Transform::Identity and the > - * sensor rotation is 180 degrees, the output transform will be > - * Transform::Rot180 to correct the images so that they appear to have > - * Transform::Identity, but only if the sensor can apply horizontal and vertical > - * flips. > + * If the requested \a orientation cannot be obtained, the \a orientation > + * parameter is adjusted to report the current image orientation and > + * Transform::Identity is returned. > * > - * \return A Transform instance that represents which transformation has been > - * applied to the camera sensor > + * If the requested \a orientation can be obtained, the function computes a > + * Transform and does not adjust \a orientation. > + * > + * Pipeline handlers are expected to verify if \a orientation has been > + * adjusted by this function and set the CameraConfiguration::status to > + * Adjusted accordingly. > + * > + * \return A Transform instance that applied to the CameraSensor produces images > + * with \a orientation > */ > -Transform CameraSensor::validateTransform(Transform *transform) const > +Transform CameraSensor::computeTransform(CameraConfiguration::Orientation *orientation) const > { > + CameraConfiguration::Orientation mountingOrientation = > + transformToOrientation(rotationTransform_); > + > /* > - * 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 = mountingOrientation; > + return Transform::Identity; > + } > > /* > - * We combine the platform and user transform, but must "adjust away" > - * any combined result that includes a transposition, as we can't do > - * those. In this case, flipping only the transpose bit is helpful to > - * applications - they either get the transform they requested, or have > - * to do a simple transpose themselves (they don't have to worry about > - * the other possible cases). > + * Now compute the required transform to obtain 'orientation' starting > + * from the mounting rotation. > + * > + * As a note: > + * orientation / mountingOrientation = transform > + * mountingOrientation * transform = orientation > */ > - if (!!(combined & Transform::Transpose)) { > - /* > - * Flipping the transpose bit in "transform" flips it in the > - * combined result too (as it's the last thing that happens), > - * which is of course clearing it. > - */ > - *transform ^= Transform::Transpose; > - combined &= ~Transform::Transpose; > - } > + Transform transform = *orientation / mountingOrientation; > > /* > - * We also check if the sensor doesn't do h/vflips at all, in which > - * case we clear them, and the application will have to do everything. > + * If transform contains any Transpose we cannot do it, so adjust > + * 'orientation' to report the image native orientation and return Identity. > */ > - if (!supportFlips_ && !!combined) { > - /* > - * If the sensor can do no transforms, then combined must be > - * changed to the identity. The only user transform that gives > - * rise to this is the inverse of the rotation. (Recall that > - * combined = transform * rotationTransform.) > - */ > - *transform = -rotationTransform_; > - combined = Transform::Identity; > + if (!!(transform & Transform::Transpose)) { > + *orientation = mountingOrientation; > + return Transform::Identity; I guess I wonder if in future we'll encounter ISPs that can do transpose, but I'm happy to ignore the question for now! > } > > - return combined; > + return transform; > } > > std::string CameraSensor::logPrefix() const > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index a81c817a1255..fa4bd0bb73e2 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -187,9 +187,9 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > * rotation and store the final combined transform that configure() will > * need to apply to the sensor to save us working it out again. > */ > - Transform requestedTransform = transform; > - combinedTransform_ = data_->cio2_.sensor()->validateTransform(&transform); > - if (transform != requestedTransform) > + Orientation requestedOrientation = orientation; > + combinedTransform_ = data_->cio2_.sensor()->computeTransform(&orientation); > + if (orientation != requestedOrientation) > status = Adjusted; > > /* Cap the number of entries to the available streams. */ > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 6efa79f29f66..586b46d64630 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -481,9 +481,9 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > status = Adjusted; > } > > - Transform requestedTransform = transform; > - Transform combined = sensor->validateTransform(&transform); > - if (transform != requestedTransform) > + Orientation requestedOrientation = orientation; > + combinedTransform_ = data_->sensor_->computeTransform(&orientation); > + if (orientation != requestedOrientation) > status = Adjusted; > > /* > @@ -595,8 +595,6 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > if (sensorFormat_.size.isNull()) > sensorFormat_.size = sensor->resolution(); > > - combinedTransform_ = combined; > - > return status; > } > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > index 179a5b81a516..5d541d71defe 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; I wonder slightly about restoring the behaviour the Pi PH had previously, but I'm happy to ignore it for now and will add a further commit at a later date if it is needed. But overall I think this is a good improvement and shows that orientations and transforms make sense together (putting aside my wobbles over transform ordering, though I think those do need addressing). Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David > > 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 05ba76bce630..911051b28498 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -889,9 +889,9 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > if (config_.empty()) > return Invalid; > > - Transform requestedTransform = transform; > - combinedTransform_ = sensor->validateTransform(&transform); > - if (transform != requestedTransform) > + Orientation requestedOrientation = orientation; > + combinedTransform_ = sensor->computeTransform(&orientation); > + if (orientation != requestedOrientation) > status = Adjusted; > > /* Cap the number of entries to the available streams. */ > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > index 02a6117c7955..6f9cf0dca6f5 100644 > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > @@ -112,8 +112,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 00e6f4c6a51f..26401ab92d15 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; > } > > -- > 2.40.1 >
Hi David On Mon, Jul 17, 2023 at 12:43:43PM +0100, David Plowman via libcamera-devel wrote: > Hi Jacopo > > Thanks for the patch. > > On Fri, 14 Jul 2023 at 15:16, Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Replace the usage of CameraConfiguration::transform with the newly > > introduced CameraConfiguration::orientation. > > > > Rework and rename the CameraSensor::validateTransform(transform) to > > CameraSensor::computeTransform(orientation), that given the desired > > image orientation computes the Transform that pipeline handlers should > > apply to the sensor to obtain it. > > > > Port all pipeline handlers to use the newly introduced function. > > > > This commit breaks existing applications as it removes the public > > CameraConfiguration::transform in favour of > > CameraConfiguration::orientation. > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > include/libcamera/camera.h | 1 - > > include/libcamera/internal/camera_sensor.h | 3 +- > > src/libcamera/camera.cpp | 15 +-- > > src/libcamera/camera_sensor.cpp | 92 +++++++++---------- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +- > > .../pipeline/rpi/common/pipeline_base.cpp | 9 +- > > src/libcamera/pipeline/simple/simple.cpp | 6 +- > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +- > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +- > > 10 files changed, 65 insertions(+), 83 deletions(-) > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > > index 55359d53f2ab..d937ffc24f54 100644 > > --- a/include/libcamera/camera.h > > +++ b/include/libcamera/camera.h > > @@ -80,7 +80,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/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index af842e70dcb0..3d52d1a0019a 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -224,7 +224,7 @@ LOG_DECLARE_CATEGORY(Camera) > > * \brief Create an empty camera configuration > > */ > > CameraConfiguration::CameraConfiguration() > > - : transform(Transform::Identity), orientation(rotate0), config_({}) > > + : orientation(rotate0), config_({}) > > { > > } > > > > @@ -476,19 +476,6 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF > > return status; > > } > > > > -/** > > - * \var CameraConfiguration::transform > > - * \brief User-specified transform to be applied to the image > > - * > > - * The transform is a user-specified 2D plane transform that will be applied > > - * to the camera images by the processing pipeline before being handed to > > - * the application. > > - * > > - * The usual 2D plane transforms are allowed here (horizontal/vertical > > - * flips, multiple of 90-degree rotations etc.), but the validate() function > > - * may adjust this field at its discretion if the selection is not supported. > > - */ > > - > > /** > > * \var CameraConfiguration::orientation > > * \brief The desired orientation of the images produced by the camera > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > index 3ba364c44a40..ac81ea3889f7 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); > > @@ -1024,69 +1024,65 @@ void CameraSensor::updateControlInfo() > > */ > > > > /** > > - * \brief Validate a transform request against the sensor capabilities > > - * \param[inout] transform The requested transformation, updated to match > > - * the sensor capabilities > > + * \brief Compute the Transform that gives the requested \a orientation > > + * \param[inout] orientation The desired image orientation > > * > > - * The input \a transform is the transform that the caller wants, and it is > > - * adjusted according to the capabilities of the sensor to represent the > > - * "nearest" transform that can actually be delivered. > > + * This function computes the Transform that the pipeline handler should apply > > + * to the CameraSensor to obtain the requested \a orientation. > > * > > - * The returned Transform is the transform applied to the sensor in order to > > - * produce the input \a transform, It is also validated against the sensor's > > - * ability to perform horizontal and vertical flips. > > + * The intended caller of this function is the validate() implementation of > > + * pipeline handlers, that pass in the application requested > > + * CameraConfiguration::orientation and obtain a Transform to apply to the > > + * camera sensor, likely at configure() time. > > * > > - * For example, if the requested \a transform is Transform::Identity and the > > - * sensor rotation is 180 degrees, the output transform will be > > - * Transform::Rot180 to correct the images so that they appear to have > > - * Transform::Identity, but only if the sensor can apply horizontal and vertical > > - * flips. > > + * If the requested \a orientation cannot be obtained, the \a orientation > > + * parameter is adjusted to report the current image orientation and > > + * Transform::Identity is returned. > > * > > - * \return A Transform instance that represents which transformation has been > > - * applied to the camera sensor > > + * If the requested \a orientation can be obtained, the function computes a > > + * Transform and does not adjust \a orientation. > > + * > > + * Pipeline handlers are expected to verify if \a orientation has been > > + * adjusted by this function and set the CameraConfiguration::status to > > + * Adjusted accordingly. > > + * > > + * \return A Transform instance that applied to the CameraSensor produces images > > + * with \a orientation > > */ > > -Transform CameraSensor::validateTransform(Transform *transform) const > > +Transform CameraSensor::computeTransform(CameraConfiguration::Orientation *orientation) const > > { > > + CameraConfiguration::Orientation mountingOrientation = > > + transformToOrientation(rotationTransform_); > > + > > /* > > - * 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 = mountingOrientation; > > + return Transform::Identity; > > + } > > > > /* > > - * We combine the platform and user transform, but must "adjust away" > > - * any combined result that includes a transposition, as we can't do > > - * those. In this case, flipping only the transpose bit is helpful to > > - * applications - they either get the transform they requested, or have > > - * to do a simple transpose themselves (they don't have to worry about > > - * the other possible cases). > > + * Now compute the required transform to obtain 'orientation' starting > > + * from the mounting rotation. > > + * > > + * As a note: > > + * orientation / mountingOrientation = transform > > + * mountingOrientation * transform = orientation > > */ > > - if (!!(combined & Transform::Transpose)) { > > - /* > > - * Flipping the transpose bit in "transform" flips it in the > > - * combined result too (as it's the last thing that happens), > > - * which is of course clearing it. > > - */ > > - *transform ^= Transform::Transpose; > > - combined &= ~Transform::Transpose; > > - } > > + Transform transform = *orientation / mountingOrientation; > > > > /* > > - * We also check if the sensor doesn't do h/vflips at all, in which > > - * case we clear them, and the application will have to do everything. > > + * If transform contains any Transpose we cannot do it, so adjust > > + * 'orientation' to report the image native orientation and return Identity. > > */ > > - if (!supportFlips_ && !!combined) { > > - /* > > - * If the sensor can do no transforms, then combined must be > > - * changed to the identity. The only user transform that gives > > - * rise to this is the inverse of the rotation. (Recall that > > - * combined = transform * rotationTransform.) > > - */ > > - *transform = -rotationTransform_; > > - combined = Transform::Identity; > > + if (!!(transform & Transform::Transpose)) { > > + *orientation = mountingOrientation; > > + return Transform::Identity; > > I guess I wonder if in future we'll encounter ISPs that can do > transpose, but I'm happy to ignore the question for now! > They won't rely on the CameraSensor class to do transform then :) > > } > > > > - return combined; > > + return transform; > > } > > > > std::string CameraSensor::logPrefix() const > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index a81c817a1255..fa4bd0bb73e2 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -187,9 +187,9 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > * rotation and store the final combined transform that configure() will > > * need to apply to the sensor to save us working it out again. > > */ > > - Transform requestedTransform = transform; > > - combinedTransform_ = data_->cio2_.sensor()->validateTransform(&transform); > > - if (transform != requestedTransform) > > + Orientation requestedOrientation = orientation; > > + combinedTransform_ = data_->cio2_.sensor()->computeTransform(&orientation); > > + if (orientation != requestedOrientation) > > status = Adjusted; > > > > /* Cap the number of entries to the available streams. */ > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 6efa79f29f66..586b46d64630 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -481,9 +481,9 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > status = Adjusted; > > } > > > > - Transform requestedTransform = transform; > > - Transform combined = sensor->validateTransform(&transform); > > - if (transform != requestedTransform) > > + Orientation requestedOrientation = orientation; > > + combinedTransform_ = data_->sensor_->computeTransform(&orientation); > > + if (orientation != requestedOrientation) > > status = Adjusted; > > > > /* > > @@ -595,8 +595,6 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > if (sensorFormat_.size.isNull()) > > sensorFormat_.size = sensor->resolution(); > > > > - combinedTransform_ = combined; > > - > > return status; > > } > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > index 179a5b81a516..5d541d71defe 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; > > I wonder slightly about restoring the behaviour the Pi PH had > previously, but I'm happy to ignore it for now and will add a further > commit at a later date if it is needed. Sorry, how is it different now ? > > But overall I think this is a good improvement and shows that > orientations and transforms make sense together (putting aside my > wobbles over transform ordering, though I think those do need > addressing). > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Thanks! > David > > > > > 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 05ba76bce630..911051b28498 100644 > > --- a/src/libcamera/pipeline/simple/simple.cpp > > +++ b/src/libcamera/pipeline/simple/simple.cpp > > @@ -889,9 +889,9 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > if (config_.empty()) > > return Invalid; > > > > - Transform requestedTransform = transform; > > - combinedTransform_ = sensor->validateTransform(&transform); > > - if (transform != requestedTransform) > > + Orientation requestedOrientation = orientation; > > + combinedTransform_ = sensor->computeTransform(&orientation); > > + if (orientation != requestedOrientation) > > status = Adjusted; > > > > /* Cap the number of entries to the available streams. */ > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > index 02a6117c7955..6f9cf0dca6f5 100644 > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > @@ -112,8 +112,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 00e6f4c6a51f..26401ab92d15 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; > > } > > > > -- > > 2.40.1 > >
HI Jacopo On Mon, 17 Jul 2023 at 14:30, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi David > > On Mon, Jul 17, 2023 at 12:43:43PM +0100, David Plowman via libcamera-devel wrote: > > Hi Jacopo > > > > Thanks for the patch. > > > > On Fri, 14 Jul 2023 at 15:16, Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Replace the usage of CameraConfiguration::transform with the newly > > > introduced CameraConfiguration::orientation. > > > > > > Rework and rename the CameraSensor::validateTransform(transform) to > > > CameraSensor::computeTransform(orientation), that given the desired > > > image orientation computes the Transform that pipeline handlers should > > > apply to the sensor to obtain it. > > > > > > Port all pipeline handlers to use the newly introduced function. > > > > > > This commit breaks existing applications as it removes the public > > > CameraConfiguration::transform in favour of > > > CameraConfiguration::orientation. > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > --- > > > include/libcamera/camera.h | 1 - > > > include/libcamera/internal/camera_sensor.h | 3 +- > > > src/libcamera/camera.cpp | 15 +-- > > > src/libcamera/camera_sensor.cpp | 92 +++++++++---------- > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +- > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +- > > > .../pipeline/rpi/common/pipeline_base.cpp | 9 +- > > > src/libcamera/pipeline/simple/simple.cpp | 6 +- > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +- > > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +- > > > 10 files changed, 65 insertions(+), 83 deletions(-) > > > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > > > index 55359d53f2ab..d937ffc24f54 100644 > > > --- a/include/libcamera/camera.h > > > +++ b/include/libcamera/camera.h > > > @@ -80,7 +80,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/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > index af842e70dcb0..3d52d1a0019a 100644 > > > --- a/src/libcamera/camera.cpp > > > +++ b/src/libcamera/camera.cpp > > > @@ -224,7 +224,7 @@ LOG_DECLARE_CATEGORY(Camera) > > > * \brief Create an empty camera configuration > > > */ > > > CameraConfiguration::CameraConfiguration() > > > - : transform(Transform::Identity), orientation(rotate0), config_({}) > > > + : orientation(rotate0), config_({}) > > > { > > > } > > > > > > @@ -476,19 +476,6 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF > > > return status; > > > } > > > > > > -/** > > > - * \var CameraConfiguration::transform > > > - * \brief User-specified transform to be applied to the image > > > - * > > > - * The transform is a user-specified 2D plane transform that will be applied > > > - * to the camera images by the processing pipeline before being handed to > > > - * the application. > > > - * > > > - * The usual 2D plane transforms are allowed here (horizontal/vertical > > > - * flips, multiple of 90-degree rotations etc.), but the validate() function > > > - * may adjust this field at its discretion if the selection is not supported. > > > - */ > > > - > > > /** > > > * \var CameraConfiguration::orientation > > > * \brief The desired orientation of the images produced by the camera > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > > index 3ba364c44a40..ac81ea3889f7 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); > > > @@ -1024,69 +1024,65 @@ void CameraSensor::updateControlInfo() > > > */ > > > > > > /** > > > - * \brief Validate a transform request against the sensor capabilities > > > - * \param[inout] transform The requested transformation, updated to match > > > - * the sensor capabilities > > > + * \brief Compute the Transform that gives the requested \a orientation > > > + * \param[inout] orientation The desired image orientation > > > * > > > - * The input \a transform is the transform that the caller wants, and it is > > > - * adjusted according to the capabilities of the sensor to represent the > > > - * "nearest" transform that can actually be delivered. > > > + * This function computes the Transform that the pipeline handler should apply > > > + * to the CameraSensor to obtain the requested \a orientation. > > > * > > > - * The returned Transform is the transform applied to the sensor in order to > > > - * produce the input \a transform, It is also validated against the sensor's > > > - * ability to perform horizontal and vertical flips. > > > + * The intended caller of this function is the validate() implementation of > > > + * pipeline handlers, that pass in the application requested > > > + * CameraConfiguration::orientation and obtain a Transform to apply to the > > > + * camera sensor, likely at configure() time. > > > * > > > - * For example, if the requested \a transform is Transform::Identity and the > > > - * sensor rotation is 180 degrees, the output transform will be > > > - * Transform::Rot180 to correct the images so that they appear to have > > > - * Transform::Identity, but only if the sensor can apply horizontal and vertical > > > - * flips. > > > + * If the requested \a orientation cannot be obtained, the \a orientation > > > + * parameter is adjusted to report the current image orientation and > > > + * Transform::Identity is returned. > > > * > > > - * \return A Transform instance that represents which transformation has been > > > - * applied to the camera sensor > > > + * If the requested \a orientation can be obtained, the function computes a > > > + * Transform and does not adjust \a orientation. > > > + * > > > + * Pipeline handlers are expected to verify if \a orientation has been > > > + * adjusted by this function and set the CameraConfiguration::status to > > > + * Adjusted accordingly. > > > + * > > > + * \return A Transform instance that applied to the CameraSensor produces images > > > + * with \a orientation > > > */ > > > -Transform CameraSensor::validateTransform(Transform *transform) const > > > +Transform CameraSensor::computeTransform(CameraConfiguration::Orientation *orientation) const > > > { > > > + CameraConfiguration::Orientation mountingOrientation = > > > + transformToOrientation(rotationTransform_); > > > + > > > /* > > > - * 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 = mountingOrientation; > > > + return Transform::Identity; > > > + } > > > > > > /* > > > - * We combine the platform and user transform, but must "adjust away" > > > - * any combined result that includes a transposition, as we can't do > > > - * those. In this case, flipping only the transpose bit is helpful to > > > - * applications - they either get the transform they requested, or have > > > - * to do a simple transpose themselves (they don't have to worry about > > > - * the other possible cases). > > > + * Now compute the required transform to obtain 'orientation' starting > > > + * from the mounting rotation. > > > + * > > > + * As a note: > > > + * orientation / mountingOrientation = transform > > > + * mountingOrientation * transform = orientation > > > */ > > > - if (!!(combined & Transform::Transpose)) { > > > - /* > > > - * Flipping the transpose bit in "transform" flips it in the > > > - * combined result too (as it's the last thing that happens), > > > - * which is of course clearing it. > > > - */ > > > - *transform ^= Transform::Transpose; > > > - combined &= ~Transform::Transpose; > > > - } > > > + Transform transform = *orientation / mountingOrientation; > > > > > > /* > > > - * We also check if the sensor doesn't do h/vflips at all, in which > > > - * case we clear them, and the application will have to do everything. > > > + * If transform contains any Transpose we cannot do it, so adjust > > > + * 'orientation' to report the image native orientation and return Identity. > > > */ > > > - if (!supportFlips_ && !!combined) { > > > - /* > > > - * If the sensor can do no transforms, then combined must be > > > - * changed to the identity. The only user transform that gives > > > - * rise to this is the inverse of the rotation. (Recall that > > > - * combined = transform * rotationTransform.) > > > - */ > > > - *transform = -rotationTransform_; > > > - combined = Transform::Identity; > > > + if (!!(transform & Transform::Transpose)) { > > > + *orientation = mountingOrientation; > > > + return Transform::Identity; > > > > I guess I wonder if in future we'll encounter ISPs that can do > > transpose, but I'm happy to ignore the question for now! > > > > They won't rely on the CameraSensor class to do transform then :) Yes, I guess that's a fair point! > > > > } > > > > > > - return combined; > > > + return transform; > > > } > > > > > > std::string CameraSensor::logPrefix() const > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > index a81c817a1255..fa4bd0bb73e2 100644 > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > @@ -187,9 +187,9 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > > * rotation and store the final combined transform that configure() will > > > * need to apply to the sensor to save us working it out again. > > > */ > > > - Transform requestedTransform = transform; > > > - combinedTransform_ = data_->cio2_.sensor()->validateTransform(&transform); > > > - if (transform != requestedTransform) > > > + Orientation requestedOrientation = orientation; > > > + combinedTransform_ = data_->cio2_.sensor()->computeTransform(&orientation); > > > + if (orientation != requestedOrientation) > > > status = Adjusted; > > > > > > /* Cap the number of entries to the available streams. */ > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > index 6efa79f29f66..586b46d64630 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > @@ -481,9 +481,9 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > > status = Adjusted; > > > } > > > > > > - Transform requestedTransform = transform; > > > - Transform combined = sensor->validateTransform(&transform); > > > - if (transform != requestedTransform) > > > + Orientation requestedOrientation = orientation; > > > + combinedTransform_ = data_->sensor_->computeTransform(&orientation); > > > + if (orientation != requestedOrientation) > > > status = Adjusted; > > > > > > /* > > > @@ -595,8 +595,6 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > > if (sensorFormat_.size.isNull()) > > > sensorFormat_.size = sensor->resolution(); > > > > > > - combinedTransform_ = combined; > > > - > > > return status; > > > } > > > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > index 179a5b81a516..5d541d71defe 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; > > > > I wonder slightly about restoring the behaviour the Pi PH had > > previously, but I'm happy to ignore it for now and will add a further > > commit at a later date if it is needed. > > Sorry, how is it different now ? Previously things were arranged on the Pi so that the residual transform for the application to do would always just be Transpose, but with the orientation/transform classes I can easily arrange that for myself (if I still want to). So I am fine with the code here. David > > > > > But overall I think this is a good improvement and shows that > > orientations and transforms make sense together (putting aside my > > wobbles over transform ordering, though I think those do need > > addressing). > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > Thanks! > > David > > > > > > > > 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 05ba76bce630..911051b28498 100644 > > > --- a/src/libcamera/pipeline/simple/simple.cpp > > > +++ b/src/libcamera/pipeline/simple/simple.cpp > > > @@ -889,9 +889,9 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > > if (config_.empty()) > > > return Invalid; > > > > > > - Transform requestedTransform = transform; > > > - combinedTransform_ = sensor->validateTransform(&transform); > > > - if (transform != requestedTransform) > > > + Orientation requestedOrientation = orientation; > > > + combinedTransform_ = sensor->computeTransform(&orientation); > > > + if (orientation != requestedOrientation) > > > status = Adjusted; > > > > > > /* Cap the number of entries to the available streams. */ > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > index 02a6117c7955..6f9cf0dca6f5 100644 > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > @@ -112,8 +112,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 00e6f4c6a51f..26401ab92d15 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; > > > } > > > > > > -- > > > 2.40.1 > > >
diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h index 55359d53f2ab..d937ffc24f54 100644 --- a/include/libcamera/camera.h +++ b/include/libcamera/camera.h @@ -80,7 +80,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/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index af842e70dcb0..3d52d1a0019a 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -224,7 +224,7 @@ LOG_DECLARE_CATEGORY(Camera) * \brief Create an empty camera configuration */ CameraConfiguration::CameraConfiguration() - : transform(Transform::Identity), orientation(rotate0), config_({}) + : orientation(rotate0), config_({}) { } @@ -476,19 +476,6 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF return status; } -/** - * \var CameraConfiguration::transform - * \brief User-specified transform to be applied to the image - * - * The transform is a user-specified 2D plane transform that will be applied - * to the camera images by the processing pipeline before being handed to - * the application. - * - * The usual 2D plane transforms are allowed here (horizontal/vertical - * flips, multiple of 90-degree rotations etc.), but the validate() function - * may adjust this field at its discretion if the selection is not supported. - */ - /** * \var CameraConfiguration::orientation * \brief The desired orientation of the images produced by the camera diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index 3ba364c44a40..ac81ea3889f7 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); @@ -1024,69 +1024,65 @@ void CameraSensor::updateControlInfo() */ /** - * \brief Validate a transform request against the sensor capabilities - * \param[inout] transform The requested transformation, updated to match - * the sensor capabilities + * \brief Compute the Transform that gives the requested \a orientation + * \param[inout] orientation The desired image orientation * - * The input \a transform is the transform that the caller wants, and it is - * adjusted according to the capabilities of the sensor to represent the - * "nearest" transform that can actually be delivered. + * This function computes the Transform that the pipeline handler should apply + * to the CameraSensor to obtain the requested \a orientation. * - * The returned Transform is the transform applied to the sensor in order to - * produce the input \a transform, It is also validated against the sensor's - * ability to perform horizontal and vertical flips. + * The intended caller of this function is the validate() implementation of + * pipeline handlers, that pass in the application requested + * CameraConfiguration::orientation and obtain a Transform to apply to the + * camera sensor, likely at configure() time. * - * For example, if the requested \a transform is Transform::Identity and the - * sensor rotation is 180 degrees, the output transform will be - * Transform::Rot180 to correct the images so that they appear to have - * Transform::Identity, but only if the sensor can apply horizontal and vertical - * flips. + * If the requested \a orientation cannot be obtained, the \a orientation + * parameter is adjusted to report the current image orientation and + * Transform::Identity is returned. * - * \return A Transform instance that represents which transformation has been - * applied to the camera sensor + * If the requested \a orientation can be obtained, the function computes a + * Transform and does not adjust \a orientation. + * + * Pipeline handlers are expected to verify if \a orientation has been + * adjusted by this function and set the CameraConfiguration::status to + * Adjusted accordingly. + * + * \return A Transform instance that applied to the CameraSensor produces images + * with \a orientation */ -Transform CameraSensor::validateTransform(Transform *transform) const +Transform CameraSensor::computeTransform(CameraConfiguration::Orientation *orientation) const { + CameraConfiguration::Orientation mountingOrientation = + transformToOrientation(rotationTransform_); + /* - * 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 = mountingOrientation; + return Transform::Identity; + } /* - * We combine the platform and user transform, but must "adjust away" - * any combined result that includes a transposition, as we can't do - * those. In this case, flipping only the transpose bit is helpful to - * applications - they either get the transform they requested, or have - * to do a simple transpose themselves (they don't have to worry about - * the other possible cases). + * Now compute the required transform to obtain 'orientation' starting + * from the mounting rotation. + * + * As a note: + * orientation / mountingOrientation = transform + * mountingOrientation * transform = orientation */ - if (!!(combined & Transform::Transpose)) { - /* - * Flipping the transpose bit in "transform" flips it in the - * combined result too (as it's the last thing that happens), - * which is of course clearing it. - */ - *transform ^= Transform::Transpose; - combined &= ~Transform::Transpose; - } + Transform transform = *orientation / mountingOrientation; /* - * We also check if the sensor doesn't do h/vflips at all, in which - * case we clear them, and the application will have to do everything. + * If transform contains any Transpose we cannot do it, so adjust + * 'orientation' to report the image native orientation and return Identity. */ - if (!supportFlips_ && !!combined) { - /* - * If the sensor can do no transforms, then combined must be - * changed to the identity. The only user transform that gives - * rise to this is the inverse of the rotation. (Recall that - * combined = transform * rotationTransform.) - */ - *transform = -rotationTransform_; - combined = Transform::Identity; + if (!!(transform & Transform::Transpose)) { + *orientation = mountingOrientation; + return Transform::Identity; } - return combined; + return transform; } std::string CameraSensor::logPrefix() const diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index a81c817a1255..fa4bd0bb73e2 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -187,9 +187,9 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() * rotation and store the final combined transform that configure() will * need to apply to the sensor to save us working it out again. */ - Transform requestedTransform = transform; - combinedTransform_ = data_->cio2_.sensor()->validateTransform(&transform); - if (transform != requestedTransform) + Orientation requestedOrientation = orientation; + combinedTransform_ = data_->cio2_.sensor()->computeTransform(&orientation); + if (orientation != requestedOrientation) status = Adjusted; /* Cap the number of entries to the available streams. */ diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 6efa79f29f66..586b46d64630 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -481,9 +481,9 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() status = Adjusted; } - Transform requestedTransform = transform; - Transform combined = sensor->validateTransform(&transform); - if (transform != requestedTransform) + Orientation requestedOrientation = orientation; + combinedTransform_ = data_->sensor_->computeTransform(&orientation); + if (orientation != requestedOrientation) status = Adjusted; /* @@ -595,8 +595,6 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() if (sensorFormat_.size.isNull()) sensorFormat_.size = sensor->resolution(); - combinedTransform_ = combined; - return status; } diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp index 179a5b81a516..5d541d71defe 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 05ba76bce630..911051b28498 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -889,9 +889,9 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() if (config_.empty()) return Invalid; - Transform requestedTransform = transform; - combinedTransform_ = sensor->validateTransform(&transform); - if (transform != requestedTransform) + Orientation requestedOrientation = orientation; + combinedTransform_ = sensor->computeTransform(&orientation); + if (orientation != requestedOrientation) status = Adjusted; /* Cap the number of entries to the available streams. */ diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp index 02a6117c7955..6f9cf0dca6f5 100644 --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp @@ -112,8 +112,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 00e6f4c6a51f..26401ab92d15 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; }
Replace the usage of CameraConfiguration::transform with the newly introduced CameraConfiguration::orientation. Rework and rename the CameraSensor::validateTransform(transform) to CameraSensor::computeTransform(orientation), that given the desired image orientation computes the Transform that pipeline handlers should apply to the sensor to obtain it. Port all pipeline handlers to use the newly introduced function. This commit breaks existing applications as it removes the public CameraConfiguration::transform in favour of CameraConfiguration::orientation. Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- include/libcamera/camera.h | 1 - include/libcamera/internal/camera_sensor.h | 3 +- src/libcamera/camera.cpp | 15 +-- src/libcamera/camera_sensor.cpp | 92 +++++++++---------- src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +- .../pipeline/rpi/common/pipeline_base.cpp | 9 +- src/libcamera/pipeline/simple/simple.cpp | 6 +- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +- src/libcamera/pipeline/vimc/vimc.cpp | 4 +- 10 files changed, 65 insertions(+), 83 deletions(-)