[libcamera-devel] [PATCH v2 5/9] libcamera: transform: Add functions to convert Orientation
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Mon Jul 17 14:53:52 CEST 2023
Hi David
On Mon, Jul 17, 2023 at 10:46:04AM +0100, David Plowman via libcamera-devel wrote:
> Hi Jacopo
>
> Thanks for the patch.
>
> On Fri, 14 Jul 2023 at 15:16, Jacopo Mondi
> <jacopo.mondi at ideasonboard.com> wrote:
> >
> > Add two helper functions to the transform.cpp file that allows to
> > convert to and from an Orientation.
> >
> > As transform.h now needs to include camera.h remove "#include
> > <transform.h>" from camera.h to avoid a cyclic inclusion loop.
> >
> > Adjust all files that need to include transform.h directly and didn't do
> > so because it was implicitly included by camera.h.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>
> The functions here seem fine to me, I guess I just wanted to ask the
> question about include files. Part of me feels that transforms are
> kind of like orientations, so if transforms have their own header,
> maybe orientations should too? Transforms seem, to me at least, like
I considered this, but so far Orientation is a simple enum, I didn't
consider it enough to break it out to a dedicated file..
> really simple things, so dragging in camera.h felt a bit weird (even
> though, in practice, an application would probably do so anyway).
>
Looking ahead to the C API introduction, Transform seems to be more
opportunely placed in the C++ API which will be built on top of the
C one.
Now, as you correctly noticed, including transform.h will drag in
camera.h. As the camera orientation lives in CameraConfiguration it
really seemed un-likely for an application to use Transform without
including camera.h
Actually, I initially wanted to forward declare Orientation and avoid
including <camera.h> in transform.h. Unfrtunately forward-declaring
inner types in C++ is not possible. One possible option would be to
move Orientation out of CameraConfiguration, possibily to its own
file as you were suggesting. This would remove the need to include
<camera.h>
> But honestly, I'm not sure. It was just the lack of symmetry between
> the two that left me feeling slightly uncomfortable.
>
> Anyway, subject to some saying they like it how it is:
Yeah opinions welcome!
>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
>
> Thanks!
> David
>
> > ---
> > include/libcamera/camera.h | 3 +-
> > include/libcamera/transform.h | 4 ++
> > src/libcamera/camera.cpp | 1 +
> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 1 +
> > src/libcamera/transform.cpp | 58 ++++++++++++++++++++
> > 5 files changed, 66 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 8132ef2506a4..55359d53f2ab 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -22,7 +22,6 @@
> > #include <libcamera/controls.h>
> > #include <libcamera/request.h>
> > #include <libcamera/stream.h>
> > -#include <libcamera/transform.h>
> >
> > namespace libcamera {
> >
> > @@ -31,6 +30,8 @@ class FrameBufferAllocator;
> > class PipelineHandler;
> > class Request;
> >
> > +enum class Transform;
> > +
> > class CameraConfiguration
> > {
> > public:
> > diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h
> > index 2a6838c78369..573adf18715d 100644
> > --- a/include/libcamera/transform.h
> > +++ b/include/libcamera/transform.h
> > @@ -9,6 +9,8 @@
> >
> > #include <string>
> >
> > +#include <libcamera/camera.h>
> > +
> > namespace libcamera {
> >
> > enum class Transform : int {
> > @@ -69,6 +71,8 @@ constexpr Transform operator~(Transform t)
> > }
> >
> > Transform transformFromRotation(int angle, bool *success = nullptr);
> > +Transform transformFromOrientation(const CameraConfiguration::Orientation &orientation);
> > +CameraConfiguration::Orientation transformToOrientation(const Transform &transform);
> >
> > const char *transformToString(Transform t);
> >
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index c8bb56d4af7f..af842e70dcb0 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -18,6 +18,7 @@
> > #include <libcamera/framebuffer_allocator.h>
> > #include <libcamera/request.h>
> > #include <libcamera/stream.h>
> > +#include <libcamera/transform.h>
> >
> > #include "libcamera/internal/camera.h"
> > #include "libcamera/internal/camera_controls.h"
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index 38f48a5d9269..02a6117c7955 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -21,6 +21,7 @@
> > #include <libcamera/property_ids.h>
> > #include <libcamera/request.h>
> > #include <libcamera/stream.h>
> > +#include <libcamera/transform.h>
> >
> > #include "libcamera/internal/camera.h"
> > #include "libcamera/internal/device_enumerator.h"
> > diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp
> > index 4668303d0676..03d2b52e7f38 100644
> > --- a/src/libcamera/transform.cpp
> > +++ b/src/libcamera/transform.cpp
> > @@ -299,6 +299,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 CameraConfiguration::Orientation &orientation)
> > +{
> > + switch (orientation) {
> > + case CameraConfiguration::rotate0:
> > + return Transform::Identity;
> > + case CameraConfiguration::rotate0Flip:
> > + return Transform::HFlip;
> > + case CameraConfiguration::rotate180:
> > + return Transform::Rot180;
> > + case CameraConfiguration::rotate180Flip:
> > + return Transform::VFlip;
> > + case CameraConfiguration::rotate90Flip:
> > + return Transform::Transpose;
> > + case CameraConfiguration::rotate90:
> > + return Transform::Rot90;
> > + case CameraConfiguration::rotate270Flip:
> > + return Transform::Rot180Transpose;
> > + case CameraConfiguration::rotate270:
> > + return Transform::Rot270;
> > + }
> > +
> > + return Transform::Identity;
> > +}
> > +
> > +/**
> > + * \brief Return the CameraConfiguration::Orientation representing \a transform
> > + * \param[in] transform The transform to convert
> > + * \return The Orientation corresponding to \a transform
> > + */
> > +CameraConfiguration::Orientation transformToOrientation(const Transform &transform)
> > +{
> > + switch (transform) {
> > + case Transform::Identity:
> > + return CameraConfiguration::rotate0;
> > + case Transform::HFlip:
> > + return CameraConfiguration::rotate0Flip;
> > + case Transform::VFlip:
> > + return CameraConfiguration::rotate180Flip;
> > + case Transform::Rot180:
> > + return CameraConfiguration::rotate180;
> > + case Transform::Transpose:
> > + return CameraConfiguration::rotate90Flip;
> > + case Transform::Rot270:
> > + return CameraConfiguration::rotate270;
> > + case Transform::Rot90:
> > + return CameraConfiguration::rotate90;
> > + case Transform::Rot180Transpose:
> > + return CameraConfiguration::rotate270Flip;
> > + }
> > +
> > + return CameraConfiguration::rotate0;
> > +}
> > +
> > /**
> > * \brief Return a character string describing the transform
> > * \param[in] t The transform to be described.
> > --
> > 2.40.1
> >
More information about the libcamera-devel
mailing list