Message ID | 20200206150504.24204-7-kieran.bingham@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
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@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 */ > + > + QImage image = viewfinder_->getCurrentImage(); > + > + QString filename = QFileDialog::getSaveFileName(this, "Save Image", "", > + "Image Files (*.png *.jpg *.jpeg)"); > + > + 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? > + 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 ... > +} > + > 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; >
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@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;
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 */ + + QImage image = viewfinder_->getCurrentImage(); + + QString filename = QFileDialog::getSaveFileName(this, "Save Image", "", + "Image Files (*.png *.jpg *.jpeg)"); + + std::cerr << "Save jpeg to " << filename.toStdString() << std::endl; + + 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? */ + 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..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;
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> --- 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(+)