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

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thu, Feb 06, 2020 at 03:12:02PM +0000, Kieran Bingham wrote:
> Hi All,
> 
> This one is a bit of an RFC due to comments in the patch:
> 
> On 06/02/2020 15:05, 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>
> > ---
> >  src/qcam/main_window.cpp | 22 ++++++++++++++++++++++
> >  src/qcam/main_window.h   |  1 +
> >  src/qcam/viewfinder.cpp  |  7 +++++++
> >  src/qcam/viewfinder.h    |  2 ++
> >  4 files changed, 32 insertions(+)
> > 
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index 0ae4c60b8699..0e994b1e9197 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -11,7 +11,10 @@
> >  #include <sys/mman.h>
> >  
> >  #include <QCoreApplication>
> > +#include <QFileDialog>
> >  #include <QIcon>
> > +#include <QImage>
> > +#include <QImageWriter>
> >  #include <QInputDialog>
> >  #include <QTimer>
> >  #include <QToolBar>
> > @@ -89,6 +92,9 @@ int MainWindow::createToolbars(CameraManager *cm)
> >  	action = toolbar_->addAction(QIcon(":stop-circle.svg"), "stop");
> >  	connect(action, &QAction::triggered, this, &MainWindow::stopCapture);
> >  
> > +	action = toolbar_->addAction(QIcon(":save.svg"), "save");
> > +	connect(action, &QAction::triggered, this, &MainWindow::saveImage);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -339,6 +345,22 @@ void MainWindow::stopCapture()
> >  	setWindowTitle(title_);
> >  }
> >  
> > +void MainWindow::saveImage()
> > +{
> > +	/* Take a lock to prevent updating the backed image, copy,
> > +	 * then ask where to save with lock released */

	/*
	 * Take a lock to prevent updating the backed image, copy,
	 * then ask where to save with lock released.
	 */

> > +
> > +	QImage image = viewfinder_->getCurrentImage();
> > +
> > +	QString filename = QFileDialog::getSaveFileName(this, "Save Image", "",
> > +							"Image Files (*.png *.jpg *.jpeg)");

This is probably OK to start with, but I wonder if we don't also (or
instead) need a quicker way to save images, skipping the dialog box. The
file name could be computed automatically based on a pattern.

> > +	std::cerr << "Save jpeg to " << filename.toStdString() << std::endl;
> 
> I think that can be removed now :-)
> 
> > +
> > +	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 b0bf16dd2a09..fc85b6a46491 100644
> > --- a/src/qcam/main_window.h
> > +++ b/src/qcam/main_window.h
> > @@ -48,6 +48,7 @@ private Q_SLOTS:
> >  
> >  	int startCapture();
> >  	void stopCapture();
> > +	void saveImage();
> >  
> >  private:
> >  	int createToolbars(CameraManager *cm);
> > diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp
> > index 6de284d1b782..3fa4a326c342 100644
> > --- a/src/qcam/viewfinder.cpp
> > +++ b/src/qcam/viewfinder.cpp
> > @@ -6,6 +6,7 @@
> >   */
> >  
> >  #include <QImage>
> > +#include <QImageWriter>
> >  #include <QPainter>
> >  
> >  #include "format_converter.h"
> > @@ -27,6 +28,12 @@ void ViewFinder::display(const unsigned char *raw, size_t size)
> >  	update();
> >  }
> >  
> > +QImage ViewFinder::getCurrentImage()
> > +{
> > +	/* Ideally need to lock, return/copy, then unlock... Can a scoped lock work? */
> 
> So I 'think' that this might actually be saved from races by the QT
> threading. But I'm not entirely sure...
> 
> Thoughts anyone?

MainWindow::requestComplete() is called from the CameraManager thread
(once the threading patches will be merged, hopefully tomorrow), and
calls ViewFinder::display() which updates the contents of the QImage.
ViewFinder::getCurrentImage() is called from the Qt GUI thread, running
the main application loop. There's thus a race.

> > +	return *image_;
> 
> And of course a "memcpy" here isn't ideal either - but I anticipate this
> could all get changed to create a new stream to perform a
> StreamRole::StillCapture type capture ...

That's true, but we will also need to support cameras that can only
provide a single stream.

I think we can implement a more efficient method, at the cost of more
complex code. One option would be to just set a flag in the saveImage()
function, and handling it in requestComplete(). If the flag is set, the
image will be saved before requeuing the buffer. We would need to
dispatch the save action to a separate thread to avoid blocking the
CameraManager thread.

Saving the last image instead of the next one is also possible, but
you'll likely have to allocate two QImage in ViewFinder, switching to
the other QImage when save is in progress. It could be a bit messy as
the viewfinder would get heavily involved in image capture, which may
not be the best design.

> > +}
> > +
> >  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..1da79d3e67aa 100644
> > --- a/src/qcam/viewfinder.h
> > +++ b/src/qcam/viewfinder.h
> > @@ -23,6 +23,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;

-- 
Regards,

Laurent Pinchart