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

[Kieran Bingham]

On 14/02/2020 11:41, Laurent Pinchart wrote:
> 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.
> 	 */

Sure, cursory testing on an x86 doesn't show any issues, but that may
not be the same case on an ARM device of course.

Do you want me to post a v3 with all of the updates made?

--
Kieran


> 
>>>  	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
--
Kieran