[libcamera-devel,RFC,1/4] libcamera: camera_sensor: Cache rotationTransform_
diff mbox series

Message ID 20230620142904.72600-2-jacopo.mondi@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: Replace CameraConfiguration::transform
Related show

Commit Message

Jacopo Mondi June 20, 2023, 2:29 p.m. UTC 
The rotationTransform_ depends on a V4L2 control whose value does not
change for the whole lifetime of the camera.

Instead of re-calculating it everytime the camera is configured, cache
it at properties initialization time.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 include/libcamera/internal/camera_sensor.h |  1 +
 src/libcamera/camera_sensor.cpp            | 54 +++++++++++++---------
 2 files changed, 32 insertions(+), 23 deletions(-)

Comments

Umang Jain June 21, 2023, 5:21 a.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On 6/20/23 7:59 PM, Jacopo Mondi via libcamera-devel wrote:
> The rotationTransform_ depends on a V4L2 control whose value does not
> change for the whole lifetime of the camera.
>
> Instead of re-calculating it everytime the camera is configured, cache
> it at properties initialization time.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>   include/libcamera/internal/camera_sensor.h |  1 +
>   src/libcamera/camera_sensor.cpp            | 54 +++++++++++++---------
>   2 files changed, 32 insertions(+), 23 deletions(-)
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 02c77ab037da..02b4b4d25e6d 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -107,6 +107,7 @@ private:
>   	Rectangle activeArea_;
>   	const BayerFormat *bayerFormat_;
>   	bool supportFlips_;
> +	Transform rotationTransform_;
>   
>   	ControlList properties_;
>   
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index f3a5aa37149f..a15a6c89c5c8 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -461,18 +461,36 @@ int CameraSensor::initProperties()
>   
>   	const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);
>   	if (rotationControl != controls.end()) {
> +		propertyValue = rotationControl->second.def().get<int32_t>();
> +
>   		/*
> -		 * 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.
> +		 * Cache the Transform associated with the camera mounting
> +		 * rotation for later use in validateTransform().
> +		 */
> +		bool success;
> +		rotationTransform_ = transformFromRotation(propertyValue, &success);

Probably a good time to convert to std::optional<> here? Or it can be 
done on top. Either way I don't mind, as long as it happens ;-)

Other than that, the code looks fine to me.

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

> +		if (!success) {
> +			LOG(CameraSensor, Warning)
> +				<< "Invalid rotation of " << propertyValue
> +				<< " degrees - ignoring";
> +			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.
>   		 */
> -		propertyValue = rotationControl->second.def().get<int32_t>();
>   		if (propertyValue == 180 && supportFlips_)
>   			propertyValue = 0;
>   		properties_.set(properties::Rotation, propertyValue);
> +	} else {
> +		LOG(CameraSensor, Warning)
> +			<< "Rotation control not available, default to 0 degreees";
> +		rotationTransform_ = Transform::Identity;
>   	}
>   
>   	properties_.set(properties::PixelArraySize, pixelArraySize_);
> @@ -1038,21 +1056,11 @@ void CameraSensor::updateControlInfo()
>    */
>   Transform CameraSensor::validateTransform(Transform *transform) const
>   {
> -	/* 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
> -					   << " degrees - ignoring";
> -
> -	Transform combined = *transform * rotationTransform;
> +	/*
> +	 * Combine the requested transform to compensate the sensor mounting
> +	 * rotation.
> +	 */
> +	Transform combined = *transform * rotationTransform_;
>   
>   	/*
>   	 * We combine the platform and user transform, but must "adjust away"
> @@ -1083,7 +1091,7 @@ Transform CameraSensor::validateTransform(Transform *transform) const
>   		 * rise to this is the inverse of the rotation. (Recall that
>   		 * combined = transform * rotationTransform.)
>   		 */
> -		*transform = -rotationTransform;
> +		*transform = -rotationTransform_;
>   		combined = Transform::Identity;
>   	}
>
Jacopo Mondi June 21, 2023, 7:40 a.m. UTC | #2 
Hi Umang

On Wed, Jun 21, 2023 at 10:51:33AM +0530, Umang Jain wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On 6/20/23 7:59 PM, Jacopo Mondi via libcamera-devel wrote:
> > The rotationTransform_ depends on a V4L2 control whose value does not
> > change for the whole lifetime of the camera.
> >
> > Instead of re-calculating it everytime the camera is configured, cache
> > it at properties initialization time.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >   include/libcamera/internal/camera_sensor.h |  1 +
> >   src/libcamera/camera_sensor.cpp            | 54 +++++++++++++---------
> >   2 files changed, 32 insertions(+), 23 deletions(-)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index 02c77ab037da..02b4b4d25e6d 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -107,6 +107,7 @@ private:
> >   	Rectangle activeArea_;
> >   	const BayerFormat *bayerFormat_;
> >   	bool supportFlips_;
> > +	Transform rotationTransform_;
> >   	ControlList properties_;
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index f3a5aa37149f..a15a6c89c5c8 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -461,18 +461,36 @@ int CameraSensor::initProperties()
> >   	const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);
> >   	if (rotationControl != controls.end()) {
> > +		propertyValue = rotationControl->second.def().get<int32_t>();
> > +
> >   		/*
> > -		 * 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.
> > +		 * Cache the Transform associated with the camera mounting
> > +		 * rotation for later use in validateTransform().
> > +		 */
> > +		bool success;
> > +		rotationTransform_ = transformFromRotation(propertyValue, &success);
>
> Probably a good time to convert to std::optional<> here? Or it can be done
> on top. Either way I don't mind, as long as it happens ;-)

Would would it give an std::optional<> here ?

In case "nothing" has to be done, transformFromRotation() returns
Identity and pipeline handlers can safely pass it to
CameraSensor::setFormat(). True we can make the optional "transform"
argument of CameraSensor::setFormat() and std::optional<> too...

>
> Other than that, the code looks fine to me.
>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
>
> > +		if (!success) {
> > +			LOG(CameraSensor, Warning)
> > +				<< "Invalid rotation of " << propertyValue
> > +				<< " degrees - ignoring";
> > +			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.
> >   		 */
> > -		propertyValue = rotationControl->second.def().get<int32_t>();
> >   		if (propertyValue == 180 && supportFlips_)
> >   			propertyValue = 0;
> >   		properties_.set(properties::Rotation, propertyValue);
> > +	} else {
> > +		LOG(CameraSensor, Warning)
> > +			<< "Rotation control not available, default to 0 degreees";
> > +		rotationTransform_ = Transform::Identity;
> >   	}
> >   	properties_.set(properties::PixelArraySize, pixelArraySize_);
> > @@ -1038,21 +1056,11 @@ void CameraSensor::updateControlInfo()
> >    */
> >   Transform CameraSensor::validateTransform(Transform *transform) const
> >   {
> > -	/* 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
> > -					   << " degrees - ignoring";
> > -
> > -	Transform combined = *transform * rotationTransform;
> > +	/*
> > +	 * Combine the requested transform to compensate the sensor mounting
> > +	 * rotation.
> > +	 */
> > +	Transform combined = *transform * rotationTransform_;
> >   	/*
> >   	 * We combine the platform and user transform, but must "adjust away"
> > @@ -1083,7 +1091,7 @@ Transform CameraSensor::validateTransform(Transform *transform) const
> >   		 * rise to this is the inverse of the rotation. (Recall that
> >   		 * combined = transform * rotationTransform.)
> >   		 */
> > -		*transform = -rotationTransform;
> > +		*transform = -rotationTransform_;
> >   		combined = Transform::Identity;
> >   	}
>
Laurent Pinchart June 22, 2023, 8:54 a.m. UTC | #3 
Hi Jacopo,

Thank you for the patch.

On Tue, Jun 20, 2023 at 04:29:01PM +0200, Jacopo Mondi via libcamera-devel wrote:
> The rotationTransform_ depends on a V4L2 control whose value does not
> change for the whole lifetime of the camera.
> 
> Instead of re-calculating it everytime the camera is configured, cache
> it at properties initialization time.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  include/libcamera/internal/camera_sensor.h |  1 +
>  src/libcamera/camera_sensor.cpp            | 54 +++++++++++++---------
>  2 files changed, 32 insertions(+), 23 deletions(-)
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 02c77ab037da..02b4b4d25e6d 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -107,6 +107,7 @@ private:
>  	Rectangle activeArea_;
>  	const BayerFormat *bayerFormat_;
>  	bool supportFlips_;
> +	Transform rotationTransform_;
>  
>  	ControlList properties_;
>  
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index f3a5aa37149f..a15a6c89c5c8 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -461,18 +461,36 @@ int CameraSensor::initProperties()
>  
>  	const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);
>  	if (rotationControl != controls.end()) {
> +		propertyValue = rotationControl->second.def().get<int32_t>();
> +
>  		/*
> -		 * 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.
> +		 * Cache the Transform associated with the camera mounting
> +		 * rotation for later use in validateTransform().
> +		 */
> +		bool success;
> +		rotationTransform_ = transformFromRotation(propertyValue, &success);
> +		if (!success) {
> +			LOG(CameraSensor, Warning)
> +				<< "Invalid rotation of " << propertyValue
> +				<< " degrees - ignoring";
> +			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.
>  		 */
> -		propertyValue = rotationControl->second.def().get<int32_t>();
>  		if (propertyValue == 180 && supportFlips_)
>  			propertyValue = 0;
>  		properties_.set(properties::Rotation, propertyValue);
> +	} else {
> +		LOG(CameraSensor, Warning)
> +			<< "Rotation control not available, default to 0 degreees";

I think "degrees" has enough e's without having to add an extra one :-)
With this fixed,

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

> +		rotationTransform_ = Transform::Identity;
>  	}
>  
>  	properties_.set(properties::PixelArraySize, pixelArraySize_);
> @@ -1038,21 +1056,11 @@ void CameraSensor::updateControlInfo()
>   */
>  Transform CameraSensor::validateTransform(Transform *transform) const
>  {
> -	/* 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
> -					   << " degrees - ignoring";
> -
> -	Transform combined = *transform * rotationTransform;
> +	/*
> +	 * Combine the requested transform to compensate the sensor mounting
> +	 * rotation.
> +	 */
> +	Transform combined = *transform * rotationTransform_;
>  
>  	/*
>  	 * We combine the platform and user transform, but must "adjust away"
> @@ -1083,7 +1091,7 @@ Transform CameraSensor::validateTransform(Transform *transform) const
>  		 * rise to this is the inverse of the rotation. (Recall that
>  		 * combined = transform * rotationTransform.)
>  		 */
> -		*transform = -rotationTransform;
> +		*transform = -rotationTransform_;
>  		combined = Transform::Identity;
>  	}
>

Patch
diff mbox series 

diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
index 02c77ab037da..02b4b4d25e6d 100644
--- a/include/libcamera/internal/camera_sensor.h
+++ b/include/libcamera/internal/camera_sensor.h
@@ -107,6 +107,7 @@  private:
 	Rectangle activeArea_;
 	const BayerFormat *bayerFormat_;
 	bool supportFlips_;
+	Transform rotationTransform_;
 
 	ControlList properties_;
 
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
index f3a5aa37149f..a15a6c89c5c8 100644
--- a/src/libcamera/camera_sensor.cpp
+++ b/src/libcamera/camera_sensor.cpp
@@ -461,18 +461,36 @@  int CameraSensor::initProperties()
 
 	const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);
 	if (rotationControl != controls.end()) {
+		propertyValue = rotationControl->second.def().get<int32_t>();
+
 		/*
-		 * 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.
+		 * Cache the Transform associated with the camera mounting
+		 * rotation for later use in validateTransform().
+		 */
+		bool success;
+		rotationTransform_ = transformFromRotation(propertyValue, &success);
+		if (!success) {
+			LOG(CameraSensor, Warning)
+				<< "Invalid rotation of " << propertyValue
+				<< " degrees - ignoring";
+			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.
 		 */
-		propertyValue = rotationControl->second.def().get<int32_t>();
 		if (propertyValue == 180 && supportFlips_)
 			propertyValue = 0;
 		properties_.set(properties::Rotation, propertyValue);
+	} else {
+		LOG(CameraSensor, Warning)
+			<< "Rotation control not available, default to 0 degreees";
+		rotationTransform_ = Transform::Identity;
 	}
 
 	properties_.set(properties::PixelArraySize, pixelArraySize_);
@@ -1038,21 +1056,11 @@  void CameraSensor::updateControlInfo()
  */
 Transform CameraSensor::validateTransform(Transform *transform) const
 {
-	/* 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
-					   << " degrees - ignoring";
-
-	Transform combined = *transform * rotationTransform;
+	/*
+	 * Combine the requested transform to compensate the sensor mounting
+	 * rotation.
+	 */
+	Transform combined = *transform * rotationTransform_;
 
 	/*
 	 * We combine the platform and user transform, but must "adjust away"
@@ -1083,7 +1091,7 @@  Transform CameraSensor::validateTransform(Transform *transform) const
 		 * rise to this is the inverse of the rotation. (Recall that
 		 * combined = transform * rotationTransform.)
 		 */
-		*transform = -rotationTransform;
+		*transform = -rotationTransform_;
 		combined = Transform::Identity;
 	}