[libcamera-devel] [PATCH v2 7/7] qcam: Provide save image functionality
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Feb 14 12:51:30 CET 2020
Hi Kieran,
On Fri, Feb 14, 2020 at 11:48:06AM +0000, Kieran Bingham wrote:
> On 14/02/2020 11:41, Laurent Pinchart wrote:
> > 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?
If you have my R-b tag for all patches there's no need to.
> >>> 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
More information about the libcamera-devel
mailing list