[libcamera-devel] [PATCH v2 08/12] libcamera: pipeline: raspberrypi: Set sensor flip based on rotation

[Jacopo Mondi]

Hi David,

On Mon, Jul 06, 2020 at 10:29:21AM +0100, David Plowman wrote:
> Hi Jacopo, Laurent, everyone!
>
> I have a couple of questions.
>
> 1. Does this allow an application to set a transformation when the
> camera is started? For example, it's not uncommon for a horizontal
> flip to be requested from the camera depending on the application.
> Certainly in the Raspberry Pi world you can't tell a priori what a
> sensor's orientation is.
>

Isn't this mostly related to allowing application provide a list of
controls at camera start/configure time ? Those will have to be taken
into account and matched with the camera sensor reported orientation
which is handled here I guess.

> 2nd question below...
>
> On Mon, 6 Jul 2020 at 10:12, Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > Hi Laurent,
> >
> > On Sat, Jul 04, 2020 at 03:52:23AM +0300, Laurent Pinchart wrote:
> > > Instead of receiving sensor orientation configuration from the IPA,
> > > retrieve it from the CameraSensor Rotation property, and configure the
> > > HFLIP and VFLIP controls accordingly.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 74bee6895dcd..fda6831e6a7e 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -16,6 +16,7 @@
> > >  #include <libcamera/formats.h>
> > >  #include <libcamera/ipa/raspberrypi.h>
> > >  #include <libcamera/logging.h>
> > > +#include <libcamera/property_ids.h>
> > >  #include <libcamera/request.h>
> > >  #include <libcamera/stream.h>
> > >
> > > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA()
> > >       ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> > >                       nullptr);
> > >
> > > +     /* Configure the H/V flip controls based on the sensor rotation. */
> > > +     ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
> > > +     int32_t rotation = sensor_->properties().get(properties::Rotation);
> > > +     ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation));
> > > +     ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation));
> >
> > mmm, I see in the IPA this, which is where I assume you got this from
> >
> >                 ctrls.set(V4L2_CID_HFLIP, (int32_t) !!(orientation & RPi::CamTransform_HFLIP));
> >                 ctrls.set(V4L2_CID_VFLIP, (int32_t) !!(orientation & RPi::CamTransform_VFLIP));
> >
> > Where the 'orientation' flag is retrieved from
> >                 RPi::CamTransform orientation = helper_->GetOrientation();
> >
> > and expressed through members of this enumeration
> >                 static constexpr CamTransform CamTransform_IDENTITY = 0;
> >                 static constexpr CamTransform CamTransform_HFLIP    = 1;
> >                 static constexpr CamTransform CamTransform_VFLIP    = 2;
> >
> > Now, 'rotation' is expressed in multiple of 90 degrees in the [0-360[
> > interval, shouldn't we:
> >         if (rotation == 180)
> >                 HFLIP

This should be VFLIP, as the control reads "mirror in the vertical
direction", not "along the horizontal axis" as I interpreted it

> >         if (rotation == 90 || roation == 270)
> >                 VFLIP

And this is just wrong. Mirroring in H/V directions won't compensate
for such rotations.

> >  ?
>
> How are we handling transformations that aren't rotations, e.g.
> horizontal flip. As I said previously, it's a significant use case.

I think they merit a control to let application handle them
I think this was meant to compensate the camera mounting rotation
only.

> Also, how are we handling platforms that don't support things like
> 90/270 degree rotations - it's certain that many platforms won't be
> able to do that.

That is a more generic question: how do pipeline handlers advertise
which control they support, or do you think H/V flips are different ?

Thanks
  j

>
> Best regards
> David
>
> >
> >
> > > +     unicam_[Unicam::Image].dev()->setControls(&ctrls);
> > > +
> > >       return 0;
> > >  }
> > >
> > > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> > >                                             { V4L2_CID_EXPOSURE, action.data[1] } });
> > >                       sensorMetadata_ = action.data[2];
> > >               }
> > > -
> > > -             /* Set the sensor orientation here as well. */
> > > -             ControlList controls = action.controls[0];
> > > -             unicam_[Unicam::Image].dev()->setControls(&controls);
> >
> > Do you think we can now remove the custom rotation handling from the
> > IPA helpers ?
> >
> > >               return;
> > >       }
> > >
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel