[libcamera-devel,v2] libcamera: Remove transform from V4L2SubdeviceFormat
diff mbox series

Message ID 20230207075210.3789-1-jacopo.mondi@ideasonboard.com
State Accepted
Headers show
Series
  • [libcamera-devel,v2] libcamera: Remove transform from V4L2SubdeviceFormat
Related show

Commit Message

Jacopo Mondi Feb. 7, 2023, 7:52 a.m. UTC 
Commit 6f6e1bf704fe ("libcamera: camera_sensor: Apply flips at
setFormat()") extended the CameraSensor::setFormat() function
to apply vertical/horizontal flips on the sensor based on the
supplied Transform. To pass the Transform to the function the
V4L2SubdeviceFormat structure has been augmented with a Transform
member.

However as the newly added Transform is not used at all in the
V4L2Subdevice class, it should not be part of V4L2SubdeviceFormat.

Fix that by removing the transform field from V4L2SubdeviceFormat
and pass it as an explicit parameter to CameraSensor::setFormat().

Fixes: 6f6e1bf704fe ("libcamera: camera_sensor: Apply flips at setFormat())
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---

Sending a v2 instead of applying it directly as there was a leftover
forward declaration of

	enum class Transform;

in camera_sensor.h which I have now removed since I'm now including the full
header flie.

---
 include/libcamera/internal/camera_sensor.h         |  6 +++---
 include/libcamera/internal/v4l2_subdevice.h        |  2 --
 src/libcamera/camera_sensor.cpp                    | 11 ++++-------
 src/libcamera/pipeline/ipu3/cio2.cpp               |  3 +--
 src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 11 ++++++-----
 src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  6 ++++--
 src/libcamera/v4l2_subdevice.cpp                   |  7 -------
 7 files changed, 18 insertions(+), 28 deletions(-)

--
2.39.0

Comments

Laurent Pinchart Feb. 7, 2023, 9:57 a.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Tue, Feb 07, 2023 at 08:52:09AM +0100, Jacopo Mondi wrote:
> Commit 6f6e1bf704fe ("libcamera: camera_sensor: Apply flips at
> setFormat()") extended the CameraSensor::setFormat() function
> to apply vertical/horizontal flips on the sensor based on the
> supplied Transform. To pass the Transform to the function the
> V4L2SubdeviceFormat structure has been augmented with a Transform
> member.
> 
> However as the newly added Transform is not used at all in the
> V4L2Subdevice class, it should not be part of V4L2SubdeviceFormat.
> 
> Fix that by removing the transform field from V4L2SubdeviceFormat
> and pass it as an explicit parameter to CameraSensor::setFormat().
> 
> Fixes: 6f6e1bf704fe ("libcamera: camera_sensor: Apply flips at setFormat())
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> 
> Sending a v2 instead of applying it directly as there was a leftover
> forward declaration of
> 
> 	enum class Transform;
> 
> in camera_sensor.h which I have now removed since I'm now including the full
> header flie.
> 
> ---
>  include/libcamera/internal/camera_sensor.h         |  6 +++---
>  include/libcamera/internal/v4l2_subdevice.h        |  2 --
>  src/libcamera/camera_sensor.cpp                    | 11 ++++-------
>  src/libcamera/pipeline/ipu3/cio2.cpp               |  3 +--
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 11 ++++++-----
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  6 ++++--
>  src/libcamera/v4l2_subdevice.cpp                   |  7 -------
>  7 files changed, 18 insertions(+), 28 deletions(-)
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index bea52badaff7..a8d980e3b439 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -17,6 +17,7 @@
>  #include <libcamera/control_ids.h>
>  #include <libcamera/controls.h>
>  #include <libcamera/geometry.h>
> +#include <libcamera/transform.h>
> 
>  #include <libcamera/ipa/core_ipa_interface.h>
> 
> @@ -29,8 +30,6 @@ class BayerFormat;
>  class CameraLens;
>  class MediaEntity;
> 
> -enum class Transform;
> -
>  struct CameraSensorProperties;
> 
>  class CameraSensor : protected Loggable
> @@ -55,7 +54,8 @@ public:
> 
>  	V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
>  				      const Size &size) const;
> -	int setFormat(V4L2SubdeviceFormat *format);
> +	int setFormat(V4L2SubdeviceFormat *format,
> +		      const Transform transform = Transform::Identity);

You could skip const.

> 
>  	const ControlInfoMap &controls() const;
>  	ControlList getControls(const std::vector<uint32_t> &ids);
> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> index 576faf971a05..69862de0585a 100644
> --- a/include/libcamera/internal/v4l2_subdevice.h
> +++ b/include/libcamera/internal/v4l2_subdevice.h
> @@ -20,7 +20,6 @@
> 
>  #include <libcamera/color_space.h>
>  #include <libcamera/geometry.h>
> -#include <libcamera/transform.h>
> 
>  #include "libcamera/internal/formats.h"
>  #include "libcamera/internal/media_object.h"
> @@ -45,7 +44,6 @@ struct V4L2SubdeviceFormat {
>  	uint32_t mbus_code;
>  	Size size;
>  	std::optional<ColorSpace> colorSpace;
> -	Transform transform = Transform::Identity;
> 
>  	const std::string toString() const;
>  	uint8_t bitsPerPixel() const;
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 274ed419ddfd..19f0ce6a47bb 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -16,7 +16,6 @@
>  #include <string.h>
> 
>  #include <libcamera/property_ids.h>
> -#include <libcamera/transform.h>
> 
>  #include <libcamera/base/utils.h>
> 
> @@ -751,7 +750,6 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbu
>  		.mbus_code = bestCode,
>  		.size = *bestSize,
>  		.colorSpace = ColorSpace::Raw,
> -		.transform = Transform::Identity,
>  	};
> 
>  	return format;
> @@ -760,6 +758,7 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbu
>  /**
>   * \brief Set the sensor output format
>   * \param[in] format The desired sensor output format
> + * \param[in] transform The transform to be applied on the sensor. Defaults to Identity.

Line wrap ?

>   *
>   * If flips are writable they are configured according to the desired Transform.
>   * Transform::Identity always corresponds to H/V flip being disabled if the
> @@ -770,18 +769,16 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbu
>   *
>   * \return 0 on success or a negative error code otherwise
>   */
> -int CameraSensor::setFormat(V4L2SubdeviceFormat *format)
> +int CameraSensor::setFormat(V4L2SubdeviceFormat *format, const Transform transform)
>  {
>  	/* Configure flips if the sensor supports that. */
>  	if (supportFlips_) {
>  		ControlList flipCtrls(subdev_->controls());
> 
>  		flipCtrls.set(V4L2_CID_HFLIP,
> -			      static_cast<int32_t>(!!(format->transform &
> -						      Transform::HFlip)));
> +			      static_cast<int32_t>(!!(transform & Transform::HFlip)));
>  		flipCtrls.set(V4L2_CID_VFLIP,
> -			      static_cast<int32_t>(!!(format->transform &
> -						      Transform::VFlip)));
> +			      static_cast<int32_t>(!!(transform & Transform::VFlip)));
> 
>  		int ret = subdev_->setControls(&flipCtrls);
>  		if (ret)
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index a819884f762d..7400cb0b644c 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -194,8 +194,7 @@ int CIO2Device::configure(const Size &size, const Transform &transform,
>  	 */
>  	std::vector<unsigned int> mbusCodes = utils::map_keys(mbusCodesToPixelFormat);
>  	sensorFormat = getSensorFormat(mbusCodes, size);
> -	sensorFormat.transform = transform;
> -	ret = sensor_->setFormat(&sensorFormat);
> +	ret = sensor_->setFormat(&sensorFormat, transform);
>  	if (ret)
>  		return ret;
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 77e860ab0e72..c0dd95513583 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -832,13 +832,14 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  		}
>  	}
> 
> -	/* First calculate the best sensor mode we can use based on the user request. */
> +	/*
> +	 * Calculate the best sensor mode we can use based on the user's
> +	 * request, and apply it to the sensor with the cached transform, if
> +	 * any.
> +	 */
>  	V4L2SubdeviceFormat sensorFormat = findBestFormat(data->sensorFormats_, rawStream ? sensorSize : maxSize, bitDepth);
> -	/* Apply any cached transform. */
>  	const RPiCameraConfiguration *rpiConfig = static_cast<const RPiCameraConfiguration *>(config);
> -	sensorFormat.transform = rpiConfig->combinedTransform_;
> -	/* Finally apply the format on the sensor. */
> -	ret = data->sensor_->setFormat(&sensorFormat);
> +	ret = data->sensor_->setFormat(&sensorFormat, rpiConfig->combinedTransform_);
>  	if (ret)
>  		return ret;
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 5f22a29d02c6..8a30fe061d04 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -125,6 +125,7 @@ public:
>  	Status validate() override;
> 
>  	const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
> +	const Transform &combinedTransform() { return combinedTransform_; }
> 
>  private:
>  	bool fitsAllPaths(const StreamConfiguration &cfg);
> @@ -138,6 +139,7 @@ private:
>  	const RkISP1CameraData *data_;
> 
>  	V4L2SubdeviceFormat sensorFormat_;
> +	Transform combinedTransform_;
>  };
> 
>  class PipelineHandlerRkISP1 : public PipelineHandler
> @@ -591,7 +593,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  	if (sensorFormat_.size.isNull())
>  		sensorFormat_.size = sensor->resolution();
> 
> -	sensorFormat_.transform = combined;
> +	combinedTransform_ = combined;
> 
>  	return status;
>  }
> @@ -720,7 +722,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  	V4L2SubdeviceFormat format = config->sensorFormat();
>  	LOG(RkISP1, Debug) << "Configuring sensor with " << format;
> 
> -	ret = sensor->setFormat(&format);
> +	ret = sensor->setFormat(&format, config->combinedTransform());
>  	if (ret < 0)
>  		return ret;
> 
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 38ff8b0c605b..15e8206a915c 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -216,13 +216,6 @@ const std::map<uint32_t, V4L2SubdeviceFormatInfo> formatInfoMap = {
>   * resulting color space is acceptable.
>   */
> 
> -/**
> - * \var V4L2SubdeviceFormat::transform
> - * \brief The transform (vertical/horizontal flips) to be applied on the subdev
> - *
> - * Default initialized to Identity (no transform).
> - */
> -
>  /**
>   * \brief Assemble and return a string describing the format
>   * \return A string describing the V4L2SubdeviceFormat

Patch
diff mbox series 

diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
index bea52badaff7..a8d980e3b439 100644
--- a/include/libcamera/internal/camera_sensor.h
+++ b/include/libcamera/internal/camera_sensor.h
@@ -17,6 +17,7 @@ 
 #include <libcamera/control_ids.h>
 #include <libcamera/controls.h>
 #include <libcamera/geometry.h>
+#include <libcamera/transform.h>

 #include <libcamera/ipa/core_ipa_interface.h>

@@ -29,8 +30,6 @@  class BayerFormat;
 class CameraLens;
 class MediaEntity;

-enum class Transform;
-
 struct CameraSensorProperties;

 class CameraSensor : protected Loggable
@@ -55,7 +54,8 @@  public:

 	V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
 				      const Size &size) const;
-	int setFormat(V4L2SubdeviceFormat *format);
+	int setFormat(V4L2SubdeviceFormat *format,
+		      const Transform transform = Transform::Identity);

 	const ControlInfoMap &controls() const;
 	ControlList getControls(const std::vector<uint32_t> &ids);
diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
index 576faf971a05..69862de0585a 100644
--- a/include/libcamera/internal/v4l2_subdevice.h
+++ b/include/libcamera/internal/v4l2_subdevice.h
@@ -20,7 +20,6 @@ 

 #include <libcamera/color_space.h>
 #include <libcamera/geometry.h>
-#include <libcamera/transform.h>

 #include "libcamera/internal/formats.h"
 #include "libcamera/internal/media_object.h"
@@ -45,7 +44,6 @@  struct V4L2SubdeviceFormat {
 	uint32_t mbus_code;
 	Size size;
 	std::optional<ColorSpace> colorSpace;
-	Transform transform = Transform::Identity;

 	const std::string toString() const;
 	uint8_t bitsPerPixel() const;
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
index 274ed419ddfd..19f0ce6a47bb 100644
--- a/src/libcamera/camera_sensor.cpp
+++ b/src/libcamera/camera_sensor.cpp
@@ -16,7 +16,6 @@ 
 #include <string.h>

 #include <libcamera/property_ids.h>
-#include <libcamera/transform.h>

 #include <libcamera/base/utils.h>

@@ -751,7 +750,6 @@  V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbu
 		.mbus_code = bestCode,
 		.size = *bestSize,
 		.colorSpace = ColorSpace::Raw,
-		.transform = Transform::Identity,
 	};

 	return format;
@@ -760,6 +758,7 @@  V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbu
 /**
  * \brief Set the sensor output format
  * \param[in] format The desired sensor output format
+ * \param[in] transform The transform to be applied on the sensor. Defaults to Identity.
  *
  * If flips are writable they are configured according to the desired Transform.
  * Transform::Identity always corresponds to H/V flip being disabled if the
@@ -770,18 +769,16 @@  V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbu
  *
  * \return 0 on success or a negative error code otherwise
  */
-int CameraSensor::setFormat(V4L2SubdeviceFormat *format)
+int CameraSensor::setFormat(V4L2SubdeviceFormat *format, const Transform transform)
 {
 	/* Configure flips if the sensor supports that. */
 	if (supportFlips_) {
 		ControlList flipCtrls(subdev_->controls());

 		flipCtrls.set(V4L2_CID_HFLIP,
-			      static_cast<int32_t>(!!(format->transform &
-						      Transform::HFlip)));
+			      static_cast<int32_t>(!!(transform & Transform::HFlip)));
 		flipCtrls.set(V4L2_CID_VFLIP,
-			      static_cast<int32_t>(!!(format->transform &
-						      Transform::VFlip)));
+			      static_cast<int32_t>(!!(transform & Transform::VFlip)));

 		int ret = subdev_->setControls(&flipCtrls);
 		if (ret)
diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
index a819884f762d..7400cb0b644c 100644
--- a/src/libcamera/pipeline/ipu3/cio2.cpp
+++ b/src/libcamera/pipeline/ipu3/cio2.cpp
@@ -194,8 +194,7 @@  int CIO2Device::configure(const Size &size, const Transform &transform,
 	 */
 	std::vector<unsigned int> mbusCodes = utils::map_keys(mbusCodesToPixelFormat);
 	sensorFormat = getSensorFormat(mbusCodes, size);
-	sensorFormat.transform = transform;
-	ret = sensor_->setFormat(&sensorFormat);
+	ret = sensor_->setFormat(&sensorFormat, transform);
 	if (ret)
 		return ret;

diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index 77e860ab0e72..c0dd95513583 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -832,13 +832,14 @@  int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
 		}
 	}

