[libcamera-devel] [PATCH v2 7/7] qcam: Provide save image functionality

[Laurent Pinchart]

Hi Kieran,

One more comment.

On Fri, Feb 14, 2020 at 01:36:02PM +0200, Laurent Pinchart wrote:
> On Fri, Feb 14, 2020 at 12:18:10AM +0000, Kieran Bingham wrote:
> > Implement a save image button on the toolbar which will take a current
> > viewfinder image and present the user with a QFileDialog to allow them
> > to choose where to save the image.
> > 
> > Utilise the QImageWriter to perform the output task.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > 
> > ---
> > v2:
> >  - Rename save to saveAs
> >  - Add empty file check
> >  - Lock concurrent access to the ViewFinder image
> > 
> >  src/qcam/assets/feathericons/feathericons.qrc |  1 +
> >  src/qcam/main_window.cpp                      | 22 +++++++++++++++++++
> >  src/qcam/main_window.h                        |  1 +
> >  src/qcam/viewfinder.cpp                       | 11 ++++++++++
> >  src/qcam/viewfinder.h                         |  5 +++++
> >  5 files changed, 40 insertions(+)
> > 
> > diff --git a/src/qcam/assets/feathericons/feathericons.qrc b/src/qcam/assets/feathericons/feathericons.qrc
> > index b8e5c2266408..6ca3a846803c 100644
> > --- a/src/qcam/assets/feathericons/feathericons.qrc
> > +++ b/src/qcam/assets/feathericons/feathericons.qrc
> > @@ -1,6 +1,7 @@
> >  <!DOCTYPE RCC><RCC version="1.0">
> >  <qresource>
> >  <file>./play-circle.svg</file>
> > +<file>./save.svg</file>
> >  <file>./stop-circle.svg</file>
> >  <file>./x-circle.svg</file>
> >  </qresource>
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index ec93e0177b41..8ee2a7d68c96 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -12,7 +12,10 @@
> >  
> >  #include <QComboBox>
> >  #include <QCoreApplication>
> > +#include <QFileDialog>
> >  #include <QIcon>
> > +#include <QImage>
> > +#include <QImageWriter>
> >  #include <QInputDialog>
> >  #include <QTimer>
> >  #include <QToolBar>
> > @@ -88,6 +91,9 @@ int MainWindow::createToolbars()
> >  	action = toolbar_->addAction(QIcon(":stop-circle.svg"), "stop");
> >  	connect(action, &QAction::triggered, this, &MainWindow::stopCapture);
> >  
> > +	action = toolbar_->addAction(QIcon(":save.svg"), "saveAs");
> > +	connect(action, &QAction::triggered, this, &MainWindow::saveImageAs);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -339,6 +345,22 @@ void MainWindow::stopCapture()
> >  	setWindowTitle(title_);
> >  }
> >  
> > +void MainWindow::saveImageAs()
> > +{
> > +	QImage image = viewfinder_->getCurrentImage();
> > +
> > +	QString filename = QFileDialog::getSaveFileName(this, "Save Image", "",
> > +							"Image Files (*.png *.jpg *.jpeg)");
> > +
> > +	std::cerr << "Save image to " << filename.toStdString() << std::endl;
> 
> Should this be cout ? It's not an error.
> 
> > +
> > +	if (filename == "")
> 
> 	if (filename.isEmpty())
> 
> is slightly more efficient.
> 
> > +		return;
> > +
> > +	QImageWriter writer(filename);
> > +	writer.write(image);
> > +}
> > +
> >  void MainWindow::requestComplete(Request *request)
> >  {
> >  	if (request->status() == Request::RequestCancelled)
> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > index 27ceed611d59..dcc39d7de948 100644
> > --- a/src/qcam/main_window.h
> > +++ b/src/qcam/main_window.h
> > @@ -48,6 +48,7 @@ private Q_SLOTS:
> >  
> >  	int startCapture();
> >  	void stopCapture();
> 
> I'd add a blank line here as it's unrelated.
> 
> > +	void saveImageAs();
> >  
> >  private:
> >  	int createToolbars();
> > diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp
> > index 6de284d1b782..211926e185d3 100644
> > --- a/src/qcam/viewfinder.cpp
> > +++ b/src/qcam/viewfinder.cpp
> > @@ -6,6 +6,8 @@
> >   */
> >  
> >  #include <QImage>
> > +#include <QImageWriter>
> > +#include <QMutexLocker>
> >  #include <QPainter>
> >  
> >  #include "format_converter.h"
> > @@ -23,10 +25,19 @@ ViewFinder::~ViewFinder()
> >  
> >  void ViewFinder::display(const unsigned char *raw, size_t size)
> >  {
> > +	QMutexLocker locker(&mutex_);
> > +

As image_->copy() is an expensive operation, this may block the pipeline
handler thread and have a very adverse effect on operation. It's OK as a
first step, but definitely something we want to fix, sooner than latter.
Could you add

	/*
	 * \todo We're not supposed to block the pipeline handler thread
	 * for long, implement a better way to save images without
	 * impacting performances.
	 */

> >  	converter_.convert(raw, size, image_);
> >  	update();
> >  }
> >  
> > +QImage ViewFinder::getCurrentImage()
> > +{
> > +	QMutexLocker locker(&mutex_);
> > +
> > +	return *image_;
> 
> I'm afraid this is still unsafe despite the lock as QImage using
> implicit data sharing. You can return image_->copy() instead as a first
> step.
> 
> > +}
> > +
> >  int ViewFinder::setFormat(unsigned int format, unsigned int width,
> >  			  unsigned int height)
> >  {
> > diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h
> > index ef5fd45b264a..06e8034fce1d 100644
> > --- a/src/qcam/viewfinder.h
> > +++ b/src/qcam/viewfinder.h
> > @@ -7,6 +7,7 @@
> >  #ifndef __QCAM_VIEWFINDER_H__
> >  #define __QCAM_VIEWFINDER_H__
> >  
> > +#include <QMutex>
> >  #include <QWidget>
> >  
> >  #include "format_converter.h"
> > @@ -23,6 +24,8 @@ public:
> >  		      unsigned int height);
> >  	void display(const unsigned char *rgb, size_t size);
> >  
> > +	QImage getCurrentImage();
> > +
> >  protected:
> >  	void paintEvent(QPaintEvent *) override;
> >  	QSize sizeHint() const override;
> > @@ -33,7 +36,9 @@ private:
> >  	unsigned int height_;
> >  
> >  	FormatConverter converter_;
> > +
> >  	QImage *image_;
> > +	QMutex mutex_; // Prevent concurrent access to image_
> 
> C-style comments as in the rest of the code base ?
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> >  };
> >  
> >  #endif /* __QCAM_VIEWFINDER__ */
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,

Laurent Pinchart