[{"id":12332,"web_url":"https://patchwork.libcamera.org/comment/12332/","msgid":"<20200906012328.GE25630@pendragon.ideasonboard.com>","date":"2020-09-06T01:23:28","subject":"Re: [libcamera-devel] [PATCH v5 2/4] qcam: new viewfinder hierarchy","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Show,\n\nThank you for the patch.\n\nOn Fri, Sep 04, 2020 at 04:43:14PM +0800, Show Liu wrote:\n> Create viewfinder base class and rename the original viewfinder\n> as default Qt render widget\n> \n> Signed-off-by: Show Liu \n> ---\n> src/qcam/meson.build | 4 +-\n> src/qcam/viewfinder.h | 60 ++++-------------\n> .../{viewfinder.cpp => viewfinder_qt.cpp} | 24 +++----\n> src/qcam/viewfinder_qt.h | 67 +++++++++++++++++++\n> 4 files changed, 94 insertions(+), 61 deletions(-)\n> rename src/qcam/{viewfinder.cpp => viewfinder_qt.cpp} (86%)\n> create mode 100644 src/qcam/viewfinder_qt.h\n> \n> diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> index e0c6f26..a4bad0a 100644\n> --- a/src/qcam/meson.build\n> +++ b/src/qcam/meson.build\n> @@ -6,12 +6,12 @@ qcam_sources = files([\n> 'format_converter.cpp',\n> 'main.cpp',\n> 'main_window.cpp',\n> - 'viewfinder.cpp',\n> + 'viewfinder_qt.cpp',\n> ])\n> \n> qcam_moc_headers = files([\n> 'main_window.h',\n> - 'viewfinder.h',\n> + 'viewfinder_qt.h',\n> ])\n> \n> qcam_resources = files([\n> diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h\n> index 26a1320..fa7785f 100644\n> --- a/src/qcam/viewfinder.h\n> +++ b/src/qcam/viewfinder.h\n> @@ -2,70 +2,36 @@\n> /*\n> * Copyright (C) 2019, Google Inc.\n> *\n> - * viewfinder.h - qcam - Viewfinder\n> + * viewfinder.h - qcam - Viewfinder base class\n> */\n> #ifndef __QCAM_VIEWFINDER_H__\n> #define __QCAM_VIEWFINDER_H__\n> \n> -#include \n> -\n> -#include \n> -#include \n> #include \n> -#include \n> +#include \n> +#include \n\nQMap isn't needed.\n\n> #include \n> -#include \n> \n> #include \n> -#include \n> -\n> -#include \"format_converter.h\"\n> -\n> -class QImage;\n> +#include \n> \n> struct MappedBuffer {\n> \tvoid *memory;\n> \tsize_t size;\n> };\n> \n> -class ViewFinder : public QWidget\n> +class ViewFinder\n> {\n> -\tQ_OBJECT\n> -\n> public:\n> -\tViewFinder(QWidget *parent);\n> -\t~ViewFinder();\n> -\n> -\tconst QList &nativeFormats() const;\n> +\tViewFinder(){};\n\nYou don't need to declare an empty constructor, you can just leave it\nout.\n\n> +\tvirtual ~ViewFinder(){};\n\nMissing space before {}, and no need for a ; at the end of the line.\n\n> \n> -\tint setFormat(const libcamera::PixelFormat &format, const QSize &size);\n> -\tvoid render(libcamera::FrameBuffer *buffer, MappedBuffer *map);\n> -\tvoid stop();\n> +\tvirtual const QList &nativeFormats() const = 0;\n> \n> -\tQImage getCurrentImage();\n> +\tvirtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) = 0;\n> +\tvirtual void render(libcamera::FrameBuffer *buffer, MappedBuffer *map) = 0;\n> +\tvirtual void stop() = 0;\n> \n> -Q_SIGNALS:\n> -\tvoid renderComplete(libcamera::FrameBuffer *buffer);\n> -\n> -protected:\n> -\tvoid paintEvent(QPaintEvent *) override;\n> -\tQSize sizeHint() const override;\n> -\n> -private:\n> -\tFormatConverter converter_;\n> -\n> -\tlibcamera::PixelFormat format_;\n> -\tQSize size_;\n> -\n> -\t/* Camera stopped icon */\n> -\tQSize vfSize_;\n> -\tQIcon icon_;\n> -\tQPixmap pixmap_;\n> -\n> -\t/* Buffer and render image */\n> -\tlibcamera::FrameBuffer *buffer_;\n> -\tQImage image_;\n> -\tQMutex mutex_; /* Prevent concurrent access to image_ */\n> +\tvirtual QImage getCurrentImage() = 0;\n> };\n> -\n\nYou can keep the blank line here.\n\n> -#endif /* __QCAM_VIEWFINDER__ */\n> +#endif /* __QCAM_VIEWFINDER_H__ */\n\nOops, good catch :-)\n\n> diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder_qt.cpp\n> similarity index 86%\n> rename from src/qcam/viewfinder.cpp\n> rename to src/qcam/viewfinder_qt.cpp\n> index dcf8a15..072f024 100644\n> --- a/src/qcam/viewfinder.cpp\n> +++ b/src/qcam/viewfinder_qt.cpp\n> @@ -2,10 +2,10 @@\n> /*\n> * Copyright (C) 2019, Google Inc.\n> *\n> - * viewfinder.cpp - qcam - Viewfinder\n> + * viewfinder_qt.cpp - qcam - default Viewfinder for rendering by Qt\n\nHow about \"QPainter-based ViewFinder\" ? Same for the header file below.\n\n> */\n> \n> -#include \"viewfinder.h\"\n> +#include \"viewfinder_qt.h\"\n> \n> #include \n> #include \n> @@ -33,24 +33,24 @@ static const QMap nativeFormats\n> \t{ libcamera::formats::BGR888, QImage::Format_RGB888 },\n> };\n> \n> -ViewFinder::ViewFinder(QWidget *parent)\n> +ViewFinderQt::ViewFinderQt(QWidget *parent)\n> \t: QWidget(parent), buffer_(nullptr)\n> {\n> \ticon_ = QIcon(\":camera-off.svg\");\n> }\n> \n> -ViewFinder::~ViewFinder()\n> +ViewFinderQt::~ViewFinderQt()\n> {\n> }\n> \n> -const QList &ViewFinder::nativeFormats() const\n> +const QList &ViewFinderQt::nativeFormats() const\n> {\n> \tstatic const QList formats = ::nativeFormats.keys();\n> \treturn formats;\n> }\n> \n> -int ViewFinder::setFormat(const libcamera::PixelFormat &format,\n> -\t\t\t const QSize &size)\n> +int ViewFinderQt::setFormat(const libcamera::PixelFormat &format,\n> +\t\t\t const QSize &size)\n> {\n> \timage_ = QImage();\n> \n> @@ -78,7 +78,7 @@ int ViewFinder::setFormat(const libcamera::PixelFormat &format,\n> \treturn 0;\n> }\n> \n> -void ViewFinder::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> +void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> {\n> \tif (buffer->planes().size() != 1) {\n> \t\tqWarning() << \"Multi-planar buffers are not supported\";\n> @@ -121,7 +121,7 @@ void ViewFinder::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> \t\trenderComplete(buffer);\n> }\n> \n> -void ViewFinder::stop()\n> +void ViewFinderQt::stop()\n> {\n> \timage_ = QImage();\n> \n> @@ -133,14 +133,14 @@ void ViewFinder::stop()\n> \tupdate();\n> }\n> \n> -QImage ViewFinder::getCurrentImage()\n> +QImage ViewFinderQt::getCurrentImage()\n> {\n> \tQMutexLocker locker(&mutex_);\n> \n> \treturn image_.copy();\n> }\n> \n> -void ViewFinder::paintEvent(QPaintEvent *)\n> +void ViewFinderQt::paintEvent(QPaintEvent *)\n> {\n> \tQPainter painter(this);\n> \n> @@ -175,7 +175,7 @@ void ViewFinder::paintEvent(QPaintEvent *)\n> \tpainter.drawPixmap(point, pixmap_);\n> }\n> \n> -QSize ViewFinder::sizeHint() const\n> +QSize ViewFinderQt::sizeHint() const\n> {\n> \treturn size_.isValid() ? size_ : QSize(640, 480);\n> }\n> diff --git a/src/qcam/viewfinder_qt.h b/src/qcam/viewfinder_qt.h\n> new file mode 100644\n> index 0000000..ee2abab\n> --- /dev/null\n> +++ b/src/qcam/viewfinder_qt.h\n> @@ -0,0 +1,67 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * viewfinder_qt.h - qcam - default Viewfinder for rendering by Qt\n> + */\n> +#ifndef __QCAM_VIEWFINDER_QT_H__\n> +#define __QCAM_VIEWFINDER_QT_H__\n> +\n> +#include \n\nstddef.h doesn't seem to be needed.\n\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"format_converter.h\"\n> +#include \"viewfinder.h\"\n> +\n> +class QImage;\n\nYou can drop this, QImage is included.\n\n> +\n> +class ViewFinderQt : public QWidget, public ViewFinder\n> +{\n> +\tQ_OBJECT\n> +\n> +public:\n> +\tViewFinderQt(QWidget *parent);\n> +\t~ViewFinderQt();\n> +\n> +\tconst QList &nativeFormats() const override;\n> +\n> +\tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n> +\tvoid render(libcamera::FrameBuffer *buffer, MappedBuffer *map) override;\n> +\tvoid stop() override;\n> +\n> +\tQImage getCurrentImage() override;\n> +\n> +Q_SIGNALS:\n> +\tvoid renderComplete(libcamera::FrameBuffer *buffer);\n> +\n> +protected:\n> +\tvoid paintEvent(QPaintEvent *) override;\n> +\tQSize sizeHint() const override;\n> +\n> +private:\n> +\tFormatConverter converter_;\n> +\n> +\tlibcamera::PixelFormat format_;\n> +\tQSize size_;\n> +\n> +\t/* Camera stopped icon */\n> +\tQSize vfSize_;\n> +\tQIcon icon_;\n> +\tQPixmap pixmap_;\n> +\n> +\t/* Buffer and render image */\n> +\tlibcamera::FrameBuffer *buffer_;\n> +\tQImage image_;\n> +\tQMutex mutex_; /* Prevent concurrent access to image_ */\n> +};\n\nMissing blank line.\n\nWith these small issues addressed,\n\nReviewed-by: Laurent Pinchart \n\n> +#endif /* __QCAM_VIEWFINDER_QT_H__ */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 301EDBDB1C\n\tfor ;\n\tSun, 6 Sep 2020 01:23:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AC11B62B5D;\n\tSun, 6 Sep 2020 03:23:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 13E1D61EA0\n\tfor ;\n\tSun, 6 Sep 2020 03:23:54 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 18E7F277;\n\tSun, 6 Sep 2020 03:23:52 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"HMmSDgTu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599355432;\n\tbh=90W7QYQZg/X3hgz+ccbhZlTOx9jqlISMeaFXJkw83KU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HMmSDgTuPy/ABbQqFW3b/Vm9xK3H1yC6716HLycSC7gnwie5xo/vt2Fk0mtcbrCVt\n\tJ/zW/V2O8hvCNVBFnEPPM/dvmc/8liBRRttHjRn7kpLBZGXwQfZj07pdeOaYRkiVE7\n\tQQ6LgycexAT8XlbX3iAsM2PGPcvsr3MDKQ+i/DWQ=","Date":"Sun, 6 Sep 2020 04:23:28 +0300","From":"Laurent Pinchart ","To":"Show Liu ","Message-ID":"<20200906012328.GE25630@pendragon.ideasonboard.com>","References":"<20200904084316.7319-1-show.liu@linaro.org>\n\t<20200904084316.7319-4-show.liu@linaro.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200904084316.7319-4-show.liu@linaro.org>","Subject":"Re: [libcamera-devel] [PATCH v5 2/4] qcam: new viewfinder hierarchy","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12333,"web_url":"https://patchwork.libcamera.org/comment/12333/","msgid":"<20200906012815.GF25630@pendragon.ideasonboard.com>","date":"2020-09-06T01:28:15","subject":"Re: [libcamera-devel] [PATCH v5 2/4] qcam: new viewfinder hierarchy","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Sun, Sep 06, 2020 at 04:23:28AM +0300, Laurent Pinchart wrote:\n> Hi Show,\n> \n> Thank you for the patch.\n\nI spoke a bit too fast with the R-b tag below, this patch doesn't\ncompile. The following diff fixes it.\n\ndiff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\nindex 612d978a73df..7406f0bd4512 100644\n--- a/src/qcam/main_window.cpp\n+++ b/src/qcam/main_window.cpp\n@@ -28,6 +28,7 @@\n #include \n\n #include \"dng_writer.h\"\n+#include \"viewfinder_qt.h\"\n\n using namespace libcamera;\n\n@@ -105,10 +106,11 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n \tsetWindowTitle(title_);\n \tconnect(&titleTimer_, SIGNAL(timeout()), this, SLOT(updateTitle()));\n\n-\tviewfinder_ = new ViewFinder(this);\n-\tconnect(viewfinder_, &ViewFinder::renderComplete,\n+\tViewFinderQt *viewfinder = new ViewFinderQt(this);\n+\tconnect(viewfinder, &ViewFinderQt::renderComplete,\n \t\tthis, &MainWindow::queueRequest);\n-\tsetCentralWidget(viewfinder_);\n+\tviewfinder_ = viewfinder;\n+\tsetCentralWidget(viewfinder);\n \tadjustSize();\n\n \t/* Hotplug/unplug support */\n\n> On Fri, Sep 04, 2020 at 04:43:14PM +0800, Show Liu wrote:\n> > Create viewfinder base class and rename the original viewfinder\n> > as default Qt render widget\n> > \n> > Signed-off-by: Show Liu \n> > ---\n> > src/qcam/meson.build | 4 +-\n> > src/qcam/viewfinder.h | 60 ++++-------------\n> > .../{viewfinder.cpp => viewfinder_qt.cpp} | 24 +++----\n> > src/qcam/viewfinder_qt.h | 67 +++++++++++++++++++\n> > 4 files changed, 94 insertions(+), 61 deletions(-)\n> > rename src/qcam/{viewfinder.cpp => viewfinder_qt.cpp} (86%)\n> > create mode 100644 src/qcam/viewfinder_qt.h\n> > \n> > diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> > index e0c6f26..a4bad0a 100644\n> > --- a/src/qcam/meson.build\n> > +++ b/src/qcam/meson.build\n> > @@ -6,12 +6,12 @@ qcam_sources = files([\n> > 'format_converter.cpp',\n> > 'main.cpp',\n> > 'main_window.cpp',\n> > - 'viewfinder.cpp',\n> > + 'viewfinder_qt.cpp',\n> > ])\n> > \n> > qcam_moc_headers = files([\n> > 'main_window.h',\n> > - 'viewfinder.h',\n> > + 'viewfinder_qt.h',\n> > ])\n> > \n> > qcam_resources = files([\n> > diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h\n> > index 26a1320..fa7785f 100644\n> > --- a/src/qcam/viewfinder.h\n> > +++ b/src/qcam/viewfinder.h\n> > @@ -2,70 +2,36 @@\n> > /*\n> > * Copyright (C) 2019, Google Inc.\n> > *\n> > - * viewfinder.h - qcam - Viewfinder\n> > + * viewfinder.h - qcam - Viewfinder base class\n> > */\n> > #ifndef __QCAM_VIEWFINDER_H__\n> > #define __QCAM_VIEWFINDER_H__\n> > \n> > -#include \n> > -\n> > -#include \n> > -#include \n> > #include \n> > -#include \n> > +#include \n> > +#include \n> \n> QMap isn't needed.\n> \n> > #include \n> > -#include \n> > \n> > #include \n> > -#include \n> > -\n> > -#include \"format_converter.h\"\n> > -\n> > -class QImage;\n> > +#include \n> > \n> > struct MappedBuffer {\n> > \tvoid *memory;\n> > \tsize_t size;\n> > };\n> > \n> > -class ViewFinder : public QWidget\n> > +class ViewFinder\n> > {\n> > -\tQ_OBJECT\n> > -\n> > public:\n> > -\tViewFinder(QWidget *parent);\n> > -\t~ViewFinder();\n> > -\n> > -\tconst QList &nativeFormats() const;\n> > +\tViewFinder(){};\n> \n> You don't need to declare an empty constructor, you can just leave it\n> out.\n> \n> > +\tvirtual ~ViewFinder(){};\n> \n> Missing space before {}, and no need for a ; at the end of the line.\n> \n> > \n> > -\tint setFormat(const libcamera::PixelFormat &format, const QSize &size);\n> > -\tvoid render(libcamera::FrameBuffer *buffer, MappedBuffer *map);\n> > -\tvoid stop();\n> > +\tvirtual const QList &nativeFormats() const = 0;\n> > \n> > -\tQImage getCurrentImage();\n> > +\tvirtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) = 0;\n> > +\tvirtual void render(libcamera::FrameBuffer *buffer, MappedBuffer *map) = 0;\n> > +\tvirtual void stop() = 0;\n> > \n> > -Q_SIGNALS:\n> > -\tvoid renderComplete(libcamera::FrameBuffer *buffer);\n> > -\n> > -protected:\n> > -\tvoid paintEvent(QPaintEvent *) override;\n> > -\tQSize sizeHint() const override;\n> > -\n> > -private:\n> > -\tFormatConverter converter_;\n> > -\n> > -\tlibcamera::PixelFormat format_;\n> > -\tQSize size_;\n> > -\n> > -\t/* Camera stopped icon */\n> > -\tQSize vfSize_;\n> > -\tQIcon icon_;\n> > -\tQPixmap pixmap_;\n> > -\n> > -\t/* Buffer and render image */\n> > -\tlibcamera::FrameBuffer *buffer_;\n> > -\tQImage image_;\n> > -\tQMutex mutex_; /* Prevent concurrent access to image_ */\n> > +\tvirtual QImage getCurrentImage() = 0;\n> > };\n> > -\n> \n> You can keep the blank line here.\n> \n> > -#endif /* __QCAM_VIEWFINDER__ */\n> > +#endif /* __QCAM_VIEWFINDER_H__ */\n> \n> Oops, good catch :-)\n> \n> > diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder_qt.cpp\n> > similarity index 86%\n> > rename from src/qcam/viewfinder.cpp\n> > rename to src/qcam/viewfinder_qt.cpp\n> > index dcf8a15..072f024 100644\n> > --- a/src/qcam/viewfinder.cpp\n> > +++ b/src/qcam/viewfinder_qt.cpp\n> > @@ -2,10 +2,10 @@\n> > /*\n> > * Copyright (C) 2019, Google Inc.\n> > *\n> > - * viewfinder.cpp - qcam - Viewfinder\n> > + * viewfinder_qt.cpp - qcam - default Viewfinder for rendering by Qt\n> \n> How about \"QPainter-based ViewFinder\" ? Same for the header file below.\n> \n> > */\n> > \n> > -#include \"viewfinder.h\"\n> > +#include \"viewfinder_qt.h\"\n> > \n> > #include \n> > #include \n> > @@ -33,24 +33,24 @@ static const QMap nativeFormats\n> > \t{ libcamera::formats::BGR888, QImage::Format_RGB888 },\n> > };\n> > \n> > -ViewFinder::ViewFinder(QWidget *parent)\n> > +ViewFinderQt::ViewFinderQt(QWidget *parent)\n> > \t: QWidget(parent), buffer_(nullptr)\n> > {\n> > \ticon_ = QIcon(\":camera-off.svg\");\n> > }\n> > \n> > -ViewFinder::~ViewFinder()\n> > +ViewFinderQt::~ViewFinderQt()\n> > {\n> > }\n> > \n> > -const QList &ViewFinder::nativeFormats() const\n> > +const QList &ViewFinderQt::nativeFormats() const\n> > {\n> > \tstatic const QList formats = ::nativeFormats.keys();\n> > \treturn formats;\n> > }\n> > \n> > -int ViewFinder::setFormat(const libcamera::PixelFormat &format,\n> > -\t\t\t const QSize &size)\n> > +int ViewFinderQt::setFormat(const libcamera::PixelFormat &format,\n> > +\t\t\t const QSize &size)\n> > {\n> > \timage_ = QImage();\n> > \n> > @@ -78,7 +78,7 @@ int ViewFinder::setFormat(const libcamera::PixelFormat &format,\n> > \treturn 0;\n> > }\n> > \n> > -void ViewFinder::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > +void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > {\n> > \tif (buffer->planes().size() != 1) {\n> > \t\tqWarning() << \"Multi-planar buffers are not supported\";\n> > @@ -121,7 +121,7 @@ void ViewFinder::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > \t\trenderComplete(buffer);\n> > }\n> > \n> > -void ViewFinder::stop()\n> > +void ViewFinderQt::stop()\n> > {\n> > \timage_ = QImage();\n> > \n> > @@ -133,14 +133,14 @@ void ViewFinder::stop()\n> > \tupdate();\n> > }\n> > \n> > -QImage ViewFinder::getCurrentImage()\n> > +QImage ViewFinderQt::getCurrentImage()\n> > {\n> > \tQMutexLocker locker(&mutex_);\n> > \n> > \treturn image_.copy();\n> > }\n> > \n> > -void ViewFinder::paintEvent(QPaintEvent *)\n> > +void ViewFinderQt::paintEvent(QPaintEvent *)\n> > {\n> > \tQPainter painter(this);\n> > \n> > @@ -175,7 +175,7 @@ void ViewFinder::paintEvent(QPaintEvent *)\n> > \tpainter.drawPixmap(point, pixmap_);\n> > }\n> > \n> > -QSize ViewFinder::sizeHint() const\n> > +QSize ViewFinderQt::sizeHint() const\n> > {\n> > \treturn size_.isValid() ? size_ : QSize(640, 480);\n> > }\n> > diff --git a/src/qcam/viewfinder_qt.h b/src/qcam/viewfinder_qt.h\n> > new file mode 100644\n> > index 0000000..ee2abab\n> > --- /dev/null\n> > +++ b/src/qcam/viewfinder_qt.h\n> > @@ -0,0 +1,67 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * viewfinder_qt.h - qcam - default Viewfinder for rendering by Qt\n> > + */\n> > +#ifndef __QCAM_VIEWFINDER_QT_H__\n> > +#define __QCAM_VIEWFINDER_QT_H__\n> > +\n> > +#include \n> \n> stddef.h doesn't seem to be needed.\n> \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"format_converter.h\"\n> > +#include \"viewfinder.h\"\n> > +\n> > +class QImage;\n> \n> You can drop this, QImage is included.\n> \n> > +\n> > +class ViewFinderQt : public QWidget, public ViewFinder\n> > +{\n> > +\tQ_OBJECT\n> > +\n> > +public:\n> > +\tViewFinderQt(QWidget *parent);\n> > +\t~ViewFinderQt();\n> > +\n> > +\tconst QList &nativeFormats() const override;\n> > +\n> > +\tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n> > +\tvoid render(libcamera::FrameBuffer *buffer, MappedBuffer *map) override;\n> > +\tvoid stop() override;\n> > +\n> > +\tQImage getCurrentImage() override;\n> > +\n> > +Q_SIGNALS:\n> > +\tvoid renderComplete(libcamera::FrameBuffer *buffer);\n> > +\n> > +protected:\n> > +\tvoid paintEvent(QPaintEvent *) override;\n> > +\tQSize sizeHint() const override;\n> > +\n> > +private:\n> > +\tFormatConverter converter_;\n> > +\n> > +\tlibcamera::PixelFormat format_;\n> > +\tQSize size_;\n> > +\n> > +\t/* Camera stopped icon */\n> > +\tQSize vfSize_;\n> > +\tQIcon icon_;\n> > +\tQPixmap pixmap_;\n> > +\n> > +\t/* Buffer and render image */\n> > +\tlibcamera::FrameBuffer *buffer_;\n> > +\tQImage image_;\n> > +\tQMutex mutex_; /* Prevent concurrent access to image_ */\n> > +};\n> \n> Missing blank line.\n> \n> With these small issues addressed,\n> \n> Reviewed-by: Laurent Pinchart \n> \n> > +#endif /* __QCAM_VIEWFINDER_QT_H__ */\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id F063CBDB1D\n\tfor ;\n\tSun, 6 Sep 2020 01:28:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 823DA629B6;\n\tSun, 6 Sep 2020 03:28:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EFB0B61EA0\n\tfor ;\n\tSun, 6 Sep 2020 03:28:39 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6FC2D277;\n\tSun, 6 Sep 2020 03:28:39 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jZwtEsCn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599355719;\n\tbh=sF97ysPLMgS7S6byOKEBvdY6aMRjm/mPBckOpv5X2pM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jZwtEsCnEypr7vuBnvHh5qcDMF9L5tcF858xl5Q0mW1QF0d0Z+SdcSDyQglmfcTQ9\n\tKJj2FHvqP0/55CcAln3Jio5L79QoYIEGP/macpMTc5R/pWCq69lKx4wwDo+0CIdBOV\n\ttmHMT6j3GNjdJy9bL57Q7NMcNjDxcC4QgLTWIor8=","Date":"Sun, 6 Sep 2020 04:28:15 +0300","From":"Laurent Pinchart ","To":"Show Liu ","Message-ID":"<20200906012815.GF25630@pendragon.ideasonboard.com>","References":"<20200904084316.7319-1-show.liu@linaro.org>\n\t<20200904084316.7319-4-show.liu@linaro.org>\n\t<20200906012328.GE25630@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200906012328.GE25630@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 2/4] qcam: new viewfinder hierarchy","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]