-	/* First calculate the best sensor mode we can use based on the user request. */
+	/*
+	 * Calculate the best sensor mode we can use based on the user's
+	 * request, and apply it to the sensor with the cached transform, if
+	 * any.
+	 */
 	V4L2SubdeviceFormat sensorFormat = findBestFormat(data->sensorFormats_, rawStream ? sensorSize : maxSize, bitDepth);
-	/* Apply any cached transform. */
 	const RPiCameraConfiguration *rpiConfig = static_cast<const RPiCameraConfiguration *>(config);
-	sensorFormat.transform = rpiConfig->combinedTransform_;
-	/* Finally apply the format on the sensor. */
-	ret = data->sensor_->setFormat(&sensorFormat);
+	ret = data->sensor_->setFormat(&sensorFormat, rpiConfig->combinedTransform_);
 	if (ret)
 		return ret;

diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 5f22a29d02c6..8a30fe061d04 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -125,6 +125,7 @@  public:
 	Status validate() override;

 	const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
+	const Transform &combinedTransform() { return combinedTransform_; }

 private:
 	bool fitsAllPaths(const StreamConfiguration &cfg);
@@ -138,6 +139,7 @@  private:
 	const RkISP1CameraData *data_;

 	V4L2SubdeviceFormat sensorFormat_;
