[libcamera-devel] [PATCH v2 08/12] libcamera: pipeline: raspberrypi: Set sensor flip based on rotation
David Plowman
david.plowman at raspberrypi.com
Mon Jul 6 12:41:13 CEST 2020
Hi Jacopo
Thanks for the quick reply! Let me just clarify my understanding of
your answers...
On Mon, 6 Jul 2020 at 11:01, Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> 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.
So far as I'm aware there is no control for transformations (h/vflip,
even transpose) currently, is that correct? I'm also not sure the
usual request->controls() mechanism is the right one as it sort-of
implies you can set it per frame, which I think is something we
shouldn't allow.
Perhaps it belongs in the StreamConfiguration? Some platforms might
allow per-stream transformations, others (many?) will not. What do
people think?
>
> > 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 ?
As I said above, I'm not sure the usual "controls" mechanism is the
right one, and it's also not clear to me how a platform should
advertise what it supports. (Do we have any mechanism for signalling
that kind of thing?) Perhaps one could take the view that, for the
time being, we just overwrite anything we don't handle and report the
configuration as "adjusted"?
(Perhaps I am hijacking the original discussion... in which case
please say and I'll re-start it on its own.)
Best regards
David
>
> 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
More information about the libcamera-devel
mailing list