| Message ID | 20230714141549.11085-6-jacopo.mondi@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo Thanks for the patch. On Fri, 14 Jul 2023 at 15:16, Jacopo Mondi <jacopo.mondi@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@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@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 >
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@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@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@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 > >
Hi again On Mon, 17 Jul 2023 at 13:53, Jacopo Mondi <jacopo.mondi@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@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@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@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 > > >
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@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@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@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@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 > > > >
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.
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@ideasonboard.com> --- 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(-) -- 2.40.1