[libcamera-devel] libcamera: camera_sensor: Adjust properties::Rotation

18734 diff mbox series

As the CameraSensor::validateTransform() function compensate
for the sensor's mounting rotation, the properties::Rotation value
should be adjusted to make sure application that receive already
"corrected" images do not get confused by Rotation still reporting
a value.

However, as an image sensor can only compensate rotations by applying
H/V flips, only correct Rotation when the mounting rotation is 180
degrees.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 src/libcamera/camera.cpp        |  3 +--
 src/libcamera/camera_sensor.cpp | 21 ++++++++++++++++++---
 2 files changed, 19 insertions(+), 5 deletions(-)

--
2.40.1

Hi Jacopo,

Thank you for the patch.

On Wed, 14 Jun 2023 at 12:00, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> As the CameraSensor::validateTransform() function compensate
> for the sensor's mounting rotation, the properties::Rotation value
> should be adjusted to make sure application that receive already
> "corrected" images do not get confused by Rotation still reporting
> a value.
>
> However, as an image sensor can only compensate rotations by applying
> H/V flips, only correct Rotation when the mounting rotation is 180
> degrees.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

This behaviour seems reasonable to me.

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>

> ---
>  src/libcamera/camera.cpp        |  3 +--
>  src/libcamera/camera_sensor.cpp | 21 ++++++++++++++++++---
>  2 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 99683e498697..3e252f3b8f8d 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -397,8 +397,7 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
>   *
>   * 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 application.
>   *
>   * The usual 2D plane transforms are allowed here (horizontal/vertical
>   * flips, multiple of 90-degree rotations etc.), but the validate() function
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 60bf87b49e6e..f3a5aa37149f 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -461,7 +461,17 @@ int CameraSensor::initProperties()
>
>         const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);
>         if (rotationControl != controls.end()) {
> +               /*
> +                * 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.
> +                */
>                 propertyValue = rotationControl->second.def().get<int32_t>();
> +               if (propertyValue == 180 && supportFlips_)
> +                       propertyValue = 0;
>                 properties_.set(properties::Rotation, propertyValue);
>         }
>
> @@ -1028,10 +1038,15 @@ void CameraSensor::updateControlInfo()
>   */
>  Transform CameraSensor::validateTransform(Transform *transform) const
>  {
> -       /* Adjust the requested transform to compensate the sensor rotation. */
> -       int32_t rotation = properties().get(properties::Rotation).value_or(0);
> -       bool success;
> +       /* Adjust the requested transform to compensate the sensor mounting rotation. */
> +       const ControlInfoMap &controls = subdev_->controls();
> +       int rotation = 0;
>
> +       const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);
> +       if (rotationControl != controls.end())
> +               rotation = rotationControl->second.def().get<int32_t>();
> +
> +       bool success;
>         Transform rotationTransform = transformFromRotation(rotation, &success);
>         if (!success)
>                 LOG(CameraSensor, Warning) << "Invalid rotation of " << rotation
> --
> 2.40.1
>

Hi Jacopo,

Thank you for the patch.

On 6/14/23 4:29 PM, Jacopo Mondi via libcamera-devel wrote:
> As the CameraSensor::validateTransform() function compensate
> for the sensor's mounting rotation, the properties::Rotation value
> should be adjusted to make sure application that receive already
> "corrected" images do not get confused by Rotation still reporting
> a value.
>
> However, as an image sensor can only compensate rotations by applying
> H/V flips, only correct Rotation when the mounting rotation is 180
> degrees.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> ---
>   src/libcamera/camera.cpp        |  3 +--
>   src/libcamera/camera_sensor.cpp | 21 ++++++++++++++++++---
>   2 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 99683e498697..3e252f3b8f8d 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -397,8 +397,7 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
>    *
>    * 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 application.
>    *
>    * The usual 2D plane transforms are allowed here (horizontal/vertical
>    * flips, multiple of 90-degree rotations etc.), but the validate() function
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 60bf87b49e6e..f3a5aa37149f 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -461,7 +461,17 @@ int CameraSensor::initProperties()
>
>   	const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);
>   	if (rotationControl != controls.end()) {
> +		/*
> +		 * 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.
> +		 */
>   		propertyValue = rotationControl->second.def().get<int32_t>();
> +		if (propertyValue == 180 && supportFlips_)
> +			propertyValue = 0;
>   		properties_.set(properties::Rotation, propertyValue);
>   	}
>
> @@ -1028,10 +1038,15 @@ void CameraSensor::updateControlInfo()
>    */
>   Transform CameraSensor::validateTransform(Transform *transform) const
>   {
> -	/* Adjust the requested transform to compensate the sensor rotation. */
> -	int32_t rotation = properties().get(properties::Rotation).value_or(0);
> -	bool success;
> +	/* Adjust the requested transform to compensate the sensor mounting rotation. */
> +	const ControlInfoMap &controls = subdev_->controls();
> +	int rotation = 0;
>
> +	const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);
> +	if (rotationControl != controls.end())
> +		rotation = rotationControl->second.def().get<int32_t>();
> +
> +	bool success;
>   	Transform rotationTransform = transformFromRotation(rotation, &success);
>   	if (!success)
>   		LOG(CameraSensor, Warning) << "Invalid rotation of " << rotation
> --
> 2.40.1
>

