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

[David Plowman]

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
really simple things, so dragging in camera.h felt a bit weird (even
though, in practice, an application would probably do so anyway).

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:

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
>