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

[Robert Mader]

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...)
>
> > +        */
> > +       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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221207/f4187697/attachment.htm>