[libcamera-devel] [PATCH 5/9] libcamera: camera_sensor: Validate Transform
David Plowman
david.plowman at raspberrypi.com
Thu Nov 24 16:52:17 CET 2022
Hi Jacopo
Thank you for this patch.
On Thu, 24 Nov 2022 at 12:12, Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> The two pipeline handlers that currently support Transform do so by
> operating H/V flips on the image sensor, instead of rotating on the ISP.
>
> As the image sensor performs the actual rotation, centralize the code
> that validates a requested Transform against the camera sensor rotation
> and capabilities in the CameraSensor class.
>
> The implementation in the IPU3 pipeline handler was copied from the
> RaspberryPi implementation, and is now centralized in CameraSensor to
> make it easier for other platforms that do not rotate on the ISP to
> implement support for Transform using the CameraSensor class.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> include/libcamera/internal/camera_sensor.h | 4 ++
> src/libcamera/camera_sensor.cpp | 59 +++++++++++++++++++
> src/libcamera/pipeline/ipu3/ipu3.cpp | 45 ++------------
> .../pipeline/raspberrypi/raspberrypi.cpp | 59 +++----------------
> 4 files changed, 76 insertions(+), 91 deletions(-)
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 878f3c28a3c9..bea52badaff7 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -29,6 +29,8 @@ class BayerFormat;
> class CameraLens;
> class MediaEntity;
>
> +enum class Transform;
> +
> struct CameraSensorProperties;
>
> class CameraSensor : protected Loggable
> @@ -68,6 +70,8 @@ public:
>
> CameraLens *focusLens() { return focusLens_.get(); }
>
> + Transform validateTransform(Transform *transform) const;
> +
> protected:
> std::string logPrefix() const override;
>
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 3afcbc482095..a4ad0ba9a099 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -982,6 +982,65 @@ void CameraSensor::updateControlInfo()
> * connected to the sensor
> */
>
> +/**
> + * \brief Validate a transform request against the sensor capabilities
> + * \param[inout] transform The requested transformation, updated to match
> + * the sensor capabilities
> + *
> + * The requested \a transform is adjusted against the sensor rotation and its
> + * capabilities.
> + *
> + * In example, if the requested \a transform is Transform::Identity and the
s/In example/For example/ reads better.
> + * sensor rotation is 180 degrees, the resulting transform returned by the
> + * function is Transform::Rot180 to automatically correct the image, but only if
> + * the sensor can actually apply horizontal and vertical flips.
> + *
> + * \return A Transform instance that represents how \a transform is applied to
> + * the camera sensor
> + */
> +Transform CameraSensor::validateTransform(Transform *transform) const
Teeny nit-pick, but I wondered if another signature might be more
convenient for callers. For example
Status CameraSensor::validateTransform(Transform &transform, Transform
&combined)
It would return "Adjusted" if the "transform out" is different from
the "transform in". But I'm not terribly bothered!!
> +{
> + /* Adjust the requested transform to the sensor rotation. */
> + int32_t rotation = properties().get(properties::Rotation).value_or(0);
> + bool success;
> +
> + Transform rotationTransform = transformFromRotation(rotation, &success);
> + if (!success)
> + LOG(CameraSensor, Warning) << "Invalid rotation of " << rotation
> + << " degrees - ignoring";
> +
> + Transform combined = *transform * rotationTransform;
I know I must have written this, but now I'm wondering if
"rotationTransform" here should be "-rotationTransform". I guess it
never matters with our cameras which are always 0 or 180 degrees
rotated, but maybe in other cases...?
> +
> + /*
> + * The camera sensor cannot do Transpose. Adjust any combined result
> + * that includes a transpose by flipping the transpose bit to notify
> + * 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 (!!(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 the sensor can do no transforms, then combined must be changed to
> + * the identity and the sensor rotation must be cleared from the
> + * requested "transform".
Don't really follow this comment. Maybe
* If the sensor cannot do transforms, then combined must be changed to
* the identity and the only transform the user can have is the sensor
* rotation itself.
(Which rather suggests the code following has got the transform the
wrong way round...)
> + */
> + if (!supportFlips_ && !!combined) {
> + *transform = -rotationTransform;
This might really be "rotationTransform" (without the inverse).
Depending on which we round we view the sensor rotation (from in front
of or behind the camera).
Also, as we remarked elsewhere, we should possibly consider the
existence of H and V flips separately.
But apart from my existential doubts about inverse rotations, it all
seems reasonable to me, though I sense we'll be coming back to this
again anyway:
Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
Thanks!
David
> + combined = Transform::Identity;
> + }
> +
> + return combined;
> +}
> +
> std::string CameraSensor::logPrefix() const
> {
> return "'" + entity_->name() + "'";
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index e4d79ea44aed..a424ac914859 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -184,48 +184,15 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> if (config_.empty())
> return Invalid;
>
> - Transform combined = transform * data_->rotationTransform_;
> -
> - /*
> - * 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 (!!(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;
> - status = Adjusted;
> - }
> -
> /*
> - * 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.
> + * Validate the requested transform against the sensor capabilities and
> + * rotation and store the final combined transform that configure() will
> + * need to apply to the sensor to save us working it out again.
> */
> - if (!data_->supportsFlips_ && !!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 = -data_->rotationTransform_;
> - combined = Transform::Identity;
> + Transform requestedTransform = transform;
> + combinedTransform_ = data_->cio2_.sensor()->validateTransform(&transform);
> + if (transform != requestedTransform)
> status = Adjusted;
> - }
> -
> - /*
> - * Store the final combined transform that configure() will need to
> - * apply to the sensor to save us working it out again.
> - */
> - combinedTransform_ = combined;
>
> /* Cap the number of entries to the available streams. */
> if (config_.size() > kMaxStreams) {
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 087c71b65700..e83984985bce 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -366,59 +366,14 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> status = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace);
>
> /*
> - * What if the platform has a non-90 degree rotation? We can't even
> - * "adjust" the configuration and carry on. Alternatively, raising an
> - * error means the platform can never run. Let's just print a warning
> - * and continue regardless; the rotation is effectively set to zero.
> + * Validate the requested transform against the sensor capabilities and
> + * rotation and store the final combined transform that configure() will
> + * need to apply to the sensor to save us working it out again.
> */
> - int32_t rotation = data_->sensor_->properties().get(properties::Rotation).value_or(0);
> - bool success;
> - Transform rotationTransform = transformFromRotation(rotation, &success);
> - if (!success)
> - LOG(RPI, Warning) << "Invalid rotation of " << rotation
> - << " degrees - ignoring";
> - Transform combined = transform * rotationTransform;
> -
> - /*
> - * We combine the platform and user transform, but must "adjust away"
> - * any combined result that includes a transform, 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 (!!(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;
> - status = Adjusted;
> - }
> -
> - /*
> - * 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 (!data_->supportsFlips_ && !!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 the inverse of the rotation. (Recall that
> - * combined = transform * rotationTransform.)
> - */
> - transform = -rotationTransform;
> - combined = Transform::Identity;
> + Transform requestedTransform = transform;
> + combinedTransform_ = data_->sensor_->validateTransform(&transform);
> + if (transform != requestedTransform)
> status = Adjusted;
> - }
> -
> - /*
> - * Store the final combined transform that configure() will need to
> - * apply to the sensor to save us working it out again.
> - */
> - combinedTransform_ = combined;
>
> unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0;
> std::pair<int, Size> outSize[2];
> @@ -453,7 +408,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> if (data_->flipsAlterBayerOrder_) {
> BayerFormat bayer = BayerFormat::fromV4L2PixelFormat(fourcc);
> bayer.order = data_->nativeBayerOrder_;
> - bayer = bayer.transform(combined);
> + bayer = bayer.transform(combinedTransform_);
> fourcc = bayer.toV4L2PixelFormat();
> }
>
> --
> 2.38.1
>
More information about the libcamera-devel
mailing list