[libcamera-devel] [PATCH v5 04/12] libcamera: properties: Make 'Rotation' the mounting rotation

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Sep 01, 2023 at 05:02:07PM +0200, Jacopo Mondi via libcamera-devel wrote:
> Specify in the documentation that properties::Rotation specifies the
> mounting rotation of the camera module. This avoids confusion with the
> image orientation which is instead expressed by
> CameraConfiguration::orientation.
> 
> For this reason, do not compensate the Rotation property when
> initializing the CameraSensor class but report the value of
> V4L2_CID_CAMERA_SENSOR_ROTATION or 0 if the control is not available.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  src/libcamera/camera_sensor.cpp | 11 +----------
>  src/libcamera/property_ids.yaml |  8 ++++----
>  2 files changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 9a033459742f..3ba364c44a40 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -476,20 +476,11 @@ int CameraSensor::initProperties()
>  			rotationTransform_ = Transform::Identity;
>  		}
>  
> -		/*
> -		 * Adjust property::Rotation as validateTransform() compensates
> -		 * for the mounting rotation. However, as a camera sensor can
> -		 * only compensate rotations by applying H/VFlips, only rotation
> -		 * of 180 degrees are automatically compensated. The other valid
> -		 * rotations (Rot90 and Rot270) require transposition, which the
> -		 * camera sensor cannot perform, so leave them untouched.
> -		 */
> -		if (propertyValue == 180 && supportFlips_)
> -			propertyValue = 0;
>  		properties_.set(properties::Rotation, propertyValue);
>  	} else {
>  		LOG(CameraSensor, Warning)
>  			<< "Rotation control not available, default to 0 degrees";
> +		properties_.set(properties::Rotation, propertyValue);

This doesn't seem right, did you mean

		properties_.set(properties::Rotation, 0);

?

To avoid further errors, I think the propertyValue variable should be
made local to the two 'if' branches where it gets used.

>  		rotationTransform_ = Transform::Identity;
>  	}
>  
> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> index 5bddafc29364..fb53081c1bd2 100644
> --- a/src/libcamera/property_ids.yaml
> +++ b/src/libcamera/property_ids.yaml
> @@ -29,10 +29,10 @@ controls:
>    - Rotation:
>        type: int32_t
>        description: |
> -        The camera rotation is expressed as the angular difference in degrees
> -        between two reference systems, one relative to the camera module, and
> -        one defined on the external world scene to be captured when projected
> -        on the image sensor pixel array.
> +        The camera physical mounting rotation, expressed as the angular
> +        difference in degrees between two reference systems, one relative to the
> +        camera module, and one defined on the external world scene to be
> +        captured when projected on the image sensor pixel array.

The longer the sentence, the harder it is to read. Let's break it.

        The camera physical mounting rotation. It is expressed as the angular
        difference in degrees between two reference systems, one relative to the
        camera module, and one defined on the external world scene to be
        captured when projected on the image sensor pixel array.

With these small issues addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  
>          A camera sensor has a 2-dimensional reference system 'Rc' defined by
>          its pixel array read-out order. The origin is set to the first pixel

-- 
Regards,

Laurent Pinchart