[libcamera-devel] [PATCH v2] libcamera: ipu3: Add rotation to ipu3 pipeline
Naushir Patuck
naush at raspberrypi.com
Fri Jan 22 14:07:32 CET 2021
Hi all,
On Fri, 22 Jan 2021 at 12:20, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:
> Hi Fabian, Jacopo, Naush, David ;-)
>
> On 22/01/2021 08:52, Jacopo Mondi wrote:
> > Hi Fabian,
> > + Naush for RPi question
>
> Fixing the +CC for the question below - but also adding David as he
> wrote the original Transforms.
>
> --
> Kieran
>
>
> >
> > On Fri, Jan 22, 2021 at 09:10:42AM +0100, Fabian Wüthrich wrote:
> >> Hi Laurent,
> >>
> >> On 22.01.21 00:37, Laurent Pinchart wrote:
> >>> Hi Jacopo and Fabian,
> >>>
> >>> On Mon, Jan 18, 2021 at 11:06:31AM +0100, Jacopo Mondi wrote:
> >>>> On Sat, Jan 16, 2021 at 04:02:58PM +0100, Fabian Wüthrich wrote:
> >>>>> Use the same transformation logic as in the raspberry pipeline to
> >>>>> implement rotations in the ipu3 pipeline.
> >>>
> >>> This should eventually be shared between different pipeline handlers,
> >>> but for now it's fine.
> >>>
> >>
> >> Agreed.
> >>
> >>>>> Tested on a Surface Book 2 with an experimental driver for OV5693.
> >>>>>
> >>>>> Signed-off-by: Fabian Wüthrich <me at fabwu.ch>
> >>>>> ---
> >>>>> Changes in v2:
> >>>>> - Cache rotationTransform in CameraData
> >>>>> - Use separate controls for sensor
> >>>>>
> >>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 81
> +++++++++++++++++++++++++++-
> >>>>> 1 file changed, 79 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp
> b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>>>> index 73304ea7..8db5f2f1 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,15 @@ public:
> >>>>> Stream outStream_;
> >>>>> Stream vfStream_;
> >>>>> Stream rawStream_;
> >>>>> +
> >>>>> + /* Save transformation given by the sensor rotation */
> >>>>> + Transform rotationTransform_;
> >>>>> +
> >>>>> + /*
> >>>>> + * Manage horizontal and vertical flips supported (or not) by the
> >>>>> + * sensor.
> >>>>> + */
> >>>>> + bool supportsFlips_;
> >>>>> };
> >>>>>
> >>>>> class IPU3CameraConfiguration : public CameraConfiguration
> >>>>> @@ -74,6 +84,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 +156,49 @@ CameraConfiguration::Status
> IPU3CameraConfiguration::validate()
> >>>>> if (config_.empty())
> >>>>> return Invalid;
> >>>>>
> >>>>> - if (transform != Transform::Identity) {
> >>>>> - transform = Transform::Identity;
> >>>>> + 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.
> >>>>> + */
> >>>>> + 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;
> >>>>> + 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 +591,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_) {
> >>>>> + ControlList sensor_ctrls(cio2->sensor()->controls());
> >>>>> + sensor_ctrls.set(V4L2_CID_HFLIP,
> >>>>> +
> static_cast<int32_t>(!!(config->combinedTransform_ & Transform::HFlip)));
> >>>>> + sensor_ctrls.set(V4L2_CID_VFLIP,
> >>>>> +
> static_cast<int32_t>(!!(config->combinedTransform_ & Transform::VFlip)));
> >>>>> + cio2->sensor()->setControls(&sensor_ctrls);
> >>>>> + }
> >>>>> +
> >>>>> return 0;
> >>>>> }
> >>>>>
> >>>>> @@ -775,9 +839,22 @@ int PipelineHandlerIPU3::registerCameras()
> >>>>> /* Initialize the camera properties. */
> >>>>> data->properties_ = cio2->sensor()->properties();
> >>>>>
> >>>>> + /* Convert the sensor rotation to a transformation */
> >>>>> + int32_t rotation =
> data->properties_.get(properties::Rotation);
> >>>>
> >>>> There's no guarantee the CameraSensor class register the Rotation
> >>>> property. It was defaulted to 0 if the information was not available
> >>>> from the firmware interface.
> >>>>
> >>>> It is now not anymore (a very recent change, sorry)
> >>>>
> https://git.libcamera.org/libcamera/libcamera.git/commit/?id=c35123e6f91b6269cf6f5ae9216aeccebc545c75
> >>>>
> >>>> Not sure what would be best, default to 0 in each pipeline handler
> >>>> that need that information ?
> >>>
> >>> That sounds good to me for now.
> >>>
> >>
> >> Ok, thanks for checking. Should I provide a patch for the raspberry
> pipeline as well?
> >
> > I would not mind, but feel free to stick to IPU3 if that's quicker.
> >
> > Cc Naush for RPi. I'm thinking that, being RPi a bit more 'strict' as
> > platform and with a little more control over sensor drivers/fw, maybe
> > they want to refuse setups where no rotation is specified.
>
I think we should be ok with defaulting to 0 if the rotation property is
unavailable. Worst that would happen is that we would produce an inverted
picture - and then the user knows to add rotation in the driver :-)
I'll let David have the final say though.
Regards,
Naush
> >
> >>
> >>>>> + bool success;
> >>>>> + data->rotationTransform_ = transformFromRotation(rotation,
> &success);
> >>>>> + if (!success)
> >>>>> + LOG(IPU3, Warning) << "Invalid rotation of " <<
> rotation << " degrees - ignoring";
> >>>>> +
> >>>>> /* 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
> >>>
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> >
>
> --
> Regards
> --
> Kieran
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210122/5f268d8a/attachment.htm>
More information about the libcamera-devel
mailing list