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

[David Plowman]

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!

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