[libcamera-devel] [PATCH] libcamera: ipu3: Add rotation to ipu3 pipeline
Jacopo Mondi
jacopo at jmondi.org
Thu Jan 7 18:33:24 CET 2021
Hi Fabian
On Wed, Jan 06, 2021 at 02:35:13PM +0100, Fabian Wüthrich wrote:
> Use the same transformation logic as in the raspberry pipeline to
> implement rotations in the ipu3 pipeline.
>
> Tested on a Surface Book 2 with an experimental driver for OV5693.
>
> Signed-off-by: Fabian Wüthrich <me at fabwu.ch>
> ---
> src/libcamera/pipeline/ipu3/ipu3.cpp | 76 +++++++++++++++++++++++++++-
> 1 file changed, 74 insertions(+), 2 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index f1151733..76d75a74 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -14,6 +14,7 @@
> #include <libcamera/camera.h>
> #include <libcamera/control_ids.h>
> #include <libcamera/formats.h>
> +#include <libcamera/property_ids.h>
> #include <libcamera/request.h>
> #include <libcamera/stream.h>
>
> @@ -62,6 +63,12 @@ public:
> Stream outStream_;
> Stream vfStream_;
> Stream rawStream_;
> +
> + /*
> + * Manage horizontal and vertical flips supported (or not) by the
> + * sensor.
> + */
> + bool supportsFlips_;
> };
>
> class IPU3CameraConfiguration : public CameraConfiguration
> @@ -74,6 +81,9 @@ public:
> const StreamConfiguration &cio2Format() const { return cio2Configuration_; }
> const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; }
>
> + /* Cache the combinedTransform_ that will be applied to the sensor */
> + Transform combinedTransform_;
> +
> private:
> /*
> * The IPU3CameraData instance is guaranteed to be valid as long as the
> @@ -143,11 +153,54 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> if (config_.empty())
> return Invalid;
>
> - if (transform != Transform::Identity) {
> - transform = Transform::Identity;
> + int32_t rotation = data_->cio2_.sensor()->properties().get(properties::Rotation);
> + bool success;
> + Transform rotationTransform = transformFromRotation(rotation, &success);
Can you cache this in CameraData ?
At registerCamera() time we initialize the properties so we can cache
the above 'rotationTransform' just after
/* Initialize the camera properties. */
data->properties_ = cio2->sensor()->properties();
Instead of re-calculating it every validation.
> + if (!success)
> + LOG(IPU3, 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).
> + */
The 'rotationTransform' obtained by inspecting the sensor rotation
does not contain any Transpose, looking at
Transform::transformFromRotation().
Can you address the transpose directly in the
CameraConfiguration::transpose field before multiplying it by
rotationTransform ?
> + 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, ad the application will have to do everything.
s/ad/and
> + */
> + 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
s/this the/this is the/
> + * combined = transform * rotationTransform.)
> + */
> + transform = -rotationTransform;
> + combined = Transform::Identity;
> + 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() > IPU3_MAX_STREAMS) {
> config_.resize(IPU3_MAX_STREAMS);
> @@ -540,6 +593,19 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> return ret;
> }
>
> + /*
> + * Configure the H/V flip controls based on the combination of
> + * the sensor and user transform.
> + */
> + if (data->supportsFlips_) {
> + ctrls = cio2->sensor()->controls();
I would not re-use ctrls, or rather we should probably:
--- a/include/libcamera/controls.h
+++ b/include/libcamera/controls.h
@@ -352,7 +352,7 @@ private:
public:
ControlList();
ControlList(const ControlIdMap &idmap, ControlValidator *validator = nullptr);
- ControlList(const ControlInfoMap &infoMap, ControlValidator *validator = nullptr);
+ explicit ControlList(const ControlInfoMap &infoMap, ControlValidator *validator = nullptr);
to avoid this assignment. But maybe it's just me
Thanks
j
> + ctrls.set(V4L2_CID_HFLIP,
> + static_cast<int32_t>(!!(config->combinedTransform_ & Transform::HFlip)));
> + ctrls.set(V4L2_CID_VFLIP,
> + static_cast<int32_t>(!!(config->combinedTransform_ & Transform::VFlip)));
> + cio2->sensor()->setControls(&ctrls);
> + }
> +
> return 0;
> }
>
> @@ -779,6 +845,12 @@ int PipelineHandlerIPU3::registerCameras()
> /* Initialze the camera controls. */
> data->controlInfo_ = IPU3Controls;
>
> + ControlList ctrls = cio2->sensor()->getControls({ V4L2_CID_HFLIP });
> + if (!ctrls.empty()) {
> + /* We assume it will support vflips too... */
> + data->supportsFlips_ = true;
> + }
> +
> /**
> * \todo Dynamically assign ImgU and output devices to each
> * stream and camera; as of now, limit support to two cameras
> --
> 2.30.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list