| Message ID | 20230901150215.11585-6-jacopo.mondi@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Fri, Sep 01, 2023 at 05:02:08PM +0200, Jacopo Mondi via libcamera-devel wrote: > Add two helper functions to the transform.cpp file that allows to > convert to and from an Orientation. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > --- > include/libcamera/transform.h | 4 +++ > src/libcamera/transform.cpp | 60 +++++++++++++++++++++++++++++++++++ > 2 files changed, 64 insertions(+) > > diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h > index 2a6838c78369..14f1db0e8572 100644 > --- a/include/libcamera/transform.h > +++ b/include/libcamera/transform.h > @@ -11,6 +11,8 @@ > > namespace libcamera { > > +enum class Orientation; > + > enum class Transform : int { > Identity = 0, > Rot0 = Identity, > @@ -69,6 +71,8 @@ constexpr Transform operator~(Transform t) > } > > Transform transformFromRotation(int angle, bool *success = nullptr); > +Transform transformFromOrientation(const Orientation &orientation); > +Orientation transformToOrientation(const Transform &transform); > > const char *transformToString(Transform t); > > diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp > index 4668303d0676..c70cac3f14ee 100644 > --- a/src/libcamera/transform.cpp > +++ b/src/libcamera/transform.cpp > @@ -7,6 +7,8 @@ > > #include <libcamera/transform.h> > > +#include <libcamera/orientation.h> > + > /** > * \file transform.h > * \brief Enum to represent and manipulate 2D plane transforms > @@ -299,6 +301,64 @@ Transform transformFromRotation(int angle, bool *success) > return Transform::Identity; > } > > +/** > + * \brief Return the transform representing \a orientation > + * \param[in] orientation The orientation to convert > + * \return The transform corresponding to \a orientation > + */ I don't like these functions much, as orientations and transforms are not intrinsincly convertible. There's no cardinal concept such as "a transform representing an orientation". Fortunately, transformToOrientation() can easily be dropped on top of this series, as one of its callers can go away (see reviews of other patches in the series), and the code can then be folded in the other caller, `Orientation operator*(const Orientation &o, const Transform &t)`. Similarly, I would like to drop the call to transformFromOrientation() in RPi::CameraData::configureIPA() and replace it there with usage of `Transform operator/(const Orientation &o1, const Orientation &o2)`. This can be done on top, as avoiding the introduction of these two functions in the series may cause annoying conflicts when rebasing and refactoring. > +Transform transformFromOrientation(const Orientation &orientation) > +{ > + switch (orientation) { > + case Orientation::rotate0: > + return Transform::Identity; > + case Orientation::rotate0Flip: > + return Transform::HFlip; > + case Orientation::rotate180: > + return Transform::Rot180; > + case Orientation::rotate180Flip: > + return Transform::VFlip; > + case Orientation::rotate90Flip: > + return Transform::Transpose; > + case Orientation::rotate90: > + return Transform::Rot90; > + case Orientation::rotate270Flip: > + return Transform::Rot180Transpose; > + case Orientation::rotate270: > + return Transform::Rot270; > + } > + > + return Transform::Identity; > +} > + > +/** > + * \brief Return the Orientation representing \a transform > + * \param[in] transform The transform to convert > + * \return The Orientation corresponding to \a transform > + */ > +Orientation transformToOrientation(const Transform &transform) > +{ > + switch (transform) { > + case Transform::Identity: > + return Orientation::rotate0; > + case Transform::HFlip: > + return Orientation::rotate0Flip; > + case Transform::VFlip: > + return Orientation::rotate180Flip; > + case Transform::Rot180: > + return Orientation::rotate180; > + case Transform::Transpose: > + return Orientation::rotate90Flip; > + case Transform::Rot270: > + return Orientation::rotate270; > + case Transform::Rot90: > + return Orientation::rotate90; > + case Transform::Rot180Transpose: > + return Orientation::rotate270Flip; > + } > + > + return Orientation::rotate0; > +} > + > /** > * \brief Return a character string describing the transform > * \param[in] t The transform to be described.
Hi Laurent On Thu, Oct 19, 2023 at 01:22:34AM +0300, Laurent Pinchart via libcamera-devel wrote: > Hi Jacopo, > > Thank you for the patch. > > On Fri, Sep 01, 2023 at 05:02:08PM +0200, Jacopo Mondi via libcamera-devel wrote: > > Add two helper functions to the transform.cpp file that allows to > > convert to and from an Orientation. > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > include/libcamera/transform.h | 4 +++ > > src/libcamera/transform.cpp | 60 +++++++++++++++++++++++++++++++++++ > > 2 files changed, 64 insertions(+) > > > > diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h > > index 2a6838c78369..14f1db0e8572 100644 > > --- a/include/libcamera/transform.h > > +++ b/include/libcamera/transform.h > > @@ -11,6 +11,8 @@ > > > > namespace libcamera { > > > > +enum class Orientation; > > + > > enum class Transform : int { > > Identity = 0, > > Rot0 = Identity, > > @@ -69,6 +71,8 @@ constexpr Transform operator~(Transform t) > > } > > > > Transform transformFromRotation(int angle, bool *success = nullptr); > > +Transform transformFromOrientation(const Orientation &orientation); > > +Orientation transformToOrientation(const Transform &transform); > > > > const char *transformToString(Transform t); > > > > diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp > > index 4668303d0676..c70cac3f14ee 100644 > > --- a/src/libcamera/transform.cpp > > +++ b/src/libcamera/transform.cpp > > @@ -7,6 +7,8 @@ > > > > #include <libcamera/transform.h> > > > > +#include <libcamera/orientation.h> > > + > > /** > > * \file transform.h > > * \brief Enum to represent and manipulate 2D plane transforms > > @@ -299,6 +301,64 @@ Transform transformFromRotation(int angle, bool *success) > > return Transform::Identity; > > } > > > > +/** > > + * \brief Return the transform representing \a orientation > > + * \param[in] orientation The orientation to convert > > + * \return The transform corresponding to \a orientation > > + */ > > I don't like these functions much, as orientations and transforms are > not intrinsincly convertible. There's no cardinal concept such as "a > transform representing an orientation". > Are we sure ? Orientation and Transform describe exactly the same set of 2d plane transformations, isn't there a 1-to-1 relationship ? > Fortunately, transformToOrientation() can easily be dropped on top of this > series, as one of its callers can go away (see reviews of other patches > in the series), and the code can then be folded in the other caller, > `Orientation operator*(const Orientation &o, const Transform &t)`. You know, I can't find any comment related to this. Can you provide a reference ? > > Similarly, I would like to drop the call to transformFromOrientation() > in RPi::CameraData::configureIPA() and replace it there with usage of > `Transform operator/(const Orientation &o1, const Orientation &o2)`. > > This can be done on top, as avoiding the introduction of these two > functions in the series may cause annoying conflicts when rebasing and > refactoring. > > > +Transform transformFromOrientation(const Orientation &orientation) > > +{ > > + switch (orientation) { > > + case Orientation::rotate0: > > + return Transform::Identity; > > + case Orientation::rotate0Flip: > > + return Transform::HFlip; > > + case Orientation::rotate180: > > + return Transform::Rot180; > > + case Orientation::rotate180Flip: > > + return Transform::VFlip; > > + case Orientation::rotate90Flip: > > + return Transform::Transpose; > > + case Orientation::rotate90: > > + return Transform::Rot90; > > + case Orientation::rotate270Flip: > > + return Transform::Rot180Transpose; > > + case Orientation::rotate270: > > + return Transform::Rot270; > > + } > > + > > + return Transform::Identity; > > +} > > + > > +/** > > + * \brief Return the Orientation representing \a transform > > + * \param[in] transform The transform to convert > > + * \return The Orientation corresponding to \a transform > > + */ > > +Orientation transformToOrientation(const Transform &transform) > > +{ > > + switch (transform) { > > + case Transform::Identity: > > + return Orientation::rotate0; > > + case Transform::HFlip: > > + return Orientation::rotate0Flip; > > + case Transform::VFlip: > > + return Orientation::rotate180Flip; > > + case Transform::Rot180: > > + return Orientation::rotate180; > > + case Transform::Transpose: > > + return Orientation::rotate90Flip; > > + case Transform::Rot270: > > + return Orientation::rotate270; > > + case Transform::Rot90: > > + return Orientation::rotate90; > > + case Transform::Rot180Transpose: > > + return Orientation::rotate270Flip; > > + } > > + > > + return Orientation::rotate0; > > +} > > + > > /** > > * \brief Return a character string describing the transform > > * \param[in] t The transform to be described. > > -- > Regards, > > Laurent Pinchart
Hi again On Thu, Oct 19, 2023 at 12:34:08PM +0200, Jacopo Mondi via libcamera-devel wrote: > Hi Laurent > > On Thu, Oct 19, 2023 at 01:22:34AM +0300, Laurent Pinchart via libcamera-devel wrote: > > Hi Jacopo, > > > > Thank you for the patch. > > > > On Fri, Sep 01, 2023 at 05:02:08PM +0200, Jacopo Mondi via libcamera-devel wrote: > > > Add two helper functions to the transform.cpp file that allows to > > > convert to and from an Orientation. > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > include/libcamera/transform.h | 4 +++ > > > src/libcamera/transform.cpp | 60 +++++++++++++++++++++++++++++++++++ > > > 2 files changed, 64 insertions(+) > > > > > > diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h > > > index 2a6838c78369..14f1db0e8572 100644 > > > --- a/include/libcamera/transform.h > > > +++ b/include/libcamera/transform.h > > > @@ -11,6 +11,8 @@ > > > > > > namespace libcamera { > > > > > > +enum class Orientation; > > > + > > > enum class Transform : int { > > > Identity = 0, > > > Rot0 = Identity, > > > @@ -69,6 +71,8 @@ constexpr Transform operator~(Transform t) > > > } > > > > > > Transform transformFromRotation(int angle, bool *success = nullptr); > > > +Transform transformFromOrientation(const Orientation &orientation); > > > +Orientation transformToOrientation(const Transform &transform); > > > > > > const char *transformToString(Transform t); > > > > > > diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp > > > index 4668303d0676..c70cac3f14ee 100644 > > > --- a/src/libcamera/transform.cpp > > > +++ b/src/libcamera/transform.cpp > > > @@ -7,6 +7,8 @@ > > > > > > #include <libcamera/transform.h> > > > > > > +#include <libcamera/orientation.h> > > > + > > > /** > > > * \file transform.h > > > * \brief Enum to represent and manipulate 2D plane transforms > > > @@ -299,6 +301,64 @@ Transform transformFromRotation(int angle, bool *success) > > > return Transform::Identity; > > > } > > > > > > +/** > > > + * \brief Return the transform representing \a orientation > > > + * \param[in] orientation The orientation to convert > > > + * \return The transform corresponding to \a orientation > > > + */ > > > > I don't like these functions much, as orientations and transforms are > > not intrinsincly convertible. There's no cardinal concept such as "a > > transform representing an orientation". > > > > Are we sure ? Orientation and Transform describe exactly the same set > of 2d plane transformations, isn't there a 1-to-1 relationship ? > > > Fortunately, transformToOrientation() can easily be dropped on top of this > > series, as one of its callers can go away (see reviews of other patches > > in the series), and the code can then be folded in the other caller, > > `Orientation operator*(const Orientation &o, const Transform &t)`. > > You know, I can't find any comment related to this. Can you provide a > reference ? > Found it in 10/12 I think we can remove these two functions by: - Introduce a orientationFromRotation() to replace rotationTransform_ = transformFromRotation(propertyValue, &success); in CameraSensor::initProperties() so that we can remove transformToOrientation() in CameraSensor::computeTransform() - Move the RPi pipeline and IPA to use Orientation and remove params.transform = static_cast<unsigned int>(transformFromOrientation(config->orientation)); so we can remove transformFromOrientation() - Replace py_transform which uses Transform This is all doable, but I would like to sync with RPi as I presume they have code in their apps which is based on these interfaces CC-ed David and Naush > > > > Similarly, I would like to drop the call to transformFromOrientation() > > in RPi::CameraData::configureIPA() and replace it there with usage of > > `Transform operator/(const Orientation &o1, const Orientation &o2)`. > > > > This can be done on top, as avoiding the introduction of these two > > functions in the series may cause annoying conflicts when rebasing and > > refactoring. > > > > > +Transform transformFromOrientation(const Orientation &orientation) > > > +{ > > > + switch (orientation) { > > > + case Orientation::rotate0: > > > + return Transform::Identity; > > > + case Orientation::rotate0Flip: > > > + return Transform::HFlip; > > > + case Orientation::rotate180: > > > + return Transform::Rot180; > > > + case Orientation::rotate180Flip: > > > + return Transform::VFlip; > > > + case Orientation::rotate90Flip: > > > + return Transform::Transpose; > > > + case Orientation::rotate90: > > > + return Transform::Rot90; > > > + case Orientation::rotate270Flip: > > > + return Transform::Rot180Transpose; > > > + case Orientation::rotate270: > > > + return Transform::Rot270; > > > + } > > > + > > > + return Transform::Identity; > > > +} > > > + > > > +/** > > > + * \brief Return the Orientation representing \a transform > > > + * \param[in] transform The transform to convert > > > + * \return The Orientation corresponding to \a transform > > > + */ > > > +Orientation transformToOrientation(const Transform &transform) > > > +{ > > > + switch (transform) { > > > + case Transform::Identity: > > > + return Orientation::rotate0; > > > + case Transform::HFlip: > > > + return Orientation::rotate0Flip; > > > + case Transform::VFlip: > > > + return Orientation::rotate180Flip; > > > + case Transform::Rot180: > > > + return Orientation::rotate180; > > > + case Transform::Transpose: > > > + return Orientation::rotate90Flip; > > > + case Transform::Rot270: > > > + return Orientation::rotate270; > > > + case Transform::Rot90: > > > + return Orientation::rotate90; > > > + case Transform::Rot180Transpose: > > > + return Orientation::rotate270Flip; > > > + } > > > + > > > + return Orientation::rotate0; > > > +} > > > + > > > /** > > > * \brief Return a character string describing the transform > > > * \param[in] t The transform to be described. > > > > -- > > Regards, > > > > Laurent Pinchart
Hi Jacopo On Thu, 19 Oct 2023 at 13:34, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi again > > On Thu, Oct 19, 2023 at 12:34:08PM +0200, Jacopo Mondi via libcamera-devel wrote: > > Hi Laurent > > > > On Thu, Oct 19, 2023 at 01:22:34AM +0300, Laurent Pinchart via libcamera-devel wrote: > > > Hi Jacopo, > > > > > > Thank you for the patch. > > > > > > On Fri, Sep 01, 2023 at 05:02:08PM +0200, Jacopo Mondi via libcamera-devel wrote: > > > > Add two helper functions to the transform.cpp file that allows to > > > > convert to and from an Orientation. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > --- > > > > include/libcamera/transform.h | 4 +++ > > > > src/libcamera/transform.cpp | 60 +++++++++++++++++++++++++++++++++++ > > > > 2 files changed, 64 insertions(+) > > > > > > > > diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h > > > > index 2a6838c78369..14f1db0e8572 100644 > > > > --- a/include/libcamera/transform.h > > > > +++ b/include/libcamera/transform.h > > > > @@ -11,6 +11,8 @@ > > > > > > > > namespace libcamera { > > > > > > > > +enum class Orientation; > > > > + > > > > enum class Transform : int { > > > > Identity = 0, > > > > Rot0 = Identity, > > > > @@ -69,6 +71,8 @@ constexpr Transform operator~(Transform t) > > > > } > > > > > > > > Transform transformFromRotation(int angle, bool *success = nullptr); > > > > +Transform transformFromOrientation(const Orientation &orientation); > > > > +Orientation transformToOrientation(const Transform &transform); > > > > > > > > const char *transformToString(Transform t); > > > > > > > > diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp > > > > index 4668303d0676..c70cac3f14ee 100644 > > > > --- a/src/libcamera/transform.cpp > > > > +++ b/src/libcamera/transform.cpp > > > > @@ -7,6 +7,8 @@ > > > > > > > > #include <libcamera/transform.h> > > > > > > > > +#include <libcamera/orientation.h> > > > > + > > > > /** > > > > * \file transform.h > > > > * \brief Enum to represent and manipulate 2D plane transforms > > > > @@ -299,6 +301,64 @@ Transform transformFromRotation(int angle, bool *success) > > > > return Transform::Identity; > > > > } > > > > > > > > +/** > > > > + * \brief Return the transform representing \a orientation > > > > + * \param[in] orientation The orientation to convert > > > > + * \return The transform corresponding to \a orientation > > > > + */ > > > > > > I don't like these functions much, as orientations and transforms are > > > not intrinsincly convertible. There's no cardinal concept such as "a > > > transform representing an orientation". > > > > > > > Are we sure ? Orientation and Transform describe exactly the same set > > of 2d plane transformations, isn't there a 1-to-1 relationship ? > > > > > Fortunately, transformToOrientation() can easily be dropped on top of this > > > series, as one of its callers can go away (see reviews of other patches > > > in the series), and the code can then be folded in the other caller, > > > `Orientation operator*(const Orientation &o, const Transform &t)`. > > > > You know, I can't find any comment related to this. Can you provide a > > reference ? > > > > Found it in 10/12 > > I think we can remove these two functions by: > > - Introduce a orientationFromRotation() to replace > > rotationTransform_ = transformFromRotation(propertyValue, &success); > > in CameraSensor::initProperties() > > so that we can remove transformToOrientation() in CameraSensor::computeTransform() > > - Move the RPi pipeline and IPA to use Orientation and remove > > params.transform = > static_cast<unsigned int>(transformFromOrientation(config->orientation)); > > so we can remove transformFromOrientation() > > - Replace py_transform which uses Transform > > This is all doable, but I would like to sync with RPi as I presume > they have code in their apps which is based on these interfaces > > CC-ed David and Naush I don't think we care too much in our C++ apps. When these patches go in, we might need to convert between Transforms and Orientations here and there but that's easily accomplished using the * operator that we defined, or / (in the other direction). I don't recall if we thought about the Python world - it's easy to overlook. Here we have an API that's in use by users and exposes Transforms. We'd have the option either to change those over to Orientations and break things, or we could leave the Transforms alone and convert them internally. In the worst case one could just convert with a lookup table, but it would be nice to compose orientations and transforms as we can in the C++ world. David > > > > > > > > Similarly, I would like to drop the call to transformFromOrientation() > > > in RPi::CameraData::configureIPA() and replace it there with usage of > > > `Transform operator/(const Orientation &o1, const Orientation &o2)`. > > > > > > This can be done on top, as avoiding the introduction of these two > > > functions in the series may cause annoying conflicts when rebasing and > > > refactoring. > > > > > > > +Transform transformFromOrientation(const Orientation &orientation) > > > > +{ > > > > + switch (orientation) { > > > > + case Orientation::rotate0: > > > > + return Transform::Identity; > > > > + case Orientation::rotate0Flip: > > > > + return Transform::HFlip; > > > > + case Orientation::rotate180: > > > > + return Transform::Rot180; > > > > + case Orientation::rotate180Flip: > > > > + return Transform::VFlip; > > > > + case Orientation::rotate90Flip: > > > > + return Transform::Transpose; > > > > + case Orientation::rotate90: > > > > + return Transform::Rot90; > > > > + case Orientation::rotate270Flip: > > > > + return Transform::Rot180Transpose; > > > > + case Orientation::rotate270: > > > > + return Transform::Rot270; > > > > + } > > > > + > > > > + return Transform::Identity; > > > > +} > > > > + > > > > +/** > > > > + * \brief Return the Orientation representing \a transform > > > > + * \param[in] transform The transform to convert > > > > + * \return The Orientation corresponding to \a transform > > > > + */ > > > > +Orientation transformToOrientation(const Transform &transform) > > > > +{ > > > > + switch (transform) { > > > > + case Transform::Identity: > > > > + return Orientation::rotate0; > > > > + case Transform::HFlip: > > > > + return Orientation::rotate0Flip; > > > > + case Transform::VFlip: > > > > + return Orientation::rotate180Flip; > > > > + case Transform::Rot180: > > > > + return Orientation::rotate180; > > > > + case Transform::Transpose: > > > > + return Orientation::rotate90Flip; > > > > + case Transform::Rot270: > > > > + return Orientation::rotate270; > > > > + case Transform::Rot90: > > > > + return Orientation::rotate90; > > > > + case Transform::Rot180Transpose: > > > > + return Orientation::rotate270Flip; > > > > + } > > > > + > > > > + return Orientation::rotate0; > > > > +} > > > > + > > > > /** > > > > * \brief Return a character string describing the transform > > > > * \param[in] t The transform to be described. > > > > > > -- > > > Regards, > > > > > > Laurent Pinchart
Hello, On Thu, Oct 19, 2023 at 02:05:38PM +0100, David Plowman wrote: > On Thu, 19 Oct 2023 at 13:34, Jacopo Mondi wrote: > > On Thu, Oct 19, 2023 at 12:34:08PM +0200, Jacopo Mondi via libcamera-devel wrote: > > > On Thu, Oct 19, 2023 at 01:22:34AM +0300, Laurent Pinchart via libcamera-devel wrote: > > > > On Fri, Sep 01, 2023 at 05:02:08PM +0200, Jacopo Mondi via libcamera-devel wrote: > > > > > Add two helper functions to the transform.cpp file that allows to > > > > > convert to and from an Orientation. > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > > --- > > > > > include/libcamera/transform.h | 4 +++ > > > > > src/libcamera/transform.cpp | 60 +++++++++++++++++++++++++++++++++++ > > > > > 2 files changed, 64 insertions(+) > > > > > > > > > > diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h > > > > > index 2a6838c78369..14f1db0e8572 100644 > > > > > --- a/include/libcamera/transform.h > > > > > +++ b/include/libcamera/transform.h > > > > > @@ -11,6 +11,8 @@ > > > > > > > > > > namespace libcamera { > > > > > > > > > > +enum class Orientation; > > > > > + > > > > > enum class Transform : int { > > > > > Identity = 0, > > > > > Rot0 = Identity, > > > > > @@ -69,6 +71,8 @@ constexpr Transform operator~(Transform t) > > > > > } > > > > > > > > > > Transform transformFromRotation(int angle, bool *success = nullptr); > > > > > +Transform transformFromOrientation(const Orientation &orientation); > > > > > +Orientation transformToOrientation(const Transform &transform); > > > > > > > > > > const char *transformToString(Transform t); > > > > > > > > > > diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp > > > > > index 4668303d0676..c70cac3f14ee 100644 > > > > > --- a/src/libcamera/transform.cpp > > > > > +++ b/src/libcamera/transform.cpp > > > > > @@ -7,6 +7,8 @@ > > > > > > > > > > #include <libcamera/transform.h> > > > > > > > > > > +#include <libcamera/orientation.h> > > > > > + > > > > > /** > > > > > * \file transform.h > > > > > * \brief Enum to represent and manipulate 2D plane transforms > > > > > @@ -299,6 +301,64 @@ Transform transformFromRotation(int angle, bool *success) > > > > > return Transform::Identity; > > > > > } > > > > > > > > > > +/** > > > > > + * \brief Return the transform representing \a orientation > > > > > + * \param[in] orientation The orientation to convert > > > > > + * \return The transform corresponding to \a orientation > > > > > + */ > > > > > > > > I don't like these functions much, as orientations and transforms are > > > > not intrinsincly convertible. There's no cardinal concept such as "a > > > > transform representing an orientation". > > > > > > Are we sure ? Orientation and Transform describe exactly the same set > > > of 2d plane transformations, isn't there a 1-to-1 relationship ? Orientation describes how an image is stored in a buffer, while Transform describes how it is processed. I think saying Orientation::Rot0 * Transform::HVFlip == Orientation::Rot180 makes sense, but saying Transform::HVFlip == Orientation::Rot180 doesn't. > > > > Fortunately, transformToOrientation() can easily be dropped on top of this > > > > series, as one of its callers can go away (see reviews of other patches > > > > in the series), and the code can then be folded in the other caller, > > > > `Orientation operator*(const Orientation &o, const Transform &t)`. > > > > > > You know, I can't find any comment related to this. Can you provide a > > > reference ? > > > > Found it in 10/12 > > > > I think we can remove these two functions by: > > > > - Introduce a orientationFromRotation() to replace > > > > rotationTransform_ = transformFromRotation(propertyValue, &success); > > > > in CameraSensor::initProperties() > > > > so that we can remove transformToOrientation() in CameraSensor::computeTransform() > > > > - Move the RPi pipeline and IPA to use Orientation and remove > > > > params.transform = > > static_cast<unsigned int>(transformFromOrientation(config->orientation)); > > > > so we can remove transformFromOrientation() > > > > - Replace py_transform which uses Transform > > > > This is all doable, but I would like to sync with RPi as I presume > > they have code in their apps which is based on these interfaces > > > > CC-ed David and Naush > > I don't think we care too much in our C++ apps. When these patches go > in, we might need to convert between Transforms and Orientations here > and there but that's easily accomplished using the * operator that we > defined, or / (in the other direction). That's what I had in mind. I'll experiment with removing these two functions and replacing them with the * and / operators. > I don't recall if we thought about the Python world - it's easy to > overlook. Indeed. Fortunately we have you and Tomi to remind us about Python when needed :-) > Here we have an API that's in use by users and exposes > Transforms. We'd have the option either to change those over to > Orientations and break things, or we could leave the Transforms alone > and convert them internally. In the worst case one could just convert > with a lookup table, but it would be nice to compose orientations and > transforms as we can in the C++ world. Ideally we should also standardize on Orientation in Python, but if there are clear needs to be able to transform orientations in application code, then exposing a Transform class would also make sense as a convenience helper (it's *so* easy to get these things wrong). It could be coded entirely in Python, no need to involve the Python bindings there. > > > > Similarly, I would like to drop the call to transformFromOrientation() > > > > in RPi::CameraData::configureIPA() and replace it there with usage of > > > > `Transform operator/(const Orientation &o1, const Orientation &o2)`. > > > > > > > > This can be done on top, as avoiding the introduction of these two > > > > functions in the series may cause annoying conflicts when rebasing and > > > > refactoring. > > > > > > > > > +Transform transformFromOrientation(const Orientation &orientation) > > > > > +{ > > > > > + switch (orientation) { > > > > > + case Orientation::rotate0: > > > > > + return Transform::Identity; > > > > > + case Orientation::rotate0Flip: > > > > > + return Transform::HFlip; > > > > > + case Orientation::rotate180: > > > > > + return Transform::Rot180; > > > > > + case Orientation::rotate180Flip: > > > > > + return Transform::VFlip; > > > > > + case Orientation::rotate90Flip: > > > > > + return Transform::Transpose; > > > > > + case Orientation::rotate90: > > > > > + return Transform::Rot90; > > > > > + case Orientation::rotate270Flip: > > > > > + return Transform::Rot180Transpose; > > > > > + case Orientation::rotate270: > > > > > + return Transform::Rot270; > > > > > + } > > > > > + > > > > > + return Transform::Identity; > > > > > +} > > > > > + > > > > > +/** > > > > > + * \brief Return the Orientation representing \a transform > > > > > + * \param[in] transform The transform to convert > > > > > + * \return The Orientation corresponding to \a transform > > > > > + */ > > > > > +Orientation transformToOrientation(const Transform &transform) > > > > > +{ > > > > > + switch (transform) { > > > > > + case Transform::Identity: > > > > > + return Orientation::rotate0; > > > > > + case Transform::HFlip: > > > > > + return Orientation::rotate0Flip; > > > > > + case Transform::VFlip: > > > > > + return Orientation::rotate180Flip; > > > > > + case Transform::Rot180: > > > > > + return Orientation::rotate180; > > > > > + case Transform::Transpose: > > > > > + return Orientation::rotate90Flip; > > > > > + case Transform::Rot270: > > > > > + return Orientation::rotate270; > > > > > + case Transform::Rot90: > > > > > + return Orientation::rotate90; > > > > > + case Transform::Rot180Transpose: > > > > > + return Orientation::rotate270Flip; > > > > > + } > > > > > + > > > > > + return Orientation::rotate0; > > > > > +} > > > > > + > > > > > /** > > > > > * \brief Return a character string describing the transform > > > > > * \param[in] t The transform to be described.
diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h index 2a6838c78369..14f1db0e8572 100644 --- a/include/libcamera/transform.h +++ b/include/libcamera/transform.h @@ -11,6 +11,8 @@ namespace libcamera { +enum class Orientation; + enum class Transform : int { Identity = 0, Rot0 = Identity, @@ -69,6 +71,8 @@ constexpr Transform operator~(Transform t) } Transform transformFromRotation(int angle, bool *success = nullptr); +Transform transformFromOrientation(const Orientation &orientation); +Orientation transformToOrientation(const Transform &transform); const char *transformToString(Transform t); diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp index 4668303d0676..c70cac3f14ee 100644 --- a/src/libcamera/transform.cpp +++ b/src/libcamera/transform.cpp @@ -7,6 +7,8 @@ #include <libcamera/transform.h> +#include <libcamera/orientation.h> + /** * \file transform.h * \brief Enum to represent and manipulate 2D plane transforms @@ -299,6 +301,64 @@ Transform transformFromRotation(int angle, bool *success) return Transform::Identity; } +/** + * \brief Return the transform representing \a orientation + * \param[in] orientation The orientation to convert + * \return The transform corresponding to \a orientation + */ +Transform transformFromOrientation(const Orientation &orientation) +{ + switch (orientation) { + case Orientation::rotate0: + return Transform::Identity; + case Orientation::rotate0Flip: + return Transform::HFlip; + case Orientation::rotate180: + return Transform::Rot180; + case Orientation::rotate180Flip: + return Transform::VFlip; + case Orientation::rotate90Flip: + return Transform::Transpose; + case Orientation::rotate90: + return Transform::Rot90; + case Orientation::rotate270Flip: + return Transform::Rot180Transpose; + case Orientation::rotate270: + return Transform::Rot270; + } + + return Transform::Identity; +} + +/** + * \brief Return the Orientation representing \a transform + * \param[in] transform The transform to convert + * \return The Orientation corresponding to \a transform + */ +Orientation transformToOrientation(const Transform &transform) +{ + switch (transform) { + case Transform::Identity: + return Orientation::rotate0; + case Transform::HFlip: + return Orientation::rotate0Flip; + case Transform::VFlip: + return Orientation::rotate180Flip; + case Transform::Rot180: + return Orientation::rotate180; + case Transform::Transpose: + return Orientation::rotate90Flip; + case Transform::Rot270: + return Orientation::rotate270; + case Transform::Rot90: + return Orientation::rotate90; + case Transform::Rot180Transpose: + return Orientation::rotate270Flip; + } + + return Orientation::rotate0; +} + /** * \brief Return a character string describing the transform * \param[in] t The transform to be described.