Hi Jacopo,

Thank you for the patch.

On Wed, Jun 14, 2023 at 12:59:57PM +0200, Jacopo Mondi via libcamera-devel wrote:
> As the CameraSensor::validateTransform() function compensate
> for the sensor's mounting rotation, the properties::Rotation value
> should be adjusted to make sure application that receive already
> "corrected" images do not get confused by Rotation still reporting
> a value.
> 
> However, as an image sensor can only compensate rotations by applying
> H/V flips, only correct Rotation when the mounting rotation is 180
> degrees.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  src/libcamera/camera.cpp        |  3 +--
>  src/libcamera/camera_sensor.cpp | 21 ++++++++++++++++++---
>  2 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 99683e498697..3e252f3b8f8d 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -397,8 +397,7 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
>   *
>   * 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 application.
>   *
>   * The usual 2D plane transforms are allowed here (horizontal/vertical
>   * flips, multiple of 90-degree rotations etc.), but the validate() function
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 60bf87b49e6e..f3a5aa37149f 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -461,7 +461,17 @@ int CameraSensor::initProperties()
> 
>  	const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);
>  	if (rotationControl != controls.end()) {
> +		/*
> +		 * 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.
> +		 */
>  		propertyValue = rotationControl->second.def().get<int32_t>();
> +		if (propertyValue == 180 && supportFlips_)
> +			propertyValue = 0;
>  		properties_.set(properties::Rotation, propertyValue);
>  	}
> 
> @@ -1028,10 +1038,15 @@ void CameraSensor::updateControlInfo()
>   */
>  Transform CameraSensor::validateTransform(Transform *transform) const
>  {
> -	/* Adjust the requested transform to compensate the sensor rotation. */
> -	int32_t rotation = properties().get(properties::Rotation).value_or(0);
> -	bool success;
> +	/* Adjust the requested transform to compensate the sensor mounting rotation. */
> +	const ControlInfoMap &controls = subdev_->controls();
> +	int rotation = 0;
> 
> +	const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);

How about caching the rotation value in the CameraSensor class instead
of looking it up every time ? You could actually even cache the value of
rotationTransform, simplifying the code in this function.

> +	if (rotationControl != controls.end())
> +		rotation = rotationControl->second.def().get<int32_t>();
> +
> +	bool success;
>  	Transform rotationTransform = transformFromRotation(rotation, &success);
>  	if (!success)
>  		LOG(CameraSensor, Warning) << "Invalid rotation of " << rotation

Hi Laurent

   on top eventually as the patch has been pushed (by me) :)

On Mon, Jun 19, 2023 at 03:00:51PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Jun 14, 2023 at 12:59:57PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > As the CameraSensor::validateTransform() function compensate
> > for the sensor's mounting rotation, the properties::Rotation value
> > should be adjusted to make sure application that receive already
> > "corrected" images do not get confused by Rotation still reporting
> > a value.
> >
> > However, as an image sensor can only compensate rotations by applying
> > H/V flips, only correct Rotation when the mounting rotation is 180
> > degrees.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  src/libcamera/camera.cpp        |  3 +--
> >  src/libcamera/camera_sensor.cpp | 21 ++++++++++++++++++---
> >  2 files changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 99683e498697..3e252f3b8f8d 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -397,8 +397,7 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
> >   *
> >   * 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 application.
> >   *
> >   * The usual 2D plane transforms are allowed here (horizontal/vertical
> >   * flips, multiple of 90-degree rotations etc.), but the validate() function
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 60bf87b49e6e..f3a5aa37149f 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -461,7 +461,17 @@ int CameraSensor::initProperties()
> >
> >  	const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);
> >  	if (rotationControl != controls.end()) {
> > +		/*
> > +		 * 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.
> > +		 */
> >  		propertyValue = rotationControl->second.def().get<int32_t>();
> > +		if (propertyValue == 180 && supportFlips_)
> > +			propertyValue = 0;
> >  		properties_.set(properties::Rotation, propertyValue);
> >  	}
> >
> > @@ -1028,10 +1038,15 @@ void CameraSensor::updateControlInfo()
> >   */
> >  Transform CameraSensor::validateTransform(Transform *transform) const
> >  {
> > -	/* Adjust the requested transform to compensate the sensor rotation. */
> > -	int32_t rotation = properties().get(properties::Rotation).value_or(0);
> > -	bool success;
> > +	/* Adjust the requested transform to compensate the sensor mounting rotation. */
> > +	const ControlInfoMap &controls = subdev_->controls();
> > +	int rotation = 0;
> >
> > +	const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);
>
> How about caching the rotation value in the CameraSensor class instead
> of looking it up every time ? You could actually even cache the value of
> rotationTransform, simplifying the code in this function.
>
> > +	if (rotationControl != controls.end())
> > +		rotation = rotationControl->second.def().get<int32_t>();
> > +
> > +	bool success;
> >  	Transform rotationTransform = transformFromRotation(rotation, &success);
> >  	if (!success)
> >  		LOG(CameraSensor, Warning) << "Invalid rotation of " << rotation
>
> --
> Regards,
>
> Laurent Pinchart