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

[Robert Mader]

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

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.

In my case that would mean:

requested CameraConfiguration::transform: Transform::Identity, 
V4L2_CID_CAMERA_SENSOR_ROTATION: 270, adjusted 
CameraConfiguration::transform: Transform::Rot270

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.

> Thanks everyone!
> David

Thanks!

Robert