[RFC PATCH v1] libcamera: {orientation, transform}FromRotation(): Return `std::optional`
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Mar 17 13:35:36 CET 2025
Quoting Barnabás Pőcze (2025-01-28 08:35:33)
> Return an empty `std::optional` on failure instead of
> using a separate out parameter for signalling success.
>
On the whole, this seems reasonable, I wonder why this was using a
success parameter in the first place? ... But again - I think we have a
public API/ABI breaking change here so we need to start definining a way
to explicitly state that in the commit message.
Any ideas?
I guess we need to define some sort of standard tag like
Breaking: ABI+API
or
ABI: Changed parameter
or something that means we can track this.
I also wonder if we we should have a way to 'accept' API/ABI breaking
changes, but keep them separated until we are ready to make a new
ABI point release...
> Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> ---
> include/libcamera/orientation.h | 3 ++-
> include/libcamera/transform.h | 4 +++-
> src/libcamera/orientation.cpp | 17 ++++-------------
> src/libcamera/sensor/camera_sensor_legacy.cpp | 8 ++++----
> src/libcamera/sensor/camera_sensor_raw.cpp | 8 ++++----
> src/libcamera/transform.cpp | 17 ++++-------------
> src/py/libcamera/py_transform.cpp | 15 +++++++--------
> 7 files changed, 28 insertions(+), 44 deletions(-)
>
> diff --git a/include/libcamera/orientation.h b/include/libcamera/orientation.h
> index a3b40e636..4e5b0f570 100644
> --- a/include/libcamera/orientation.h
> +++ b/include/libcamera/orientation.h
> @@ -8,6 +8,7 @@
> #pragma once
>
> #include <iostream>
> +#include <optional>
>
> namespace libcamera {
>
> @@ -23,7 +24,7 @@ enum class Orientation {
> Rotate90,
> };
>
> -Orientation orientationFromRotation(int angle, bool *success = nullptr);
> +std::optional<Orientation> orientationFromRotation(int angle);
>
> std::ostream &operator<<(std::ostream &out, const Orientation &orientation);
>
> diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h
> index 4517412a8..4769dc2e9 100644
> --- a/include/libcamera/transform.h
> +++ b/include/libcamera/transform.h
> @@ -7,6 +7,8 @@
>
> #pragma once
>
> +#include <optional>
> +
> namespace libcamera {
>
> enum class Orientation;
> @@ -68,7 +70,7 @@ constexpr Transform operator~(Transform t)
> return static_cast<Transform>(~static_cast<int>(t) & 7);
> }
>
> -Transform transformFromRotation(int angle, bool *success = nullptr);
> +std::optional<Transform> transformFromRotation(int angle);
>
> Transform operator/(const Orientation &o1, const Orientation &o2);
> Orientation operator*(const Orientation &o, const Transform &t);
> diff --git a/src/libcamera/orientation.cpp b/src/libcamera/orientation.cpp
> index 7d7d21ae8..9f118f834 100644
> --- a/src/libcamera/orientation.cpp
> +++ b/src/libcamera/orientation.cpp
> @@ -59,21 +59,15 @@ namespace libcamera {
> * \brief Return the orientation representing a rotation of the given angle
> * clockwise
> * \param[in] angle The angle of rotation in a clockwise sense. Negative values
> - * can be used to represent anticlockwise rotations
> - * \param[out] success Set to `true` if the angle is a multiple of 90 degrees,
> - * otherwise `false`
> - * \return The orientation corresponding to the rotation if \a success was set
> - * to `true`, otherwise the `Rotate0` orientation
> + * can be used to represent anticlockwise rotations. Must be a multiple of 90.
> + * \return The orientation corresponding to the rotation
> */
> -Orientation orientationFromRotation(int angle, bool *success)
> +std::optional<Orientation> orientationFromRotation(int angle)
> {
> angle = angle % 360;
> if (angle < 0)
> angle += 360;
>
> - if (success != nullptr)
> - *success = true;
> -
> switch (angle) {
> case 0:
> return Orientation::Rotate0;
> @@ -85,10 +79,7 @@ Orientation orientationFromRotation(int angle, bool *success)
> return Orientation::Rotate270;
> }
>
> - if (success != nullptr)
> - *success = false;
> -
> - return Orientation::Rotate0;
> + return {};
> }
>
> /**
> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
> index b0c6abde4..d8c0bd41b 100644
> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp
> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp
> @@ -602,15 +602,15 @@ int CameraSensorLegacy::initProperties()
> * Cache the Transform associated with the camera mounting
> * rotation for later use in computeTransform().
> */
> - bool success;
> - mountingOrientation_ = orientationFromRotation(propertyValue, &success);
> - if (!success) {
> + auto mountingOrientation = orientationFromRotation(propertyValue);
> + if (!mountingOrientation) {
> LOG(CameraSensor, Warning)
> << "Invalid rotation of " << propertyValue
> << " degrees - ignoring";
> - mountingOrientation_ = Orientation::Rotate0;
> }
>
> + mountingOrientation_ = mountingOrientation.value_or(Orientation::Rotate0);
> +
> properties_.set(properties::Rotation, propertyValue);
> } else {
> LOG(CameraSensor, Warning)
> diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp
> index ab75b1f82..3a069ef9d 100644
> --- a/src/libcamera/sensor/camera_sensor_raw.cpp
> +++ b/src/libcamera/sensor/camera_sensor_raw.cpp
> @@ -607,15 +607,15 @@ int CameraSensorRaw::initProperties()
> * Cache the Transform associated with the camera mounting
> * rotation for later use in computeTransform().
> */
> - bool success;
> - mountingOrientation_ = orientationFromRotation(propertyValue, &success);
> - if (!success) {
> + auto mountingOrientation = orientationFromRotation(propertyValue);
> + if (!mountingOrientation) {
> LOG(CameraSensor, Warning)
> << "Invalid rotation of " << propertyValue
> << " degrees - ignoring";
> - mountingOrientation_ = Orientation::Rotate0;
> }
>
> + mountingOrientation_ = mountingOrientation.value_or(Orientation::Rotate0);
> +
> properties_.set(properties::Rotation, propertyValue);
> } else {
> LOG(CameraSensor, Warning)
> diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp
> index 9fe8b5620..6636b23e4 100644
> --- a/src/libcamera/transform.cpp
> +++ b/src/libcamera/transform.cpp
> @@ -269,21 +269,15 @@ Transform operator-(Transform t)
> * \brief Return the transform representing a rotation of the given angle
> * clockwise
> * \param[in] angle The angle of rotation in a clockwise sense. Negative values
> - * can be used to represent anticlockwise rotations
> - * \param[out] success Set to `true` if the angle is a multiple of 90 degrees,
> - * otherwise `false`
> - * \return The transform corresponding to the rotation if \a success was set to
> - * `true`, otherwise the `Identity` transform
> + * can be used to represent anticlockwise rotations. Must be a multiple of 90.
> + * \return The transform corresponding to the rotation
> */
> -Transform transformFromRotation(int angle, bool *success)
> +std::optional<Transform> transformFromRotation(int angle)
> {
> angle = angle % 360;
> if (angle < 0)
> angle += 360;
>
> - if (success != nullptr)
> - *success = true;
> -
> switch (angle) {
> case 0:
> return Transform::Identity;
> @@ -295,10 +289,7 @@ Transform transformFromRotation(int angle, bool *success)
> return Transform::Rot270;
> }
>
> - if (success != nullptr)
> - *success = false;
> -
> - return Transform::Identity;
> + return {};
> }
>
> namespace {
> diff --git a/src/py/libcamera/py_transform.cpp b/src/py/libcamera/py_transform.cpp
> index 768260ffc..fc25d5a32 100644
> --- a/src/py/libcamera/py_transform.cpp
> +++ b/src/py/libcamera/py_transform.cpp
> @@ -24,19 +24,18 @@ void init_py_transform(py::module &m)
>
> pyTransform
> .def(py::init([](int rotation, bool hflip, bool vflip, bool transpose) {
> - bool ok;
> -
> - Transform t = transformFromRotation(rotation, &ok);
> - if (!ok)
> + auto t = transformFromRotation(rotation);
> + if (!t)
> throw std::invalid_argument("Invalid rotation");
>
> if (hflip)
> - t ^= Transform::HFlip;
> + *t ^= Transform::HFlip;
> if (vflip)
> - t ^= Transform::VFlip;
> + *t ^= Transform::VFlip;
> if (transpose)
> - t ^= Transform::Transpose;
> - return t;
> + *t ^= Transform::Transpose;
> +
> + return *t;
> }), py::arg("rotation") = 0, py::arg("hflip") = false,
> py::arg("vflip") = false, py::arg("transpose") = false)
> .def(py::init([](Transform &other) { return other; }))
> --
> 2.48.1
>
>
More information about the libcamera-devel
mailing list