[libcamera-devel] [PATCH v2 5/9] libcamera: transform: Add functions to convert Orientation

[Jacopo Mondi]

Hi David

On Mon, Jul 17, 2023 at 02:35:31PM +0100, David Plowman via libcamera-devel wrote:
> Hi again
>
> On Mon, 17 Jul 2023 at 13:53, Jacopo Mondi
> <jacopo.mondi at ideasonboard.com> wrote:
> >
> > 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!
>
> Another thing to consider is what happens in the Python bindings. Are
> we going to have both orientations and transforms? I think you could
> get by with only transforms (and convert them implicitly when needed),
> or we could expose them there too. One to think about!
>

We're strving to express the image orientation using the EXIF values
for greater interoperability, I don't see a reason not to have them
exposed in Python or any other language bindings.

> David
>
> >
> > >
> > > 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
> > > >