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

[David Plowman]

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.

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
>         if (rotation == 90 || roation == 270)
>                 VFLIP
>  ?

How are we handling transformations that aren't rotations, e.g.
horizontal flip. As I said previously, it's a significant use case.
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.

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