[libcamera-devel] [PATCH v5 2/4] qcam: new viewfinder hierarchy
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Sep 6 03:28:15 CEST 2020
On Sun, Sep 06, 2020 at 04:23:28AM +0300, Laurent Pinchart wrote:
> Hi Show,
>
> Thank you for the patch.
I spoke a bit too fast with the R-b tag below, this patch doesn't
compile. The following diff fixes it.
diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
index 612d978a73df..7406f0bd4512 100644
--- a/src/qcam/main_window.cpp
+++ b/src/qcam/main_window.cpp
@@ -28,6 +28,7 @@
#include <libcamera/version.h>
#include "dng_writer.h"
+#include "viewfinder_qt.h"
using namespace libcamera;
@@ -105,10 +106,11 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
setWindowTitle(title_);
connect(&titleTimer_, SIGNAL(timeout()), this, SLOT(updateTitle()));
- viewfinder_ = new ViewFinder(this);
- connect(viewfinder_, &ViewFinder::renderComplete,
+ ViewFinderQt *viewfinder = new ViewFinderQt(this);
+ connect(viewfinder, &ViewFinderQt::renderComplete,
this, &MainWindow::queueRequest);
- setCentralWidget(viewfinder_);
+ viewfinder_ = viewfinder;
+ setCentralWidget(viewfinder);
adjustSize();
/* Hotplug/unplug support */
> On Fri, Sep 04, 2020 at 04:43:14PM +0800, Show Liu wrote:
> > Create viewfinder base class and rename the original viewfinder
> > as default Qt render widget
> >
> > Signed-off-by: Show Liu <show.liu at linaro.org>
> > ---
> > src/qcam/meson.build | 4 +-
> > src/qcam/viewfinder.h | 60 ++++-------------
> > .../{viewfinder.cpp => viewfinder_qt.cpp} | 24 +++----
> > src/qcam/viewfinder_qt.h | 67 +++++++++++++++++++
> > 4 files changed, 94 insertions(+), 61 deletions(-)
> > rename src/qcam/{viewfinder.cpp => viewfinder_qt.cpp} (86%)
> > create mode 100644 src/qcam/viewfinder_qt.h
> >
> > diff --git a/src/qcam/meson.build b/src/qcam/meson.build
> > index e0c6f26..a4bad0a 100644
> > --- a/src/qcam/meson.build
> > +++ b/src/qcam/meson.build
> > @@ -6,12 +6,12 @@ qcam_sources = files([
> > 'format_converter.cpp',
> > 'main.cpp',
> > 'main_window.cpp',
> > - 'viewfinder.cpp',
> > + 'viewfinder_qt.cpp',
> > ])
> >
> > qcam_moc_headers = files([
> > 'main_window.h',
> > - 'viewfinder.h',
> > + 'viewfinder_qt.h',
> > ])
> >
> > qcam_resources = files([
> > diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h
> > index 26a1320..fa7785f 100644
> > --- a/src/qcam/viewfinder.h
> > +++ b/src/qcam/viewfinder.h
> > @@ -2,70 +2,36 @@
> > /*
> > * Copyright (C) 2019, Google Inc.
> > *
> > - * viewfinder.h - qcam - Viewfinder
> > + * viewfinder.h - qcam - Viewfinder base class
> > */
> > #ifndef __QCAM_VIEWFINDER_H__
> > #define __QCAM_VIEWFINDER_H__
> >
> > -#include <stddef.h>
> > -
> > -#include <QIcon>
> > -#include <QList>
> > #include <QImage>
> > -#include <QMutex>
> > +#include <QList>
> > +#include <QMap>
>
> QMap isn't needed.
>
> > #include <QSize>
> > -#include <QWidget>
> >
> > #include <libcamera/buffer.h>
> > -#include <libcamera/pixel_format.h>
> > -
> > -#include "format_converter.h"
> > -
> > -class QImage;
> > +#include <libcamera/formats.h>
> >
> > struct MappedBuffer {
> > void *memory;
> > size_t size;
> > };
> >
> > -class ViewFinder : public QWidget
> > +class ViewFinder
> > {
> > - Q_OBJECT
> > -
> > public:
> > - ViewFinder(QWidget *parent);
> > - ~ViewFinder();
> > -
> > - const QList<libcamera::PixelFormat> &nativeFormats() const;
> > + ViewFinder(){};
>
> You don't need to declare an empty constructor, you can just leave it
> out.
>
> > + virtual ~ViewFinder(){};
>
> Missing space before {}, and no need for a ; at the end of the line.
>
> >
> > - int setFormat(const libcamera::PixelFormat &format, const QSize &size);
> > - void render(libcamera::FrameBuffer *buffer, MappedBuffer *map);
> > - void stop();
> > + virtual const QList<libcamera::PixelFormat> &nativeFormats() const = 0;
> >
> > - QImage getCurrentImage();
> > + virtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) = 0;
> > + virtual void render(libcamera::FrameBuffer *buffer, MappedBuffer *map) = 0;
> > + virtual void stop() = 0;
> >
> > -Q_SIGNALS:
> > - void renderComplete(libcamera::FrameBuffer *buffer);
> > -
> > -protected:
> > - void paintEvent(QPaintEvent *) override;
> > - QSize sizeHint() const override;
> > -
> > -private:
> > - FormatConverter converter_;
> > -
> > - libcamera::PixelFormat format_;
> > - QSize size_;
> > -
> > - /* Camera stopped icon */
> > - QSize vfSize_;
> > - QIcon icon_;
> > - QPixmap pixmap_;
> > -
> > - /* Buffer and render image */
> > - libcamera::FrameBuffer *buffer_;
> > - QImage image_;
> > - QMutex mutex_; /* Prevent concurrent access to image_ */
> > + virtual QImage getCurrentImage() = 0;
> > };
> > -
>
> You can keep the blank line here.
>
> > -#endif /* __QCAM_VIEWFINDER__ */
> > +#endif /* __QCAM_VIEWFINDER_H__ */
>
> Oops, good catch :-)
>
> > diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder_qt.cpp
> > similarity index 86%
> > rename from src/qcam/viewfinder.cpp
> > rename to src/qcam/viewfinder_qt.cpp
> > index dcf8a15..072f024 100644
> > --- a/src/qcam/viewfinder.cpp
> > +++ b/src/qcam/viewfinder_qt.cpp
> > @@ -2,10 +2,10 @@
> > /*
> > * Copyright (C) 2019, Google Inc.
> > *
> > - * viewfinder.cpp - qcam - Viewfinder
> > + * viewfinder_qt.cpp - qcam - default Viewfinder for rendering by Qt
>
> How about "QPainter-based ViewFinder" ? Same for the header file below.
>
> > */
> >
> > -#include "viewfinder.h"
> > +#include "viewfinder_qt.h"
> >
> > #include <stdint.h>
> > #include <utility>
> > @@ -33,24 +33,24 @@ static const QMap<libcamera::PixelFormat, QImage::Format> nativeFormats
> > { libcamera::formats::BGR888, QImage::Format_RGB888 },
> > };
> >
> > -ViewFinder::ViewFinder(QWidget *parent)
> > +ViewFinderQt::ViewFinderQt(QWidget *parent)
> > : QWidget(parent), buffer_(nullptr)
> > {
> > icon_ = QIcon(":camera-off.svg");
> > }
> >
> > -ViewFinder::~ViewFinder()
> > +ViewFinderQt::~ViewFinderQt()
> > {
> > }
> >
> > -const QList<libcamera::PixelFormat> &ViewFinder::nativeFormats() const
> > +const QList<libcamera::PixelFormat> &ViewFinderQt::nativeFormats() const
> > {
> > static const QList<libcamera::PixelFormat> formats = ::nativeFormats.keys();
> > return formats;
> > }
> >
> > -int ViewFinder::setFormat(const libcamera::PixelFormat &format,
> > - const QSize &size)
> > +int ViewFinderQt::setFormat(const libcamera::PixelFormat &format,
> > + const QSize &size)
> > {
> > image_ = QImage();
> >
> > @@ -78,7 +78,7 @@ int ViewFinder::setFormat(const libcamera::PixelFormat &format,
> > return 0;
> > }
> >
> > -void ViewFinder::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)
> > +void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)
> > {
> > if (buffer->planes().size() != 1) {
> > qWarning() << "Multi-planar buffers are not supported";
> > @@ -121,7 +121,7 @@ void ViewFinder::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)
> > renderComplete(buffer);
> > }
> >
> > -void ViewFinder::stop()
> > +void ViewFinderQt::stop()
> > {
> > image_ = QImage();
> >
> > @@ -133,14 +133,14 @@ void ViewFinder::stop()
> > update();
> > }
> >
> > -QImage ViewFinder::getCurrentImage()
> > +QImage ViewFinderQt::getCurrentImage()
> > {
> > QMutexLocker locker(&mutex_);
> >
> > return image_.copy();
> > }
> >
> > -void ViewFinder::paintEvent(QPaintEvent *)
> > +void ViewFinderQt::paintEvent(QPaintEvent *)
> > {
> > QPainter painter(this);
> >
> > @@ -175,7 +175,7 @@ void ViewFinder::paintEvent(QPaintEvent *)
> > painter.drawPixmap(point, pixmap_);
> > }
> >
> > -QSize ViewFinder::sizeHint() const
> > +QSize ViewFinderQt::sizeHint() const
> > {
> > return size_.isValid() ? size_ : QSize(640, 480);
> > }
> > diff --git a/src/qcam/viewfinder_qt.h b/src/qcam/viewfinder_qt.h
> > new file mode 100644
> > index 0000000..ee2abab
> > --- /dev/null
> > +++ b/src/qcam/viewfinder_qt.h
> > @@ -0,0 +1,67 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * viewfinder_qt.h - qcam - default Viewfinder for rendering by Qt
> > + */
> > +#ifndef __QCAM_VIEWFINDER_QT_H__
> > +#define __QCAM_VIEWFINDER_QT_H__
> > +
> > +#include <stddef.h>
>
> stddef.h doesn't seem to be needed.
>
> > +
> > +#include <QIcon>
> > +#include <QImage>
> > +#include <QList>
> > +#include <QMutex>
> > +#include <QSize>
> > +#include <QWidget>
> > +
> > +#include <libcamera/buffer.h>
> > +#include <libcamera/formats.h>
> > +#include <libcamera/pixel_format.h>
> > +
> > +#include "format_converter.h"
> > +#include "viewfinder.h"
> > +
> > +class QImage;
>
> You can drop this, QImage is included.
>
> > +
> > +class ViewFinderQt : public QWidget, public ViewFinder
> > +{
> > + Q_OBJECT
> > +
> > +public:
> > + ViewFinderQt(QWidget *parent);
> > + ~ViewFinderQt();
> > +
> > + const QList<libcamera::PixelFormat> &nativeFormats() const override;
> > +
> > + int setFormat(const libcamera::PixelFormat &format, const QSize &size) override;
> > + void render(libcamera::FrameBuffer *buffer, MappedBuffer *map) override;
> > + void stop() override;
> > +
> > + QImage getCurrentImage() override;
> > +
> > +Q_SIGNALS:
> > + void renderComplete(libcamera::FrameBuffer *buffer);
> > +
> > +protected:
> > + void paintEvent(QPaintEvent *) override;
> > + QSize sizeHint() const override;
> > +
> > +private:
> > + FormatConverter converter_;
> > +
> > + libcamera::PixelFormat format_;
> > + QSize size_;
> > +
> > + /* Camera stopped icon */
> > + QSize vfSize_;
> > + QIcon icon_;
> > + QPixmap pixmap_;
> > +
> > + /* Buffer and render image */
> > + libcamera::FrameBuffer *buffer_;
> > + QImage image_;
> > + QMutex mutex_; /* Prevent concurrent access to image_ */
> > +};
>
> Missing blank line.
>
> With these small issues addressed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > +#endif /* __QCAM_VIEWFINDER_QT_H__ */
>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list