+	Transform combinedTransform_;
 };

 class PipelineHandlerRkISP1 : public PipelineHandler
@@ -591,7 +593,7 @@  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
 	if (sensorFormat_.size.isNull())
 		sensorFormat_.size = sensor->resolution();

-	sensorFormat_.transform = combined;
+	combinedTransform_ = combined;

 	return status;
 }
@@ -720,7 +722,7 @@  int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
 	V4L2SubdeviceFormat format = config->sensorFormat();
 	LOG(RkISP1, Debug) << "Configuring sensor with " << format;

-	ret = sensor->setFormat(&format);
+	ret = sensor->setFormat(&format, config->combinedTransform());
 	if (ret < 0)
 		return ret;

diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
index 38ff8b0c605b..15e8206a915c 100644
--- a/src/libcamera/v4l2_subdevice.cpp
+++ b/src/libcamera/v4l2_subdevice.cpp
@@ -216,13 +216,6 @@  const std::map<uint32_t, V4L2SubdeviceFormatInfo> formatInfoMap = {
  * resulting color space is acceptable.
  */

-/**
- * \var V4L2SubdeviceFormat::transform
- * \brief The transform (vertical/horizontal flips) to be applied on the subdev
- *
- * Default initialized to Identity (no transform).
- */
-
 /**
  * \brief Assemble and return a string describing the format
  * \return A string describing the V4L2SubdeviceFormat