[libcamera-devel] [PATCH 5/9] libcamera: camera_sensor: Validate Transform

[Jacopo Mondi]

Hi,

On Wed, Dec 07, 2022 at 01:32:04PM +0100, Robert Mader via libcamera-devel wrote:
> Hi!
>
> I'd love if we could clarify the following part to ensure the Pipewire
> implementation is correct:
>
> > > +
> > > +       /*
> > > +        * The camera sensor cannot do Transpose. Adjust any combined result
> > > +        * that includes a transpose by flipping the transpose bit to notify
> > > +        * applications they either get the transform they requested, or have
> > > +        * to do a simple transpose themselves (they don't have to worry about
> > > +        * the other possible cases).
> > > +        */
> > > +       if (!!(combined & Transform::Transpose)) {
> > > +               /*
> > > +                * Flipping the transpose bit in "transform" flips it in the
> > > +                * combined result too (as it's the last thing that happens),
> > > +                * which is of course clearing it.
> > > +                */
> > > +               *transform ^= Transform::Transpose;
> > > +               combined &= ~Transform::Transpose;
> > > +       }
> > > +
> > > +       /*
> > > +        * If the sensor can do no transforms, then combined must be changed to
> > > +        * the identity and the sensor rotation must be cleared from the
> > > +        * requested "transform".
> >
> > Don't really follow this comment. Maybe
> >
> >          * If the sensor cannot do transforms, then combined must be changed to
> >          * the identity and the only transform the user can have is the sensor
> >          * rotation itself.
> >
> > (Which rather suggests the code following has got the transform the
> > wrong way round...)
> >

I should have replied earlier to David on the patch series for this
one.

What I actually meant is "If the sensor cannot do any flip" and the
code has been copied from
https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n402

Will reply to rest of the discussion below in a separate email.

> > > +        */
> > > +       if (!supportFlips_ && !!combined) {
> > > +               *transform = -rotationTransform;
> >
> > This might really be "rotationTransform" (without the inverse).
> > Depending on which we round we view the sensor rotation (from in front
> > of or behind the camera).
> >
> > Also, as we remarked elsewhere, we should possibly consider the
> > existence of H and V flips separately.
> >
> > But apart from my existential doubts about inverse rotations, it all
> > seems reasonable to me, though I sense we'll be coming back to this
> > again anyway:
>
> As example I'd like to take the back-camera of the Pinephone Pro (imx258).
> Pipewire will ask for an identity transform and the kernel will most likely
> report `V4L2_CID_CAMERA_SENSOR_ROTATION == 270` (once driver patches and
> device tree updates land), so the initial values we'll get are 
> `properties::Rotation == 270` -> `combined == Transform::Rot270`.
>
> Right now neither transpose nor flips are supported, so the steps in the
> current revision are:
>
> 1. `combined == rot270` / `transform == identity`
> 2. `combined == hflip` / `transform == transpose`
> 3. `combined == `identity` / `transform == rot90`
>
> The value from `transform` will then be used for
> `CameraConfiguration::transform`, signalling to the client that it will have
> to compensate accordingly. Following the comment to not use the inverse it
> would be `rot270` instead.
>
> From an API-point of view, I'd say `rot90` is the more intuitive value for
> clients, because they can then just follow the documentation in
> camera_sensor.cpp and apply a transformation accordingly. Would you agree
> that this should be the desired outcome? I.e. should we stick to the
> proposed value?
>
> Best regards,
>
> Robert