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

[Jacopo Mondi]

Hi again,

On Wed, Dec 07, 2022 at 03:48:40PM +0000, Robert Mader via libcamera-devel wrote:
> On 07.12.22 15:28, David Plowman via libcamera-devel wrote:
> > Hi Robert
> >
> > Thanks for prodding us all on this question!
>
> Hi, thanks for the quick reply! And a pleasure :P
>
> > 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)
>
> The current kernel docu can be found at
>
> https://github.com/torvalds/linux/blob/8ed710da2873c2aeb3bb805864a699affaf1d03b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>
> the libcamera one at
>
> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/transform.cpp#n54
>
> >  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?
> Yep, agreed. That would mean V4L2_CID_CAMERA_SENSOR_ROTATION==90 would
> translate to Transform::Rot90.

Let's here put some definitions in place

V4L2_CID_CAMERA_SENSOR_ROTATION
This read-only control describes the rotation correction in degrees in
the counter-clockwise direction to be applied to the captured images
once captured to memory to compensate for the camera sensor mounting
rotation.

 \var Transform::Rot270
 Rotation by 270 degrees clockwise (90 degrees anticlockwise).

 Let's start by saying we did a sub-optimal (to be nice) job in
 having Transform and V4L2_CID_ROTATION operate in two different
 directions. Before the usage of Transform in libcamera gets widely
 adopted sould we align the two ?

 Hence if I'm not mistaken, if your camera has Rotation=90 and an
 application supplies Transform::Identity in
 CameraConfiguration::transform what you should get back is Rot270

 Correct ?

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

It might be the case today, but I wonder if that's desirable.

If an application supplies CameraConfiguration::transform = Identity
it means it doesn't want any rotation applied by the library. It will
get back 270 to indicate that that's how the image is rotated and will
get an Adjusted configuration.

If instead an application supplies CameraConfiguration::transform = Rot270
and your camera -cannot- do any rotation, it will still receive back
Rot270 and the state is Valid.

If it asks for CameraConfiguration::transform = Rot270 and the camera
-can- do that, it will clear CameraConfiguration::transform to
Identity and return Adjusted ?

Is this what happens (I'm looking at the above code with David's
suggestion to use the inverse of roationTransform).


> >
> > 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?
>
> Ah, I think the question is whether the adjusted value means: "this is what
> you'll get" vs "this is what you have to do yourself".
>
> Above I argued in favor of the later, but I guess the first one is what the
> API is supposed to mean. That would imply that the client has compute the
> difference between requested and adjusted value itself.

I always presumed the latter was meant to happen..

>
> In my case that would mean:
>
> requested CameraConfiguration::transform: Transform::Identity,
> V4L2_CID_CAMERA_SENSOR_ROTATION: 270, adjusted
> CameraConfiguration::transform: Transform::Rot270
>

I guess it's Rot90 because of the clockwise/counter-clockwise issue ?

> To compute what the client has to do, it would need to do:
> Transform::Identity - Transform::Rot270 = Transform::Rot90. And then apply
> that, a 90 degr. rotation clockwise.
>
> Does that sound reasonable? In that case the patch here should indeed be
> `*transform = rotationTransform;`, without the inverse.
>

        /0\

before we plumb this into and make assumptions that will be then
set into stone in our adaption layers, can we take a step back and
define what kind of API we would like to have ?

Let's start by the definition of CameraConfiguration::transform.

It's a member of CameraConfiguration, and as by definition it is
filled-in by the application and eventually adjusted by the Camera to
a value that's acceptable/doable for the current configuration.

1) Application to Camera  = (counter?)clockwise degrees the camera
system should rotate the image before presenting it to the user

2) Camera to application = the rotation the camera can actually do
(ie. only sensor flips == no Transpose)

The current documentation doesn't explain how it gets adjusted

/**
 * \var CameraConfiguration::transform
 * \brief User-specified transform to be applied to the image
 *
 * The transform is a user-specified 2D plane transform that will be applied
 * to the camera images by the processing pipeline before being handed to
 * the application. This is subsequent to any transform that is already
 * required to fix up any platform-defined rotation.
 *
 * The usual 2D plane transforms are allowed here (horizontal/vertical
 * flips, multiple of 90-degree rotations etc.), but the validate() function
 * may adjust this field at its discretion if the selection is not supported.
 */

The last part in particular, "at its discrection" it's really too
generic.

Let's start from the beginning: what application should use transform
for ? I presume they should:

1) Inspect propertis::Rotation
2) Correct the image for the properties::Rotation amount (actually,
the inverse because of the clockwise/counter-clockwise thing)
3) Add any additional rotation they would like on top of this

How should transform be adjusted ?

Identity:
If application->identity the camera does not have to apply any
transform. Should the transform be adjutes like it happens today to
compensate the Rotation (at least that's what I presume happens) ? I
presume no, applications can infer rotation from properties and if they
chose Identity either other layers above will do the transform or
they're fine with images as they are.

A supported/non-supported transform:
An application asks for a VFLIP, the camera can do it, transform is
not changed. If the camera cannot flip, the transform is set back to
Identity and the state is adjusted.

A partially non-supported transform:
An application asks for Rot270 (Transpose | HFLIP) and the camera can
only do HFLIP. transform sould be then set to HFLIP and applications
know that the remaining transformation should be handled elsewhere.

Are we missing anything with such behaviour ? I presume it's rather
similar to what happens today if not for the fact that the validate()
implementation adjusts transform to rotation in case it cannot flip

        Transform combined = transform * data_->rotationTransform_;
        if (!data_->supportsFlips_ && !!combined)
                transform = -data_->rotationTransform_;

Not really sure I got why.

All of this, but an even more fundamental question: is it too late/is
it worh it to adjust the Transform definition to adopt the same
direction as the kernel defined rotation ?

Sorry for the long email :/

> > Thanks everyone!
> > David
>
> Thanks!
>
> Robert
>