[libcamera-devel] [PATCH] libcamera: ipu3: Add rotation to ipu3 pipeline
Jacopo Mondi
jacopo at jmondi.org
Mon Jan 11 16:45:27 CET 2021
Hi Fabian
On Mon, Jan 11, 2021 at 04:21:49PM +0100, Fabian Wüthrich wrote:
> Hi Jacopo
>
> Thanks for your review.
>
> On 07.01.21 18:33, Jacopo Mondi wrote:
> > 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.
> >
>
> Done
>
> >> + 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 ?
> >
>
> I copied the code from the raspberry pipeline (see
> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp line 287 ff.) and
> assumed that this is the correct way to handle transpositions.
>
> Should I just flip the transpose bit of CameraConfiguration::transpose
> and then combine it with the sensor rotation?
>
> And if so should I send a patch for the raspberry pipeline as well?
>
Ok, didn't know it came from there. I think it's a minor
optimization, and changing it in two places might delay the acceptance
of this patch (and require you more work). If the two implementations are
algigned, they can be changed in one go later.
With the other comments addressed
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks
j
> >> + 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
> >
>
> Done
>
> >> + */
> >> + 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/
> >
>
> Done
>
> >> + * 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
> >
>
> I created a sensor_ctrls variable to prevent the re-use.
>
> > 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