[libcamera-devel] [PATCH v2] libcamera: Remove transform from V4L2SubdeviceFormat
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Feb 7 10:57:38 CET 2023
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 at ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at 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
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list