[PATCH 1/2] qcam: rotate the viewfinder output as per camera properties
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jun 21 15:24:43 CEST 2024
On Fri, Jun 21, 2024 at 10:27:23AM +0200, Jacopo Mondi wrote:
> Hi Joel
>
> On Thu, Jun 20, 2024 at 04:36:06PM GMT, Joel Selvaraj wrote:
> > Devicetrees may specify the rotation at which the camera is
> > mounted in the hardware. If available, this gets populated in camera
> > properties. Rotate the viewfinder output accordingly in qcam qt and
> > opengl renderers.
> >
> > Signed-off-by: Joel Selvaraj <joelselvaraj.oss at gmail.com>
> > ---
> > include/libcamera/orientation.h | 2 ++
> > src/apps/qcam/main_window.cpp | 9 ++++++-
> > src/apps/qcam/viewfinder.h | 4 ++-
> > src/apps/qcam/viewfinder_gl.cpp | 46 ++++++++++++++++++++++++++++++---
> > src/apps/qcam/viewfinder_gl.h | 4 ++-
> > src/apps/qcam/viewfinder_qt.cpp | 18 +++++++++++--
> > src/apps/qcam/viewfinder_qt.h | 5 +++-
> > src/libcamera/orientation.cpp | 34 ++++++++++++++++++++++++
> > 8 files changed, 113 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/libcamera/orientation.h b/include/libcamera/orientation.h
> > index a3b40e63..e32f5eb8 100644
> > --- a/include/libcamera/orientation.h
> > +++ b/include/libcamera/orientation.h
> > @@ -25,6 +25,8 @@ enum class Orientation {
> >
> > Orientation orientationFromRotation(int angle, bool *success = nullptr);
> >
> > +int rotationFromOrientation(const Orientation &orientation, bool *success = nullptr);
> > +
> > std::ostream &operator<<(std::ostream &out, const Orientation &orientation);
> >
> > } /* namespace libcamera */
> > diff --git a/src/apps/qcam/main_window.cpp b/src/apps/qcam/main_window.cpp
> > index d515beed..18c94cf3 100644
> > --- a/src/apps/qcam/main_window.cpp
> > +++ b/src/apps/qcam/main_window.cpp
> > @@ -363,6 +363,7 @@ void MainWindow::toggleCapture(bool start)
> > int MainWindow::startCapture()
> > {
> > std::vector<StreamRole> roles = StreamKeyValueParser::roles(options_[OptStream]);
> > + Orientation orientation = Orientation::Rotate0;
> > int ret;
> >
> > /* Verify roles are supported. */
> > @@ -392,6 +393,12 @@ int MainWindow::startCapture()
> > return -EINVAL;
> > }
> >
> > + /* Get orientation provided by camera kernel driver */
The orientation comes from libcamera. It may be provided by kernel
drivers in some cases, but that's an implementation detail. I would
only mention here that we get the camera orientation.
> > + const ControlList &properties = camera_->properties();
> > + const auto &rotation = properties.get(properties::Rotation);
> > + if (rotation)
> > + orientation = orientationFromRotation(*rotation);
I think it would be more efficient to do this a bit differently. While
most cameras can't rotate by 90° or 270°, rotation by 180° is not
uncommon, and it would be useful to make use of that feature if the
camera supports it.
You can set the CameraConfiguration::orientation member to
Orientation::Rotate0 before calling validate() on the configuration.
After validating it, the member will be adjusted to the orientation you
will get in the buffer. If the camera can correct the mounting rotation
(for instance when the camera is mounted upside-down and the hardware is
capable of 180° rotation) then you will get Orientation::Rotate0 back
after validate(). Otherwise you will get another value that tells you
how the images will be oriented in the buffers. You can then pass that
value to the viewfinder to correct the orientation by rotating the
images when displaying.
> > +
> > StreamConfiguration &vfConfig = config_->at(0);
> >
> > /* Use a format supported by the viewfinder if available. */
> > @@ -444,7 +451,7 @@ int MainWindow::startCapture()
> > ret = viewfinder_->setFormat(vfConfig.pixelFormat,
> > QSize(vfConfig.size.width, vfConfig.size.height),
> > vfConfig.colorSpace.value_or(ColorSpace::Sycc),
> > - vfConfig.stride);
> > + vfConfig.stride, orientation);
> > if (ret < 0) {
> > qInfo() << "Failed to set viewfinder format";
> > return ret;
> > diff --git a/src/apps/qcam/viewfinder.h b/src/apps/qcam/viewfinder.h
> > index 914f88ec..f17cc4a1 100644
> > --- a/src/apps/qcam/viewfinder.h
> > +++ b/src/apps/qcam/viewfinder.h
> > @@ -14,6 +14,7 @@
> > #include <libcamera/color_space.h>
> > #include <libcamera/formats.h>
> > #include <libcamera/framebuffer.h>
> > +#include <libcamera/orientation.h>
> >
> > class Image;
> >
> > @@ -26,7 +27,8 @@ public:
> >
> > virtual int setFormat(const libcamera::PixelFormat &format, const QSize &size,
> > const libcamera::ColorSpace &colorSpace,
> > - unsigned int stride) = 0;
> > + unsigned int stride,
> > + const libcamera::Orientation &orientation) = 0;
libcamera::Orientation is a simple enum, so you can pass it by value
instead of const reference.
> > virtual void render(libcamera::FrameBuffer *buffer, Image *image) = 0;
> > virtual void stop() = 0;
> >
> > diff --git a/src/apps/qcam/viewfinder_gl.cpp b/src/apps/qcam/viewfinder_gl.cpp
> > index 9d2a6960..c4b10e1e 100644
> > --- a/src/apps/qcam/viewfinder_gl.cpp
> > +++ b/src/apps/qcam/viewfinder_gl.cpp
> > @@ -77,7 +77,7 @@ const QList<libcamera::PixelFormat> &ViewFinderGL::nativeFormats() const
> >
> > int ViewFinderGL::setFormat(const libcamera::PixelFormat &format, const QSize &size,
> > const libcamera::ColorSpace &colorSpace,
> > - unsigned int stride)
> > + unsigned int stride, const libcamera::Orientation &orientation)
> > {
> > if (format != format_ || colorSpace != colorSpace_) {
> > /*
> > @@ -101,6 +101,7 @@ int ViewFinderGL::setFormat(const libcamera::PixelFormat &format, const QSize &s
> >
> > size_ = size;
> > stride_ = stride;
> > + orientation_ = orientation;
> >
> > updateGeometry();
> > return 0;
> > @@ -511,7 +512,7 @@ void ViewFinderGL::initializeGL()
> > glEnable(GL_TEXTURE_2D);
> > glDisable(GL_DEPTH_TEST);
> >
> > - static const GLfloat coordinates[2][4][2]{
> > + GLfloat coordinates[2][4][2]{
> > {
> > /* Vertex coordinates */
> > { -1.0f, -1.0f },
> > @@ -528,6 +529,41 @@ void ViewFinderGL::initializeGL()
> > },
> > };
> >
> > + switch (orientation_) {
> > + case libcamera::Orientation::Rotate90:
> > + coordinates[0][0][0] = -1.0f;
> > + coordinates[0][0][1] = +1.0f;
> > + coordinates[0][1][0] = +1.0f;
> > + coordinates[0][1][1] = +1.0f;
> > + coordinates[0][2][0] = +1.0f;
> > + coordinates[0][2][1] = -1.0f;
> > + coordinates[0][3][0] = -1.0f;
> > + coordinates[0][3][1] = -1.0f;
> > + break;
> > + case libcamera::Orientation::Rotate180:
> > + coordinates[0][0][0] = +1.0f;
> > + coordinates[0][0][1] = +1.0f;
> > + coordinates[0][1][0] = +1.0f;
> > + coordinates[0][1][1] = -1.0f;
> > + coordinates[0][2][0] = -1.0f;
> > + coordinates[0][2][1] = -1.0f;
> > + coordinates[0][3][0] = -1.0f;
> > + coordinates[0][3][1] = +1.0f;
> > + break;
> > + case libcamera::Orientation::Rotate270:
> > + coordinates[0][0][0] = +1.0f;
> > + coordinates[0][0][1] = -1.0f;
> > + coordinates[0][1][0] = -1.0f;
> > + coordinates[0][1][1] = -1.0f;
> > + coordinates[0][2][0] = -1.0f;
> > + coordinates[0][2][1] = +1.0f;
> > + coordinates[0][3][0] = +1.0f;
> > + coordinates[0][3][1] = +1.0f;
> > + break;
> > + default:
> > + break;
> > + }
I'll have a look at how to improve this :-)
> > +
> > vertexBuffer_.create();
> > vertexBuffer_.bind();
> > vertexBuffer_.allocate(coordinates, sizeof(coordinates));
> > @@ -831,5 +867,9 @@ void ViewFinderGL::resizeGL(int w, int h)
> >
> > QSize ViewFinderGL::sizeHint() const
> > {
> > - return size_.isValid() ? size_ : QSize(640, 480);
> > + if (orientation_ == libcamera::Orientation::Rotate90 ||
> > + orientation_ == libcamera::Orientation::Rotate270)
> > + return size_.isValid() ? size_.transposed() : QSize(480, 640);
> > + else
> > + return size_.isValid() ? size_ : QSize(640, 480);
This would avoid hardcoding the default VGA size twice:
QSize size = size_.isValid() ? size_ : QSize(640, 480);
if (orientation_ == libcamera::Orientation::Rotate90 ||
orientation_ == libcamera::Orientation::Rotate270)
size.transpose();
return size;
> > }
> > diff --git a/src/apps/qcam/viewfinder_gl.h b/src/apps/qcam/viewfinder_gl.h
> > index 23744b41..1fc0f827 100644
> > --- a/src/apps/qcam/viewfinder_gl.h
> > +++ b/src/apps/qcam/viewfinder_gl.h
> > @@ -39,7 +39,8 @@ public:
> >
> > int setFormat(const libcamera::PixelFormat &format, const QSize &size,
> > const libcamera::ColorSpace &colorSpace,
> > - unsigned int stride) override;
> > + unsigned int stride,
> > + const libcamera::Orientation &orientation) override;
> > void render(libcamera::FrameBuffer *buffer, Image *image) override;
> > void stop() override;
> >
> > @@ -68,6 +69,7 @@ private:
> > libcamera::FrameBuffer *buffer_;
> > libcamera::PixelFormat format_;
> > libcamera::ColorSpace colorSpace_;
> > + libcamera::Orientation orientation_;
> > QSize size_;
> > unsigned int stride_;
> > Image *image_;
> > diff --git a/src/apps/qcam/viewfinder_qt.cpp b/src/apps/qcam/viewfinder_qt.cpp
> > index 4821c27d..583a74f7 100644
> > --- a/src/apps/qcam/viewfinder_qt.cpp
> > +++ b/src/apps/qcam/viewfinder_qt.cpp
> > @@ -57,7 +57,7 @@ const QList<libcamera::PixelFormat> &ViewFinderQt::nativeFormats() const
> >
> > int ViewFinderQt::setFormat(const libcamera::PixelFormat &format, const QSize &size,
> > [[maybe_unused]] const libcamera::ColorSpace &colorSpace,
> > - unsigned int stride)
> > + unsigned int stride, const libcamera::Orientation &orientation)
> > {
> > image_ = QImage();
> >
> > @@ -80,6 +80,15 @@ int ViewFinderQt::setFormat(const libcamera::PixelFormat &format, const QSize &s
> >
> > format_ = format;
> > size_ = size;
> > + orientation_ = orientation;
> > +
> > + bool success;
> > + int angle = libcamera::rotationFromOrientation(orientation, &success);
> > + if (!success) {
> > + qWarning() << "Unsupported orientation";
> > + return -EINVAL;
> > + }
> > + transform_ = QTransform().rotate(angle);
> >
> > updateGeometry();
> > return 0;
> > @@ -148,6 +157,7 @@ void ViewFinderQt::paintEvent(QPaintEvent *)
> >
> > /* If we have an image, draw it. */
> > if (!image_.isNull()) {
> > + image_ = image_.transformed(transform_);
This is quite inefficient, as it will result in a copy of the image. The
QPainter class supports applying transformations when drawing, could you
try it out ?
> > painter.drawImage(rect(), image_, image_.rect());
> > return;
> > }
> > @@ -179,5 +189,9 @@ void ViewFinderQt::paintEvent(QPaintEvent *)
> >
> > QSize ViewFinderQt::sizeHint() const
> > {
> > - return size_.isValid() ? size_ : QSize(640, 480);
> > + if (orientation_ == libcamera::Orientation::Rotate90 ||
> > + orientation_ == libcamera::Orientation::Rotate270)
> > + return size_.isValid() ? size_.transposed() : QSize(480, 640);
> > + else
> > + return size_.isValid() ? size_ : QSize(640, 480);
Same comment as above.
> > }
> > diff --git a/src/apps/qcam/viewfinder_qt.h b/src/apps/qcam/viewfinder_qt.h
> > index 4f4b9f11..309b39e5 100644
> > --- a/src/apps/qcam/viewfinder_qt.h
> > +++ b/src/apps/qcam/viewfinder_qt.h
> > @@ -33,7 +33,8 @@ public:
> >
> > int setFormat(const libcamera::PixelFormat &format, const QSize &size,
> > const libcamera::ColorSpace &colorSpace,
> > - unsigned int stride) override;
> > + unsigned int stride,
> > + const libcamera::Orientation &orientation) override;
> > void render(libcamera::FrameBuffer *buffer, Image *image) override;
> > void stop() override;
> >
> > @@ -51,6 +52,8 @@ private:
> >
> > libcamera::PixelFormat format_;
> > QSize size_;
> > + QTransform transform_;
> > + libcamera::Orientation orientation_;
> >
> > /* Camera stopped icon */
> > QSize vfSize_;
> > diff --git a/src/libcamera/orientation.cpp b/src/libcamera/orientation.cpp
> > index 47fd6a32..7afb37a8 100644
> > --- a/src/libcamera/orientation.cpp
> > +++ b/src/libcamera/orientation.cpp
> > @@ -92,6 +92,40 @@ Orientation orientationFromRotation(int angle, bool *success)
> > return Orientation::Rotate0;
> > }
> >
> > +/**
> > + * \brief Return the rotation angle for a given orientation
>
> I'm a bit unconfortable about having this in the API, as it might
> result in something more complex than what it actually looks like.
>
> All the 2d plane transforms expressed by Orientation include
> an optional horizontal flip, something that cannot be expressed with
> an angular rotation only.
>
> Look at the example images we have in the Orientation class
> documentation. If you only report the rotation angle (*)
> you won't be able to identify if an image has been flipped or not, and
> the result might not be what you expect.
>
> (*) Also expressing the rotation angle only is less trivial than what
> one might expect. First of all, if you want to return a rotation
> angle, you have to specify what the rotation describes:
>
> 1) The image rotation in the memory buffers
> 2) The correction to be applied to display the image in its natural
> orientation
>
> The Orientation class describes the image rotation in memory buffers,
> so I presume that's what you're expressing here, but it has to be made
> explicit in the documentation of the function. The direction of the
> rotation has to be specified as well. All of this is documented in the
> Orientation class, but if this function is made part of the API, it
> has to be specified here as well.
>
> I would suggest to infer the rotation angle from Orientation in the
> application and explicitly say that mirroring is not corrected there.
I agree. libcamera reports the image orientation, and it's up to the
application to decide what to then do. Different applications may have
different needs, so it's best to move this to the application side.
On a side note, if we kept this in libcamera, the patch would have
needed to be split in two, with one patch to add the new function, and a
separate patch for qcam. We try to stick to the "one change, one patch"
rule.
> > + * \param[in] orientation The orientation to convert to a rotation angle
> > + * \param[out] success Set to `true` if the given orientation is valid,
> > + * otherwise `false`
> > + * \return The rotation angle corresponding to the given orientation
> > + * if \a success was set to `true`, otherwise 0.
> > + */
> > +int rotationFromOrientation(const Orientation &orientation, bool *success)
> > +{
> > + if (success != nullptr)
> > + *success = true;
> > +
> > + switch (orientation) {
> > + case Orientation::Rotate0:
> > + case Orientation::Rotate0Mirror:
> > + return 0;
> > + case Orientation::Rotate90:
> > + case Orientation::Rotate90Mirror:
> > + return 90;
> > + case Orientation::Rotate180:
> > + case Orientation::Rotate180Mirror:
> > + return 180;
> > + case Orientation::Rotate270:
> > + case Orientation::Rotate270Mirror:
> > + return 270;
> > + }
> > +
> > + if (success != nullptr)
> > + *success = false;
> > +
> > + return 0;
> > +}
> > +
> > /**
> > * \brief Prints human-friendly names for Orientation items
> > * \param[in] out The output stream
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list