[{"id":3764,"web_url":"https://patchwork.libcamera.org/comment/3764/","msgid":"<20200214113602.GE4831@pendragon.ideasonboard.com>","date":"2020-02-14T11:36:02","subject":"Re: [libcamera-devel] [PATCH v2 7/7] qcam: Provide save image\n\tfunctionality","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Fri, Feb 14, 2020 at 12:18:10AM +0000, Kieran Bingham wrote:\n> Implement a save image button on the toolbar which will take a current\n> viewfinder image and present the user with a QFileDialog to allow them\n> to choose where to save the image.\n> \n> Utilise the QImageWriter to perform the output task.\n> \n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> \n> ---\n> v2:\n>  - Rename save to saveAs\n>  - Add empty file check\n>  - Lock concurrent access to the ViewFinder image\n> \n>  src/qcam/assets/feathericons/feathericons.qrc |  1 +\n>  src/qcam/main_window.cpp                      | 22 +++++++++++++++++++\n>  src/qcam/main_window.h                        |  1 +\n>  src/qcam/viewfinder.cpp                       | 11 ++++++++++\n>  src/qcam/viewfinder.h                         |  5 +++++\n>  5 files changed, 40 insertions(+)\n> \n> diff --git a/src/qcam/assets/feathericons/feathericons.qrc b/src/qcam/assets/feathericons/feathericons.qrc\n> index b8e5c2266408..6ca3a846803c 100644\n> --- a/src/qcam/assets/feathericons/feathericons.qrc\n> +++ b/src/qcam/assets/feathericons/feathericons.qrc\n> @@ -1,6 +1,7 @@\n>  <!DOCTYPE RCC><RCC version=\"1.0\">\n>  <qresource>\n>  <file>./play-circle.svg</file>\n> +<file>./save.svg</file>\n>  <file>./stop-circle.svg</file>\n>  <file>./x-circle.svg</file>\n>  </qresource>\n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index ec93e0177b41..8ee2a7d68c96 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -12,7 +12,10 @@\n>  \n>  #include <QComboBox>\n>  #include <QCoreApplication>\n> +#include <QFileDialog>\n>  #include <QIcon>\n> +#include <QImage>\n> +#include <QImageWriter>\n>  #include <QInputDialog>\n>  #include <QTimer>\n>  #include <QToolBar>\n> @@ -88,6 +91,9 @@ int MainWindow::createToolbars()\n>  \taction = toolbar_->addAction(QIcon(\":stop-circle.svg\"), \"stop\");\n>  \tconnect(action, &QAction::triggered, this, &MainWindow::stopCapture);\n>  \n> +\taction = toolbar_->addAction(QIcon(\":save.svg\"), \"saveAs\");\n> +\tconnect(action, &QAction::triggered, this, &MainWindow::saveImageAs);\n> +\n>  \treturn 0;\n>  }\n>  \n> @@ -339,6 +345,22 @@ void MainWindow::stopCapture()\n>  \tsetWindowTitle(title_);\n>  }\n>  \n> +void MainWindow::saveImageAs()\n> +{\n> +\tQImage image = viewfinder_->getCurrentImage();\n> +\n> +\tQString filename = QFileDialog::getSaveFileName(this, \"Save Image\", \"\",\n> +\t\t\t\t\t\t\t\"Image Files (*.png *.jpg *.jpeg)\");\n> +\n> +\tstd::cerr << \"Save image to \" << filename.toStdString() << std::endl;\n\nShould this be cout ? It's not an error.\n\n> +\n> +\tif (filename == \"\")\n\n\tif (filename.isEmpty())\n\nis slightly more efficient.\n\n> +\t\treturn;\n> +\n> +\tQImageWriter writer(filename);\n> +\twriter.write(image);\n> +}\n> +\n>  void MainWindow::requestComplete(Request *request)\n>  {\n>  \tif (request->status() == Request::RequestCancelled)\n> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> index 27ceed611d59..dcc39d7de948 100644\n> --- a/src/qcam/main_window.h\n> +++ b/src/qcam/main_window.h\n> @@ -48,6 +48,7 @@ private Q_SLOTS:\n>  \n>  \tint startCapture();\n>  \tvoid stopCapture();\n\nI'd add a blank line here as it's unrelated.\n\n> +\tvoid saveImageAs();\n>  \n>  private:\n>  \tint createToolbars();\n> diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp\n> index 6de284d1b782..211926e185d3 100644\n> --- a/src/qcam/viewfinder.cpp\n> +++ b/src/qcam/viewfinder.cpp\n> @@ -6,6 +6,8 @@\n>   */\n>  \n>  #include <QImage>\n> +#include <QImageWriter>\n> +#include <QMutexLocker>\n>  #include <QPainter>\n>  \n>  #include \"format_converter.h\"\n> @@ -23,10 +25,19 @@ ViewFinder::~ViewFinder()\n>  \n>  void ViewFinder::display(const unsigned char *raw, size_t size)\n>  {\n> +\tQMutexLocker locker(&mutex_);\n> +\n>  \tconverter_.convert(raw, size, image_);\n>  \tupdate();\n>  }\n>  \n> +QImage ViewFinder::getCurrentImage()\n> +{\n> +\tQMutexLocker locker(&mutex_);\n> +\n> +\treturn *image_;\n\nI'm afraid this is still unsafe despite the lock as QImage using\nimplicit data sharing. You can return image_->copy() instead as a first\nstep.\n\n> +}\n> +\n>  int ViewFinder::setFormat(unsigned int format, unsigned int width,\n>  \t\t\t  unsigned int height)\n>  {\n> diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h\n> index ef5fd45b264a..06e8034fce1d 100644\n> --- a/src/qcam/viewfinder.h\n> +++ b/src/qcam/viewfinder.h\n> @@ -7,6 +7,7 @@\n>  #ifndef __QCAM_VIEWFINDER_H__\n>  #define __QCAM_VIEWFINDER_H__\n>  \n> +#include <QMutex>\n>  #include <QWidget>\n>  \n>  #include \"format_converter.h\"\n> @@ -23,6 +24,8 @@ public:\n>  \t\t      unsigned int height);\n>  \tvoid display(const unsigned char *rgb, size_t size);\n>  \n> +\tQImage getCurrentImage();\n> +\n>  protected:\n>  \tvoid paintEvent(QPaintEvent *) override;\n>  \tQSize sizeHint() const override;\n> @@ -33,7 +36,9 @@ private:\n>  \tunsigned int height_;\n>  \n>  \tFormatConverter converter_;\n> +\n>  \tQImage *image_;\n> +\tQMutex mutex_; // Prevent concurrent access to image_\n\nC-style comments as in the rest of the code base ?\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n>  };\n>  \n>  #endif /* __QCAM_VIEWFINDER__ */","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4940761020\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 14 Feb 2020 12:36:21 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 54E18504;\n\tFri, 14 Feb 2020 12:36:20 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581680180;\n\tbh=3bjTkpv/X5QpDbCMFQZWyDRWRWQ1odcazPrldCi4Zww=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vWcaovUFHVFPnfOGuiZNyD9Xy9qwWIBqIFJrSEJcmGczGxdy4OVxlD/68SIEDyfDU\n\tCMCIuQ6Nd77u3A5MoGZ9XkKdlTEy298v0xTJnSPVPTpLu9tSjU7hTgSW6KG/wyf9j6\n\tMSx+g8PHRfwYEfxa1YCXfxEh6haOirNV+025VvlI=","Date":"Fri, 14 Feb 2020 13:36:02 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Message-ID":"<20200214113602.GE4831@pendragon.ideasonboard.com>","References":"<20200214001810.19302-1-kieran.bingham@ideasonboard.com>\n\t<20200214001810.19302-8-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200214001810.19302-8-kieran.bingham@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 7/7] qcam: Provide save image\n\tfunctionality","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 14 Feb 2020 11:36:21 -0000"}},{"id":3765,"web_url":"https://patchwork.libcamera.org/comment/3765/","msgid":"<20200214114125.GF4831@pendragon.ideasonboard.com>","date":"2020-02-14T11:41:25","subject":"Re: [libcamera-devel] [PATCH v2 7/7] qcam: Provide save image\n\tfunctionality","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOne more comment.\n\nOn Fri, Feb 14, 2020 at 01:36:02PM +0200, Laurent Pinchart wrote:\n> On Fri, Feb 14, 2020 at 12:18:10AM +0000, Kieran Bingham wrote:\n> > Implement a save image button on the toolbar which will take a current\n> > viewfinder image and present the user with a QFileDialog to allow them\n> > to choose where to save the image.\n> > \n> > Utilise the QImageWriter to perform the output task.\n> > \n> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > \n> > ---\n> > v2:\n> >  - Rename save to saveAs\n> >  - Add empty file check\n> >  - Lock concurrent access to the ViewFinder image\n> > \n> >  src/qcam/assets/feathericons/feathericons.qrc |  1 +\n> >  src/qcam/main_window.cpp                      | 22 +++++++++++++++++++\n> >  src/qcam/main_window.h                        |  1 +\n> >  src/qcam/viewfinder.cpp                       | 11 ++++++++++\n> >  src/qcam/viewfinder.h                         |  5 +++++\n> >  5 files changed, 40 insertions(+)\n> > \n> > diff --git a/src/qcam/assets/feathericons/feathericons.qrc b/src/qcam/assets/feathericons/feathericons.qrc\n> > index b8e5c2266408..6ca3a846803c 100644\n> > --- a/src/qcam/assets/feathericons/feathericons.qrc\n> > +++ b/src/qcam/assets/feathericons/feathericons.qrc\n> > @@ -1,6 +1,7 @@\n> >  <!DOCTYPE RCC><RCC version=\"1.0\">\n> >  <qresource>\n> >  <file>./play-circle.svg</file>\n> > +<file>./save.svg</file>\n> >  <file>./stop-circle.svg</file>\n> >  <file>./x-circle.svg</file>\n> >  </qresource>\n> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > index ec93e0177b41..8ee2a7d68c96 100644\n> > --- a/src/qcam/main_window.cpp\n> > +++ b/src/qcam/main_window.cpp\n> > @@ -12,7 +12,10 @@\n> >  \n> >  #include <QComboBox>\n> >  #include <QCoreApplication>\n> > +#include <QFileDialog>\n> >  #include <QIcon>\n> > +#include <QImage>\n> > +#include <QImageWriter>\n> >  #include <QInputDialog>\n> >  #include <QTimer>\n> >  #include <QToolBar>\n> > @@ -88,6 +91,9 @@ int MainWindow::createToolbars()\n> >  \taction = toolbar_->addAction(QIcon(\":stop-circle.svg\"), \"stop\");\n> >  \tconnect(action, &QAction::triggered, this, &MainWindow::stopCapture);\n> >  \n> > +\taction = toolbar_->addAction(QIcon(\":save.svg\"), \"saveAs\");\n> > +\tconnect(action, &QAction::triggered, this, &MainWindow::saveImageAs);\n> > +\n> >  \treturn 0;\n> >  }\n> >  \n> > @@ -339,6 +345,22 @@ void MainWindow::stopCapture()\n> >  \tsetWindowTitle(title_);\n> >  }\n> >  \n> > +void MainWindow::saveImageAs()\n> > +{\n> > +\tQImage image = viewfinder_->getCurrentImage();\n> > +\n> > +\tQString filename = QFileDialog::getSaveFileName(this, \"Save Image\", \"\",\n> > +\t\t\t\t\t\t\t\"Image Files (*.png *.jpg *.jpeg)\");\n> > +\n> > +\tstd::cerr << \"Save image to \" << filename.toStdString() << std::endl;\n> \n> Should this be cout ? It's not an error.\n> \n> > +\n> > +\tif (filename == \"\")\n> \n> \tif (filename.isEmpty())\n> \n> is slightly more efficient.\n> \n> > +\t\treturn;\n> > +\n> > +\tQImageWriter writer(filename);\n> > +\twriter.write(image);\n> > +}\n> > +\n> >  void MainWindow::requestComplete(Request *request)\n> >  {\n> >  \tif (request->status() == Request::RequestCancelled)\n> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> > index 27ceed611d59..dcc39d7de948 100644\n> > --- a/src/qcam/main_window.h\n> > +++ b/src/qcam/main_window.h\n> > @@ -48,6 +48,7 @@ private Q_SLOTS:\n> >  \n> >  \tint startCapture();\n> >  \tvoid stopCapture();\n> \n> I'd add a blank line here as it's unrelated.\n> \n> > +\tvoid saveImageAs();\n> >  \n> >  private:\n> >  \tint createToolbars();\n> > diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp\n> > index 6de284d1b782..211926e185d3 100644\n> > --- a/src/qcam/viewfinder.cpp\n> > +++ b/src/qcam/viewfinder.cpp\n> > @@ -6,6 +6,8 @@\n> >   */\n> >  \n> >  #include <QImage>\n> > +#include <QImageWriter>\n> > +#include <QMutexLocker>\n> >  #include <QPainter>\n> >  \n> >  #include \"format_converter.h\"\n> > @@ -23,10 +25,19 @@ ViewFinder::~ViewFinder()\n> >  \n> >  void ViewFinder::display(const unsigned char *raw, size_t size)\n> >  {\n> > +\tQMutexLocker locker(&mutex_);\n> > +\n\nAs image_->copy() is an expensive operation, this may block the pipeline\nhandler thread and have a very adverse effect on operation. It's OK as a\nfirst step, but definitely something we want to fix, sooner than latter.\nCould you add\n\n\t/*\n\t * \\todo We're not supposed to block the pipeline handler thread\n\t * for long, implement a better way to save images without\n\t * impacting performances.\n\t */\n\n> >  \tconverter_.convert(raw, size, image_);\n> >  \tupdate();\n> >  }\n> >  \n> > +QImage ViewFinder::getCurrentImage()\n> > +{\n> > +\tQMutexLocker locker(&mutex_);\n> > +\n> > +\treturn *image_;\n> \n> I'm afraid this is still unsafe despite the lock as QImage using\n> implicit data sharing. You can return image_->copy() instead as a first\n> step.\n> \n> > +}\n> > +\n> >  int ViewFinder::setFormat(unsigned int format, unsigned int width,\n> >  \t\t\t  unsigned int height)\n> >  {\n> > diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h\n> > index ef5fd45b264a..06e8034fce1d 100644\n> > --- a/src/qcam/viewfinder.h\n> > +++ b/src/qcam/viewfinder.h\n> > @@ -7,6 +7,7 @@\n> >  #ifndef __QCAM_VIEWFINDER_H__\n> >  #define __QCAM_VIEWFINDER_H__\n> >  \n> > +#include <QMutex>\n> >  #include <QWidget>\n> >  \n> >  #include \"format_converter.h\"\n> > @@ -23,6 +24,8 @@ public:\n> >  \t\t      unsigned int height);\n> >  \tvoid display(const unsigned char *rgb, size_t size);\n> >  \n> > +\tQImage getCurrentImage();\n> > +\n> >  protected:\n> >  \tvoid paintEvent(QPaintEvent *) override;\n> >  \tQSize sizeHint() const override;\n> > @@ -33,7 +36,9 @@ private:\n> >  \tunsigned int height_;\n> >  \n> >  \tFormatConverter converter_;\n> > +\n> >  \tQImage *image_;\n> > +\tQMutex mutex_; // Prevent concurrent access to image_\n> \n> C-style comments as in the rest of the code base ?\n> \n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> \n> >  };\n> >  \n> >  #endif /* __QCAM_VIEWFINDER__ */\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":"<laurent.pinchart@ideasonboard.com>","Received":["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 9F25361020\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 14 Feb 2020 12:41:42 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 19A7B504;\n\tFri, 14 Feb 2020 12:41:42 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581680502;\n\tbh=rNYH5oNMCF1sDInbUys8kuSg+2dLp/Pd5JNHRL4g4XA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=v+EDZsZgW3pzRRFzNcu1bYvZ4AZmIN14lPTK7Hu0Kg4nq6I53voSR5vt0kpOkcuKB\n\t72gjmxpUw83ItLMIkTAlxN52U6ztujBotIyMxghf6Rt70AhW3ozHC+GwMTgF5LIEoW\n\t5RCMZIu99vZSesv3AFn25APkhq340a3ivjxcVfLo=","Date":"Fri, 14 Feb 2020 13:41:25 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Message-ID":"<20200214114125.GF4831@pendragon.ideasonboard.com>","References":"<20200214001810.19302-1-kieran.bingham@ideasonboard.com>\n\t<20200214001810.19302-8-kieran.bingham@ideasonboard.com>\n\t<20200214113602.GE4831@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200214113602.GE4831@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 7/7] qcam: Provide save image\n\tfunctionality","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 14 Feb 2020 11:41:42 -0000"}},{"id":3766,"web_url":"https://patchwork.libcamera.org/comment/3766/","msgid":"<976c77b6-d7ca-58b4-b2fd-64f017ef2493@ideasonboard.com>","date":"2020-02-14T11:48:06","subject":"Re: [libcamera-devel] [PATCH v2 7/7] qcam: Provide save image\n\tfunctionality","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 14/02/2020 11:41, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> One more comment.\n> \n> On Fri, Feb 14, 2020 at 01:36:02PM +0200, Laurent Pinchart wrote:\n>> On Fri, Feb 14, 2020 at 12:18:10AM +0000, Kieran Bingham wrote:\n>>> Implement a save image button on the toolbar which will take a current\n>>> viewfinder image and present the user with a QFileDialog to allow them\n>>> to choose where to save the image.\n>>>\n>>> Utilise the QImageWriter to perform the output task.\n>>>\n>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>>>\n>>> ---\n>>> v2:\n>>>  - Rename save to saveAs\n>>>  - Add empty file check\n>>>  - Lock concurrent access to the ViewFinder image\n>>>\n>>>  src/qcam/assets/feathericons/feathericons.qrc |  1 +\n>>>  src/qcam/main_window.cpp                      | 22 +++++++++++++++++++\n>>>  src/qcam/main_window.h                        |  1 +\n>>>  src/qcam/viewfinder.cpp                       | 11 ++++++++++\n>>>  src/qcam/viewfinder.h                         |  5 +++++\n>>>  5 files changed, 40 insertions(+)\n>>>\n>>> diff --git a/src/qcam/assets/feathericons/feathericons.qrc b/src/qcam/assets/feathericons/feathericons.qrc\n>>> index b8e5c2266408..6ca3a846803c 100644\n>>> --- a/src/qcam/assets/feathericons/feathericons.qrc\n>>> +++ b/src/qcam/assets/feathericons/feathericons.qrc\n>>> @@ -1,6 +1,7 @@\n>>>  <!DOCTYPE RCC><RCC version=\"1.0\">\n>>>  <qresource>\n>>>  <file>./play-circle.svg</file>\n>>> +<file>./save.svg</file>\n>>>  <file>./stop-circle.svg</file>\n>>>  <file>./x-circle.svg</file>\n>>>  </qresource>\n>>> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n>>> index ec93e0177b41..8ee2a7d68c96 100644\n>>> --- a/src/qcam/main_window.cpp\n>>> +++ b/src/qcam/main_window.cpp\n>>> @@ -12,7 +12,10 @@\n>>>  \n>>>  #include <QComboBox>\n>>>  #include <QCoreApplication>\n>>> +#include <QFileDialog>\n>>>  #include <QIcon>\n>>> +#include <QImage>\n>>> +#include <QImageWriter>\n>>>  #include <QInputDialog>\n>>>  #include <QTimer>\n>>>  #include <QToolBar>\n>>> @@ -88,6 +91,9 @@ int MainWindow::createToolbars()\n>>>  \taction = toolbar_->addAction(QIcon(\":stop-circle.svg\"), \"stop\");\n>>>  \tconnect(action, &QAction::triggered, this, &MainWindow::stopCapture);\n>>>  \n>>> +\taction = toolbar_->addAction(QIcon(\":save.svg\"), \"saveAs\");\n>>> +\tconnect(action, &QAction::triggered, this, &MainWindow::saveImageAs);\n>>> +\n>>>  \treturn 0;\n>>>  }\n>>>  \n>>> @@ -339,6 +345,22 @@ void MainWindow::stopCapture()\n>>>  \tsetWindowTitle(title_);\n>>>  }\n>>>  \n>>> +void MainWindow::saveImageAs()\n>>> +{\n>>> +\tQImage image = viewfinder_->getCurrentImage();\n>>> +\n>>> +\tQString filename = QFileDialog::getSaveFileName(this, \"Save Image\", \"\",\n>>> +\t\t\t\t\t\t\t\"Image Files (*.png *.jpg *.jpeg)\");\n>>> +\n>>> +\tstd::cerr << \"Save image to \" << filename.toStdString() << std::endl;\n>>\n>> Should this be cout ? It's not an error.\n>>\n>>> +\n>>> +\tif (filename == \"\")\n>>\n>> \tif (filename.isEmpty())\n>>\n>> is slightly more efficient.\n>>\n>>> +\t\treturn;\n>>> +\n>>> +\tQImageWriter writer(filename);\n>>> +\twriter.write(image);\n>>> +}\n>>> +\n>>>  void MainWindow::requestComplete(Request *request)\n>>>  {\n>>>  \tif (request->status() == Request::RequestCancelled)\n>>> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n>>> index 27ceed611d59..dcc39d7de948 100644\n>>> --- a/src/qcam/main_window.h\n>>> +++ b/src/qcam/main_window.h\n>>> @@ -48,6 +48,7 @@ private Q_SLOTS:\n>>>  \n>>>  \tint startCapture();\n>>>  \tvoid stopCapture();\n>>\n>> I'd add a blank line here as it's unrelated.\n>>\n>>> +\tvoid saveImageAs();\n>>>  \n>>>  private:\n>>>  \tint createToolbars();\n>>> diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp\n>>> index 6de284d1b782..211926e185d3 100644\n>>> --- a/src/qcam/viewfinder.cpp\n>>> +++ b/src/qcam/viewfinder.cpp\n>>> @@ -6,6 +6,8 @@\n>>>   */\n>>>  \n>>>  #include <QImage>\n>>> +#include <QImageWriter>\n>>> +#include <QMutexLocker>\n>>>  #include <QPainter>\n>>>  \n>>>  #include \"format_converter.h\"\n>>> @@ -23,10 +25,19 @@ ViewFinder::~ViewFinder()\n>>>  \n>>>  void ViewFinder::display(const unsigned char *raw, size_t size)\n>>>  {\n>>> +\tQMutexLocker locker(&mutex_);\n>>> +\n> \n> As image_->copy() is an expensive operation, this may block the pipeline\n> handler thread and have a very adverse effect on operation. It's OK as a\n> first step, but definitely something we want to fix, sooner than latter.\n> Could you add\n> \n> \t/*\n> \t * \\todo We're not supposed to block the pipeline handler thread\n> \t * for long, implement a better way to save images without\n> \t * impacting performances.\n> \t */\n\nSure, cursory testing on an x86 doesn't show any issues, but that may\nnot be the same case on an ARM device of course.\n\nDo you want me to post a v3 with all of the updates made?\n\n--\nKieran\n\n\n> \n>>>  \tconverter_.convert(raw, size, image_);\n>>>  \tupdate();\n>>>  }\n>>>  \n>>> +QImage ViewFinder::getCurrentImage()\n>>> +{\n>>> +\tQMutexLocker locker(&mutex_);\n>>> +\n>>> +\treturn *image_;\n>>\n>> I'm afraid this is still unsafe despite the lock as QImage using\n>> implicit data sharing. You can return image_->copy() instead as a first\n>> step.\n>>\n>>> +}\n>>> +\n>>>  int ViewFinder::setFormat(unsigned int format, unsigned int width,\n>>>  \t\t\t  unsigned int height)\n>>>  {\n>>> diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h\n>>> index ef5fd45b264a..06e8034fce1d 100644\n>>> --- a/src/qcam/viewfinder.h\n>>> +++ b/src/qcam/viewfinder.h\n>>> @@ -7,6 +7,7 @@\n>>>  #ifndef __QCAM_VIEWFINDER_H__\n>>>  #define __QCAM_VIEWFINDER_H__\n>>>  \n>>> +#include <QMutex>\n>>>  #include <QWidget>\n>>>  \n>>>  #include \"format_converter.h\"\n>>> @@ -23,6 +24,8 @@ public:\n>>>  \t\t      unsigned int height);\n>>>  \tvoid display(const unsigned char *rgb, size_t size);\n>>>  \n>>> +\tQImage getCurrentImage();\n>>> +\n>>>  protected:\n>>>  \tvoid paintEvent(QPaintEvent *) override;\n>>>  \tQSize sizeHint() const override;\n>>> @@ -33,7 +36,9 @@ private:\n>>>  \tunsigned int height_;\n>>>  \n>>>  \tFormatConverter converter_;\n>>> +\n>>>  \tQImage *image_;\n>>> +\tQMutex mutex_; // Prevent concurrent access to image_\n>>\n>> C-style comments as in the rest of the code base ?\n>>\n>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n>>\n>>>  };\n>>>  \n>>>  #endif /* __QCAM_VIEWFINDER__ */\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\n>","headers":{"Return-Path":"<kieran.bingham@ideasonboard.com>","Received":["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 25F1F61020\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 14 Feb 2020 12:48:10 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 697EA504;\n\tFri, 14 Feb 2020 12:48:09 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581680889;\n\tbh=cuadqtRmuxxcIiOpSusvSm2nu1eQsVEAj1oVC7ZICW4=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=d3KL0NGEmZTgTyg95XJdLy65gk1g2kyp6Z4UWCG3fdKU4YR9nn1QY+UxJv1sYLJ7h\n\tbjI7xuW6mnCXpyNTGSMgWACIIyZ/q4X2kmsJvbYNTTW8JpBACpZcLTySDwF5+niMBa\n\tsqv9W1cKBBIPsDrsr5JQoJOyGn53zWrosbrUnLvg=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","References":"<20200214001810.19302-1-kieran.bingham@ideasonboard.com>\n\t<20200214001810.19302-8-kieran.bingham@ideasonboard.com>\n\t<20200214113602.GE4831@pendragon.ideasonboard.com>\n\t<20200214114125.GF4831@pendragon.ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<976c77b6-d7ca-58b4-b2fd-64f017ef2493@ideasonboard.com>","Date":"Fri, 14 Feb 2020 11:48:06 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.1","MIME-Version":"1.0","In-Reply-To":"<20200214114125.GF4831@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 7/7] qcam: Provide save image\n\tfunctionality","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 14 Feb 2020 11:48:10 -0000"}},{"id":3767,"web_url":"https://patchwork.libcamera.org/comment/3767/","msgid":"<20200214115130.GG4831@pendragon.ideasonboard.com>","date":"2020-02-14T11:51:30","subject":"Re: [libcamera-devel] [PATCH v2 7/7] qcam: Provide save image\n\tfunctionality","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, Feb 14, 2020 at 11:48:06AM +0000, Kieran Bingham wrote:\n> On 14/02/2020 11:41, Laurent Pinchart wrote:\n> > On Fri, Feb 14, 2020 at 01:36:02PM +0200, Laurent Pinchart wrote:\n> >> On Fri, Feb 14, 2020 at 12:18:10AM +0000, Kieran Bingham wrote:\n> >>> Implement a save image button on the toolbar which will take a current\n> >>> viewfinder image and present the user with a QFileDialog to allow them\n> >>> to choose where to save the image.\n> >>>\n> >>> Utilise the QImageWriter to perform the output task.\n> >>>\n> >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >>>\n> >>> ---\n> >>> v2:\n> >>>  - Rename save to saveAs\n> >>>  - Add empty file check\n> >>>  - Lock concurrent access to the ViewFinder image\n> >>>\n> >>>  src/qcam/assets/feathericons/feathericons.qrc |  1 +\n> >>>  src/qcam/main_window.cpp                      | 22 +++++++++++++++++++\n> >>>  src/qcam/main_window.h                        |  1 +\n> >>>  src/qcam/viewfinder.cpp                       | 11 ++++++++++\n> >>>  src/qcam/viewfinder.h                         |  5 +++++\n> >>>  5 files changed, 40 insertions(+)\n> >>>\n> >>> diff --git a/src/qcam/assets/feathericons/feathericons.qrc b/src/qcam/assets/feathericons/feathericons.qrc\n> >>> index b8e5c2266408..6ca3a846803c 100644\n> >>> --- a/src/qcam/assets/feathericons/feathericons.qrc\n> >>> +++ b/src/qcam/assets/feathericons/feathericons.qrc\n> >>> @@ -1,6 +1,7 @@\n> >>>  <!DOCTYPE RCC><RCC version=\"1.0\">\n> >>>  <qresource>\n> >>>  <file>./play-circle.svg</file>\n> >>> +<file>./save.svg</file>\n> >>>  <file>./stop-circle.svg</file>\n> >>>  <file>./x-circle.svg</file>\n> >>>  </qresource>\n> >>> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> >>> index ec93e0177b41..8ee2a7d68c96 100644\n> >>> --- a/src/qcam/main_window.cpp\n> >>> +++ b/src/qcam/main_window.cpp\n> >>> @@ -12,7 +12,10 @@\n> >>>  \n> >>>  #include <QComboBox>\n> >>>  #include <QCoreApplication>\n> >>> +#include <QFileDialog>\n> >>>  #include <QIcon>\n> >>> +#include <QImage>\n> >>> +#include <QImageWriter>\n> >>>  #include <QInputDialog>\n> >>>  #include <QTimer>\n> >>>  #include <QToolBar>\n> >>> @@ -88,6 +91,9 @@ int MainWindow::createToolbars()\n> >>>  \taction = toolbar_->addAction(QIcon(\":stop-circle.svg\"), \"stop\");\n> >>>  \tconnect(action, &QAction::triggered, this, &MainWindow::stopCapture);\n> >>>  \n> >>> +\taction = toolbar_->addAction(QIcon(\":save.svg\"), \"saveAs\");\n> >>> +\tconnect(action, &QAction::triggered, this, &MainWindow::saveImageAs);\n> >>> +\n> >>>  \treturn 0;\n> >>>  }\n> >>>  \n> >>> @@ -339,6 +345,22 @@ void MainWindow::stopCapture()\n> >>>  \tsetWindowTitle(title_);\n> >>>  }\n> >>>  \n> >>> +void MainWindow::saveImageAs()\n> >>> +{\n> >>> +\tQImage image = viewfinder_->getCurrentImage();\n> >>> +\n> >>> +\tQString filename = QFileDialog::getSaveFileName(this, \"Save Image\", \"\",\n> >>> +\t\t\t\t\t\t\t\"Image Files (*.png *.jpg *.jpeg)\");\n> >>> +\n> >>> +\tstd::cerr << \"Save image to \" << filename.toStdString() << std::endl;\n> >>\n> >> Should this be cout ? It's not an error.\n> >>\n> >>> +\n> >>> +\tif (filename == \"\")\n> >>\n> >> \tif (filename.isEmpty())\n> >>\n> >> is slightly more efficient.\n> >>\n> >>> +\t\treturn;\n> >>> +\n> >>> +\tQImageWriter writer(filename);\n> >>> +\twriter.write(image);\n> >>> +}\n> >>> +\n> >>>  void MainWindow::requestComplete(Request *request)\n> >>>  {\n> >>>  \tif (request->status() == Request::RequestCancelled)\n> >>> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> >>> index 27ceed611d59..dcc39d7de948 100644\n> >>> --- a/src/qcam/main_window.h\n> >>> +++ b/src/qcam/main_window.h\n> >>> @@ -48,6 +48,7 @@ private Q_SLOTS:\n> >>>  \n> >>>  \tint startCapture();\n> >>>  \tvoid stopCapture();\n> >>\n> >> I'd add a blank line here as it's unrelated.\n> >>\n> >>> +\tvoid saveImageAs();\n> >>>  \n> >>>  private:\n> >>>  \tint createToolbars();\n> >>> diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp\n> >>> index 6de284d1b782..211926e185d3 100644\n> >>> --- a/src/qcam/viewfinder.cpp\n> >>> +++ b/src/qcam/viewfinder.cpp\n> >>> @@ -6,6 +6,8 @@\n> >>>   */\n> >>>  \n> >>>  #include <QImage>\n> >>> +#include <QImageWriter>\n> >>> +#include <QMutexLocker>\n> >>>  #include <QPainter>\n> >>>  \n> >>>  #include \"format_converter.h\"\n> >>> @@ -23,10 +25,19 @@ ViewFinder::~ViewFinder()\n> >>>  \n> >>>  void ViewFinder::display(const unsigned char *raw, size_t size)\n> >>>  {\n> >>> +\tQMutexLocker locker(&mutex_);\n> >>> +\n> > \n> > As image_->copy() is an expensive operation, this may block the pipeline\n> > handler thread and have a very adverse effect on operation. It's OK as a\n> > first step, but definitely something we want to fix, sooner than latter.\n> > Could you add\n> > \n> > \t/*\n> > \t * \\todo We're not supposed to block the pipeline handler thread\n> > \t * for long, implement a better way to save images without\n> > \t * impacting performances.\n> > \t */\n> \n> Sure, cursory testing on an x86 doesn't show any issues, but that may\n> not be the same case on an ARM device of course.\n> \n> Do you want me to post a v3 with all of the updates made?\n\nIf you have my R-b tag for all patches there's no need to.\n\n> >>>  \tconverter_.convert(raw, size, image_);\n> >>>  \tupdate();\n> >>>  }\n> >>>  \n> >>> +QImage ViewFinder::getCurrentImage()\n> >>> +{\n> >>> +\tQMutexLocker locker(&mutex_);\n> >>> +\n> >>> +\treturn *image_;\n> >>\n> >> I'm afraid this is still unsafe despite the lock as QImage using\n> >> implicit data sharing. You can return image_->copy() instead as a first\n> >> step.\n> >>\n> >>> +}\n> >>> +\n> >>>  int ViewFinder::setFormat(unsigned int format, unsigned int width,\n> >>>  \t\t\t  unsigned int height)\n> >>>  {\n> >>> diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h\n> >>> index ef5fd45b264a..06e8034fce1d 100644\n> >>> --- a/src/qcam/viewfinder.h\n> >>> +++ b/src/qcam/viewfinder.h\n> >>> @@ -7,6 +7,7 @@\n> >>>  #ifndef __QCAM_VIEWFINDER_H__\n> >>>  #define __QCAM_VIEWFINDER_H__\n> >>>  \n> >>> +#include <QMutex>\n> >>>  #include <QWidget>\n> >>>  \n> >>>  #include \"format_converter.h\"\n> >>> @@ -23,6 +24,8 @@ public:\n> >>>  \t\t      unsigned int height);\n> >>>  \tvoid display(const unsigned char *rgb, size_t size);\n> >>>  \n> >>> +\tQImage getCurrentImage();\n> >>> +\n> >>>  protected:\n> >>>  \tvoid paintEvent(QPaintEvent *) override;\n> >>>  \tQSize sizeHint() const override;\n> >>> @@ -33,7 +36,9 @@ private:\n> >>>  \tunsigned int height_;\n> >>>  \n> >>>  \tFormatConverter converter_;\n> >>> +\n> >>>  \tQImage *image_;\n> >>> +\tQMutex mutex_; // Prevent concurrent access to image_\n> >>\n> >> C-style comments as in the rest of the code base ?\n> >>\n> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> >>\n> >>>  };\n> >>>  \n> >>>  #endif /* __QCAM_VIEWFINDER__ */","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0F9F461020\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 14 Feb 2020 12:51:48 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5D353504;\n\tFri, 14 Feb 2020 12:51:47 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581681107;\n\tbh=+glHZmEkMiIhO1bu8On4/h9XMy0IkJTyvbMDP219LnY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qUKCufSMOk6uQQHEu+E9aXMd0DqCUyJiHk1DqFYPzXUW0hFPTyoEnRE9YWu1ZS3eH\n\tniCDf0/wMQjsC6Cxv9ZILFJdPjPKg9/ge6b318wh7zq8+/J0o/vQwd+70e2SPJu3b/\n\tu5AJ/lYtzISeLav9Tk5fySg8dG9SCHJMyVar72OU=","Date":"Fri, 14 Feb 2020 13:51:30 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Message-ID":"<20200214115130.GG4831@pendragon.ideasonboard.com>","References":"<20200214001810.19302-1-kieran.bingham@ideasonboard.com>\n\t<20200214001810.19302-8-kieran.bingham@ideasonboard.com>\n\t<20200214113602.GE4831@pendragon.ideasonboard.com>\n\t<20200214114125.GF4831@pendragon.ideasonboard.com>\n\t<976c77b6-d7ca-58b4-b2fd-64f017ef2493@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<976c77b6-d7ca-58b4-b2fd-64f017ef2493@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 7/7] qcam: Provide save image\n\tfunctionality","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 14 Feb 2020 11:51:48 -0000"}}]