| Message ID | 20200214001810.19302-8-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. 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@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_); > + > 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@ideasonboard.com> > }; > > #endif /* __QCAM_VIEWFINDER__ */
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@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@ideasonboard.com> > > > }; > > > > #endif /* __QCAM_VIEWFINDER__ */ > > -- > Regards, > > Laurent Pinchart > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@ideasonboard.com> >> >>> }; >>> >>> #endif /* __QCAM_VIEWFINDER__ */ >> >> -- >> Regards, >> >> Laurent Pinchart >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel >
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@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@ideasonboard.com> > >> > >>> }; > >>> > >>> #endif /* __QCAM_VIEWFINDER__ */
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; + + if (filename == "") + 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(); + 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_); + converter_.convert(raw, size, image_); update(); } +QImage ViewFinder::getCurrentImage() +{ + QMutexLocker locker(&mutex_); + + return *image_; +} + 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_ }; #endif /* __QCAM_VIEWFINDER__ */
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@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(+)