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

[David Plowman]

Hi Robert

Thanks for prodding us all on this question!

On Wed, 7 Dec 2022 at 12:32, Robert Mader via libcamera-devel
<libcamera-devel at lists.libcamera.org> 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...)
>
> > +        */
> > +       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:
>
> `combined == rot270` / `transform == identity`
> `combined == hflip` / `transform == transpose`
> `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

(What document is that? It wasn't immediately clear to me...)

> transformation accordingly. Would you agree that this should be the desired outcome? I.e. should we stick to the proposed value?

I looked back over this again, and found Jacopo's description of the
V4L2_CID_CAMERA_SENSOR_ROTATION control
(https://patchwork.kernel.org/project/linux-media/patch/20191204091056.4842-5-jacopo@jmondi.org/
is what I stumbled into)

>From that, and the nice example with the stick person, I deduce that
if your camera's V4L2_CID_CAMERA_SENSOR_ROTATION property says 90
degrees, then the image you capture (if nothing else happens to it)
will look like it has had a 90 degree clockwise rotation performed.
Everyone agree with that?

So in turn, if you have a camera/ISP that can apply no transforms at
all, then the only value for the user transform that will not come
back as adjusted is "rot90", because it looks like it's had a 90
degree clockwise rotation performed (and libcamera transforms count
rotations as being clockwise). Does that also make sense?

So by a slightly arbitrary mixture of conventions, it looks to me as
though the use of the inverse that I queried is indeed wrong, because
we "need camera 90 degree rotation" => "user transform rot90" in this
case.

Does that sound plausible... what do folks think?

Thanks everyone!
David

>
> Best regards,
>
> Robert