[libcamera-devel] [PATCH v2 08/12] libcamera: pipeline: raspberrypi: Set sensor flip based on rotation
Jacopo Mondi
jacopo at jmondi.org
Thu Jul 9 00:02:13 CEST 2020
Hi Naush,
On Mon, Jul 06, 2020 at 04:02:17PM +0100, Naushir Patuck wrote:
> Hi David and Jacopo,
>
>
> On Mon, 6 Jul 2020 at 11:41, David Plowman
> <david.plowman at raspberrypi.com> wrote:
> >
> > 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.)
>
> If I am not mistaken, the pipeline handlers need to maintain a list of
> controls supported, like in include/libcamera/ipa/raspberrypi.h
> (RPiControls). This then gets exported to the CameraData base class
> member controlInfo_. ARequest will not allow setting a
> libcamera::control that is not listed in CameraData::controlInfo_
That's correct, it seems to me from this discussion, a ControlInfo
could be augmented with information about a control being supported
per-frame or just at at configuration time (or else...)
>
> Regards,
> Naush
>
>
> >
> > 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
> > _______________________________________________
> > 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