[RFC PATCH v1] libcamera: {orientation, transform}FromRotation(): Return `std::optional`
Barnabás Pőcze
barnabas.pocze at ideasonboard.com
Mon Mar 24 17:01:51 CET 2025
Hi
2025. 03. 17. 13:35 keltezéssel, Kieran Bingham írta:
> 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...
>
I don't have a concrete idea, but I like the second one better.
Or maybe something like `{Tag,Label}: api-break, abi-break`.
Regards,
Barnabás Pőcze
>
>
>> 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