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

[Laurent Pinchart]

Hi David and Jacopo,

On Mon, Jul 06, 2020 at 11:41:13AM +0100, David Plowman wrote:
> On Mon, 6 Jul 2020 at 11:01, Jacopo Mondi <jacopo at jmondi.org> wrote:
> > 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?

Correct. I think they should be added though.

> 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.

Why not ? If platforms allow per-frame control of flipping, wouldn't it
make sense to allow it in the API ?

We've thought about adding a ControlList to CameraConfiguration to set
controls that can't change per frame. Pipeline handlers could then
decide if they want to support the flip controls per frame, or only at
configure time. We could extend the ControlInfo class to report whether
a control can be set per frame or only at configure time.

> 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.)

Hijacking discussions is how we usually operate, no worries about that
:-) I don't think it should prevent this patch from being merged though.

> >>>> +     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